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, 1 Mar 2018 21:36:51 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.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)

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.
>