Re: git: 1bbdfb0b4386 - main - gve: Add PNP info to PCI attachment of gve(4) driver.

On 2023-06-05 10:04 PM, Jessica Clarke wrote:
> On 6 Jun 2023, at 05:31, Warner Losh <imp@bsdimp.com> wrote:
>>
>>
>>
>> On Mon, Jun 5, 2023, 10:15 PM Jessica Clarke <jrtc27@freebsd.org> wrote:
>> On 6 Jun 2023, at 05:06, Xin LI <delphij@FreeBSD.org> wrote:
>>>
>>> The branch main has been updated by delphij:
>>>
>>> URL: https://cgit.FreeBSD.org/src/commit/?id=1bbdfb0b438689a839e29094fcb582a104cbabd9
>>>
>>> commit 1bbdfb0b438689a839e29094fcb582a104cbabd9
>>> Author:     Xin LI <delphij@FreeBSD.org>
>>> AuthorDate: 2023-06-06 00:58:43 +0000
>>> Commit:     Xin LI <delphij@FreeBSD.org>
>>> CommitDate: 2023-06-06 04:05:55 +0000
>>>
>>>     gve: Add PNP info to PCI attachment of gve(4) driver.
>>>
>>>     Reviewed-by:            imp
>>>     Differential Revision:  https://reviews.freebsd.org/D40429
>>> ---
>>> sys/dev/gve/gve_main.c | 28 ++++++++++++++++++++++++----
>>> 1 file changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/sys/dev/gve/gve_main.c b/sys/dev/gve/gve_main.c
>>> index ae45a0cfc24a..383fd326d33a 100644
>>> --- a/sys/dev/gve/gve_main.c
>>> +++ b/sys/dev/gve/gve_main.c
>>> @@ -38,6 +38,16 @@
>>>
>>> #define GVE_DEFAULT_RX_COPYBREAK 256
>>>
>>> +/* Devices supported by this driver. */
>>> +static struct gve_dev {
>>> +        uint16_t vendor_id;
>>> +        uint16_t device_id;
>>> +        const char *name;
>>> +} gve_devs[] = {
>>> + { PCI_VENDOR_ID_GOOGLE, PCI_DEV_ID_GVNIC, "gVNIC" }
>>> +};
>>> +#define GVE_DEVS_COUNT nitems(gve_devs)
>>
>> IMO this obfuscates rather than helps given the loop iterates over
>> gve_devs (and the macro references it).
>>
>>> +
>>> struct sx gve_global_lock;
>>>
>>> static int
>>> @@ -701,10 +711,18 @@ gve_service_task(void *arg, int pending)
>>> static int
>>> gve_probe(device_t dev)
>>> {
>>> - if (pci_get_vendor(dev) == PCI_VENDOR_ID_GOOGLE &&
>>> -    pci_get_device(dev) == PCI_DEV_ID_GVNIC) {
>>> - device_set_desc(dev, "gVNIC");
>>> - return (BUS_PROBE_DEFAULT);
>>> + uint16_t deviceid, vendorid;
>>> + int i;
>>> +
>>> + vendorid = pci_get_vendor(dev);
>>> + deviceid = pci_get_device(dev);
>>> +
>>> + for (i = 0; i < GVE_DEVS_COUNT; i++) {
>>> + if (vendorid == gve_devs[i].vendor_id &&
>>> +    deviceid == gve_devs[i].device_id) {
>>> + device_set_desc(dev, gve_devs[i].name);
>>> + return (BUS_PROBE_DEFAULT);
>>> + }
>>> }
>>> return (ENXIO);
>>> }
>>> @@ -851,3 +869,5 @@ DRIVER_MODULE(gve, pci, gve_driver, gve_devclass, 0, 0);
>>> #else
>>> DRIVER_MODULE(gve, pci, gve_driver, 0, 0);
>>> #endif
>>> +MODULE_PNP_INFO("U16:vendor;U16:device", pci, gve, gve_devs,
>>
>> This is missing ;D:# so it will break as soon as you have a second
>> entry in the table.
>>
>> While the first bit is true, I though we encoded the per entry size so trailing garbage didn't matter.. d# will be more useful in the future maybe.
> 
> Hm indeed you’re right. Might as well include it still though.

I've created https://reviews.freebsd.org/D40430 in attempt to address 
both issues.  After the change, the 'gVNIC' would be in linker.hints 
(but it's not clear to me if the data is really useful for tools today).

I can split this into two changes if you like.

Cheers,