Page MenuHomeFreeBSD

iflib: Fix PNP matching
AbandonedPublic

Authored by mindal_semihalf.com on Jul 29 2021, 12:28 PM.

Details

Reviewers
cem
mw
wma
shurd
imp
Group Reviewers
iflib
Summary

Change the wildcard value of not significant fields from 0 to (uint32_t)-1.
Also change their type to "V", this way the -1 wildcard value will match to anything read from device.
devmatch(8) tries to match all integers, even those without a name.
If the corresponding value was not found in device the value of -1 is used for comparison.
This fixed autoloading of if_em.ko on my i350 NIC.

Without this patch devmatch -uv produced the following:

---------- Entry 22 ----------
Matching vendor;I:device;I:#;I:#;I:#;I:#;D:#; (I) table=0x8086 tomatch=0x8086
Matching device;I:#;I:#;I:#;I:#;D:#; (I) table=0x1521 tomatch=0x1521
Matching #;I:#;I:#;I:#;D:#; (I) table=0xffffffff tomatch=0
Matching #;I:#;I:#;D:#; (I) table=0xffffffff tomatch=0
Matching #;I:#;D:#; (I) table=0xffffffff tomatch=0
Matching #;D:#; (I) table=0xffffffff tomatch=0

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Hi,

Any thoughts on this?
Sorry to bother you, but if I understand phabricator correctly this revision needs to be ACKed by someone from the iflib group before it can be committed.

imp requested changes to this revision.Aug 9 2021, 7:59 PM
imp added a subscriber: imp.

Any thoughts on this?
Sorry to bother you, but if I understand phabricator correctly this revision needs to be ACKed by someone from the iflib group before it can be committed.

I don't think this is quite right. Parts of it likely are an improvement, but I think more of an improvement can be had by the pci_device_table suggestion I made.

If devmatch isn't ignoring '#' in its decision making processes, it should be fixed. A quick examination of the code suggests the following fix: https://reviews.freebsd.org/D31482

sys/net/iflib.c
4559

I'll note in passing that pci_vendor_info should be converted to use the common pci_device_table
which is used most other places and could mean that this routine could largely be a wrapper for it.

4570

Anyting that uses PVID_OEM breaks here, since existing code passes '0' in. The 'ice' driver is the only one that popped up on grep.

The pci_device_table stuff solves this problem by making each entry have a bitmask of fields that are populated with live numbers and giving a number of marcos to build up that table, which would fit well with the PVID and PVID_OEM macaros already provided. I'd be tempted to ditch the latter and/or make it more general, but changing the interface
may have compat issus that would be better served with a new macro.

sys/net/iflib.h
178

Why'd you need to change these? # should have skipped them... If devmatch is looking for *THAT* as a field, that's a bug in devmatch which should be fixed. I don't want to play whack-a-mole all over the tree for this.

Also, the revid thing breaks ice I think, since it uses '0' for the revision which used to mean wildcard.

This revision now requires changes to proceed.Aug 9 2021, 7:59 PM

You're right, your approach is much better. I tested D31482 and it fixes the issue.