diff mbox series

[v2,04/15] PCI: Add pci_find_vsec_capability() to find a specific VSEC

Message ID 2ecb33dfee5dc05efc05de0731b0cb77bc246f54.1612269537.git.gustavo.pimentel@synopsys.com (mailing list archive)
State Changes Requested
Headers show
Series dmaengine: dw-edma: HDMA support | expand

Commit Message

Gustavo Pimentel Feb. 2, 2021, 12:40 p.m. UTC
Add pci_find_vsec_capability() that crawls through the device config
space searching in all Vendor-Specific Extended Capabilities for a
particular capability ID.

Vendor-Specific Extended Capability (VSEC) is a PCIe capability (acts
like a wrapper) specified by PCI-SIG that allows the vendor to create
their own and specific capability in the device config space.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/pci/pci.c             | 29 +++++++++++++++++++++++++++++
 include/linux/pci.h           |  2 ++
 include/uapi/linux/pci_regs.h |  6 ++++++
 3 files changed, 37 insertions(+)

Comments

Lukas Wunner Feb. 2, 2021, 6:08 p.m. UTC | #1
On Tue, Feb 02, 2021 at 01:40:18PM +0100, Gustavo Pimentel wrote:
>  /**
> + * pci_find_vsec_capability - Find a vendor-specific extended capability
> + * @dev: PCI device to query
> + * @cap: vendor-specific capability ID code
> + *
> + * Returns the address of the vendor-specific structure that matches the
> + * requested capability ID code within the device's PCI configuration space
> + * or 0 if it does not find a match.
> + */
> +u16 pci_find_vsec_capability(struct pci_dev *dev, int vsec_cap_id)
> +{

As the name implies, the capability is "vendor-specific", so it is perfectly
possible that two vendors use the same VSEC ID for different things.

To make sure you're looking for the right capability, you need to pass
a u16 vendor into this function and bail out if dev->vendor is different.


> +	u16 vsec;
> +	u32 header;
> +
> +	vsec = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VNDR);
> +	while (vsec) {
> +		if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR,
> +					  &header) == PCIBIOS_SUCCESSFUL &&
> +		    PCI_VSEC_CAP_ID(header) == vsec_cap_id)
> +			return vsec;
> +
> +		vsec = pci_find_next_ext_capability(dev, vsec,
> +						    PCI_EXT_CAP_ID_VNDR);
> +	}

FWIW, a more succinct implementation would be:

	while ((vsec = pci_find_next_ext_capability(...))) { ... }

See set_pcie_thunderbolt() in drivers/pci/probe.c for an example.
Please consider refactoring that function to use your new helper.

Thanks,

Lukas
Gustavo Pimentel Feb. 3, 2021, 1:54 a.m. UTC | #2
Hi Lukas,

On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:

> On Tue, Feb 02, 2021 at 01:40:18PM +0100, Gustavo Pimentel wrote:
> >  /**
> > + * pci_find_vsec_capability - Find a vendor-specific extended capability
> > + * @dev: PCI device to query
> > + * @cap: vendor-specific capability ID code
> > + *
> > + * Returns the address of the vendor-specific structure that matches the
> > + * requested capability ID code within the device's PCI configuration space
> > + * or 0 if it does not find a match.
> > + */
> > +u16 pci_find_vsec_capability(struct pci_dev *dev, int vsec_cap_id)
> > +{
> 
> As the name implies, the capability is "vendor-specific", so it is perfectly
> possible that two vendors use the same VSEC ID for different things.
> 
> To make sure you're looking for the right capability, you need to pass
> a u16 vendor into this function and bail out if dev->vendor is different.

This function will be called by the driver that will pass the correct 
device which will be already pointing to the config space associated with 
the endpoint for instance. Because the driver is already attached to the 
endpoint through the vendor ID and device ID specified, there is no need 
to do that validation, it will be redundant.

> 
> 
> > +	u16 vsec;
> > +	u32 header;
> > +
> > +	vsec = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VNDR);
> > +	while (vsec) {
> > +		if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR,
> > +					  &header) == PCIBIOS_SUCCESSFUL &&
> > +		    PCI_VSEC_CAP_ID(header) == vsec_cap_id)
> > +			return vsec;
> > +
> > +		vsec = pci_find_next_ext_capability(dev, vsec,
> > +						    PCI_EXT_CAP_ID_VNDR);
> > +	}
> 
> FWIW, a more succinct implementation would be:
> 
> 	while ((vsec = pci_find_next_ext_capability(...))) { ... }
> 
> See set_pcie_thunderbolt() in drivers/pci/probe.c for an example.
> Please consider refactoring that function to use your new helper.

