Implementation details of PCI Managed (devres) Functions

Implementation details of PCI Managed (devres) Functions

[Philipp Stanner]

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 :)

Regards,
P.

Re: Implementation details of PCI Managed (devres) Functions

[Bjorn Helgaas]

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.

The pci_request_region() managed/unmanaged thing does seem like it
could be unexpected and lead to issues as you point out.

Bjorn

Re: Implementation details of PCI Managed (devres) Functions

[Philipp Stanner]

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
> 

Re: Implementation details of PCI Managed (devres) Functions

[Philipp Stanner]

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.
> 

Re: Implementation details of PCI Managed (devres) Functions

[Danilo Krummrich]

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.
> > 
> 

Re: Implementation details of PCI Managed (devres) Functions

[Tejun Heo]

Hello, Philipp.

On Wed, Nov 08, 2023 at 10:02:29PM +0100, Philipp Stanner wrote:
...
> 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.

It was so long ago that I don't remember much but I do remember taking a
shortcut there for convenience / simplicity. I'm sure it's already clear but
there's no deeper reason, so if you wanna put in the work to make it
consistent, that'd be great.

Thanks.

-- 
tejun

Re: Implementation details of PCI Managed (devres) Functions

[Philipp Stanner]

Hi Tejun,

On Thu, 2023-11-09 at 08:52 -1000, Tejun Heo wrote:
> Hello, Philipp.
> 
> On Wed, Nov 08, 2023 at 10:02:29PM +0100, Philipp Stanner wrote:
> ...
> > 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.
> 
> It was so long ago that I don't remember much but I do remember
> taking a
> shortcut there for convenience / simplicity. I'm sure it's already
> clear but
> there's no deeper reason, so if you wanna put in the work to make it
> consistent, that'd be great.
> 

Alright, it's good to know that there seems to be no functional or
semantic reason or something behind it.

I'll think it through. Maybe we can design something clever

P.


> Thanks.
>