[PATCH 0/3] agp/amd64: Remove support for probing unlisted PCI devices

[PATCH 0/3] agp/amd64: Remove support for probing unlisted PCI devices

[Hans de Goede]

Hi all,

As discussed in this thread:

https://lore.kernel.org/dri-devel/b29e7fbfc6d146f947603d0ebaef44cbd2f0d754.1751468802.git.lukas@wunner.de/

The amd64_agp driver's support for trying to probe unsupported devices does
not make sense anymore given that no new AGP devices have been produced for
a while now.

This patch series disabled the probing of unsupported devices in 2 steps
(for easier reverting and/or for applying one step at a time) first default
it to off and then completely remove it.

The first patch is a resend of Lukas' patch for 6.16-rc# to fix
the bootsplash regression caused by the probing of unsupported devices

I've included this patch so that the series applies cleanly, this means
that drm-misc-fixes (once it lands there) will needs to be backmerged
for the last patch to apply.

Regards,

Hans


Hans de Goede (2):
  agp/amd64: Change agp_try_unsupported default to 0
  agp/amd64: Remove support for probing unlisted PCI devices

Lukas Wunner (1):
  agp/amd64: Check AGP Capability before binding to unsupported devices

 drivers/char/agp/Kconfig     |  3 +--
 drivers/char/agp/agp.h       |  1 -
 drivers/char/agp/amd64-agp.c | 41 +-----------------------------------
 drivers/char/agp/backend.c   |  4 ----
 4 files changed, 2 insertions(+), 47 deletions(-)

-- 
2.49.0

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

[Hans de Goede]

From: Lukas Wunner <lukas@wunner.de>

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>
Tested-by: Hans de Goede <hansg@kernel.org>
Reviewed-by: Hans de Goede <hansg@kernel.org>
Link: https://lore.kernel.org/r/b29e7fbfc6d146f947603d0ebaef44cbd2f0d754.1751468802.git.lukas@wunner.de
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 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.49.0

[PATCH 2/3] agp/amd64: Change agp_try_unsupported default to 0

[Hans de Goede]

AMD64 boards with AGP support are so old that the agp_amd64_pci_table
should be complete and there is no need to probe unlisted PCI devices
by default.

This also brings the driver inline with the global agp=try_unsupported
(agp_try_unsupported_boot) parameter which also default to 0 and brings
things inline with the Kconfig which says that agp_try_unsupported=1
should be passed to try unsupported bridges.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/char/agp/amd64-agp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index 2505df1f4e69..f883c06b538a 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -34,7 +34,7 @@
 #define ULI_X86_64_ENU_SCR_REG		0x54
 
 static struct resource *aperture_resource;
-static bool __initdata agp_try_unsupported = 1;
+static bool agp_try_unsupported __initdata;
 static int agp_bridges_found;
 
 static void amd64_tlbflush(struct agp_memory *temp)
-- 
2.49.0

[PATCH 3/3] agp/amd64: Remove support for probing unlisted PCI devices

[Hans de Goede]

AMD64 boards with AGP support are so old that the agp_amd64_pci_table
should be complete and there is no need to probe unlisted PCI devices,
so lets completely remove support for probing unlisted PCI devices.

Suggested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Hans de Goede <hansg@kernel.org>
---
 drivers/char/agp/Kconfig     |  3 +--
 drivers/char/agp/agp.h       |  1 -
 drivers/char/agp/amd64-agp.c | 41 +-----------------------------------
 drivers/char/agp/backend.c   |  4 ----
 4 files changed, 2 insertions(+), 47 deletions(-)

diff --git a/drivers/char/agp/Kconfig b/drivers/char/agp/Kconfig
index c47eb7bf06d4..752b18901613 100644
--- a/drivers/char/agp/Kconfig
+++ b/drivers/char/agp/Kconfig
@@ -63,8 +63,7 @@ config AGP_AMD64
 	  This option gives you AGP support for the GLX component of
 	  X using the on-CPU northbridge of the AMD Athlon64/Opteron CPUs.
 	  You still need an external AGP bridge like the AMD 8151, VIA
-	  K8T400M, SiS755. It may also support other AGP bridges when loaded
-	  with agp_try_unsupported=1.
+	  K8T400M, SiS755.
 
 config AGP_INTEL
 	tristate "Intel 440LX/BX/GX, I8xx and E7x05 chipset support"
diff --git a/drivers/char/agp/agp.h b/drivers/char/agp/agp.h
index 67d7be800a7c..9fbe100c8f5e 100644
--- a/drivers/char/agp/agp.h
+++ b/drivers/char/agp/agp.h
@@ -237,7 +237,6 @@ void agp3_generic_cleanup(void);
 extern const struct aper_size_info_16 agp3_generic_sizes[];
 
 extern int agp_off;
-extern int agp_try_unsupported_boot;
 
 long compat_agp_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 
diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index f883c06b538a..e63827724bb1 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -34,7 +34,6 @@
 #define ULI_X86_64_ENU_SCR_REG		0x54
 
 static struct resource *aperture_resource;
-static bool agp_try_unsupported __initdata;
 static int agp_bridges_found;
 
 static void amd64_tlbflush(struct agp_memory *temp)
@@ -734,47 +733,10 @@ 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)
 		return -EINVAL;
 
