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@xxxxxxxxxx>
[...]
>>> 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
