linux-kernel - Re: [PATCH] ata: Replace deprecated PCI devres functions

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] [day] [month] [year] [list]

Message-ID: <c4111bbb-b59c-be35-561b-4eb79809918d@omp.ru>
Date: Wed, 21 Aug 2024 23:49:49 +0300
From: Sergey Shtylyov <s.shtylyov@....ru>
To: Philipp Stanner <pstanner@...hat.com>, Damien Le Moal
	<dlemoal@...nel.org>, Niklas Cassel <cassel@...nel.org>, Mikael Pettersson
	<mikpelinux@...il.com>
CC: <linux-ide@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ata: Replace deprecated PCI devres functions

On 8/16/24 10:47 AM, Philipp Stanner wrote:
[...]

>>> The ata subsystem uses the PCI devres functions pcim_iomap_table()
>>> and
>>> pcim_request_regions(), which have been deprecated in commit
>>> e354bb84a4c1
>>> ("PCI: Deprecate pcim_iomap_table(),
>>> pcim_iomap_regions_request_all()").
>>>
>>> These functions internally already use their successors, notably
>>> pcim_request_region(), so they are quite trivial to replace.
>>>
>>> However, one thing special about ata is that it stores the iomap
>>> table
>>> provided by pcim_iomap_table() in struct ata_host. This can be
>>> replaced
>>> with a __iomem pointer table, statically allocated with size
>>> PCI_STD_NUM_BARS so it can house the maximum number of PCI BARs.
>>> The
>>> only further modification then necessary is to explicitly fill that
>>> table, whereas before it was filled implicitly by
>>> pcim_request_regions().
>>>
>>> Modify the iomap table in struct ata_host.
>>>
>>> Replace all calls to pcim_request_region() with ones to
>>> pcim_request_region().

[...]

>>> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
>>> index 250f7dae05fd..d58db8226436 100644
>>> --- a/drivers/ata/libata-sff.c
>>> +++ b/drivers/ata/libata-sff.c
>> [...]
>>> @@ -2172,8 +2173,41 @@ int ata_pci_sff_init_host(struct ata_host
>>> *host)
>>>  			continue;
>>>  		}
>>>  
>>> -		rc = pcim_iomap_regions(pdev, 0x3 << base,
>>> -					dev_driver_string(gdev));
>>> +		/*
>>> +		 * In a first loop run, we want to get BARs 0 and
>>> 1.
>>> +		 * In a second run, we want BARs 2 and 3.
>>> +		 */
>>> +		if (i == 0) {
>>> +			io_tmp = pcim_iomap_region(pdev, 0,
>>> drv_name);
>>> +			if (IS_ERR(io_tmp)) {
>>> +				rc = PTR_ERR(io_tmp);
>>> +				goto err;
>>> +			}
>>> +			host->iomap[0] = io_tmp;
>>> +
>>> +			io_tmp = pcim_iomap_region(pdev, 1,
>>> drv_name);
>>> +			if (IS_ERR(io_tmp)) {
>>> +				rc = PTR_ERR(io_tmp);
>>> +				goto err;
>>> +			}
>>> +			host->iomap[1] = io_tmp;
>>> +		} else {
>>> +			io_tmp = pcim_iomap_region(pdev, 2,
>>> drv_name);
>>> +			if (IS_ERR(io_tmp)) {
>>> +				rc = PTR_ERR(io_tmp);
>>> +				goto err;
>>> +			}
>>> +			host->iomap[2] = io_tmp;
>>> +
>>> +			io_tmp = pcim_iomap_region(pdev, 3,
>>> drv_name);
>>> +			if (IS_ERR(io_tmp)) {
>>> +				rc = PTR_ERR(io_tmp);
>>> +				goto err;
>>> +			}
>>> +			host->iomap[3] = io_tmp;
>>> +		}
>>> +
>>
>>    Ugh... Why you couldn't keep using base (or just i * 2) and avoid
>> such code duplication?
> 
> I mean, this would at least make it perfectly readable what's being
> done.

   It looks pretty horrible, to my taste... :-)

> I guess we could do something like this, maybe with a comment explining
> what is going on:
> 
> for_each_set_bit(j, 0x3 << base, PCI_STD_NUM_BARS) {

   We're only interested in 4 first BARs, not all 6 of 'em. :-)
And you can't just pass 3 << base to for_each_set_bit() -- it needs
a bitmap ptr... :-)
   Why not just do s/th like:

for (j = base, j < base + 2, j++) {

> 	host->iomap[j] = pcim_iomap_region(pdev, j, drv_name);
> 	if (IS_ERR(host->iomap[j])) {
> 		rc = PTR_ERR(host->iomap[j]);
> 		break;
> 	}
> }
> 
> if (rc) {
> 	dev_warn(gdev,

[...]

MBR, Sergey