That looks more clean. I will do it. Thanks!

> 
> Thanks,
> 
> Lukas
Lukas Wunner Feb. 3, 2021, 7:51 a.m. UTC | #3
On Wed, Feb 03, 2021 at 01:54:49AM +0000, Gustavo Pimentel wrote:
> On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:
> > As the name implies, the capability is "vendor-specific", so it is
> > perfectly possible that two vendors use the same VSEC ID for different
> > things.
> > 
> > To make sure you're looking for the right capability, you need to pass
> > a u16 vendor into this function and bail out if dev->vendor is different.
> 
> This function will be called by the driver that will pass the correct 
> device which will be already pointing to the config space associated with 
> the endpoint for instance. Because the driver is already attached to the 
> endpoint through the vendor ID and device ID specified, there is no need 
> to do that validation, it will be redundant.

Okay.  Please amend the kernel-doc to make it explicit that it's the
caller's responsibility to check the vendor ID.

Thanks,

Lukas
Gustavo Pimentel Feb. 3, 2021, 9:11 a.m. UTC | #4
On Wed, Feb 3, 2021 at 7:51:3, Lukas Wunner <lukas@wunner.de> wrote:

> On Wed, Feb 03, 2021 at 01:54:49AM +0000, Gustavo Pimentel wrote:
> > On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:
> > > As the name implies, the capability is "vendor-specific", so it is
> > > perfectly possible that two vendors use the same VSEC ID for different
> > > things.
> > > 
> > > To make sure you're looking for the right capability, you need to pass
> > > a u16 vendor into this function and bail out if dev->vendor is different.
> > 
> > This function will be called by the driver that will pass the correct 
> > device which will be already pointing to the config space associated with 
> > the endpoint for instance. Because the driver is already attached to the 
> > endpoint through the vendor ID and device ID specified, there is no need 
> > to do that validation, it will be redundant.
> 
> Okay.  Please amend the kernel-doc to make it explicit that it's the
> caller's responsibility to check the vendor ID.

I don't think that would be necessary, as I said, the 'struct pci_dev *' 
already points exclusively for the device' config space, which contains 
all the capabilities for that particular device by his turn will be 
attached to a specific driver by the Vendor and Device IDs to a specific 
driver, that will know, firstly search for the specific device vendor ID, 
 and then secondly how to decode it, and thirdly to do something with it.

> 
> Thanks,
> 
> Lukas
Lukas Wunner Feb. 3, 2021, 9:36 a.m. UTC | #5
On Wed, Feb 03, 2021 at 09:11:03AM +0000, Gustavo Pimentel wrote:
> On Wed, Feb 3, 2021 at 7:51:3, Lukas Wunner <lukas@wunner.de> wrote:
> > On Wed, Feb 03, 2021 at 01:54:49AM +0000, Gustavo Pimentel wrote:
> > > On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:
> > > > As the name implies, the capability is "vendor-specific", so it is
> > > > perfectly possible that two vendors use the same VSEC ID for different
> > > > things.
> > > > 
> > > > To make sure you're looking for the right capability, you need to pass
> > > > a u16 vendor into this function and bail out if dev->vendor is
> > > > different.
> > > 
> > > This function will be called by the driver that will pass the correct 
> > > device which will be already pointing to the config space associated with
> > > the endpoint for instance. Because the driver is already attached to the 
> > > endpoint through the vendor ID and device ID specified, there is no need 
> > > to do that validation, it will be redundant.
> > 
> > Okay.  Please amend the kernel-doc to make it explicit that it's the
> > caller's responsibility to check the vendor ID.
> 
> I don't think that would be necessary, as I said, the 'struct pci_dev *' 
> already points exclusively for the device' config space, which contains 
> all the capabilities for that particular device by his turn will be 
> attached to a specific driver by the Vendor and Device IDs to a specific 
> driver, that will know, firstly search for the specific device vendor ID, 
>  and then secondly how to decode it, and thirdly to do something with it.

The helper you're adding may not only be called from drivers but also
from generic PCI code (such as set_pcie_thunderbolt()).  In that case
the vendor ID is arbitrary.  Also, it doesn't *hurt* documenting this
requirement, does it?

Thanks,

Lukas
Gustavo Pimentel Feb. 3, 2021, 9:58 a.m. UTC | #6
On Wed, Feb 3, 2021 at 9:36:54, Lukas Wunner <lukas@wunner.de> wrote:

