Messages in this thread Patch in this message | | | Subject | Re: [PATCH 09/11] PCI: add matching checks for driver_override binding | From | Max Gurtovoy <> | Date | Thu, 17 Jun 2021 02:28:36 +0300 |
| |
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.
But you're right for static mixed table (I assumed that this will never happen I guess).
If we put the any_id_override id before the non_override AMAZON device entry in the pci-pf-stub we'll fail with the matching to the AMAZON device.
What about the bellow untested addition (also remove condition c):
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c index 296de7bc9dc9..2d46f6cd96f7 100644 --- a/drivers/pci/pci-driver.c +++ b/drivers/pci/pci-driver.c @@ -136,7 +136,7 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv, struct pci_dev *dev) { struct pci_dynid *dynid; - const struct pci_device_id *found_id = NULL; + const struct pci_device_id *found_id = NULL, *ids;
/* When driver_override is set, only bind to the matching driver */ if (dev->driver_override && strcmp(dev->driver_override, drv->name)) @@ -155,8 +155,8 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv, if (found_id) return found_id;
- found_id = pci_match_id(drv->id_table, dev); - if (found_id) { + ids = drv->id_table; + while ((found_id = pci_match_id(ids, dev))) { /* * if we found id in the static table, we must fulfill the * matching flags (i.e. if PCI_ID_F_DRIVER_OVERRIDE flag is @@ -164,17 +164,19 @@ static const struct pci_device_id *pci_match_device(struct pci_driver *drv, */ bool is_driver_override = (found_id->flags & PCI_ID_F_DRIVER_OVERRIDE) != 0; - if ((is_driver_override && !dev->driver_override) || - (dev->driver_override && !is_driver_override)) - return NULL; - } else if (dev->driver_override) { - /* - * if we didn't find suitable id in the static table, - * driver_override will still , send a dummy id - */ - found_id = &pci_device_id_any; + if (is_driver_override && !dev->driver_override) + ids = found_id++; /* continue searching */ + else + break; }
+ /* + * if no static match, driver_override will always match, send a dummy + * id. + */ + if (!found_id && dev->driver_override) + found_id = &pci_device_id_any; + return found_id; }
diff --git a/drivers/pci/pci-pf-stub.c b/drivers/pci/pci-pf-stub.c index 45855a5e9fca..49544ba9a7af 100644 --- a/drivers/pci/pci-pf-stub.c +++ 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 } };
> > We have to fix this patch so flags isn't an anti-match to make it work > without user regression. > > Jason
| |