[PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Lukas Wunner]

Since commit 172efbb40333 ("AGP: Try unsupported AGP chipsets on x86-64
by default"), the AGP driver for AMD Opteron/Athlon64 CPUs has attempted
to bind to any PCI device possessing an AGP Capability.

Commit 6fd024893911 ("amd64-agp: Probe unknown AGP devices the right
way") subsequently reworked the driver to perform a bind attempt to
any PCI device (regardless of AGP Capability) and reject a device in
the driver's ->probe() hook if it lacks the AGP Capability.

On modern CPUs exposing an AMD IOMMU, this subtle change results in an
annoying message with KERN_CRIT severity:

  pci 0000:00:00.2: Resources present before probing

The message is emitted by the driver core prior to invoking a driver's
->probe() hook.  The check for an AGP Capability in the ->probe() hook
happens too late to prevent the message.

The message has appeared only recently with commit 3be5fa236649 (Revert
"iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices").
Prior to the commit, no driver could bind to AMD IOMMUs.

The reason for the message is that an MSI is requested early on for the
AMD IOMMU, which results in a call from msi_sysfs_create_group() to
devm_device_add_group().  A devres resource is thus attached to the
driver-less AMD IOMMU, which is normally not allowed, but presumably
cannot be avoided because requesting the MSI from a regular PCI driver
might be too late.

Avoid the message by once again checking for an AGP Capability *before*
binding to an unsupported device.  Achieve that by way of the PCI core's
dynid functionality.

pci_add_dynid() can fail only with -ENOMEM (on allocation failure) or
-EINVAL (on bus_to_subsys() failure).  It doesn't seem worth the extra
code to propagate those error codes out of the for_each_pci_dev() loop,
so simply error out with -ENODEV if there was no successful bind attempt.
In the -ENOMEM case, a splat is emitted anyway, and the -EINVAL case can
never happen because it requires failure of bus_register(&pci_bus_type),
in which case there's no driver probing of PCI devices.

Hans has voiced a preference to no longer probe unsupported devices by
default (i.e. set agp_try_unsupported = 0).  In fact, the help text for
CONFIG_AGP_AMD64 pretends this to be the default.  Alternatively, he
proposes probing only devices with PCI_CLASS_BRIDGE_HOST.  However these
approaches risk regressing users who depend on the existing behavior.

Fixes: 3be5fa236649 (Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices")
Reported-by: Fedor Pchelkin <pchelkin@ispras.ru>
Closes: https://lore.kernel.org/r/wpoivftgshz5b5aovxbkxl6ivvquinukqfvb5z6yi4mv7d25ew@edtzr2p74ckg/
Reported-by: Hans de Goede <hansg@kernel.org>
Closes: https://lore.kernel.org/r/20250625112411.4123-1-hansg@kernel.org/
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
Changes v1 -> v2:
 * Use pci_add_dynid() to bind only to devices with AGP Capability
   (based on a suggestion from Ben).
 * Rephrase commit message to hopefully explain the history more accurately.
   Explain why resources are attached to the driver-less AMD IOMMU
   (requested by Ben).
 * Acknowledge Hans as reporter.

 drivers/char/agp/amd64-agp.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index bf490967241a..2505df1f4e69 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -720,11 +720,6 @@ static const struct pci_device_id agp_amd64_pci_table[] = {
 
 MODULE_DEVICE_TABLE(pci, agp_amd64_pci_table);
 
-static const struct pci_device_id agp_amd64_pci_promisc_table[] = {
-	{ PCI_DEVICE_CLASS(0, 0) },
-	{ }
-};
-
 static DEFINE_SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, NULL, agp_amd64_resume);
 
 static struct pci_driver agp_amd64_pci_driver = {
@@ -739,6 +734,7 @@ static struct pci_driver agp_amd64_pci_driver = {
 /* Not static due to IOMMU code calling it early. */
 int __init agp_amd64_init(void)
 {
+	struct pci_dev *pdev = NULL;
 	int err = 0;
 
 	if (agp_off)
@@ -767,9 +763,13 @@ int __init agp_amd64_init(void)
 		}
 
 		/* Look for any AGP bridge */
-		agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table;
-		err = driver_attach(&agp_amd64_pci_driver.driver);
-		if (err == 0 && agp_bridges_found == 0) {
+		for_each_pci_dev(pdev)
+			if (pci_find_capability(pdev, PCI_CAP_ID_AGP))
+				pci_add_dynid(&agp_amd64_pci_driver,
+					      pdev->vendor, pdev->device,
+					      pdev->subsystem_vendor,
+					      pdev->subsystem_device, 0, 0, 0);
+		if (agp_bridges_found == 0) {
 			pci_unregister_driver(&agp_amd64_pci_driver);
 			err = -ENODEV;
 		}
-- 
2.47.2

Re: [PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Hans de Goede]

Hi Lukas,

On 2-Jul-25 5:15 PM, Lukas Wunner wrote:
> Since commit 172efbb40333 ("AGP: Try unsupported AGP chipsets on x86-64
> by default"), the AGP driver for AMD Opteron/Athlon64 CPUs has attempted
> to bind to any PCI device possessing an AGP Capability.
> 
> Commit 6fd024893911 ("amd64-agp: Probe unknown AGP devices the right
> way") subsequently reworked the driver to perform a bind attempt to
> any PCI device (regardless of AGP Capability) and reject a device in
> the driver's ->probe() hook if it lacks the AGP Capability.
> 
> On modern CPUs exposing an AMD IOMMU, this subtle change results in an
> annoying message with KERN_CRIT severity:
> 
>   pci 0000:00:00.2: Resources present before probing
> 
> The message is emitted by the driver core prior to invoking a driver's
> ->probe() hook.  The check for an AGP Capability in the ->probe() hook
> happens too late to prevent the message.
> 
> The message has appeared only recently with commit 3be5fa236649 (Revert
> "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices").
> Prior to the commit, no driver could bind to AMD IOMMUs.
> 
> The reason for the message is that an MSI is requested early on for the
> AMD IOMMU, which results in a call from msi_sysfs_create_group() to
> devm_device_add_group().  A devres resource is thus attached to the
> driver-less AMD IOMMU, which is normally not allowed, but presumably
> cannot be avoided because requesting the MSI from a regular PCI driver
> might be too late.
> 
> Avoid the message by once again checking for an AGP Capability *before*
> binding to an unsupported device.  Achieve that by way of the PCI core's
> dynid functionality.
> 
> pci_add_dynid() can fail only with -ENOMEM (on allocation failure) or
> -EINVAL (on bus_to_subsys() failure).  It doesn't seem worth the extra
> code to propagate those error codes out of the for_each_pci_dev() loop,
> so simply error out with -ENODEV if there was no successful bind attempt.
> In the -ENOMEM case, a splat is emitted anyway, and the -EINVAL case can
> never happen because it requires failure of bus_register(&pci_bus_type),
> in which case there's no driver probing of PCI devices.
> 
> Hans has voiced a preference to no longer probe unsupported devices by
> default (i.e. set agp_try_unsupported = 0).  In fact, the help text for
> CONFIG_AGP_AMD64 pretends this to be the default.  Alternatively, he
> proposes probing only devices with PCI_CLASS_BRIDGE_HOST.  However these
> approaches risk regressing users who depend on the existing behavior.
> 
> Fixes: 3be5fa236649 (Revert "iommu/amd: Prevent binding other PCI drivers to IOMMU PCI devices")
> Reported-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Closes: https://lore.kernel.org/r/wpoivftgshz5b5aovxbkxl6ivvquinukqfvb5z6yi4mv7d25ew@edtzr2p74ckg/
> Reported-by: Hans de Goede <hansg@kernel.org>
> Closes: https://lore.kernel.org/r/20250625112411.4123-1-hansg@kernel.org/
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
> Changes v1 -> v2:
>  * Use pci_add_dynid() to bind only to devices with AGP Capability
>    (based on a suggestion from Ben).
>  * Rephrase commit message to hopefully explain the history more accurately.
>    Explain why resources are attached to the driver-less AMD IOMMU
>    (requested by Ben).
>  * Acknowledge Hans as reporter.

Thank you for the new version.

I can confirm that this fixes the issue for me and the code also looks
good to me:

Tested-by: Hans de Goede <hansg@kernel.org>
Reviewed-by: Hans de Goede <hansg@kernel.org>

Regards,

Hans




>  drivers/char/agp/amd64-agp.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> index bf490967241a..2505df1f4e69 100644
> --- a/drivers/char/agp/amd64-agp.c
> +++ b/drivers/char/agp/amd64-agp.c
> @@ -720,11 +720,6 @@ static const struct pci_device_id agp_amd64_pci_table[] = {
>  
>  MODULE_DEVICE_TABLE(pci, agp_amd64_pci_table);
>  
> -static const struct pci_device_id agp_amd64_pci_promisc_table[] = {
> -	{ PCI_DEVICE_CLASS(0, 0) },
> -	{ }
> -};
> -
>  static DEFINE_SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, NULL, agp_amd64_resume);
>  
>  static struct pci_driver agp_amd64_pci_driver = {
> @@ -739,6 +734,7 @@ static struct pci_driver agp_amd64_pci_driver = {
>  /* Not static due to IOMMU code calling it early. */
>  int __init agp_amd64_init(void)
>  {
> +	struct pci_dev *pdev = NULL;
>  	int err = 0;
>  
>  	if (agp_off)
> @@ -767,9 +763,13 @@ int __init agp_amd64_init(void)
>  		}
>  
>  		/* Look for any AGP bridge */
> -		agp_amd64_pci_driver.id_table = agp_amd64_pci_promisc_table;
> -		err = driver_attach(&agp_amd64_pci_driver.driver);
> -		if (err == 0 && agp_bridges_found == 0) {
> +		for_each_pci_dev(pdev)
> +			if (pci_find_capability(pdev, PCI_CAP_ID_AGP))
> +				pci_add_dynid(&agp_amd64_pci_driver,
> +					      pdev->vendor, pdev->device,
> +					      pdev->subsystem_vendor,
> +					      pdev->subsystem_device, 0, 0, 0);
> +		if (agp_bridges_found == 0) {
>  			pci_unregister_driver(&agp_amd64_pci_driver);
>  			err = -ENODEV;
>  		}

Re: [PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Andi Kleen]

I suspect these days it would be also reasonable to drop it this old
hack.

If any of these old chipsets are still missing I would rather adds its
PCI-ID.

There will be certainly not any new unknown ones for these old CPUs.

Also there shouldn't be that many high speed devices that need the 
old 4GB IOMMU anyways, and for low speed ones it's fine to use swiotlb
instead.

-Andi

Re: [PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Hans de Goede]

Hi Andi,

On 3-Jul-25 21:29, Andi Kleen wrote:
> 
> I suspect these days it would be also reasonable to drop it this old
> hack.
> 
> If any of these old chipsets are still missing I would rather adds its
> PCI-ID.
> 
> There will be certainly not any new unknown ones for these old CPUs.

Right, I plan to submit a patch to disable the probing of unsupported
devices by default. I'll likely even do so today.

But that is not entirely without a risk of regressions and atm this
is causing a regression (breaks flicker free boot) in 6.16-rc# .

So I think we should move forward with Lukas' fix dor 6.16 and then
my patch to disable probing of unsupported devices by default can
be merged into linux-next .

Regards,

Hans

Re: [PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Lukas Wunner]

On Mon, Jul 07, 2025 at 02:53:32PM +0200, Hans de Goede wrote:
> So I think we should move forward with Lukas' fix dor 6.16 and then
> my patch to disable probing of unsupported devices by default can
> be merged into linux-next .

Sounds good to me.

Dave is out all week and has not commented on this matter at all so far:

https://lore.kernel.org/r/CAPM=9tzrmRS9++MP_Y4ab95W71UxjFLzTd176Mok7akwdT2q+w@mail.gmail.com/

I assume Bjorn may not be comfortable applying my patch without an ack
from Dave.  I am technically able to apply my own patch through drm-misc
and I believe Hans' Reviewed-by is sufficient to allow me to do that.

I'd feel more comfortable having additional acks or Reviewed-by's though.
I'm contemplating applying the patch to drm-misc by Wednesday evening,
that would allow it to land in Linus' tree before v6.16-rc6.

If anyone has objections, needs more time to review or wants to apply
the patch, please let me know.

Thanks,

Lukas

Re: [PATCH v2] agp/amd64: Check AGP Capability before binding to unsupported devices

[Bjorn Helgaas]

On Mon, Jul 07, 2025 at 03:58:30PM +0200, Lukas Wunner wrote:
> On Mon, Jul 07, 2025 at 02:53:32PM +0200, Hans de Goede wrote:
> > So I think we should move forward with Lukas' fix dor 6.16 and then
> > my patch to disable probing of unsupported devices by default can
> > be merged into linux-next .
> 
> Sounds good to me.
> 
> Dave is out all week and has not commented on this matter at all so far:
> 
> https://lore.kernel.org/r/CAPM=9tzrmRS9++MP_Y4ab95W71UxjFLzTd176Mok7akwdT2q+w@mail.gmail.com/
> 
> I assume Bjorn may not be comfortable applying my patch without an ack
> from Dave.  I am technically able to apply my own patch through drm-misc
> and I believe Hans' Reviewed-by is sufficient to allow me to do that.
> 
> I'd feel more comfortable having additional acks or Reviewed-by's though.
> I'm contemplating applying the patch to drm-misc by Wednesday evening,
> that would allow it to land in Linus' tree before v6.16-rc6.
> 
> If anyone has objections, needs more time to review or wants to apply
> the patch, please let me know.

Looks like this is now upstream:
https://git.kernel.org/linus/d88dfb756d55 ("agp/amd64: Check AGP Capability before binding to unsupported devices")

Seems OK to me, but I'm certainly not an AGP expert.

Bjorn