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
