Re: [PATCH V2 09/12] PCI: Add 'override_only' bitmap to struct pci_device_id

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Aug 19, 2021 at 07:16:20PM +0300, Yishai Hadas wrote:
> On 8/19/2021 6:15 PM, Bjorn Helgaas wrote:
> > On Wed, Aug 18, 2021 at 06:16:03PM +0300, Yishai Hadas wrote:
> > > From: Max Gurtovoy <mgurtovoy@nvidia.com>

> > >   /**
> > >    * struct pci_device_id - PCI device ID structure
> > >    * @vendor:		Vendor ID to match (or PCI_ANY_ID)
> > > @@ -34,12 +38,14 @@ typedef unsigned long kernel_ulong_t;
> > >    *			Best practice is to use driver_data as an index
> > >    *			into a static list of equivalent device types,
> > >    *			instead of using it as a pointer.
> > > + * @override_only:	Bitmap for override_only PCI drivers.
> > "Match only when dev->driver_override is this driver"?
> 
> Just to be aligned here,
> 
> This field will stay __u32 and may hold at the most 1 bit value set to
> represent the actual subsystem/driver.

The PCI core does not require "at most 1 bit is set."

Actually, I don't think even the file2alias code requires that.  If
you set two bits, you can generate two aliases.

> This is required to later on set the correct prefix in the modules.alias
> file, and you just suggested to change the comment as of above, right ?

Yes, __u32 is fine and I'm only suggesting a comment change here.

> > As far as PCI core is concerned there's no need for this to be a
> > bitmap.
> > 
> > I think this would make more sense if split into two patches.  The
> > first would add override_only and change pci_match_device().  Then
> > there's no confusion about whether this is specific to VFIO.
> 
> Splitting may end-up the first patch with a dead-code on below, as
> found_id->override_only will be always 0.
> 
> If you still believe that this is better we can do it.

I think it's fine to add the functionality in one patch and use it in
the next if it makes the commit clearer.  I wouldn't want to add
functionality that's not used at all in the series, but it's OK when
they're both posted together.

> if (found_id->override_only) {
>     if (dev->driver_override)
>       return found_id;
>   } else
>     return found_id;
> 
> > The second can add PCI_ID_F_VFIO_DRIVER_OVERRIDE and make the
> > file2alias.c changes.  Most of the commit log applies to this part.