Message-ID: <1e964a74ca51e9e28202a47af22917e468050039.camel@redhat.com>
Date:   Wed, 08 Nov 2023 22:02:29 +0100
From:   Philipp Stanner <pstanner@...hat.com>
To:     linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     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

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.

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