[PATCH 0/2] PCI: Clean up match_driver flag usage

[PATCH 0/2] PCI: Clean up match_driver flag usage

[Lukas Wunner]

A small puzzle piece to improve maintainability of the PCI core:

The match_driver flag in struct pci_dev is used to postpone driver
binding until all PCI devices have been enumerated.

The AMD IOMMU driver fiddles with the flag to work around breakage
introduced 10 years ago.  The breaking change has since been reverted,
so accessing the flag appears to be superfluous and is hereby dropped
(patch [1/2]).  The patch needs an ack from AMD IOMMU maintainers.

This clears the way for moving the flag to struct pci_dev's priv_flags
and thus prevent any further abuse outside the PCI core (patch [2/2]).

There are already two patches queued up in this cycle which amend
priv_flags with new definitions for bits 4, 5 and 6 (on the pci/hotplug
and pci/bwctrl topic branches), hence the bit number used here is 7.

Lukas Wunner (2):
  Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI
    devices"
  PCI: Limit visibility of match_driver flag to PCI core

 drivers/iommu/amd/init.c |  3 ---
 drivers/pci/bus.c        |  4 +++-
 drivers/pci/pci-driver.c |  2 +-
 drivers/pci/pci.h        | 11 +++++++++++
 drivers/pci/probe.c      |  1 -
 include/linux/pci.h      |  2 --
 6 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.47.2

[PATCH 1/2] Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices"

[Lukas Wunner]

Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
pcibios_free_irq()") changed IRQ handling on PCI driver probing.
It inadvertently broke resume from system sleep on AMD platforms:

  https://lore.kernel.org/r/20150926164651.GA3640@pd.tnic/

This was fixed by two independent commits:

* 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
* cbbc00be2ce3 ("iommu/amd: Prevent binding other PCI drivers to IOMMU
                 PCI devices")

The breaking change and one of these two fixes were subsequently reverted:

* fe25d078874f ("Revert "x86/PCI: Don't alloc pcibios-irq when MSI is
                 enabled"")
* 6c777e8799a9 ("Revert "PCI, x86: Implement pcibios_alloc_irq() and
                 pcibios_free_irq()"")

This rendered the second fix unnecessary, so revert it as well.  It used
the match_driver flag in struct pci_dev, which is internal to the PCI core
and not supposed to be touched by arbitrary drivers.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
I would have cc'ed Jiang Liu (author of the commit reverted here)
but his Intel e-mail address appears to no longer be working.
Someone with the same name has recently started to contribute
using an Alibaba e-mail address, but I'm not sure it's the same
person.

 drivers/iommu/amd/init.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index dd9e26b..33b6e12 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2030,9 +2030,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	if (!iommu->dev)
 		return -ENODEV;
 
-	/* Prevent binding other PCI device drivers to IOMMU devices */
-	iommu->dev->match_driver = false;
-
 	/* ACPI _PRT won't have an IRQ for IOMMU */
 	iommu->dev->irq_managed = 1;
 
-- 
2.47.2

Re: [PATCH 1/2] Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices"

[Lukas Wunner]

Dear AMD IOMMU maintainers,

Just a gentle reminder:

Please consider ack'ing the patch below, for merging through the pci tree.

The patch removes code which prevents driver binding to the pci_dev
exposed by an AMD IOMMU.  No other IOMMU driver does that or needs that.

The code was added to work around breakage introduced by 991de2e59090:
With that commit, resume from system sleep was broken when a driver was
bound to the AMD IOMMU (e.g. vfio-pci).  The commit has since been
reverted, so there is no apparent reason to continue preventing
driver binding to the AMD IOMMU.

Random drivers are not supposed to fiddle with the match_driver flag
in struct pci_dev and having the AMD IOMMU driver do that is a
maintance burden.

So unless there's a reason to keep this code, please ack the patch below.

Thank you!

Lukas

On Fri, Apr 25, 2025 at 11:24:21AM +0200, Lukas Wunner wrote:
> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
> pcibios_free_irq()") changed IRQ handling on PCI driver probing.
> It inadvertently broke resume from system sleep on AMD platforms:
> 
>   https://lore.kernel.org/r/20150926164651.GA3640@pd.tnic/
> 
> This was fixed by two independent commits:
> 
> * 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
> * cbbc00be2ce3 ("iommu/amd: Prevent binding other PCI drivers to IOMMU
>                  PCI devices")
> 
> The breaking change and one of these two fixes were subsequently reverted:
> 
> * fe25d078874f ("Revert "x86/PCI: Don't alloc pcibios-irq when MSI is
>                  enabled"")
> * 6c777e8799a9 ("Revert "PCI, x86: Implement pcibios_alloc_irq() and
>                  pcibios_free_irq()"")
> 
> This rendered the second fix unnecessary, so revert it as well.  It used
> the match_driver flag in struct pci_dev, which is internal to the PCI core
> and not supposed to be touched by arbitrary drivers.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> I would have cc'ed Jiang Liu (author of the commit reverted here)
> but his Intel e-mail address appears to no longer be working.
> Someone with the same name has recently started to contribute
> using an Alibaba e-mail address, but I'm not sure it's the same
> person.
> 
>  drivers/iommu/amd/init.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index dd9e26b..33b6e12 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -2030,9 +2030,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
>  	if (!iommu->dev)
>  		return -ENODEV;
>  
> -	/* Prevent binding other PCI device drivers to IOMMU devices */
> -	iommu->dev->match_driver = false;
> -
>  	/* ACPI _PRT won't have an IRQ for IOMMU */
>  	iommu->dev->irq_managed = 1;
>  
> -- 
> 2.47.2

Re: [PATCH 1/2] Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices"

[Joerg Roedel]

On Fri, Apr 25, 2025 at 11:24:21AM +0200, Lukas Wunner wrote:
> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
> pcibios_free_irq()") changed IRQ handling on PCI driver probing.
> It inadvertently broke resume from system sleep on AMD platforms:
> 
>   https://lore.kernel.org/r/20150926164651.GA3640@pd.tnic/
> 
> This was fixed by two independent commits:
> 
> * 8affb487d4a4 ("x86/PCI: Don't alloc pcibios-irq when MSI is enabled")
> * cbbc00be2ce3 ("iommu/amd: Prevent binding other PCI drivers to IOMMU
>                  PCI devices")
> 
> The breaking change and one of these two fixes were subsequently reverted:
> 
> * fe25d078874f ("Revert "x86/PCI: Don't alloc pcibios-irq when MSI is
>                  enabled"")
> * 6c777e8799a9 ("Revert "PCI, x86: Implement pcibios_alloc_irq() and
>                  pcibios_free_irq()"")
> 
> This rendered the second fix unnecessary, so revert it as well.  It used
> the match_driver flag in struct pci_dev, which is internal to the PCI core
> and not supposed to be touched by arbitrary drivers.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Acked-by: Joerg Roedel <jroedel@suse.de>

[PATCH 2/2] PCI: Limit visibility of match_driver flag to PCI core

[Lukas Wunner]

Since commit 58d9a38f6fac ("PCI: Skip attaching driver in device_add()"),
PCI enumeration is split into two steps:  In the first step, all devices
are published in sysfs with device_add().  In the second step, drivers are
bound to the devices with device_attach().  To delay driver binding until
the second step, a "bool match_driver" in struct pci_dev is used.

Instead of a bool, use a bit in the "unsigned long priv_flags" to shrink
struct pci_dev a little and prevent use of the bool outside the PCI core
(as has happened with commit cbbc00be2ce3 ("iommu/amd: Prevent binding
other PCI drivers to IOMMU PCI devices")).

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/pci/bus.c        |  4 +++-
 drivers/pci/pci-driver.c |  2 +-
 drivers/pci/pci.h        | 11 +++++++++++
 drivers/pci/probe.c      |  1 -
 include/linux/pci.h      |  2 --
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index b685110..6904886 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -369,7 +369,9 @@ void pci_bus_add_device(struct pci_dev *dev)
 				pdev->name);
 	}
 
-	dev->match_driver = !dn || of_device_is_available(dn);
+	if (!dn || of_device_is_available(dn))
+		pci_dev_allow_binding(dev);
+
 	retval = device_attach(&dev->dev);
 	if (retval < 0 && retval != -EPROBE_DEFER)
 		pci_warn(dev, "device attach failed (%d)\n", retval);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index c8bd71a..0c5bdb8 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -1507,7 +1507,7 @@ static int pci_bus_match(struct device *dev, const struct device_driver *drv)
 	struct pci_driver *pci_drv;
 	const struct pci_device_id *found_id;
 
-	if (!pci_dev->match_driver)
+	if (pci_dev_binding_disallowed(pci_dev))
 		return 0;
 
 	pci_drv = (struct pci_driver *)to_pci_driver(drv);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index b81e99c..de5d4ef 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -557,6 +557,7 @@ static inline int pci_dev_set_disconnected(struct pci_dev *dev, void *unused)
 #define PCI_DPC_RECOVERED 1
 #define PCI_DPC_RECOVERING 2
 #define PCI_DEV_REMOVED 3
+#define PCI_DEV_ALLOW_BINDING 7
 
 static inline void pci_dev_assign_added(struct pci_dev *dev)
 {
@@ -580,6 +581,16 @@ static inline bool pci_dev_test_and_set_removed(struct pci_dev *dev)
 	return test_and_set_bit(PCI_DEV_REMOVED, &dev->priv_flags);
 }
 
+static inline void pci_dev_allow_binding(struct pci_dev *dev)
+{
+	set_bit(PCI_DEV_ALLOW_BINDING, &dev->priv_flags);
+}
+
+static inline bool pci_dev_binding_disallowed(struct pci_dev *dev)
+{
+	return !test_bit(PCI_DEV_ALLOW_BINDING, &dev->priv_flags);
+}
+
 #ifdef CONFIG_PCIEAER
 #include <linux/aer.h>
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 364fa2a..126ae1e 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2711,7 +2711,6 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
 	pci_set_msi_domain(dev);
 
 	/* Notifier could use PCI capabilities */
-	dev->match_driver = false;
 	ret = device_add(&dev->dev);
 	WARN_ON(ret < 0);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 51e2bd6..e29d7d4 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -425,8 +425,6 @@ struct pci_dev {
 	struct resource resource[DEVICE_COUNT_RESOURCE]; /* I/O and memory regions + expansion ROMs */
 	struct resource driver_exclusive_resource;	 /* driver exclusive resource ranges */
 
-	bool		match_driver;		/* Skip attaching driver */
-
 	unsigned int	transparent:1;		/* Subtractive decode bridge */
 	unsigned int	io_window:1;		/* Bridge has I/O window */
 	unsigned int	pref_window:1;		/* Bridge has pref mem window */
-- 
2.47.2

Re: [PATCH 0/2] PCI: Clean up match_driver flag usage

[Bjorn Helgaas]

On Fri, Apr 25, 2025 at 11:24:20AM +0200, Lukas Wunner wrote:
> A small puzzle piece to improve maintainability of the PCI core:
> 
> The match_driver flag in struct pci_dev is used to postpone driver
> binding until all PCI devices have been enumerated.
> 
> The AMD IOMMU driver fiddles with the flag to work around breakage
> introduced 10 years ago.  The breaking change has since been reverted,
> so accessing the flag appears to be superfluous and is hereby dropped
> (patch [1/2]).  The patch needs an ack from AMD IOMMU maintainers.
> 
> This clears the way for moving the flag to struct pci_dev's priv_flags
> and thus prevent any further abuse outside the PCI core (patch [2/2]).
> 
> There are already two patches queued up in this cycle which amend
> priv_flags with new definitions for bits 4, 5 and 6 (on the pci/hotplug
> and pci/bwctrl topic branches), hence the bit number used here is 7.
> 
> Lukas Wunner (2):
>   Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI
>     devices"
>   PCI: Limit visibility of match_driver flag to PCI core
> 
>  drivers/iommu/amd/init.c |  3 ---
>  drivers/pci/bus.c        |  4 +++-
>  drivers/pci/pci-driver.c |  2 +-
>  drivers/pci/pci.h        | 11 +++++++++++
>  drivers/pci/probe.c      |  1 -
>  include/linux/pci.h      |  2 --
>  6 files changed, 15 insertions(+), 8 deletions(-)

Provisionally applied to pci/enumeration for build testing, pending
ack from AMD IOMMU folks.