Re: [PATCH] PCI: add helper function to find DVSEC

[Dave Jiang <dave.jiang@xxxxxxxxx>]

On 10/30/2020 10:35 PM, Randy Dunlap wrote:
> On 10/30/20 3:42 PM, Dave Jiang wrote:
> > Add function that searches for DVSEC and returns the offset in PCI
> > configuration space for the interested DVSEC capability.
> >
> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > Reviewed-by: Ashok Raj <ashok.raj@xxxxxxxxx>
> > ---
> >
> > The patch has dependency on David Box’s dvsec definition patch:
> > https://lore.kernel.org/linux-pci/bc5f059c5bae957daebde699945c80808286bf45.camel@xxxxxxxxxxxxxxx/T/#m1d0dc12e3b2c739e2c37106a45f325bb8f001774
> >
> >   drivers/pci/pci.c   |   30 ++++++++++++++++++++++++++++++
> >   include/linux/pci.h |    3 +++
> >   2 files changed, 33 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 6d4d5a2f923d..49e57b831509 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -589,6 +589,36 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
> >   }
> >   EXPORT_SYMBOL_GPL(pci_find_ext_capability);
> >   
> > +/**
> > + * pci_find_dvsec - return position of DVSEC with provided vendor and DVSEC ID
> > + * @dev: the PCI device
> > + * @vendor: vendor for the DVSEC
> > + * @id: the DVSEC capibility ID
>
>                       capability
>
> > + *
> > + * Return the offset of DVSEC on success or -ENOTSUPP if not found
>
>      * Return: the offset of DVSEC on success or -ENOTSUPP if not found
>
> > + */
> > +int pci_find_dvsec(struct pci_dev *dev, u16 vendor, u16 id)
> > +{
> > +	u16 dev_vendor, dev_id;
> > +	int pos;
> > +
> > +	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DVSEC);
> > +	if (!pos)
> > +		return -ENOTSUPP;
> > +
> > +	while (pos) {
> > +		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER1, &dev_vendor);
> > +		pci_read_config_word(dev, pos + PCI_DVSEC_HEADER2, &dev_id);
> > +		if (dev_vendor == vendor && dev_id == id)
> > +			return pos;
> > +
> > +		pos = pci_find_next_ext_capability(dev, pos, PCI_EXT_CAP_ID_DVSEC);
> > +	}
> > +
> > +	return -ENOTSUPP;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_find_dvsec);
> > +
> >   /**
> >    * pci_get_dsn - Read and return the 8-byte Device Serial Number
> >    * @dev: PCI device to query
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 22207a79762c..6c692d32c82a 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1069,6 +1069,7 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap);
> >   int pci_find_next_ext_capability(struct pci_dev *dev, int pos, int cap);
> >   int pci_find_ht_capability(struct pci_dev *dev, int ht_cap);
> >   int pci_find_next_ht_capability(struct pci_dev *dev, int pos, int ht_cap);
> > +int pci_find_dvsec(struct pci_dev *dev, u16 vendor, u16 id);
> >   struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
> >   
> >   u64 pci_get_dsn(struct pci_dev *dev);
> > @@ -1726,6 +1727,8 @@ static inline int pci_find_next_capability(struct pci_dev *dev, u8 post,
> >   { return 0; }
> >   static inline int pci_find_ext_capability(struct pci_dev *dev, int cap)
> >   { return 0; }
> > +static inline int pci_find_dvsec(struct pci_dev *dev, u16 vendor, u16 id)
> > +{ return 0; }
>
> Why shouldn't this return -ENOTSUPP instead of 0?

Actually looking at the other find cap functions. Should I be returning 0 for 
all failures?

> >   
> >   static inline u64 pci_get_dsn(struct pci_dev *dev)
> >   { return 0; }