Re: [PATCH v4 2/2] r8169: switch to device-managed functions in probe (part 2)

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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 <andriy.shevchenko@linux.intel.com>
Intel Finland Oy