Message-ID: <305e0b056b8a7ecb61c4ff5e3eaf4ae169b43c70.camel@redhat.com>
Date:   Wed, 08 Nov 2023 22:11:40 +0100
From:   Philipp Stanner <pstanner@...hat.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Tejun Heo <htejun@...il.com>, dakr@...hat.com,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ben Dooks <ben.dooks@...ethink.co.uk>, jeff@...zik.org,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: Implementation details of PCI Managed (devres) Functions

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
> 

Powered by blists - more mailing lists