-	err = pci_register_driver(&agp_amd64_pci_driver);
-	if (err < 0)
-		return err;
-
-	if (agp_bridges_found == 0) {
-		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
-			printk(KERN_INFO PFX "No supported AGP bridge found.\n");
-#ifdef MODULE
-			printk(KERN_INFO PFX "You can try agp_try_unsupported=1\n");
-#else
-			printk(KERN_INFO PFX "You can boot with agp=try_unsupported\n");
-#endif
-			pci_unregister_driver(&agp_amd64_pci_driver);
-			return -ENODEV;
-		}
-
-		/* First check that we have at least one AMD64 NB */
-		if (!amd_nb_num()) {
-			pci_unregister_driver(&agp_amd64_pci_driver);
-			return -ENODEV;
-		}
-
-		/* Look for any AGP bridge */
-		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;
-		}
-	}
-	return err;
+	return pci_register_driver(&agp_amd64_pci_driver);
 }
 
 static int __init agp_amd64_mod_init(void)
@@ -801,6 +763,5 @@ module_init(agp_amd64_mod_init);
 module_exit(agp_amd64_cleanup);
 
 MODULE_AUTHOR("Dave Jones, Andi Kleen");
-module_param(agp_try_unsupported, bool, 0);
 MODULE_DESCRIPTION("GART driver for the AMD Opteron/Athlon64 on-CPU northbridge");
 MODULE_LICENSE("GPL");
diff --git a/drivers/char/agp/backend.c b/drivers/char/agp/backend.c
index 1776afd3ee07..ca9c3472d4b7 100644
--- a/drivers/char/agp/backend.c
+++ b/drivers/char/agp/backend.c
@@ -319,9 +319,7 @@ void agp_remove_bridge(struct agp_bridge_data *bridge)
 EXPORT_SYMBOL_GPL(agp_remove_bridge);
 
 int agp_off;
-int agp_try_unsupported_boot;
 EXPORT_SYMBOL(agp_off);
-EXPORT_SYMBOL(agp_try_unsupported_boot);
 
 static int __init agp_init(void)
 {
@@ -340,8 +338,6 @@ static __init int agp_setup(char *s)
 {
 	if (!strcmp(s,"off"))
 		agp_off = 1;
-	if (!strcmp(s,"try_unsupported"))
-		agp_try_unsupported_boot = 1;
 	return 1;
 }
 __setup("agp=", agp_setup);
-- 
2.49.0

Re: [PATCH 3/3] agp/amd64: Remove support for probing unlisted PCI devices

[Lukas Wunner]

On Mon, Jul 07, 2025 at 07:37:10PM +0200, Hans de Goede wrote:
> AMD64 boards with AGP support are so old that the agp_amd64_pci_table
> should be complete and there is no need to probe unlisted PCI devices,
> so lets completely remove support for probing unlisted PCI devices.
[...]
> --- a/drivers/char/agp/amd64-agp.c
> +++ b/drivers/char/agp/amd64-agp.c
> @@ -734,47 +733,10 @@ 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)
>  		return -EINVAL;
>  
> -	err = pci_register_driver(&agp_amd64_pci_driver);
> -	if (err < 0)
> -		return err;
> -
> -	if (agp_bridges_found == 0) {
> -		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
> -			printk(KERN_INFO PFX "No supported AGP bridge found.\n");
> -#ifdef MODULE
> -			printk(KERN_INFO PFX "You can try agp_try_unsupported=1\n");
> -#else
> -			printk(KERN_INFO PFX "You can boot with agp=try_unsupported\n");
> -#endif
> -			pci_unregister_driver(&agp_amd64_pci_driver);
> -			return -ENODEV;
> -		}
> -
> -		/* First check that we have at least one AMD64 NB */
> -		if (!amd_nb_num()) {
> -			pci_unregister_driver(&agp_amd64_pci_driver);
> -			return -ENODEV;
> -		}

Here the PCI driver used to be unregistered if no AGP bridges were found
and successfully probed...

> -
> -		/* Look for any AGP bridge */
> -		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;
> -		}
> -	}
> -	return err;
> +	return pci_register_driver(&agp_amd64_pci_driver);
>  }

... and now the PCI driver will be kept around.  Is that intentional?

Keeping the PCI driver around only makes sense if an AGP bridge is
later on hot-plugged.  I guess that never happens with AGP bridges,
at least it's not supported by this driver so far.

Thanks,

Lukas

Re: [PATCH 0/3] agp/amd64: Remove support for probing unlisted PCI devices

[Andi Kleen]

On Mon, Jul 07, 2025 at 07:37:07PM +0200, Hans de Goede wrote:
> Hi all,
> 
> As discussed in this thread:
> 
> https://lore.kernel.org/dri-devel/b29e7fbfc6d146f947603d0ebaef44cbd2f0d754.1751468802.git.lukas@wunner.de/
> 
> The amd64_agp driver's support for trying to probe unsupported devices does
> not make sense anymore given that no new AGP devices have been produced for
> a while now.

It's not about AGP devices. By the time the K8 existed it was already
PCI only.

The K8 CPU integrated northbridge uses PCI AGP enumeration to expose its
integrated IOMMU, but it's not real AGP, just uses a mechanism similar
to the AGP aperture.

But you still have to probe the chipset hostbridge, and for these CPUs there
were multiple third party chipsets with different PCI IDs, so it needed
an own ID for each of them.

The reason why it's not needed is that there are no new third party K8
chipsets anymore.


-Andi