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

[Hans de Goede <hansg@kernel.org>]

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;
>  		}