Date: Mon, 5 Jun 2023 22:31:56 -0600 From: Warner Losh <imp@bsdimp.com> To: Jessica Clarke <jrtc27@freebsd.org> Cc: Xin LI <delphij@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, "<dev-commits-src-main@freebsd.org>" <dev-commits-src-main@freebsd.org> Subject: Re: git: 1bbdfb0b4386 - main - gve: Add PNP info to PCI attachment of gve(4) driver. Message-ID: <CANCZdfpKY4OYKcuyEfD0bJ06NzaTj6PKmyWBm=uSt6vfy1qzqA@mail.gmail.com> In-Reply-To: <6A5F859A-033A-43E1-81CB-B364662DE2F3@freebsd.org> References: <202306060406.35646hbA078041@gitrepo.freebsd.org> <6A5F859A-033A-43E1-81CB-B364662DE2F3@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--00000000000083cf7805fd6e8084 Content-Type: text/plain; charset="UTF-8" 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. Warner Jess > > > + GVE_DEVS_COUNT); > --00000000000083cf7805fd6e8084 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"auto"><div><br><br><div class=3D"gmail_quote"><div dir=3D"ltr" = class=3D"gmail_attr">On Mon, Jun 5, 2023, 10:15 PM Jessica Clarke <<a hr= ef=3D"mailto:jrtc27@freebsd.org">jrtc27@freebsd.org</a>> wrote:<br></div= ><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1= px #ccc solid;padding-left:1ex">On 6 Jun 2023, at 05:06, Xin LI <delphij= @FreeBSD.org> wrote:<br> > <br> > The branch main has been updated by delphij:<br> > <br> > URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D1bbdfb0b4386= 89a839e29094fcb582a104cbabd9" rel=3D"noreferrer noreferrer" target=3D"_blan= k">https://cgit.FreeBSD.org/src/commit/?id=3D1bbdfb0b438689a839e29094fcb582= a104cbabd9</a><br> > <br> > commit 1bbdfb0b438689a839e29094fcb582a104cbabd9<br> > Author:=C2=A0 =C2=A0 =C2=A0Xin LI <delphij@FreeBSD.org><br> > AuthorDate: 2023-06-06 00:58:43 +0000<br> > Commit:=C2=A0 =C2=A0 =C2=A0Xin LI <delphij@FreeBSD.org><br> > CommitDate: 2023-06-06 04:05:55 +0000<br> > <br> >=C2=A0 =C2=A0 gve: Add PNP info to PCI attachment of gve(4) driver.<br> > <br> >=C2=A0 =C2=A0 Reviewed-by:=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 imp= <br> >=C2=A0 =C2=A0 Differential Revision:=C2=A0 <a href=3D"https://reviews.f= reebsd.org/D40429" rel=3D"noreferrer noreferrer" target=3D"_blank">https://= reviews.freebsd.org/D40429</a><br> > ---<br> > sys/dev/gve/gve_main.c | 28 ++++++++++++++++++++++++----<br> > 1 file changed, 24 insertions(+), 4 deletions(-)<br> > <br> > diff --git a/sys/dev/gve/gve_main.c b/sys/dev/gve/gve_main.c<br> > index ae45a0cfc24a..383fd326d33a 100644<br> > --- a/sys/dev/gve/gve_main.c<br> > +++ b/sys/dev/gve/gve_main.c<br> > @@ -38,6 +38,16 @@<br> > <br> > #define GVE_DEFAULT_RX_COPYBREAK 256<br> > <br> > +/* Devices supported by this driver. */<br> > +static struct gve_dev {<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint16_t vendor_id;<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 uint16_t device_id;<br> > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 const char *name;<br> > +} gve_devs[] =3D {<br> > + { PCI_VENDOR_ID_GOOGLE, PCI_DEV_ID_GVNIC, "gVNIC" }<br> > +};<br> > +#define GVE_DEVS_COUNT nitems(gve_devs)<br> <br> IMO this obfuscates rather than helps given the loop iterates over<br> gve_devs (and the macro references it).<br> <br> > +<br> > struct sx gve_global_lock;<br> > <br> > static int<br> > @@ -701,10 +711,18 @@ gve_service_task(void *arg, int pending)<br> > static int<br> > gve_probe(device_t dev)<br> > {<br> > - if (pci_get_vendor(dev) =3D=3D PCI_VENDOR_ID_GOOGLE &&<br> > -=C2=A0 =C2=A0 pci_get_device(dev) =3D=3D PCI_DEV_ID_GVNIC) {<br> > - device_set_desc(dev, "gVNIC");<br> > - return (BUS_PROBE_DEFAULT);<br> > + uint16_t deviceid, vendorid;<br> > + int i;<br> > +<br> > + vendorid =3D pci_get_vendor(dev);<br> > + deviceid =3D pci_get_device(dev);<br> > +<br> > + for (i =3D 0; i < GVE_DEVS_COUNT; i++) {<br> > + if (vendorid =3D=3D gve_devs[i].vendor_id &&<br> > +=C2=A0 =C2=A0 deviceid =3D=3D gve_devs[i].device_id) {<br> > + device_set_desc(dev, gve_devs[i].name);<br> > + return (BUS_PROBE_DEFAULT);<br> > + }<br> > }<br> > return (ENXIO);<br> > }<br> > @@ -851,3 +869,5 @@ DRIVER_MODULE(gve, pci, gve_driver, gve_devclass, = 0, 0);<br> > #else<br> > DRIVER_MODULE(gve, pci, gve_driver, 0, 0);<br> > #endif<br> > +MODULE_PNP_INFO("U16:vendor;U16:device", pci, gve, gve_devs= ,<br> <br> This is missing ;D:# so it will break as soon as you have a second<br> entry in the table.<br></blockquote></div></div><div dir=3D"auto"><br></div= ><div dir=3D"auto">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.</div><div dir=3D"auto"><br></div><div dir=3D"auto">War= ner</div><div dir=3D"auto"><br></div><div dir=3D"auto"><div class=3D"gmail_= quote"><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-= left:1px #ccc solid;padding-left:1ex"> Jess<br> <br> > +=C2=A0 =C2=A0 GVE_DEVS_COUNT);<br> </blockquote></div></div></div> --00000000000083cf7805fd6e8084--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpKY4OYKcuyEfD0bJ06NzaTj6PKmyWBm=uSt6vfy1qzqA>