Re: [PATCH V4 10/13] PCI / VFIO: Add 'override_only' support for VFIO PCI sub system

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Jason



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux