Re: [RFC PATCH] amd64-agp: do not bind to pci driver if probing fails

[Lukas Wunner <lukas@wunner.de>]

On Sat, Jun 21, 2025 at 07:15:31PM +0300, Ahmed Salem wrote:
> On 25/06/21 11:46AM, Lukas Wunner wrote:
> > On Sat, Jun 21, 2025 at 04:55:52AM +0300, Ahmed Salem wrote:
> > > --- a/drivers/char/agp/amd64-agp.c
> > > +++ b/drivers/char/agp/amd64-agp.c
> > > @@ -768,10 +768,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);
> > > -		if (err == 0 && agp_bridges_found == 0) {
> > > +		if ((int *)agp_amd64_pci_driver.probe != 0) {
> > >  			pci_unregister_driver(&agp_amd64_pci_driver);
> > >  			err = -ENODEV;
> > > +		} else {
> > > +			err = driver_attach(&agp_amd64_pci_driver.driver);
> > > +			if (err == 0 && agp_bridges_found == 0) {
> > > +				pci_unregister_driver(&agp_amd64_pci_driver);
> > > +				err = -ENODEV;
> > > +			}
> > 
> > Is the "probe" member in agp_amd64_pci_driver overwritten with a
> > zero pointer anywhere?  I don't see that it is, so it seems the
> > else-branch is never entered.
> 
> That is a great question. I thought since pci_register_driver calls the
> probe function, it would return with or without a zero, saving that
> value in the .probe member.

You'd have to add parentheses and parameters, i.e.

  agp_amd64_pci_driver.probe(...)

to invoke the probe hook directly.  However, that wouldn't be an
acceptable approach, one needs to go through the API of the
driver core and not do things behind the driver core's back.


> > I had already prepared a fix for this, but waited for 0-day to
> > crunch through it.  I've just submitted it, so that's what I had
> > in mind:
> > 
> > https://lore.kernel.org/r/f8ff40f35a9a5836d1371f60e85c09c5735e3c5e.1750497201.git.lukas@wunner.de/
> 
> That one I've seen even prior to catching this one, and this is
> originally what I had in mind based on what commit 6fd024893911
> ("amd64-agp: Probe unknown AGP devices the right way") removed (i.e.
> !pci_find_capability) when you suggested checking for caps beforehand,
> but I figured "why make other calls when .probe already does it right
> off the bat?"

Right, it is somewhat silly, but this driver is for 20+ year old hardware
which is likely no longer in heavy use, the driver itself isn't actively
maintained anymore and might be dropped in a few years, so this approach
seemed like the least ugly and most acceptable option.

The real crime was to probe *any* PCI device and even make that the
default.  I think vfio_pci is probably the only other driver that
probes *any* PCI device and it does that only if requested by user
space I believe.  We'd risk regressing users if we changed the
"probe everything by default" behavior, so that's not a good option.

Thanks,

Lukas