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

Message-ID: <d7f0735e-9dd4-c1db-e057-bbdbafe3887f@omp.ru>
Date: Wed, 21 Aug 2024 00:33:41 +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 11:37 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().
>>
>>    Huh? :-)
>>    Besides, I'm not seeing pcim_request_region() anywhere in this
>> patch...
>>
>>> Remove all calls to pcim_iomap_table().
>>>
>>> Signed-off-by: Philipp Stanner <pstanner@...hat.com>
[...]

>>> diff --git a/drivers/ata/pata_sil680.c b/drivers/ata/pata_sil680.c
>>> index abe64b5f83cf..8a17df73412e 100644
>>> --- a/drivers/ata/pata_sil680.c
>>> +++ b/drivers/ata/pata_sil680.c
>>> @@ -360,15 +360,16 @@ static int sil680_init_one(struct pci_dev
>>> *pdev, const struct pci_device_id *id)
>>>  	/* Try to acquire MMIO resources and fallback to PIO if
>>>  	 * that fails
>>>  	 */
>>> -	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR,
>>> DRV_NAME);
>>> -	if (rc)
>>> +	mmio_base = pcim_iomap_region(pdev, SIL680_MMIO_BAR,
>>> DRV_NAME);
>>> +	if (IS_ERR(mmio_base)) {
>>> +		rc = PTR_ERR(mmio_base);
>>   		goto use_ioports;
>>
>>    The code under that label ignores rc, no?
> 
> Oh, forgot to address this.
> 
> Yes, it does ignore it, but it behaves as the existing code does. The

   You mean, by setting rc?

> existing version jumps into ata_pci_bmdma_init_one() if it cannot
> request or ioremap the BAR.

   Yep, it cannot use MMIO in this case, so falls back to the good old
I/O ports, (confusingly?) named PIO in the comment above...

> 	/* Try to acquire MMIO resources and fallback to PIO if
> 	 * that fails
> 	 */
> 	rc = pcim_iomap_regions(pdev, 1 << SIL680_MMIO_BAR, DRV_NAME);
> 	if (rc)
> 		goto use_ioports;
> 
> 
> Is that a bug in the existing version, too?

   I'm not sure what you're considering a bug here...

> The comment hints to me that this is fine and intended.
> 
> Otherwise we want to remain consistent with the pre-existing behavior.

    I don't see a point here, rc is correctly ignored under that label,
and gcc drops this assignment anyway when generating the code...

[...]

MBR, Sergey