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

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.

Jess

> Warner
> 
> Jess
> 
> > +    GVE_DEVS_COUNT);