On 25/06/21 09:21PM, Lukas Wunner wrote:
> 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.
>
Noted!
>
> > > 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.lu...@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.
>
Gotcha..That clarifies a whole lot, thank you so much!
--
Best regards,
Ahmed Salem <x0rw...@gmail.com>
> Thanks,
>
> Lukas
