On Tue, 2024-01-16 at 23:27 +0200, andy.shevchenko@xxxxxxxxx wrote:
> 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?
>
> ...
There are 126 users in the kernel.
When I began with all of this I indeed checked whether we might burn it
completely.
Indeed the majority of driver users are wise enough to do something
like
driver->map0 = pcim_iomap_table(pdev)[0];
driver->map1 = pcim_iomap_table(pdev)[1];
So we could just easily replace it with
+ drivers->map0 = pcim_iomap_region(pdev, 0, "driver");
+ if (IS_ERR(drivers->map0))
Buuuuuuuuuut.. certain people, such as the crypto folks, do indeed not
store the mapping-address in their own driver-struct but call
pcim_iomap_table() to get the address whenever they need it.
That's where I gave up. It's too much work for and would take forever,
even if the code were sane, until you get it all merged.
We want that new API for DRM, that's why I'm working on it in the first
place.
That's why this API should have never been merged back in 2006/7. But
no one dared to touch it for 16 years, so here we are.
>
> > + * This function is DEPRECATED. Do not use it in new code.
>
> We have __deprecated IIRC, can it be used?
It seems the Boss has deprecated __deprecated a few years ago :D
/*
* Don't. Just don't. See commit 771c035372a0 ("deprecate the '__deprecated'
* attribute warnings entirely and for good") for more information.
*
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-deprecated-function-attribute
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-deprecated-type-attribute
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-deprecated-variable-attribute
* gcc: https://gcc.gnu.org/onlinedocs/gcc/Enumerator-Attributes.html#index-deprecated-enumerator-attribute
* clang: https://clang.llvm.org/docs/AttributeReference.html#deprecated
*/
#define __deprecated
>
> ...
>
> > + 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() ?
Yo, we could use it
>
> > +}
>
> 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().
But wouldn't that iterate too frequently?
It would check all bits, whereas this for-loop (that I inherited from
the existing implementation, I just tried to keep the functionality
identical) loops until DEVICE_COUNT_RESOURCE, which might be !=
sizeof(int).
>
> ...
>
> > + 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.
I won't die on this hill, but do not really agree.
>
> > + 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?
No reason. The max bar number in PCI is 6.
We can un-sign it
>
> > + 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;
Yo, can do that
P.
>
> ...
>
> > + 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);
> > }
>
