On Wed, 2023-11-08 at 11:35 -0600, Bjorn Helgaas wrote:
> On Tue, Nov 07, 2023 at 08:38:18PM +0100, Philipp Stanner wrote:
> > Hi all,
> >
> > I'm currently working on porting more drivers in DRM to managed
> > pci-
> > functions. During this process I discovered something that might be
> > called an inconsistency in the implementation.
> >
> > First, there would be the pcim_ functions being scattered across
> > several files. Some are implemented in drivers/pci/pci.c, others in
> > lib/devres.c, where they are guarded by #ifdef CONFIG_PCI
> > – this originates from an old cleanup, done in
> > 5ea8176994003483a18c8fed580901e2125f8a83
> >
> > Additionally, there is lib/pci_iomap.c, which contains the non-
> > managed
> > pci_iomap() functions.
> >
> > At first and second glance it's not obvious to me why these pci-
> > functions are scattered. Hints?
> >
> >
> > Second, it seems there are two competing philosophies behind
> > managed
> > resource reservations. Some pci_ functions have pcim_ counterparts,
> > such as pci_iomap() <-> pcim_iomap(). So the API-user might expect
> > that
> > relevant pci_ functions that do not have a managed counterpart do
> > so
> > because no one bothered implementing them so far.
> >
> > However, it turns out that for pci_request_region(), there is no
> > counterpart because a different mechanism / semantic was used to
> > make
> > the function _sometimes_ managed:
> >
> > 1. If you use pcim_enable_device(), the member is_managed in
> > struct
> > pci_dev is set to 1.
> > 2. This value is then evaluated in pci_request_region()'s call
> > to
> > find_pci_dr()
> >
> > Effectively, this makes pci_request_region() sometimes managed.
> > Why has it been implemented that way and not as a separate function
> > –
> > like, e.g., pcim_iomap()?
> >
> > That's where an inconsistency lies. For things like iomappings
> > there
> > are separate managed functions, for the region-request there's a
> > universal function doing managed or unmanaged, respectively.
> >
> > Furthermore, look at pcim_iomap_regions() – that function uses a
> > mix
> > between the obviously managed function pcim_iomap() and
> > pci_request_region(), which appears unmanaged judging by the name,
> > but,
> > nevertheless, is (sometimes) managed below the surface.
> > Consequently, pcim_iomap_regions() could even be a little buggy:
> > When
> > someone uses pci_enable_device() + pcim_iomap_regions(), wouldn't
> > that
> > leak the resource regions?
> >
> > The change to pci_request_region() hasn't grown historically but
> > was
> > implemented that way in one run with the first set of managed
> > functions
> > in commit 9ac7849e35f70. So this implies it has been implemented
> > that
> > way on purpose.
> >
> > What was that purpose?
> >
> > Would be great if someone can give some hints :)
>
> Sorry, I don't know or remember all the history behind this, so can't
> give any useful hints. If the devm functions are mostly wrappers
> without interesting content, it might make sense to collect them
> together. If they *do* have interesting content, it probably makes
> sense to put them next to their non-devm counterparts.
Most of the magic seems to happen here in lib/devres.c
struct pcim_iomap_devres {
void __iomem *table[PCIM_IOMAP_MAX];
};
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]);
}
/**
* pcim_iomap_table - access iomap allocation table
* @pdev: PCI device to access iomap table for
*
* Access iomap allocation table for @dev. If iomap table doesn't
* exist and @pdev is managed, it will be allocated. All iomaps
* recorded in the iomap table are automatically unmapped on driver
* detach.
*
* This function might sleep when the table is first allocated but can
* be safely called without context and guaranteed to succeed once
* allocated.
*/
void __iomem * const *pcim_iomap_table(struct pci_dev *pdev)
{
struct pcim_iomap_devres *dr, *new_dr;
dr = devres_find(&pdev->dev, pcim_iomap_release, NULL, NULL);
if (dr)
return dr->table;
new_dr = devres_alloc_node(pcim_iomap_release, sizeof(*new_dr), GFP_KERNEL,
dev_to_node(&pdev->dev));
if (!new_dr)
return NULL;
dr = devres_get(&pdev->dev, new_dr, NULL, NULL);
return dr->table;
}
In devres_alloc_node() it registers the cleanup-callback and places the
struct that keeps track of the already mapped BARs in the devres-node.
Besides, the managed wrappers usually call directly into the unmanaged
pci_ functions, as you can for example see above in
pcim_iomap_release()
Not sure if that qualifies for "interesting content", but it certainly
might be a bit difficult to extend (see my other answer in this thread)
^^'
>
> The pci_request_region() managed/unmanaged thing does seem like it
> could be unexpected and lead to issues as you point out.
Unfortunately it already did. I discovered such a bug today. Still
trying to figure out how to solve it, though. Once that's done I link
it in this thread.
P.
>
> Bjorn
>
