Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
From: Andy Shevchenko
Date: Thu, 1 Mar 2018 13:27:35 +0200
> This is a follow up to the commit
>
> 4c45d24a759d ("r8169: switch to device-managed functions in probe")
>
> to move towards managed resources even more.
>
> Cc: Heiner Kallweit
> Signed-off-by: Andy Shevchenko
Applied.
Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
On Thu, 2018-03-01 at 21:36 +0100, Heiner Kallweit wrote:
> Am 01.03.2018 um 21:15 schrieb Andy Shevchenko:
> > On Thu, 2018-03-01 at 20:54 +0100, Heiner Kallweit wrote:
> > > Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:
> > > > - rc = pci_request_regions(pdev, MODULENAME);
> > > > + rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
> > > > if (rc < 0) {
> > > > - netif_err(tp, probe, dev, "could not request
> > > > regions\n");
> > > > + netif_err(tp, probe, dev, "cannot remap MMIO,
> > > > aborting\n");
> > > > return rc;
> > > > }
> > > >
> > > >
> > > > + tp->mmio_addr = pcim_iomap_table(pdev)[region];
> > > >
> > >
> > > pcim_iomap_table() can return NULL in case of an error.
> >
> > No.
> >
> > > Shouldn't we catch this?
> >
> > No.
> >
> > Yeah, I'm a bit tired to explain everyone that pcim_iomap_table()
> > will
> > never fail if previous pcim_iomap_regions() not failed.
> >
>
> Ahh, missed this because I didn't dig that deep into these functions.
> Would be nice if pcim_iomap_regions() returned iomap directly
> (or an ERRPTR). Then the additional call to pcim_iomap_table()
> wouldn't be needed.
You can't do that, since you may ask to map several regions at once.
And moreover, some of them can be optional (then you indeed need to
check the return value of pcim_iomap_table() helper).
>
>
> > > Typical benefit of switching to device-managed functions is that
> > > we
> > > don't
> > > have to clean up in the probe() error path and in remove(). With
> > > the
> > > change here we don't have any such benefit and we just exchange
> > > two
> > > calls
> > > against two other calls w/o functional change (AFAICS).
> > > Which benefit do you see justifying this patch?
> >
> > -67% statistics is a good sign, no?
> >
> > > However I don't know the PCI API's good enough to be able to judge
> > > whether
> > > one set of calls is preferable.
> >
> > More than above, it's about consistency. While you switch to devm_,
> > for
> > PCI driver it's naturally to go for pcim_.
> >
> > Esp. taking into account that pcim_enable_device() _is_ already
> > there.
> >
>
>
--
Andy Shevchenko
Intel Finland Oy
Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
Am 01.03.2018 um 21:15 schrieb Andy Shevchenko:
> On Thu, 2018-03-01 at 20:54 +0100, Heiner Kallweit wrote:
>> Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:
>
>>> - rc = pci_request_regions(pdev, MODULENAME);
>>> + rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
>>> if (rc < 0) {
>>> - netif_err(tp, probe, dev, "could not request
>>> regions\n");
>>> + netif_err(tp, probe, dev, "cannot remap MMIO,
>>> aborting\n");
>>> return rc;
>>> }
>>>
>>>
>
>>> + tp->mmio_addr = pcim_iomap_table(pdev)[region];
>>>
>>
>> pcim_iomap_table() can return NULL in case of an error.
>
> No.
>
>> Shouldn't we catch this?
>
> No.
>
> Yeah, I'm a bit tired to explain everyone that pcim_iomap_table() will
> never fail if previous pcim_iomap_regions() not failed.
>
Ahh, missed this because I didn't dig that deep into these functions.
Would be nice if pcim_iomap_regions() returned iomap directly
(or an ERRPTR). Then the additional call to pcim_iomap_table()
wouldn't be needed.
>> Typical benefit of switching to device-managed functions is that we
>> don't
>> have to clean up in the probe() error path and in remove(). With the
>> change here we don't have any such benefit and we just exchange two
>> calls
>> against two other calls w/o functional change (AFAICS).
>> Which benefit do you see justifying this patch?
>
> -67% statistics is a good sign, no?
>
>> However I don't know the PCI API's good enough to be able to judge
>> whether
>> one set of calls is preferable.
>
> More than above, it's about consistency. While you switch to devm_, for
> PCI driver it's naturally to go for pcim_.
>
> Esp. taking into account that pcim_enable_device() _is_ already there.
>
Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
On Thu, 2018-03-01 at 20:54 +0100, Heiner Kallweit wrote:
> Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:
> > - rc = pci_request_regions(pdev, MODULENAME);
> > + rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
> > if (rc < 0) {
> > - netif_err(tp, probe, dev, "could not request
> > regions\n");
> > + netif_err(tp, probe, dev, "cannot remap MMIO,
> > aborting\n");
> > return rc;
> > }
> >
> >
> > + tp->mmio_addr = pcim_iomap_table(pdev)[region];
> >
>
> pcim_iomap_table() can return NULL in case of an error.
No.
> Shouldn't we catch this?
No.
Yeah, I'm a bit tired to explain everyone that pcim_iomap_table() will
never fail if previous pcim_iomap_regions() not failed.
> Typical benefit of switching to device-managed functions is that we
> don't
> have to clean up in the probe() error path and in remove(). With the
> change here we don't have any such benefit and we just exchange two
> calls
> against two other calls w/o functional change (AFAICS).
> Which benefit do you see justifying this patch?
-67% statistics is a good sign, no?
> However I don't know the PCI API's good enough to be able to judge
> whether
> one set of calls is preferable.
More than above, it's about consistency. While you switch to devm_, for
PCI driver it's naturally to go for pcim_.
Esp. taking into account that pcim_enable_device() _is_ already there.
--
Andy Shevchenko
Intel Finland Oy
Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:
> This is a follow up to the commit
>
> 4c45d24a759d ("r8169: switch to device-managed functions in probe")
>
> to move towards managed resources even more.
>
> Cc: Heiner Kallweit
> Signed-off-by: Andy Shevchenko
> ---
> drivers/net/ethernet/realtek/r8169.c | 16
> 1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/realtek/r8169.c
> b/drivers/net/ethernet/realtek/r8169.c
> index b6098e081a25..7055db161b1b 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
> @@ -8169,7 +8169,6 @@ static int rtl_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> struct rtl8169_private *tp;
> struct mii_if_info *mii;
> struct net_device *dev;
> - void __iomem *ioaddr;
> int chipset, i;
> int rc;
>
> @@ -8227,20 +8226,13 @@ static int rtl_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> return -ENODEV;
> }
>
> - rc = pci_request_regions(pdev, MODULENAME);
> + rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
> if (rc < 0) {
> - netif_err(tp, probe, dev, "could not request regions\n");
> + netif_err(tp, probe, dev, "cannot remap MMIO, aborting\n");
> return rc;
> }
>
> - /* ioremap MMIO region */
> - ioaddr = devm_ioremap(&pdev->dev, pci_resource_start(pdev, region),
> - R8169_REGS_SIZE);
> - if (!ioaddr) {
> - netif_err(tp, probe, dev, "cannot remap MMIO, aborting\n");
> - return -EIO;
> - }
> - tp->mmio_addr = ioaddr;
> + tp->mmio_addr = pcim_iomap_table(pdev)[region];
>
pcim_iomap_table() can return NULL in case of an error.
Shouldn't we catch this?
Typical benefit of switching to device-managed functions is that we don't
have to clean up in the probe() error path and in remove(). With the
change here we don't have any such benefit and we just exchange two calls
against two other calls w/o functional change (AFAICS).
Which benefit do you see justifying this patch?
However I don't know the PCI API's good enough to be able to judge whether
one set of calls is preferable.
> if (!pci_is_pcie(pdev))
> netif_info(tp, probe, dev, "not PCI Express\n");
> @@ -8412,7 +8404,7 @@ static int rtl_init_one(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> pci_set_drvdata(pdev, dev);
>
> netif_info(tp, probe, dev, "%s at 0x%p, %pM, XID %08x IRQ %d\n",
> -rtl_chip_infos[chipset].name, ioaddr, dev->dev_addr,
> +rtl_chip_infos[chipset].name, tp->mmio_addr, dev->dev_addr,
> (u32)(RTL_R32(tp, TxConfig) & 0x9cf0f8ff),
> pci_irq_vector(pdev, 0));
> if (rtl_chip_infos[chipset].jumbo_max != JUMBO_1K) {
>
[PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)
This is a follow up to the commit
4c45d24a759d ("r8169: switch to device-managed functions in probe")
to move towards managed resources even more.
Cc: Heiner Kallweit
Signed-off-by: Andy Shevchenko
---
drivers/net/ethernet/realtek/r8169.c | 16
1 file changed, 4 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c
b/drivers/net/ethernet/realtek/r8169.c
index b6098e081a25..7055db161b1b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -8169,7 +8169,6 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
struct rtl8169_private *tp;
struct mii_if_info *mii;
struct net_device *dev;
- void __iomem *ioaddr;
int chipset, i;
int rc;
@@ -8227,20 +8226,13 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
return -ENODEV;
}
- rc = pci_request_regions(pdev, MODULENAME);
+ rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
if (rc < 0) {
- netif_err(tp, probe, dev, "could not request regions\n");
+ netif_err(tp, probe, dev, "cannot remap MMIO, aborting\n");
return rc;
}
- /* ioremap MMIO region */
- ioaddr = devm_ioremap(&pdev->dev, pci_resource_start(pdev, region),
- R8169_REGS_SIZE);
- if (!ioaddr) {
- netif_err(tp, probe, dev, "cannot remap MMIO, aborting\n");
- return -EIO;
- }
- tp->mmio_addr = ioaddr;
+ tp->mmio_addr = pcim_iomap_table(pdev)[region];
if (!pci_is_pcie(pdev))
netif_info(tp, probe, dev, "not PCI Express\n");
@@ -8412,7 +8404,7 @@ static int rtl_init_one(struct pci_dev *pdev, const
struct pci_device_id *ent)
pci_set_drvdata(pdev, dev);
netif_info(tp, probe, dev, "%s at 0x%p, %pM, XID %08x IRQ %d\n",
- rtl_chip_infos[chipset].name, ioaddr, dev->dev_addr,
+ rtl_chip_infos[chipset].name, tp->mmio_addr, dev->dev_addr,
(u32)(RTL_R32(tp, TxConfig) & 0x9cf0f8ff),
pci_irq_vector(pdev, 0));
if (rtl_chip_infos[chipset].jumbo_max != JUMBO_1K) {
--
2.16.1
