Re: [PATCH 02/10] pci: deprecate iomap-table functions

andy.shevchenko
2024-01-16T21:29:04Z

Mon, Jan 15, 2024 at 03:46:13PM +0100, Philipp Stanner kirjoitti:
> The old plural devres functions tie PCI's devres implementation to the
> iomap-table mechanism which unfortunately is not extensible.
>
> As the purlal functions are almost never used with more than one bit set
> in their bit-mask, deprecating them and encouraging users to use the new
> singular functions instead is reasonable.
>
> Furthermore, to make the implementation more consistent and extensible,
> the plural functions should use the singular functions.
>
> Add new wrapper to request / release all BARs.
> Make the plural functions call into the singular functions.
> Mark the plural functions as deprecated.
> Remove as much of the iomap-table-mechanism as possible.
> Add comments describing the path towards a cleaned-up API.

...

> static void pcim_iomap_release(struct device *gendev, void *res)
> {
> - struct pci_dev *dev = to_pci_dev(gendev);
> - struct pcim_iomap_devres *this = res;
> - int i;
> -
> - for (i = 0; i < PCIM_IOMAP_MAX; i++)
> - if (this->table[i])
> - pci_iounmap(dev, this->table[i]);
> + /*
> + * Do nothing. This is legacy code.
> + *
> + * Cleanup of the mappings is now done directly through the callbacks
> + * registered when creating them.
> + */
> }

How many users we have? Can't we simply kill it for good?

...

> + * This function is DEPRECATED. Do not use it in new code.

We have __deprecated IIRC, can it be used?

...

> + if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)

Redundant ' != 0' part.

> + goto err_table;

...

> +static inline bool mask_contains_bar(int mask, int bar)
> +{
> + return mask & (1 << bar);

BIT() ?

> +}

But I believe this function is not needed (see below).

...

> /**
> - * pcim_iomap_regions - Request and iomap PCI BARs
> + * pcim_iomap_regions - Request and iomap PCI BARs (DEPRECATED)
> * @pdev: PCI device to map IO resources for
> * @mask: Mask of BARs to request and iomap
> * @name: Name associated with the requests
> *
> + * Returns 0 on success, negative error code on failure.

Validate the kernel-doc.

> * Request and iomap regions specified by @mask.
> + *
> + * This function is DEPRECATED. Don't use it in new code.
> + * Use pcim_iomap_region() instead.
> */

...

> + for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> + if (!mask_contains_bar(mask, bar))
> + continue;

NIH for_each_set_bit().

...

> + ret = pcim_add_mapping_to_legacy_table(pdev, mapping, bar);
> + if (ret != 0)

if (ret)

> + goto err;
> + }

...

> + err:

Better to name it like

err_unmap_and_remove:

> + while (--bar >= 0) {

while (bar--)

is easier to read.

> + pcim_iounmap_region(pdev, bar);
> + pcim_remove_bar_from_legacy_table(pdev, bar);
> + }

...

> +/**
> + * pcim_request_all_regions - Request all regions
> + * @pdev: PCI device to map IO resources for
> + * @name: name associated with the request
> + *
> + * Requested regions will automatically be released at driver detach. If desired,
> + * release individual regions with pcim_release_region() or all of them at once
> + * with pcim_release_all_regions().
> + */

Validate kernel-doc.

...

> + ret = pcim_request_region(pdev, bar, name);
> + if (ret != 0)

if (ret)

> + goto err;

...

> + short bar;

Why signed?

> + int ret = -1;

Oy vei!

...

> + ret = pcim_request_all_regions(pdev, name);
> + if (ret != 0)

Here and in plenty other places

if (ret)

> + return ret;

> + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> + if (!mask_contains_bar(mask, bar))
> + continue;

NIH for_each_set_bit()

> + if (!pcim_iomap(pdev, bar, 0))
> + goto err;
> + }

...

> + if (!legacy_iomap_table)

What's wrong with positive check?

> + ret = -ENOMEM;
> + else
> + ret = -EINVAL;

Can be even one liner

What's wrong with positive check?

ret = legacy_iomap_table ? -EINVAL : -ENOMEM;

...

> + while (--bar >= 0)

while (bar--)

> + pcim_iounmap(pdev, legacy_iomap_table[bar]);

...

> + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> + if (!mask_contains_bar(mask, bar))
> continue;

NIH for_each_set_bit()

> - pcim_iounmap(pdev, iomap[i]);
> - pci_release_region(pdev, i);
> + pcim_iounmap_region(pdev, bar);
> + pcim_remove_bar_from_legacy_table(pdev, bar);
> }

--
With Best Regards,
Andy Shevchenko