Re: [PATCH v6 07/12] PCI: Refactor matching logic for pci_dev_acs_ops

[David Matlack <dmatlack@google.com>]

On 2026-06-07 07:01 PM, Pranjal Shrivastava wrote:
> On Fri, May 22, 2026 at 08:24:05PM +0000, David Matlack wrote:
> > Refactor the logic to match devices to pci_dev_acs_ops by factoring out
> > the loop and device matching into its own routine. This eliminates some
> > duplicate code between pci_dev_specific_enable_acs() and
> > pci_dev_specific_disable_acs_redir(), and will also be used in a
> > subsequent commit to check if a device requires device-specific
> > enable_acs() during a Live Update.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: David Matlack <dmatlack@google.com>
> > ---
> >  drivers/pci/quirks.c | 50 ++++++++++++++++++--------------------------
> >  1 file changed, 20 insertions(+), 30 deletions(-)
> > 
> 
> [...]
> 
> >  } pci_dev_acs_ops[] = {
> >  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
> > +	    .match = pci_quirk_intel_pch_acs_match,
> >  	    .enable_acs = pci_quirk_enable_intel_pch_acs,
> >  	},
> >  	{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
> > +	    .match = pci_quirk_intel_spt_pch_acs_match,
> >  	    .enable_acs = pci_quirk_enable_intel_spt_pch_acs,
> >  	    .disable_acs_redir = pci_quirk_disable_intel_spt_pch_acs_redir,
> >  	},
> >  };
> >  
> > -int pci_dev_specific_enable_acs(struct pci_dev *dev)
> > +static const struct pci_dev_acs_ops *pci_dev_acs_ops_get(struct pci_dev *dev)
> >  {
> >  	const struct pci_dev_acs_ops *p;
> > -	int i, ret;
> > +	int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(pci_dev_acs_ops); i++) {
> >  		p = &pci_dev_acs_ops[i];
> > @@ -5481,33 +5475,29 @@ int pci_dev_specific_enable_acs(struct pci_dev *dev)
> >  		     p->vendor == (u16)PCI_ANY_ID) &&
> >  		    (p->device == dev->device ||
> >  		     p->device == (u16)PCI_ANY_ID) &&
> > -		    p->enable_acs) {
> > -			ret = p->enable_acs(dev);
> > -			if (ret >= 0)
> > -				return ret;
> > -		}
> > +		    p->match(dev))
> > +			return p;
> 
> Nit:
> Should we check if (p->match != NULL) like we check for p->enable_acs &
> p->disable_acs_redir(). 
> 
> Otherwise, it seems like we're mandating the existence of a match op in
> the pci_dev_acs_ops here? Today, we just have two Intel entries in that
> array, both of which need the match op. However, AFAICT, it shouldn't be
> mandatory for future SoCs that might only need a simple vid + devid match

*shrug*

I would usually say those future SoCs should be the ones to make it
optional if and when they need to.

But making p->matc optional now isn't so bad:

        for (i = 0; i < ARRAY_SIZE(pci_dev_acs_ops); i++) {
                p = &pci_dev_acs_ops[i];
                if ((p->vendor == dev->vendor ||
                     p->vendor == (u16)PCI_ANY_ID) &&
                    (p->device == dev->device ||
-                    p->device == (u16)PCI_ANY_ID) &&
-                   p->enable_acs) {
-                       ret = p->enable_acs(dev);
-                       if (ret >= 0)
-                               return ret;
+                    p->device == (u16)PCI_ANY_ID)) {
+                       if (!p->match || p->match(dev))
+                               return p;
                }
        }

I can include this in v7 if you would like.


> 
> [...]
> 
> with that nit:
> Reviewed-by: Pranjal Shrivastava <praan@google.com>
> 
> Thanks,
> Praan