| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <95d9f7d9-6569-4252-a54d-cbe38ade706b@163.com>
Date: Sun, 30 Mar 2025 00:03:46 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, lpieralisi@...nel.org, kw@...ux.com,
robh@...nel.org, jingoohan1@...il.com, thomas.richard@...tlin.com,
linux-pci@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
finding the capabilities
On 2025/3/28 19:42, Ilpo Järvinen wrote:
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 47b31ad724fa..0d5dfd010a01 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1198,6 +1198,64 @@ void pci_sort_breadthfirst(void);
>>
>> /* Generic PCI functions exported to card drivers */
>>
>> +/* Extended capability finder */
>> +#define PCI_FIND_NEXT_CAP_TTL(priv, devfn, read_cfg, start, cap) \
>
> Please put it into drivers/pci/pci.h but other than that this is certainly
> to the direction I was suggesting.
Hi Ilpo,
I'm sorry for not replying to you in time. I tried to understand your
suggestion, modify it, and then experiment on the actual SOC platform.
Thank you very much for your reply and patient advice.
I'm going to put it in the drivers/pci/pci.h file.
>
> You don't need to have those priv and devfn there like that but you can
> use the args... trick (see linux/iopoll.h) and put them as last parameters
> to the macro so they can wary based on the caller needs, assuming args
> will work like this, I've not tested:
>
> read_cfg(args, __pos, &val)
>
I have modified it in the following reply, but I only modify it like
this at present: read_cfg(args, __pos, size, &val)
> The size parameter is the most annoying one as it's in between where and
> *val arguments so args... trick won't work for it. I suggest just
> providing a function with the same signature as the related pci/access.c
> function for now.
>
Currently DWC, CDNS, and pci.c seem to be unable to unify a common
function to read config.
I don't quite understand what you mean here. Is the current
dw_pcie_read_cfg, cdns_pcie_read_cfg, __pci_bus_read_config correct?
Please look at my latest modification. If it is not correct, please
explain it again. Thank you very much.
> A few nits related to the existing code quality of the cap parser function
> which should be addressed while we touch this function (probably in own
> patches indepedent of this code move/rearrange patch itself).
>
I will revise it in my final submission. The following reply has been
modified.
>> + ({ \
>> + typeof(start) __pos = (start); \
>> + int __ttl = PCI_FIND_CAP_TTL; \
>> + u16 __ent = 0; \
>> + u8 __found_pos = 0; \
>> + u8 __id; \
>> + \
>> + read_cfg((priv), (devfn), __pos, 1, (u32 *)&__pos); \
>> + \
>> + while (__ttl--) { \
>> + if (__pos < 0x40) \
>
> I started to wonder if there's something for this and it turn out we've
> PCI_STD_HEADER_SIZEOF.
It has been modified.
>
>> + break; \
>> + __pos &= ~3; \
>
> This could use some align helper.
It has been modified.
>
>> + read_cfg((priv), (devfn), __pos, 2, (u32 *)&__ent); \
>> + \
>> + __id = __ent & 0xff; \
>> + if (__id == 0xff) \
>> + break; \
>> + if (__id == (cap)) { \
>> + __found_pos = __pos; \
>> + break; \
>> + } \
>> + __pos = (__ent >> 8); \
>
> I'd add these into uapi/linux/pci_regs.h:
This means that you will submit, and I will submit after you?
Or should I submit this series of patches together?
>
> #define PCI_CAP_ID_MASK 0x00ff
> #define PCI_CAP_LIST_NEXT_MASK 0xff00
>
> And then use FIELD_GET() to extract the fields.
It has been modified.
>
>> + } \
>> + __found_pos; \
>> + })
>> +
>> +/* Extended capability finder */
>> +#define PCI_FIND_NEXT_EXT_CAPABILITY(priv, devfn, read_cfg, start, cap) \
>> + ({ \
>> + u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE; \
>> + u16 __found_pos = 0; \
>> + int __ttl, __ret; \
>> + u32 __header; \
>> + \
>> + __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8; \
>> + while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) { \
>> + __ret = read_cfg((priv), (devfn), __pos, 4, \
>> + &__header); \
>> + if (__ret != PCIBIOS_SUCCESSFUL) \
>> + break; \
>> + \
>> + if (__header == 0) \
>> + break; \
>> + \
>> + if (PCI_EXT_CAP_ID(__header) == (cap) && \
>> + __pos != start) { \
>> + __found_pos = __pos; \
>> + break; \
>> + } \
>> + \
>> + __pos = PCI_EXT_CAP_NEXT(__header); \
>> + } \
>> + __found_pos; \
>> + })
>> +
>> u8 pci_bus_find_capability(struct pci_bus *bus, unsigned int devfn, int cap);
>> u8 pci_find_capability(struct pci_dev *dev, int cap);
>> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap);
>
>
> I started to wonder though if the controller drivers could simply create
> an "early" struct pci_dev & pci_bus just so they can use the normal
> accessors while the real structs are not yet created. It looks not
> much is needed from those structs to let the accessors to work.
>
Here are a few questions:
1. We need to initialize some variables for pci_dev. For example,
dev->cfg_size needs to be initialized to 4K for PCIe.
u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
{
......
if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
return 0;
......
2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom,
Rockchip, etc). It leads to a lot of code that feels weird.
I still prefer the approach we are discussing now.
This is a modified patchs based on your suggestion:
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c
b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..3991cc4c58d6 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -7,6 +7,32 @@
#include <linux/of.h>
#include "pcie-cadence.h"
+#include "../../pci.h"
+
+static int cdns_pcie_read_cfg(void *priv, int where, int size, u32 *val)
+{
+ struct cdns_pcie *pcie = priv;
+
+ if (size == 4)
+ *val = cdns_pcie_readl(pcie, where);
+ else if (size == 2)
+ *val = cdns_pcie_readw(pcie, where);
+ else if (size == 1)
+ *val = cdns_pcie_readb(pcie, where);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
+{
+ return PCI_FIND_NEXT_CAP_TTL(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
+ cap, pcie);
+}
+
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
+{
+ return PCI_FIND_NEXT_EXT_CAPABILITY(cdns_pcie_read_cfg, 0, cap, pcie);
+}
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie)
{
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
b/drivers/pci/controller/cadence/pcie-cadence.h
index f5eeff834ec1..dd7cb6b6b291 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -398,6 +398,16 @@ static inline u32 cdns_pcie_readl(struct cdns_pcie
*pcie, u32 reg)
return readl(pcie->reg_base + reg);
}
+static inline u32 cdns_pcie_readw(struct cdns_pcie *pcie, u32 reg)
+{
+ return readw(pcie->reg_base + reg);
+}
+
+static inline u32 cdns_pcie_readb(struct cdns_pcie *pcie, u32 reg)
+{
+ return readb(pcie->reg_base + reg);
+}
+
static inline u32 cdns_pcie_read_sz(void __iomem *addr, int size)
{
void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
@@ -557,6 +567,9 @@ static inline int cdns_pcie_ep_setup(struct
cdns_pcie_ep *ep)
}
#endif
+u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap);
+u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap);
+
void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);
void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 busnr,
u8 fn,
diff --git a/drivers/pci/controller/dwc/pcie-designware.c
b/drivers/pci/controller/dwc/pcie-designware.c
index 145e7f579072..e9a9a80b1085 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -203,83 +203,25 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
pci->type = ver;
}
-/*
- * These interfaces resemble the pci_find_*capability() interfaces, but
these
- * are for configuring host controllers, which are bridges *to* PCI
devices but
- * are not PCI devices themselves.
- */
-static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
- u8 cap)
+static int dw_pcie_read_cfg(void *priv, int where, int size, u32 *val)
{
- u8 cap_id, next_cap_ptr;
- u16 reg;
-
- if (!cap_ptr)
- return 0;
-
- reg = dw_pcie_readw_dbi(pci, cap_ptr);
- cap_id = (reg & 0x00ff);
-
- if (cap_id > PCI_CAP_ID_MAX)
- return 0;
+ struct dw_pcie *pcie = priv;
- if (cap_id == cap)
- return cap_ptr;
+ *val = dw_pcie_read_dbi(pcie, where, size);
- next_cap_ptr = (reg & 0xff00) >> 8;
- return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+ return PCIBIOS_SUCCESSFUL;
}
u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
{
- u8 next_cap_ptr;
- u16 reg;
-
- reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
- next_cap_ptr = (reg & 0x00ff);
-
- return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
+ return PCI_FIND_NEXT_CAP_TTL(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
+ pcie);
}
EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
-static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
- u8 cap)
-{
- u32 header;
- int ttl;
- int pos = PCI_CFG_SPACE_SIZE;
-
- /* minimum 8 bytes per capability */
- ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
- if (start)
- pos = start;
-
- header = dw_pcie_readl_dbi(pci, pos);
- /*
- * If we have no capabilities, this is indicated by cap ID,
- * cap version and next pointer all being 0.
- */
- if (header == 0)
- return 0;
-
- while (ttl-- > 0) {
- if (PCI_EXT_CAP_ID(header) == cap && pos != start)
- return pos;
-
- pos = PCI_EXT_CAP_NEXT(header);
- if (pos < PCI_CFG_SPACE_SIZE)
- break;
-
- header = dw_pcie_readl_dbi(pci, pos);
- }
-
- return 0;
-}
-
u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
{
- return dw_pcie_find_next_ext_capability(pci, 0, cap);
+ return PCI_FIND_NEXT_EXT_CAPABILITY(dw_pcie_read_cfg, 0, cap, pcie);
}
EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..778e366ea72e 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -423,28 +423,28 @@ static int pci_dev_str_match(struct pci_dev *dev,
const char *p,
return 1;
}
-static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
- u8 pos, int cap, int *ttl)
+static int __pci_bus_read_config(void *priv, unsigned int devfn, int where,
+ u32 size, u32 *val)
{
- u8 id;
- u16 ent;
+ struct pci_bus *bus = priv;
+ int ret;
- pci_bus_read_config_byte(bus, devfn, pos, &pos);
+ if (size == 1)
+ ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
+ else if (size == 2)
+ ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
+ else
+ ret = pci_bus_read_config_dword(bus, devfn, where, val);
- while ((*ttl)--) {
- if (pos < 0x40)
- break;
- pos &= ~3;
- pci_bus_read_config_word(bus, devfn, pos, &ent);
+ return ret;
+}
- id = ent & 0xff;
- if (id == 0xff)
- break;
- if (id == cap)
- return pos;
- pos = (ent >> 8);
- }
- return 0;
+static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
+ u8 pos, int cap, int *ttl)
+{
+ // If accepted, I will remove all use of "int *ttl" in a future patch.
+ return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
+ devfn);
}
static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
@@ -553,42 +553,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
*/
u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
{
- u32 header;
- int ttl;
- u16 pos = PCI_CFG_SPACE_SIZE;
-
- /* minimum 8 bytes per capability */
- ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
-
if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
return 0;
- if (start)
- pos = start;
-
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
- return 0;
-
- /*
- * If we have no capabilities, this is indicated by cap ID,
- * cap version and next pointer all being 0.
- */
- if (header == 0)
- return 0;
-
- while (ttl-- > 0) {
- if (PCI_EXT_CAP_ID(header) == cap && pos != start)
- return pos;
-
- pos = PCI_EXT_CAP_NEXT(header);
- if (pos < PCI_CFG_SPACE_SIZE)
- break;
-
- if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
- break;
- }
-
- return 0;
+ return PCI_FIND_NEXT_EXT_CAPABILITY(__pci_bus_read_config, start, cap,
+ dev->bus, dev->devfn);
}
EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..68c111be521d 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -4,6 +4,65 @@
#include <linux/pci.h>
+/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
+#define PCI_CAP_ID_MASK 0x00ff
+#define PCI_CAP_LIST_NEXT_MASK 0xff00
+
+/* Standard capability finder */
+#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
+({ \
+ u8 __pos = (start); \
+ int __ttl = PCI_FIND_CAP_TTL; \
+ u16 __ent; \
+ u8 __found_pos = 0; \
+ u8 __id; \
+ \
+ read_cfg(args, __pos, 1, (u32 *)&__pos); \
+ \
+ while (__ttl--) { \
+ if (__pos < PCI_STD_HEADER_SIZEOF) \
+ break; \
+ __pos = ALIGN_DOWN(__pos, 4); \
+ read_cfg(args, __pos, 2, (u32 *)&__ent); \
+ __id = FIELD_GET(PCI_CAP_ID_MASK, __ent); \
+ if (__id == 0xff) \
+ break; \
+ if (__id == (cap)) { \
+ __found_pos = __pos; \
+ break; \
+ } \
+ __pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent); \
+ } \
+ __found_pos; \
+})
+
+/* Extended capability finder */
+#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...) \
+({ \
+ u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE; \
+ u16 __found_pos = 0; \
+ int __ttl, __ret; \
+ u32 __header; \
+ \
+ __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8; \
+ while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) { \
+ __ret = read_cfg(args, __pos, 4, &__header); \
+ if (__ret != PCIBIOS_SUCCESSFUL) \
+ break; \
+ \
+ if (__header == 0) \
+ break; \
+ \
+ if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) {\
+ __found_pos = __pos; \
+ break; \
+ } \
+ \
+ __pos = PCI_EXT_CAP_NEXT(__header); \
+ } \
+ __found_pos; \
+})
+
struct pcie_tlp_log;
/* Number of possible devfns: 0.0 to 1f.7 inclusive */
Looking forward to your latest suggestions.
Best regards,
Hans
Powered by blists - more mailing lists