Re: [PATCH v6 04/15] PCI: Add pci_find_vsec_capability() to find a specific VSEC
From: Bjorn Helgaas <helgaas@kernel.org>
Date: 2021-02-17 22:28:45
Also in:
linux-pci, lkml
[+cc Krzysztof, since he commented on a previous version] [+cc Lukas, who previously proposed exactly what I suggest below, sorry for repeating. I think Lukas was right to propose passing in the vendor ID because it makes it easier to read the caller.] When you post new versions of a series, please cc people who commented on previous versions. On Fri, Feb 12, 2021 at 06:37:39PM +0100, Gustavo Pimentel wrote:
Adds another helper to ones that already exist called pci_find_vsec_capability. This helper crawls through the device PCI config space searching for a specific ID on the Vendor-Specific Extended Capabilities section.
Add pci_find_vsec_capability() to locate a Vendor-Specific Extended Capability with the specified VSEC ID.
The Vendor-Specific Extended Capability (VSEC) is a special PCI capability (acts like container) defined by PCI-SIG that allows the one or more proprietary capabilities defined by the vendor which aren't standard or shared between the manufactures.
s/is a special ... by PCI-SIG that// s/allows the one/allows one/ s/the manufactures/manufacturers/ (or maybe "vendors" to match previous use)
quoted hunk ↗ jump to hunk
Signed-off-by: Gustavo Pimentel <redacted> --- drivers/pci/pci.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 ++ include/uapi/linux/pci_regs.h | 6 ++++++ 3 files changed, 42 insertions(+)diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index b9fecc2..628aa9f 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c@@ -693,6 +693,40 @@ 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 + * + * Typically this function will be called by the PCI driver, which passes + * through argument the 'struct pci_dev *' already pointing for the device + * config space that is associated with the vendor and device ID which will + * know which ID to search and what to do with it, however, there might be + * cases that this function could be called outside of this scope and + * therefore is the caller responsibility to check the vendor and/or + * device ID first.
This is important because it's a bit subtle. IIUC, each vendor
(identified by Vendor ID at 0x00 in config space) can define its own
VSEC IDs, so it can define up to 2^16 == 64K VSEC structures.
Of course there's not room for that many in config space; but the
point is that the vendor chooses its own VSEC IDs and doesn't need to
coordinate with anybody.
So a VSEC ID 0x0006 in a Synopsys device (Vendor ID 0x16c3) has
nothing to do with a VSEC ID 0x0006 in an Intel device (Vendor ID
0x8086), and it's up to the caller to make sure it's using the correct
one.
I wonder if it would help avoid mistakes if we made the interface look
like this:
u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int vsec_cap_id)
{
if (vendor != dev->vendor)
return 0;
while ((vsec = ...))
...
}
so calls would look like this:
vsec = pci_find_vsec_capability(dev, PCI_VENDOR_ID_SYNOPSYS, DW_PCIE_VSEC_DMA_ID);
which would make it more obvious that DW_PCIE_VSEC_DMA_ID is only
valid in a Synopsys device.
The function comment could be something like this:
pci_find_vsec_capability - Find a vendor-specific extended capability
@dev: PCI device to query
@vendor: Vendor ID for which capability is defined
@vsec_cap_id: Vendor-specific capability ID
If @dev has Vendor ID @vendor, search for a VSEC capability with
VSEC ID @vsec_cap_id. If found, return the capability offset in
config space; otherwise return 0.
Or maybe it's even more subtle than I thought, and I'm missing
something :)
quoted hunk ↗ jump to hunk
+ * 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 = 0; + u32 header; + + while ((vsec = pci_find_next_ext_capability(dev, vsec, + PCI_EXT_CAP_ID_VNDR))) { + if (pci_read_config_dword(dev, vsec + PCI_VSEC_HDR, + &header) == PCIBIOS_SUCCESSFUL && + PCI_VSEC_CAP_ID(header) == vsec_cap_id) + return vsec; + } + + 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 searcheddiff --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);
Can you put this declaration up with the other "find_capability" declarations just a few lines up?
quoted hunk ↗ jump to hunk
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)
The VSEC #defines are a mess. We have already have some duplication and inconsistent names. But I think what you need is already there: /* Vendor-Specific (VSEC, PCI_EXT_CAP_ID_VNDR) */ #define PCI_VNDR_HEADER 4 /* Vendor-Specific Header */ #define PCI_VNDR_HEADER_ID(x) ((x) & 0xffff) #define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf) #define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff)
/* SATA capability */ #define PCI_SATA_REGS 4 /* SATA REGs specifier */ #define PCI_SATA_REGS_MASK 0xF /* location - BAR#/inline */ -- 2.7.4