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@xxxxxxxxxx> 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
