Message-ID: <ZUweTfT7ehduedf9@pollux>
Date:   Thu, 9 Nov 2023 00:48:29 +0100
From:   Danilo Krummrich <dakr@...hat.com>
To:     Philipp Stanner <pstanner@...hat.com>
Cc:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Tejun Heo <htejun@...il.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, Nov 08, 2023 at 10:02:29PM +0100, Philipp Stanner wrote:
> So, today I stared at the code for a while and come to a somewhat
> interesting insight:
> 
> 
> On Tue, 2023-11-07 at 20:38 +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.
> 
> I think I figured out why not all pci_ functions have a pcim_
> counterpart.
> 
> I was interested in implementing pcim_iomap_range(), since we could use
> that for some drivers.

I think you don't need all the "per bar" stuff below for that. You can just use
the existing pci_iomap_range() (which simply uses ioremap() internally) and
connect it to devres.

> 
> It turns out, that implementing this would be quite complicated (if I'm
> not mistaken).
> 
> lib/devres.c:
> 
> struct pcim_iomap_devres {
> 	void __iomem *table[PCIM_IOMAP_MAX];
> };
> 
> 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;
> }
> 
> That struct keeps track of the requested BARs. That's why there can
> only be one mapping per BAR, because that table is statically allocated
> and is indexed with the bar-number.
> pcim_iomap_table() now only ever returns the table with the pointers to
> the BARs. Adding tables to that struct that keep track of which
> mappings exist in which bars would be a bit tricky and require probably
> an API change for everyone who currently uses pcim_iomap_table(), and
> that's more than 100 C-files.
> 
> So, it seems that a concern back in 2007 was to keep things simple and
> skip the more complex data structures necessary for keeping track of
> the various mappings within a bar.
> In theory, there could be an almost unlimited number of such mappings
> of various sizes, almost forcing you to do book-keeping with the help
> of heap-allocations.
> 
> I guess one way to keep things extensible would have been to name the
> function pcim_iomap_bar_table(), so you could later implement one like
> pcim_iomap_range_table() or something.
> But now it is what it is..
> 
> That still doesn't explain why there's no pcim_request_region(),
> though...
> 
> 
> P.
> 
> > 
> > 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 :)
> > 
> > Regards,
> > P.
> > 
> 

Powered by blists - more mailing lists