> On Wed, Feb 03, 2021 at 09:11:03AM +0000, Gustavo Pimentel wrote:
> > On Wed, Feb 3, 2021 at 7:51:3, Lukas Wunner <lukas@wunner.de> wrote:
> > > On Wed, Feb 03, 2021 at 01:54:49AM +0000, Gustavo Pimentel wrote:
> > > > On Tue, Feb 2, 2021 at 18:8:55, Lukas Wunner <lukas@wunner.de> wrote:
> > > > > As the name implies, the capability is "vendor-specific", so it is
> > > > > perfectly possible that two vendors use the same VSEC ID for different
> > > > > things.
> > > > > 
> > > > > To make sure you're looking for the right capability, you need to pass
> > > > > a u16 vendor into this function and bail out if dev->vendor is
> > > > > different.
> > > > 
> > > > This function will be called by the driver that will pass the correct 
> > > > device which will be already pointing to the config space associated with
> > > > the endpoint for instance. Because the driver is already attached to the 
> > > > endpoint through the vendor ID and device ID specified, there is no need 
> > > > to do that validation, it will be redundant.
> > > 
> > > Okay.  Please amend the kernel-doc to make it explicit that it's the
> > > caller's responsibility to check the vendor ID.
> > 
> > I don't think that would be necessary, as I said, the 'struct pci_dev *' 
> > already points exclusively for the device' config space, which contains 
> > all the capabilities for that particular device by his turn will be 
> > attached to a specific driver by the Vendor and Device IDs to a specific 
> > driver, that will know, firstly search for the specific device vendor ID, 
> >  and then secondly how to decode it, and thirdly to do something with it.
> 
> The helper you're adding may not only be called from drivers but also
> from generic PCI code (such as set_pcie_thunderbolt()).  In that case
> the vendor ID is arbitrary.  Also, it doesn't *hurt* documenting this
> requirement, does it?

I understand now your PoV. In that case, I can add that info to the 
function comment.

> 
> Thanks,
> 
> Lukas
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc2..4ea6dd4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -693,6 +693,35 @@  u8 pci_find_ht_capability(struct pci_dev *dev, int ht_cap)
 EXPORT_SYMBOL_GPL(pci_find_ht_capability);
 
 /**
+ * pci_find_vsec_capability - Find a vendor-specific extended capability
+ * @dev: PCI device to query
+ * @cap: vendor-specific capability ID code
+ *
+ * Returns the address of the vendor-specific structure that matches the
+ * requested capability ID code within the device's PCI configuration space
+ * or 0 if it does not find a match.
+ */
+u16 pci_find_vsec_capability(struct pci_dev *dev, int vsec_cap_id)
+{
+	u16 vsec;
+	u32 header;
+
+	vsec = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_VNDR);
+	while (vsec) {
+		if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR,
+					  &header) == PCIBIOS_SUCCESSFUL &&
+		    PCI_VSEC_CAP_ID(header) == vsec_cap_id)
+			return vsec;
+
+		vsec = pci_find_next_ext_capability(dev, vsec,
+						    PCI_EXT_CAP_ID_VNDR);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_find_vsec_capability);
+
+/**
  * pci_find_parent_resource - return resource region of parent bus of given
  *			      region
  * @dev: PCI device structure contains resources to be searched
diff --git a/include/linux/pci.h b/include/linux/pci.h
index b32126d..da6ab6a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1080,6 +1080,8 @@  struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
 
 u64 pci_get_dsn(struct pci_dev *dev);
 
+u16 pci_find_vsec_capability(struct pci_dev *dev, int vsec_cap_id);
+
 struct pci_dev *pci_get_device(unsigned int vendor, unsigned int device,
 			       struct pci_dev *from);
 struct pci_dev *pci_get_subsys(unsigned int vendor, unsigned int device,
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e709ae8..deae275 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -983,6 +983,12 @@ 
 #define PCI_VSEC_HDR		4	/* extended cap - vendor-specific */
 #define  PCI_VSEC_HDR_LEN_SHIFT	20	/* shift for length field */
 
+/* Vendor-Specific Extended Capabilities */
+#define PCI_VSEC_HEADER		4	/* Vendor-Specific Header */
+#define  PCI_VSEC_CAP_ID(x)	((x) & 0xffff)
+#define  PCI_VSEC_CAP_REV(x)	(((x) >> 16) & 0xf)
+#define  PCI_VSEC_CAP_LEN(x)	(((x) >> 20) & 0xfff)
+
 /* SATA capability */
 #define PCI_SATA_REGS		4	/* SATA REGs specifier */
 #define  PCI_SATA_REGS_MASK	0xF	/* location - BAR#/inline */