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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 01 Mar 2018 22:15:26 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Heiner Kallweit <hkallweit1@...il.com>, nic_swsd@...ltek.com,
        "David S . Miller" <davem@...emloft.net>
Cc:     netdev@...r.kernel.org
Subject: 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 <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy