Messages in this thread | | | Subject | Re: [PATCH 09/11] PCI: add matching checks for driver_override binding | From | Max Gurtovoy <> | Date | Thu, 17 Jun 2021 02:42:46 +0300 |
| |
On 6/17/2021 2:33 AM, Jason Gunthorpe wrote: > On Thu, Jun 17, 2021 at 02:28:36AM +0300, Max Gurtovoy wrote: >> On 6/16/2021 3:34 AM, Jason Gunthorpe wrote: >>> On Tue, Jun 15, 2021 at 06:22:45PM -0600, Alex Williamson wrote: >>>> On Tue, 15 Jun 2021 20:32:57 -0300 >>>> Jason Gunthorpe <jgg@nvidia.com> wrote: >>>> >>>>> On Tue, Jun 15, 2021 at 05:22:42PM -0600, Alex Williamson wrote: >>>>> >>>>>>>> b) alone is a functional, runtime difference. >>>>>>> I would state b) differently: >>>>>>> >>>>>>> b) Ignore the driver-override-only match entries in the ID table. >>>>>> No, pci_match_device() returns NULL if a match is found that is marked >>>>>> driver-override-only and a driver_override is not specified. That's >>>>>> the same as no match at all. We don't then go on to search past that >>>>>> match in the table, we fail to bind the driver. That's effectively an >>>>>> anti-match when there's no driver_override on the device. >>>>> anti-match isn't the intention. The deployment will have match tables >>>>> where all entires are either flags=0 or are driver-override-only. >>>> I'd expect pci-pf-stub to have one of each, an any-id with >>>> override-only flag and the one device ID currently in the table with >>>> no flag. >>> Oh Hum. Actually I think this shows the anti-match behavior is >>> actually a bug.. :( >>> >>> For something like pci_pf_stub_whitelist, if we add a >>> driver_override-only using the PCI any id then it effectively disables >>> new_id completely because the match search will alway find the >>> driver_override match first and stop searching. There is no chance to >>> see things new_id adds. >> Actually the dynamic table is the first table the driver search. So new_id >> works exactly the same AFAIU. > Oh, even better, so it isn't really an issue > >> But you're right for static mixed table (I assumed that this will never >> happen I guess). > Me too, we could organize the driver-overrides to be last
Yes we could, but in 2 years from now I'll forget this rule :)
And others may not be aware of it.
> >> - found_id = pci_match_id(drv->id_table, dev); >> - if (found_id) { >> + ids = drv->id_table; >> + while ((found_id = pci_match_id(ids, dev))) { > Yeah, keep searching makes logical sense to me > >> diff --git a/drivers/pci/pci-pf-stub.c b/drivers/pci/pci-pf-stub.c >> index 45855a5e9fca..49544ba9a7af 100644 >> +++ b/drivers/pci/pci-pf-stub.c >> @@ -19,6 +19,7 @@ >> */ >> static const struct pci_device_id pci_pf_stub_whitelist[] = { >> { PCI_VDEVICE(AMAZON, 0x0053) }, >> + { PCI_DEVICE_FLAGS(PCI_ANY_ID, PCI_ANY_ID, >> PCI_ID_F_STUB_DRIVER_OVERRIDE) }, /* match all by default (override) */ >> /* required last entry */ >> { 0 } > And we don't really want this change any more right? No reason to put > pci_stub in the module.alias file?
I actually did it in the patches I attached earlier.
It will look like:
stub_pci:v*d*sv*sd*bc*sc*i*
pci:v00001D0Fd00000053sv*sd*bc*sc*i*
I think it's good practice to avoid matching automatically and auto loading any_id_override and vfio_override drivers in general.
Do you see a reason not adding this alias for stub drivers but adding it to vfio_pci drivers ?
> > Thanks, > Jason
| |