Re: [PATCH] agp/amd64: Bind to unsupported devices only if AGP is present

[Lukas Wunner]

On Tue, Jun 24, 2025 at 11:54:46PM +0200, Ben Hutchings wrote:
> On Sat, 2025-06-21 at 16:05 +0200, Lukas Wunner wrote:
> > So please propose a more accurate explanation.
> 
> Something like "The driver iterates over all PCI devices, checking for
> an AGP capability.  Since commit 6fd024893911 ("amd64-agp: Probe unknown
> AGP devices the right way") this is done with driver_attach() and a
> wildcard PCI ID table, and the preparation for probing the IOMMU device
> produces this error message."

Thanks, I will respin and amend the commit message.


> Thinking about this further:
> 
> - Why *does* the IOMMU device have resources assigned but no driver
>   bound?  Is that the real bug?

I don't really know, I was hoping the AMD IOMMU maintainers could
comment on that.


> - If not, and there's a general problem with this promiscuous probing,
>   would it make more sense to:
>   1. Restore the search for an AGP capability in agp_amd64_init().
>   2. If and only if an AGP device is found, poke the appropriate device
>      ID into agp_amd64_pci_promisc_table and then call driver_attach().
>   ?

I like the idea.  I've realized that we've got pci_add_dynid() for
just this sort of thing.  It avoids the need to poke device IDs
into an array at runtime.  The (as yet completely untested) patch
below should do the trick.

-- >8 --

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index bf49096..9b9c473 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -720,11 +720,6 @@ static int agp_amd64_resume(struct device *dev)
 
 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,7 +734,8 @@ static int agp_amd64_resume(struct device *dev)
 /* Not static due to IOMMU code calling it early. */
 int __init agp_amd64_init(void)
 {
-       int err = 0;
+       struct pci_dev *pdev = NULL;
+       int err, ret;
 
        if (agp_off)
                return -EINVAL;
@@ -767,8 +763,15 @@ 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);
+               for_each_pci_dev(pdev)
+                       if (pci_find_capability(pdev, PCI_CAP_ID_AGP)) {
+                               ret = pci_add_dynid(&agp_amd64_pci_driver,
+                                                   pdev->vendor, pdev->device,
+                                                   pdev->subvendor,
+                                                   pdev->subdevice, 0, 0, 0);
+                               if (ret)
+                                       err = ret;
+                       }
                if (err == 0 && agp_bridges_found == 0) {
                        pci_unregister_driver(&agp_amd64_pci_driver);
                        err = -ENODEV;