On Wed, 25 Aug 2021 19:24:43 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> On Wed, Aug 25, 2021 at 04:05:46PM -0600, Alex Williamson wrote:
> > On Wed, 25 Aug 2021 16:51:36 +0300
> > Yishai Hadas <yishaih@xxxxxxxxxx> wrote:
> > > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > > index 7c97fa8e36bc..c3edbf73157e 100644
> > > +++ b/scripts/mod/file2alias.c
> > > @@ -426,7 +426,7 @@ static int do_ieee1394_entry(const char *filename,
> > > return 1;
> > > }
> > >
> > > -/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
> > > +/* Looks like: pci:vNdNsvNsdNbcNscNiN or <prefix>_pci:vNdNsvNsdNbcNscNiN. */
> > > static int do_pci_entry(const char *filename,
> > > void *symval, char *alias)
> > > {
> > > @@ -440,8 +440,12 @@ static int do_pci_entry(const char *filename,
> > > DEF_FIELD(symval, pci_device_id, subdevice);
> > > DEF_FIELD(symval, pci_device_id, class);
> > > DEF_FIELD(symval, pci_device_id, class_mask);
> > > + DEF_FIELD(symval, pci_device_id, override_only);
> > >
> > > - strcpy(alias, "pci:");
> > > + if (override_only & PCI_ID_F_VFIO_DRIVER_OVERRIDE)
> > > + strcpy(alias, "vfio_pci:");
> > > + else
> > > + strcpy(alias, "pci:");
> >
> > I'm a little concerned that we're allowing unknown, non-zero
> > override_only values to fall through to create "pci:" alias matches.
> > Should this be something like:
> >
> > if (override_only & PCI_ID_F_VFIO_DRIVER_OVERRIDE) {
>
> Should probably be == not &, since in this new arrangement it is
> really more of an enum than a bit flags... A switch would be OK here
> too
>
> > strcpy(alias, "vfio_pci:");
> > } else if (override_only) {
> > warn("Unknown PCI driver_override alias %08X\n",
> > driver_override);
> > return 0;
> > } else {
> > strcpy(alias, "pci:");
> > }
>
> It seems reasonable to me to throw a warn, it signals to a future
> developer that kbuild is not working right.
>
> > And then if we can only have a single bit set in override_only (I
> > can't think of a use case for a single entry to have multiple
> > override options), should PCI_DEVICE_DRIVER_OVERRIDE() be defined to
> > take a "driver_override_shift" value where .driver_override is assigned
> > (1 << driver_override_shift)? That would encode the semantics in the
> > prototypes a little better.
>
> I think it is just an enum of overrides, no reason to make it one hot
> encoded. Previously when it was flags the bit encode had a certain
> amount of logic, but no longer.
Yeah, a switch statement handling pci:/vfio_pci:/warn rather than
testing bits would clear things up for me. Thanks,
Alex
