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