* [v7 1/5] PCI: Refactor capability search into common macros
2025-04-02 4:20 [v7 0/5] Refactor capability search into common macros Hans Zhang
@ 2025-04-02 4:20 ` Hans Zhang
2025-04-02 12:42 ` Ilpo Järvinen
2025-04-02 4:20 ` [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication Hans Zhang
` (3 subsequent siblings)
4 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 4:20 UTC (permalink / raw)
To: lpieralisi, bhelgaas
Cc: kw, manivannan.sadhasivam, ilpo.jarvinen, robh, jingoohan1,
thomas.richard, linux-pci, linux-kernel, Hans Zhang
Introduce PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY macros
to consolidate duplicate PCI capability search logic found throughout the
driver tree. This refactoring:
1. Eliminates code duplication in capability scanning routines
2. Provides a standardized, maintainable implementation
3. Reduces error-prone copy-paste implementations
4. Maintains identical functionality to existing code
The macros abstract the low-level capability register scanning while
preserving the existing PCI configuration space access patterns. They will
enable future conversions of multiple capability search implementations
across various drivers (e.g., PCI core, controller drivers) to use
this centralized logic.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/pci.h | 81 +++++++++++++++++++++++++++++++++++
include/uapi/linux/pci_regs.h | 2 +
2 files changed, 83 insertions(+)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..f705b8bd3084 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
+/* Standard Capability finder */
+/**
+ * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search
+ * @cap: Capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Iterates through the capability list in PCI config space to find
+ * the specified capability. Implements TTL (time-to-live) protection
+ * against infinite loops.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#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 */
+/**
+ * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search (0 for initial search)
+ * @cap: Extended capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Searches the extended capability space in PCI config registers
+ * for the specified capability. Implements TTL protection against
+ * infinite loops using a calculated maximum search count.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#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; \
+})
+
/* Functions internal to the PCI core code */
#ifdef CONFIG_DMI
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 3445c4970e4d..a11ebbab99fc 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -206,6 +206,8 @@
/* 0x48-0x7f reserved */
/* Capability lists */
+#define PCI_CAP_ID_MASK 0x00ff
+#define PCI_CAP_LIST_NEXT_MASK 0xff00
#define PCI_CAP_LIST_ID 0 /* Capability ID */
#define PCI_CAP_ID_PM 0x01 /* Power Management */
--
2.25.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 1/5] PCI: Refactor capability search into common macros
2025-04-02 4:20 ` [v7 1/5] PCI: " Hans Zhang
@ 2025-04-02 12:42 ` Ilpo Järvinen
2025-04-02 15:31 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-02 12:42 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On Wed, 2 Apr 2025, Hans Zhang wrote:
> Introduce PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY macros
> to consolidate duplicate PCI capability search logic found throughout the
> driver tree. This refactoring:
>
> 1. Eliminates code duplication in capability scanning routines
> 2. Provides a standardized, maintainable implementation
> 3. Reduces error-prone copy-paste implementations
> 4. Maintains identical functionality to existing code
>
> The macros abstract the low-level capability register scanning while
> preserving the existing PCI configuration space access patterns. They will
> enable future conversions of multiple capability search implementations
> across various drivers (e.g., PCI core, controller drivers) to use
> this centralized logic.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/pci.h | 81 +++++++++++++++++++++++++++++++++++
> include/uapi/linux/pci_regs.h | 2 +
> 2 files changed, 83 insertions(+)
>
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e9cf26a9ee9..f705b8bd3084 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>
> +/* Standard Capability finder */
> +/**
> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search
> + * @cap: Capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Iterates through the capability list in PCI config space to find
> + * the specified capability. Implements TTL (time-to-live) protection
> + * against infinite loops.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#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); \
Could you please separate the coding style cleanups into own patch that
is before the actual move patch. IMO, all those cleanups can be in the
same patch.
You also need to add #includes for the defines you now started to use.
> + } \
> + __found_pos; \
> +})
> +
> +/* Extended Capability finder */
> +/**
> + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search (0 for initial search)
> + * @cap: Extended capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Searches the extended capability space in PCI config registers
> + * for the specified capability. Implements TTL protection against
> + * infinite loops using a calculated maximum search count.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#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; \
> +})
> +
> /* Functions internal to the PCI core code */
>
> #ifdef CONFIG_DMI
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 3445c4970e4d..a11ebbab99fc 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -206,6 +206,8 @@
> /* 0x48-0x7f reserved */
>
> /* Capability lists */
> +#define PCI_CAP_ID_MASK 0x00ff
> +#define PCI_CAP_LIST_NEXT_MASK 0xff00
>
> #define PCI_CAP_LIST_ID 0 /* Capability ID */
> #define PCI_CAP_ID_PM 0x01 /* Power Management */
>
--
i.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 1/5] PCI: Refactor capability search into common macros
2025-04-02 12:42 ` Ilpo Järvinen
@ 2025-04-02 15:31 ` Hans Zhang
2025-04-03 9:10 ` Ilpo Järvinen
0 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 15:31 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/2 20:42, Ilpo Järvinen wrote:
> On Wed, 2 Apr 2025, Hans Zhang wrote:
>
>> Introduce PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY macros
>> to consolidate duplicate PCI capability search logic found throughout the
>> driver tree. This refactoring:
>>
>> 1. Eliminates code duplication in capability scanning routines
>> 2. Provides a standardized, maintainable implementation
>> 3. Reduces error-prone copy-paste implementations
>> 4. Maintains identical functionality to existing code
>>
>> The macros abstract the low-level capability register scanning while
>> preserving the existing PCI configuration space access patterns. They will
>> enable future conversions of multiple capability search implementations
>> across various drivers (e.g., PCI core, controller drivers) to use
>> this centralized logic.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/pci.h | 81 +++++++++++++++++++++++++++++++++++
>> include/uapi/linux/pci_regs.h | 2 +
>> 2 files changed, 83 insertions(+)
>>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 2e9cf26a9ee9..f705b8bd3084 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
>> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>>
>> +/* Standard Capability finder */
>> +/**
>> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
>> + * @read_cfg: Function pointer for reading PCI config space
>> + * @start: Starting position to begin search
>> + * @cap: Capability ID to find
>> + * @args: Arguments to pass to read_cfg function
>> + *
>> + * Iterates through the capability list in PCI config space to find
>> + * the specified capability. Implements TTL (time-to-live) protection
>> + * against infinite loops.
>> + *
>> + * Returns: Position of the capability if found, 0 otherwise.
>> + */
>> +#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); \
>
> Could you please separate the coding style cleanups into own patch that
> is before the actual move patch. IMO, all those cleanups can be in the
> same patch.
>
Hi Ilpo,
Thanks your for reply. I don't understand. Is it like this?
#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
({ \
int __ttl = PCI_FIND_CAP_TTL; \
u8 __id, __found_pos = 0; \
u8 __pos = (start); \
u16 __ent; \
\
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; \
})
> You also need to add #includes for the defines you now started to use.
>
Is that what you mean?
+#include <linux/bitfield.h>
+#include <linux/align.h>
+#include <uapi/linux/pci_regs.h>
Best regards,
Hans
>> + } \
>> + __found_pos; \
>> +})
>> +
>> +/* Extended Capability finder */
>> +/**
>> + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
>> + * @read_cfg: Function pointer for reading PCI config space
>> + * @start: Starting position to begin search (0 for initial search)
>> + * @cap: Extended capability ID to find
>> + * @args: Arguments to pass to read_cfg function
>> + *
>> + * Searches the extended capability space in PCI config registers
>> + * for the specified capability. Implements TTL protection against
>> + * infinite loops using a calculated maximum search count.
>> + *
>> + * Returns: Position of the capability if found, 0 otherwise.
>> + */
>> +#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; \
>> +})
>> +
>> /* Functions internal to the PCI core code */
>>
>> #ifdef CONFIG_DMI
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index 3445c4970e4d..a11ebbab99fc 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -206,6 +206,8 @@
>> /* 0x48-0x7f reserved */
>>
>> /* Capability lists */
>> +#define PCI_CAP_ID_MASK 0x00ff
>> +#define PCI_CAP_LIST_NEXT_MASK 0xff00
>>
>> #define PCI_CAP_LIST_ID 0 /* Capability ID */
>> #define PCI_CAP_ID_PM 0x01 /* Power Management */
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 1/5] PCI: Refactor capability search into common macros
2025-04-02 15:31 ` Hans Zhang
@ 2025-04-03 9:10 ` Ilpo Järvinen
2025-04-03 12:22 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-03 9:10 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 7227 bytes --]
On Wed, 2 Apr 2025, Hans Zhang wrote:
> On 2025/4/2 20:42, Ilpo Järvinen wrote:
> > On Wed, 2 Apr 2025, Hans Zhang wrote:
> >
> > > Introduce PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY macros
> > > to consolidate duplicate PCI capability search logic found throughout the
> > > driver tree. This refactoring:
> > >
> > > 1. Eliminates code duplication in capability scanning routines
> > > 2. Provides a standardized, maintainable implementation
> > > 3. Reduces error-prone copy-paste implementations
> > > 4. Maintains identical functionality to existing code
> > >
> > > The macros abstract the low-level capability register scanning while
> > > preserving the existing PCI configuration space access patterns. They will
> > > enable future conversions of multiple capability search implementations
> > > across various drivers (e.g., PCI core, controller drivers) to use
> > > this centralized logic.
> > >
> > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > ---
> > > drivers/pci/pci.h | 81 +++++++++++++++++++++++++++++++++++
> > > include/uapi/linux/pci_regs.h | 2 +
> > > 2 files changed, 83 insertions(+)
> > >
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index 2e9cf26a9ee9..f705b8bd3084 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> > > bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
> > > bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> > > +/* Standard Capability finder */
> > > +/**
> > > + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> > > + * @read_cfg: Function pointer for reading PCI config space
> > > + * @start: Starting position to begin search
> > > + * @cap: Capability ID to find
> > > + * @args: Arguments to pass to read_cfg function
> > > + *
> > > + * Iterates through the capability list in PCI config space to find
> > > + * the specified capability. Implements TTL (time-to-live) protection
> > > + * against infinite loops.
> > > + *
> > > + * Returns: Position of the capability if found, 0 otherwise.
> > > + */
> > > +#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); \
> >
> > Could you please separate the coding style cleanups into own patch that
> > is before the actual move patch. IMO, all those cleanups can be in the
> > same patch.
> >
>
> Hi Ilpo,
>
> Thanks your for reply. I don't understand. Is it like this?
Add a patch before the first patch which does only the cleanups to
__pci_find_next_cap_ttl(). The patch that creates PCI_FIND_NEXT_CAP_TTL()
and converts its PCI core users (most of the patches 1&2) is to be based
on top of that cleanup patch.
> #define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
> ({ \
> int __ttl = PCI_FIND_CAP_TTL; \
> u8 __id, __found_pos = 0; \
> u8 __pos = (start); \
> u16 __ent; \
> \
> 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; \
> })
>
> > You also need to add #includes for the defines you now started to use.
> >
>
> Is that what you mean?
>
> +#include <linux/bitfield.h>
> +#include <linux/align.h>
> +#include <uapi/linux/pci_regs.h>
Almost, including pci_regs.h is not strictly necessary as linux/pci.h will
always pull that one in (not that it would hurt).
Also, sort the includes alphabetically.
--
i.
>
> Best regards,
> Hans
>
> > > + } \
> > > + __found_pos; \
> > > +})
> > > +
> > > +/* Extended Capability finder */
> > > +/**
> > > + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
> > > + * @read_cfg: Function pointer for reading PCI config space
> > > + * @start: Starting position to begin search (0 for initial search)
> > > + * @cap: Extended capability ID to find
> > > + * @args: Arguments to pass to read_cfg function
> > > + *
> > > + * Searches the extended capability space in PCI config registers
> > > + * for the specified capability. Implements TTL protection against
> > > + * infinite loops using a calculated maximum search count.
> > > + *
> > > + * Returns: Position of the capability if found, 0 otherwise.
> > > + */
> > > +#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;
> > > \
> > > +})
> > > +
> > > /* Functions internal to the PCI core code */
> > > #ifdef CONFIG_DMI
> > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > > index 3445c4970e4d..a11ebbab99fc 100644
> > > --- a/include/uapi/linux/pci_regs.h
> > > +++ b/include/uapi/linux/pci_regs.h
> > > @@ -206,6 +206,8 @@
> > > /* 0x48-0x7f reserved */
> > > /* Capability lists */
> > > +#define PCI_CAP_ID_MASK 0x00ff
> > > +#define PCI_CAP_LIST_NEXT_MASK 0xff00
> > > #define PCI_CAP_LIST_ID 0 /* Capability ID */
> > > #define PCI_CAP_ID_PM 0x01 /* Power Management */
> > >
> >
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 1/5] PCI: Refactor capability search into common macros
2025-04-03 9:10 ` Ilpo Järvinen
@ 2025-04-03 12:22 ` Hans Zhang
2025-04-03 16:31 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-03 12:22 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/3 17:10, Ilpo Järvinen wrote:
>>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>>>> index 2e9cf26a9ee9..f705b8bd3084 100644
>>>> --- a/drivers/pci/pci.h
>>>> +++ b/drivers/pci/pci.h
>>>> @@ -89,6 +89,87 @@ bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>>>> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
>>>> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>>>> +/* Standard Capability finder */
>>>> +/**
>>>> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
>>>> + * @read_cfg: Function pointer for reading PCI config space
>>>> + * @start: Starting position to begin search
>>>> + * @cap: Capability ID to find
>>>> + * @args: Arguments to pass to read_cfg function
>>>> + *
>>>> + * Iterates through the capability list in PCI config space to find
>>>> + * the specified capability. Implements TTL (time-to-live) protection
>>>> + * against infinite loops.
>>>> + *
>>>> + * Returns: Position of the capability if found, 0 otherwise.
>>>> + */
>>>> +#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); \
>>>
>>> Could you please separate the coding style cleanups into own patch that
>>> is before the actual move patch. IMO, all those cleanups can be in the
>>> same patch.
>>>
>>
>> Hi Ilpo,
>>
>> Thanks your for reply. I don't understand. Is it like this?
>
> Add a patch before the first patch which does only the cleanups to
> __pci_find_next_cap_ttl(). The patch that creates PCI_FIND_NEXT_CAP_TTL()
> and converts its PCI core users (most of the patches 1&2) is to be based
> on top of that cleanup patch.
>
Hi Ilpo,
Thank you so much for your patience in explaining it to me.
>> #define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
>> ({ \
>> int __ttl = PCI_FIND_CAP_TTL; \
>> u8 __id, __found_pos = 0; \
>> u8 __pos = (start); \
>> u16 __ent; \
>> \
>> 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; \
>> })
>>
>>> You also need to add #includes for the defines you now started to use.
>>>
>>
>> Is that what you mean?
>>
>> +#include <linux/bitfield.h>
>> +#include <linux/align.h>
>> +#include <uapi/linux/pci_regs.h>
>
> Almost, including pci_regs.h is not strictly necessary as linux/pci.h will
> always pull that one in (not that it would hurt).
>
> Also, sort the includes alphabetically.
>
OK，will change.
Best regards,
Hans
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 1/5] PCI: Refactor capability search into common macros
2025-04-03 12:22 ` Hans Zhang
@ 2025-04-03 16:31 ` Hans Zhang
0 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-03 16:31 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/3 20:22, Hans Zhang wrote:
>>>>> +#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); \
>>>>
>>>> Could you please separate the coding style cleanups into own patch that
>>>> is before the actual move patch. IMO, all those cleanups can be in the
>>>> same patch.
>>>>
>>>
>>> Thanks your for reply. I don't understand. Is it like this?
>>
>> Add a patch before the first patch which does only the cleanups to
>> __pci_find_next_cap_ttl(). The patch that creates PCI_FIND_NEXT_CAP_TTL()
>> and converts its PCI core users (most of the patches 1&2) is to be based
>> on top of that cleanup patch.
>>
>
> Thank you so much for your patience in explaining it to me.
Hi Ilpo,
The [v9 2/6]patch I plan to submit is as follows, please review it.
From 300fe1f428930d0bf8a361ea1d1a3272a6153107 Mon Sep 17 00:00:00 2001
From: Hans Zhang <18255117159@163.com>
Date: Fri, 4 Apr 2025 00:20:03 +0800
Subject: [v9 2/6] PCI: Clean up __pci_find_next_cap_ttl() readability
Refactor the __pci_find_next_cap_ttl() to improve code clarity:
- Replace magic number 0x40 with PCI_STD_HEADER_SIZEOF.
- Use ALIGN_DOWN() for position alignment instead of manual bitmask.
- Extract PCI capability fields via FIELD_GET() with standardized masks.
- Add necessary headers (linux/align.h, uapi/linux/pci_regs.h).
The changes are purely non-functional cleanups, ensuring behavior remains
identical to the original implementation.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/pci.c | 10 ++++++----
include/uapi/linux/pci_regs.h | 2 ++
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..e4d3719b653d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,6 +9,7 @@
*/
#include <linux/acpi.h>
+#include <linux/align.h>
#include <linux/kernel.h>
#include <linux/delay.h>
#include <linux/dmi.h>
@@ -30,6 +31,7 @@
#include <asm/dma.h>
#include <linux/aer.h>
#include <linux/bitfield.h>
+#include <uapi/linux/pci_regs.h>
#include "pci.h"
DEFINE_MUTEX(pci_slot_mutex);
@@ -432,17 +434,17 @@ static u8 __pci_find_next_cap_ttl(struct pci_bus
*bus, unsigned int devfn,
pci_bus_read_config_byte(bus, devfn, pos, &pos);
while ((*ttl)--) {
- if (pos < 0x40)
+ if (pos < PCI_STD_HEADER_SIZEOF)
break;
- pos &= ~3;
+ pos = ALIGN_DOWN(pos, 4);
pci_bus_read_config_word(bus, devfn, pos, &ent);
- id = ent & 0xff;
+ id = FIELD_GET(PCI_CAP_ID_MASK, ent);
if (id == 0xff)
break;
if (id == cap)
return pos;
- pos = (ent >> 8);
+ pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
}
return 0;
}
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index 3445c4970e4d..a11ebbab99fc 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -206,6 +206,8 @@
/* 0x48-0x7f reserved */
/* Capability lists */
+#define PCI_CAP_ID_MASK 0x00ff
+#define PCI_CAP_LIST_NEXT_MASK 0xff00
#define PCI_CAP_LIST_ID 0 /* Capability ID */
#define PCI_CAP_ID_PM 0x01 /* Power Management */
Best regards,
Hans
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 4:20 [v7 0/5] Refactor capability search into common macros Hans Zhang
2025-04-02 4:20 ` [v7 1/5] PCI: " Hans Zhang
@ 2025-04-02 4:20 ` Hans Zhang
2025-04-02 9:19 ` kernel test robot
2025-04-02 12:38 ` Ilpo Järvinen
2025-04-02 4:20 ` [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities Hans Zhang
` (2 subsequent siblings)
4 siblings, 2 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 4:20 UTC (permalink / raw)
To: lpieralisi, bhelgaas
Cc: kw, manivannan.sadhasivam, ilpo.jarvinen, robh, jingoohan1,
thomas.richard, linux-pci, linux-kernel, Hans Zhang
Refactor the PCI capability and extended capability search functions
by consolidating common code patterns into reusable macros
(PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
changes include:
1. Introducing a unified config space read helper (__pci_bus_read_config).
2. Removing duplicate search logic from __pci_find_next_cap_ttl and
pci_find_next_ext_capability.
3. Implementing consistent capability discovery using the new macros.
4. Simplifying HyperTransport capability lookup by leveraging the
refactored code.
The refactoring maintains existing functionality while reducing code
duplication and improving maintainability. By centralizing the search
logic, we achieve better code consistency and make future updates easier.
This change has been verified to maintain backward compatibility with
existing capability discovery patterns through thorough testing of PCI
device enumeration and capability probing.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/pci.c | 79 +++++++++++++----------------------------------
1 file changed, 22 insertions(+), 57 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..521096c73686 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -423,36 +423,33 @@ 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)
+{
+ 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,
u8 pos, int cap)
{
- int ttl = PCI_FIND_CAP_TTL;
-
- return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
+ return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
}
u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
@@ -553,42 +550,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);
@@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
{
- int rc, ttl = PCI_FIND_CAP_TTL;
u8 cap, mask;
if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
@@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
mask = HT_5BIT_CAP_MASK;
pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
- PCI_CAP_ID_HT, &ttl);
+ PCI_CAP_ID_HT);
while (pos) {
rc = pci_read_config_byte(dev, pos + 3, &cap);
if (rc != PCIBIOS_SUCCESSFUL)
@@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
pos + PCI_CAP_LIST_NEXT,
- PCI_CAP_ID_HT, &ttl);
+ PCI_CAP_ID_HT);
}
return 0;
--
2.25.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 4:20 ` [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication Hans Zhang
@ 2025-04-02 9:19 ` kernel test robot
2025-04-02 10:42 ` Hans Zhang
2025-04-02 12:38 ` Ilpo Järvinen
1 sibling, 1 reply; 25+ messages in thread
From: kernel test robot @ 2025-04-02 9:19 UTC (permalink / raw)
To: Hans Zhang, lpieralisi, bhelgaas
Cc: oe-kbuild-all, kw, manivannan.sadhasivam, ilpo.jarvinen, robh,
jingoohan1, thomas.richard, linux-pci, linux-kernel, Hans Zhang
Hi Hans,
kernel test robot noticed the following build errors:
[auto build test ERROR on acb4f33713b9f6cadb6143f211714c343465411c]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Refactor-capability-search-into-common-macros/20250402-122544
base: acb4f33713b9f6cadb6143f211714c343465411c
patch link: https://lore.kernel.org/r/20250402042020.48681-3-18255117159%40163.com
patch subject: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
config: loongarch-randconfig-001-20250402 (https://download.01.org/0day-ci/archive/20250402/202504021623.LgnqoZPE-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250402/202504021623.LgnqoZPE-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504021623.LgnqoZPE-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/pci/pci.c: In function '__pci_find_next_ht_cap':
>> drivers/pci/pci.c:627:17: error: 'rc' undeclared (first use in this function); did you mean 'rq'?
627 | rc = pci_read_config_byte(dev, pos + 3, &cap);
| ^~
| rq
drivers/pci/pci.c:627:17: note: each undeclared identifier is reported only once for each function it appears in
vim +627 drivers/pci/pci.c
70c0923b0ef10b Jacob Keller 2020-03-02 614
f646c2a0a6685a Puranjay Mohan 2020-11-29 615 static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
687d5fe3dc3379 Michael Ellerman 2006-11-22 616 {
687d5fe3dc3379 Michael Ellerman 2006-11-22 617 u8 cap, mask;
687d5fe3dc3379 Michael Ellerman 2006-11-22 618
687d5fe3dc3379 Michael Ellerman 2006-11-22 619 if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
687d5fe3dc3379 Michael Ellerman 2006-11-22 620 mask = HT_3BIT_CAP_MASK;
687d5fe3dc3379 Michael Ellerman 2006-11-22 621 else
687d5fe3dc3379 Michael Ellerman 2006-11-22 622 mask = HT_5BIT_CAP_MASK;
687d5fe3dc3379 Michael Ellerman 2006-11-22 623
687d5fe3dc3379 Michael Ellerman 2006-11-22 624 pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
2675dfd086c109 Hans Zhang 2025-04-02 625 PCI_CAP_ID_HT);
687d5fe3dc3379 Michael Ellerman 2006-11-22 626 while (pos) {
687d5fe3dc3379 Michael Ellerman 2006-11-22 @627 rc = pci_read_config_byte(dev, pos + 3, &cap);
687d5fe3dc3379 Michael Ellerman 2006-11-22 628 if (rc != PCIBIOS_SUCCESSFUL)
687d5fe3dc3379 Michael Ellerman 2006-11-22 629 return 0;
687d5fe3dc3379 Michael Ellerman 2006-11-22 630
687d5fe3dc3379 Michael Ellerman 2006-11-22 631 if ((cap & mask) == ht_cap)
687d5fe3dc3379 Michael Ellerman 2006-11-22 632 return pos;
687d5fe3dc3379 Michael Ellerman 2006-11-22 633
47a4d5be7c50b2 Brice Goglin 2007-01-10 634 pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
47a4d5be7c50b2 Brice Goglin 2007-01-10 635 pos + PCI_CAP_LIST_NEXT,
2675dfd086c109 Hans Zhang 2025-04-02 636 PCI_CAP_ID_HT);
687d5fe3dc3379 Michael Ellerman 2006-11-22 637 }
687d5fe3dc3379 Michael Ellerman 2006-11-22 638
687d5fe3dc3379 Michael Ellerman 2006-11-22 639 return 0;
687d5fe3dc3379 Michael Ellerman 2006-11-22 640 }
f646c2a0a6685a Puranjay Mohan 2020-11-29 641
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 9:19 ` kernel test robot
@ 2025-04-02 10:42 ` Hans Zhang
0 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 10:42 UTC (permalink / raw)
To: kernel test robot, lpieralisi, bhelgaas
Cc: oe-kbuild-all, kw, manivannan.sadhasivam, ilpo.jarvinen, robh,
jingoohan1, thomas.richard, linux-pci, linux-kernel
On 2025/4/2 17:19, kernel test robot wrote:
> Hi Hans,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on acb4f33713b9f6cadb6143f211714c343465411c]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Refactor-capability-search-into-common-macros/20250402-122544
> base: acb4f33713b9f6cadb6143f211714c343465411c
> patch link: https://lore.kernel.org/r/20250402042020.48681-3-18255117159%40163.com
> patch subject: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
> config: loongarch-randconfig-001-20250402 (https://download.01.org/0day-ci/archive/20250402/202504021623.LgnqoZPE-lkp@intel.com/config)
> compiler: loongarch64-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250402/202504021623.LgnqoZPE-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202504021623.LgnqoZPE-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> drivers/pci/pci.c: In function '__pci_find_next_ht_cap':
>>> drivers/pci/pci.c:627:17: error: 'rc' undeclared (first use in this function); did you mean 'rq'?
> 627 | rc = pci_read_config_byte(dev, pos + 3, &cap);
> | ^~
> | rq
> drivers/pci/pci.c:627:17: note: each undeclared identifier is reported only once for each function it appears in
>
static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
{
- int rc, ttl = PCI_FIND_CAP_TTL;
u8 cap, mask;
The rc variable was deleted by mistake and will be changed in the next
version.
static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
{
- int rc;
u8 cap, mask;
Best regards,
Hans
>
> vim +627 drivers/pci/pci.c
>
> 70c0923b0ef10b Jacob Keller 2020-03-02 614
> f646c2a0a6685a Puranjay Mohan 2020-11-29 615 static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 616 {
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 617 u8 cap, mask;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 618
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 619 if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 620 mask = HT_3BIT_CAP_MASK;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 621 else
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 622 mask = HT_5BIT_CAP_MASK;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 623
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 624 pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> 2675dfd086c109 Hans Zhang 2025-04-02 625 PCI_CAP_ID_HT);
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 626 while (pos) {
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 @627 rc = pci_read_config_byte(dev, pos + 3, &cap);
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 628 if (rc != PCIBIOS_SUCCESSFUL)
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 629 return 0;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 630
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 631 if ((cap & mask) == ht_cap)
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 632 return pos;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 633
> 47a4d5be7c50b2 Brice Goglin 2007-01-10 634 pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> 47a4d5be7c50b2 Brice Goglin 2007-01-10 635 pos + PCI_CAP_LIST_NEXT,
> 2675dfd086c109 Hans Zhang 2025-04-02 636 PCI_CAP_ID_HT);
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 637 }
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 638
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 639 return 0;
> 687d5fe3dc3379 Michael Ellerman 2006-11-22 640 }
> f646c2a0a6685a Puranjay Mohan 2020-11-29 641
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 4:20 ` [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication Hans Zhang
2025-04-02 9:19 ` kernel test robot
@ 2025-04-02 12:38 ` Ilpo Järvinen
2025-04-02 15:37 ` Hans Zhang
1 sibling, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-02 12:38 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On Wed, 2 Apr 2025, Hans Zhang wrote:
> Refactor the PCI capability and extended capability search functions
> by consolidating common code patterns into reusable macros
> (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
> changes include:
>
> 1. Introducing a unified config space read helper (__pci_bus_read_config).
> 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
> pci_find_next_ext_capability.
> 3. Implementing consistent capability discovery using the new macros.
> 4. Simplifying HyperTransport capability lookup by leveraging the
> refactored code.
>
> The refactoring maintains existing functionality while reducing code
> duplication and improving maintainability. By centralizing the search
> logic, we achieve better code consistency and make future updates easier.
>
> This change has been verified to maintain backward compatibility with
> existing capability discovery patterns through thorough testing of PCI
> device enumeration and capability probing.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/pci.c | 79 +++++++++++++----------------------------------
> 1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..521096c73686 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -423,36 +423,33 @@ 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)
This probably should be where the other accessors are so in access.c. I'd
put its prototype into drivers/pci/pci.h only for now.
> {
> - 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)
> +{
> + 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,
> u8 pos, int cap)
> {
> - int ttl = PCI_FIND_CAP_TTL;
> -
> - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
> }
>
> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -553,42 +550,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);
I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
together. However, (IMO) you can introduce the new all-size config space
accessor in a separate patch before the combined patch.
> }
> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>
> @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>
> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> - int rc, ttl = PCI_FIND_CAP_TTL;
> u8 cap, mask;
>
> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> mask = HT_5BIT_CAP_MASK;
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> while (pos) {
> rc = pci_read_config_byte(dev, pos + 3, &cap);
> if (rc != PCIBIOS_SUCCESSFUL)
> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> pos + PCI_CAP_LIST_NEXT,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
This function kind of had the idea to share the ttl but I suppose that was
just a final safeguard to make sure the loop will always terminate in case
the config space is corrupted so the unsharing is not a big issue.
> }
>
> return 0;
>
--
i.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 12:38 ` Ilpo Järvinen
@ 2025-04-02 15:37 ` Hans Zhang
2025-04-03 9:15 ` Ilpo Järvinen
0 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 15:37 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/2 20:38, Ilpo Järvinen wrote:
> On Wed, 2 Apr 2025, Hans Zhang wrote:
>
>> Refactor the PCI capability and extended capability search functions
>> by consolidating common code patterns into reusable macros
>> (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
>> changes include:
>>
>> 1. Introducing a unified config space read helper (__pci_bus_read_config).
>> 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
>> pci_find_next_ext_capability.
>> 3. Implementing consistent capability discovery using the new macros.
>> 4. Simplifying HyperTransport capability lookup by leveraging the
>> refactored code.
>>
>> The refactoring maintains existing functionality while reducing code
>> duplication and improving maintainability. By centralizing the search
>> logic, we achieve better code consistency and make future updates easier.
>>
>> This change has been verified to maintain backward compatibility with
>> existing capability discovery patterns through thorough testing of PCI
>> device enumeration and capability probing.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> drivers/pci/pci.c | 79 +++++++++++++----------------------------------
>> 1 file changed, 22 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 869d204a70a3..521096c73686 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -423,36 +423,33 @@ 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)
>
> This probably should be where the other accessors are so in access.c. I'd
> put its prototype into drivers/pci/pci.h only for now.
>
Hi Ilpo,
Thank you very much for your guidance. Will change.
>> {
>> - 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)
>> +{
>> + 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,
>> u8 pos, int cap)
>> {
>> - int ttl = PCI_FIND_CAP_TTL;
>> -
>> - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
>> + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
>> }
>>
>> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
>> @@ -553,42 +550,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);
>
> I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
> together. However, (IMO) you can introduce the new all-size config space
> accessor in a separate patch before the combined patch.
>
Ok. I'll change it to the following. The rest I'll combine into a patch.
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b63b..bb2e26c2eb81 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
EXPORT_SYMBOL(pci_bus_write_config_word);
EXPORT_SYMBOL(pci_bus_write_config_dword);
+
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val)
+{
+ struct pci_bus *bus = priv;
+ int ret;
+
+ 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);
+
+ return ret;
+}
+
int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..6a7c88b9cd35 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -88,6 +88,8 @@ extern bool pci_early_dump;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val);
/* Functions internal to the PCI core code */
>> }
>> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>>
>> @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>>
>> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>> {
>> - int rc, ttl = PCI_FIND_CAP_TTL;
>> u8 cap, mask;
>>
>> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
>> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>> mask = HT_5BIT_CAP_MASK;
>>
>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
>> - PCI_CAP_ID_HT, &ttl);
>> + PCI_CAP_ID_HT);
>> while (pos) {
>> rc = pci_read_config_byte(dev, pos + 3, &cap);
>> if (rc != PCIBIOS_SUCCESSFUL)
>> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>>
>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
>> pos + PCI_CAP_LIST_NEXT,
>> - PCI_CAP_ID_HT, &ttl);
>> + PCI_CAP_ID_HT);
>
> This function kind of had the idea to share the ttl but I suppose that was
> just a final safeguard to make sure the loop will always terminate in case
> the config space is corrupted so the unsharing is not a big issue.
>
__pci_find_next_cap_ttl
// This macro definition already has ttl loop restrictions inside it.
PCI_FIND_NEXT_CAP_TTL
Do I understand that you agree to remove ttl initialization and
parameter passing?
Best regards,
Hans
>> }
>>
>> return 0;
>>
>
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-02 15:37 ` Hans Zhang
@ 2025-04-03 9:15 ` Ilpo Järvinen
2025-04-03 12:24 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-03 9:15 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 8413 bytes --]
On Wed, 2 Apr 2025, Hans Zhang wrote:
> On 2025/4/2 20:38, Ilpo Järvinen wrote:
> > On Wed, 2 Apr 2025, Hans Zhang wrote:
> >
> > > Refactor the PCI capability and extended capability search functions
> > > by consolidating common code patterns into reusable macros
> > > (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
> > > changes include:
> > >
> > > 1. Introducing a unified config space read helper (__pci_bus_read_config).
> > > 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
> > > pci_find_next_ext_capability.
> > > 3. Implementing consistent capability discovery using the new macros.
> > > 4. Simplifying HyperTransport capability lookup by leveraging the
> > > refactored code.
> > >
> > > The refactoring maintains existing functionality while reducing code
> > > duplication and improving maintainability. By centralizing the search
> > > logic, we achieve better code consistency and make future updates easier.
> > >
> > > This change has been verified to maintain backward compatibility with
> > > existing capability discovery patterns through thorough testing of PCI
> > > device enumeration and capability probing.
> > >
> > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > ---
> > > drivers/pci/pci.c | 79 +++++++++++++----------------------------------
> > > 1 file changed, 22 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 869d204a70a3..521096c73686 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -423,36 +423,33 @@ 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)
> >
> > This probably should be where the other accessors are so in access.c. I'd
> > put its prototype into drivers/pci/pci.h only for now.
> >
>
> Hi Ilpo,
>
> Thank you very much for your guidance. Will change.
>
>
> > > {
> > > - 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)
> > > +{
> > > + 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,
> > > u8 pos, int cap)
> > > {
> > > - int ttl = PCI_FIND_CAP_TTL;
> > > -
> > > - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> > > + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
> > > }
> > > u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> > > @@ -553,42 +550,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);
> >
> > I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
> > together. However, (IMO) you can introduce the new all-size config space
> > accessor in a separate patch before the combined patch.
> >
>
> Ok. I'll change it to the following. The rest I'll combine into a patch.
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index b123da16b63b..bb2e26c2eb81 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
> EXPORT_SYMBOL(pci_bus_write_config_word);
> EXPORT_SYMBOL(pci_bus_write_config_dword);
>
> +
Extra newline
> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> + u32 *val)
> +{
> + struct pci_bus *bus = priv;
> + int ret;
> +
> + 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);
> +
> + return ret;
> +}
> +
> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> int where, int size, u32 *val)
> {
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 2e9cf26a9ee9..6a7c88b9cd35 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -88,6 +88,8 @@ extern bool pci_early_dump;
> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> + u32 *val);
>
> /* Functions internal to the PCI core code */
>
>
> > > }
> > > EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
> > > @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
> > > static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int
> > > ht_cap)
> > > {
> > > - int rc, ttl = PCI_FIND_CAP_TTL;
> > > u8 cap, mask;
> > > if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> > > @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > > mask = HT_5BIT_CAP_MASK;
> > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> > > - PCI_CAP_ID_HT, &ttl);
> > > + PCI_CAP_ID_HT);
> > > while (pos) {
> > > rc = pci_read_config_byte(dev, pos + 3, &cap);
> > > if (rc != PCIBIOS_SUCCESSFUL)
> > > @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> > > pos + PCI_CAP_LIST_NEXT,
> > > - PCI_CAP_ID_HT, &ttl);
> > > + PCI_CAP_ID_HT);
> >
> > This function kind of had the idea to share the ttl but I suppose that was
> > just a final safeguard to make sure the loop will always terminate in case
> > the config space is corrupted so the unsharing is not a big issue.
> >
>
> __pci_find_next_cap_ttl
> // This macro definition already has ttl loop restrictions inside it.
> PCI_FIND_NEXT_CAP_TTL
>
> Do I understand that you agree to remove ttl initialization and parameter
> passing?
Yes, I agree with it but doing anything like this (although I'd mention
the reasoning in the changelog myself).
--
i.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-03 9:15 ` Ilpo Järvinen
@ 2025-04-03 12:24 ` Hans Zhang
2025-04-03 16:29 ` Hans Zhang
2025-04-03 16:35 ` Hans Zhang
0 siblings, 2 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-03 12:24 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/3 17:15, Ilpo Järvinen wrote:
>>> I don't like how 1 & 2 patches are split into two. IMO, they mostly belong
>>> together. However, (IMO) you can introduce the new all-size config space
>>> accessor in a separate patch before the combined patch.
>>>
>>
>> Ok. I'll change it to the following. The rest I'll combine into a patch.
>>
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index b123da16b63b..bb2e26c2eb81 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
>> EXPORT_SYMBOL(pci_bus_write_config_word);
>> EXPORT_SYMBOL(pci_bus_write_config_dword);
>>
>> +
>
> Extra newline
>
Hi Ilpo,
Thanks your for reply. Will delete.
>> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
>> + u32 *val)
>> +{
>> + struct pci_bus *bus = priv;
>> + int ret;
>> +
>> + 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);
>> +
>> + return ret;
>> +}
>> +
>> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>> int where, int size, u32 *val)
>> {
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 2e9cf26a9ee9..6a7c88b9cd35 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -88,6 +88,8 @@ extern bool pci_early_dump;
>> bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
>> bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
>> bool pcie_cap_has_rtctl(const struct pci_dev *dev);
>> +int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
>> + u32 *val);
>>
>> /* Functions internal to the PCI core code */
>>
>>
>>>> }
>>>> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>>>> @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>>>> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int
>>>> ht_cap)
>>>> {
>>>> - int rc, ttl = PCI_FIND_CAP_TTL;
>>>> u8 cap, mask;
>>>> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
>>>> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
>>>> u8 pos, int ht_cap)
>>>> mask = HT_5BIT_CAP_MASK;
>>>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
>>>> - PCI_CAP_ID_HT, &ttl);
>>>> + PCI_CAP_ID_HT);
>>>> while (pos) {
>>>> rc = pci_read_config_byte(dev, pos + 3, &cap);
>>>> if (rc != PCIBIOS_SUCCESSFUL)
>>>> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
>>>> u8 pos, int ht_cap)
>>>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
>>>> pos + PCI_CAP_LIST_NEXT,
>>>> - PCI_CAP_ID_HT, &ttl);
>>>> + PCI_CAP_ID_HT);
>>>
>>> This function kind of had the idea to share the ttl but I suppose that was
>>> just a final safeguard to make sure the loop will always terminate in case
>>> the config space is corrupted so the unsharing is not a big issue.
>>>
>>
>> __pci_find_next_cap_ttl
>> // This macro definition already has ttl loop restrictions inside it.
>> PCI_FIND_NEXT_CAP_TTL
>>
>> Do I understand that you agree to remove ttl initialization and parameter
>> passing?
>
> Yes, I agree with it but doing anything like this (although I'd mention
> the reasoning in the changelog myself).
>
Ok, I see. I will give the reasons.
Best regards,
Hans
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-03 12:24 ` Hans Zhang
@ 2025-04-03 16:29 ` Hans Zhang
2025-04-03 16:35 ` Hans Zhang
1 sibling, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-03 16:29 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/3 20:24, Hans Zhang wrote:
>
>
> On 2025/4/3 17:15, Ilpo Järvinen wrote:
>>>> I don't like how 1 & 2 patches are split into two. IMO, they mostly
>>>> belong
>>>> together. However, (IMO) you can introduce the new all-size config
>>>> space
>>>> accessor in a separate patch before the combined patch.
>>>>
>>>
>>> Ok. I'll change it to the following. The rest I'll combine into a patch.
>>>
>>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>>> index b123da16b63b..bb2e26c2eb81 100644
>>> --- a/drivers/pci/access.c
>>> +++ b/drivers/pci/access.c
>>> @@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
>>> EXPORT_SYMBOL(pci_bus_write_config_word);
>>> EXPORT_SYMBOL(pci_bus_write_config_dword);
>>>
>>> +
>>
>> Extra newline
>>
>
> Hi Ilpo,
>
> Thanks your for reply. Will delete.
>
Hi Ilpo,
The [v9 1/6]patch I plan to submit is as follows, please review it.
From c099691ff1e980ff4633c55e94abcd888000e2cc Mon Sep 17 00:00:00 2001
From: Hans Zhang <18255117159@163.com>
Date: Fri, 4 Apr 2025 00:19:32 +0800
Subject: [v9 1/6] PCI: Introduce generic bus config read helper function
The primary PCI config space accessors are tied to the size of the read
(byte/word/dword). Upcoming refactoring of PCI capability discovery logic
requires passing a config accessor function that must be able to perform
read with different sizes.
Add any size config space read accessor pci_bus_read_config() to allow
giving it as the config space accessor to the upcoming PCI capability
discovery macro.
Reconstructs the PCI function discovery logic to prepare for unified
configuration of access modes. No function changes are intended.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/access.c | 17 +++++++++++++++++
drivers/pci/pci.h | 2 ++
2 files changed, 19 insertions(+)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b63b..603332658ab3 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
EXPORT_SYMBOL(pci_bus_write_config_word);
EXPORT_SYMBOL(pci_bus_write_config_dword);
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val)
+{
+ struct pci_bus *bus = priv;
+ int ret;
+
+ 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);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pci_bus_read_config);
+
int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
int where, int size, u32 *val)
{
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..6a7c88b9cd35 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -88,6 +88,8 @@ extern bool pci_early_dump;
bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
bool pcie_cap_has_rtctl(const struct pci_dev *dev);
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
+ u32 *val);
/* Functions internal to the PCI core code */
Best regards,
Hans
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-03 12:24 ` Hans Zhang
2025-04-03 16:29 ` Hans Zhang
@ 2025-04-03 16:35 ` Hans Zhang
2025-04-07 17:03 ` Ilpo Järvinen
1 sibling, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-03 16:35 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/3 20:24, Hans Zhang wrote:
>>>>> }
>>>>> EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>>>>> @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>>>>> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int
>>>>> ht_cap)
>>>>> {
>>>>> - int rc, ttl = PCI_FIND_CAP_TTL;
>>>>> u8 cap, mask;
>>>>> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
>>>>> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
>>>>> *dev,
>>>>> u8 pos, int ht_cap)
>>>>> mask = HT_5BIT_CAP_MASK;
>>>>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
>>>>> - PCI_CAP_ID_HT, &ttl);
>>>>> + PCI_CAP_ID_HT);
>>>>> while (pos) {
>>>>> rc = pci_read_config_byte(dev, pos + 3, &cap);
>>>>> if (rc != PCIBIOS_SUCCESSFUL)
>>>>> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
>>>>> *dev,
>>>>> u8 pos, int ht_cap)
>>>>> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
>>>>> pos + PCI_CAP_LIST_NEXT,
>>>>> - PCI_CAP_ID_HT, &ttl);
>>>>> + PCI_CAP_ID_HT);
>>>>
>>>> This function kind of had the idea to share the ttl but I suppose
>>>> that was
>>>> just a final safeguard to make sure the loop will always terminate
>>>> in case
>>>> the config space is corrupted so the unsharing is not a big issue.
>>>>
>>>
>>> __pci_find_next_cap_ttl
>>> // This macro definition already has ttl loop restrictions inside it.
>>> PCI_FIND_NEXT_CAP_TTL
>>>
>>> Do I understand that you agree to remove ttl initialization and
>>> parameter
>>> passing?
>>
>> Yes, I agree with it but doing anything like this (although I'd mention
>> the reasoning in the changelog myself).
>>
>
> Ok, I see. I will give the reasons.
Hi Ilpo,
The [v9 3/6]patch I plan to submit is as follows, please review it.
From 6da415d130e76b57ecf401f14bf0b66f20407839 Mon Sep 17 00:00:00 2001
From: Hans Zhang <18255117159@163.com>
Date: Fri, 4 Apr 2025 00:20:29 +0800
Subject: [v9 3/6] PCI: Refactor capability search into common macros
- Capability search is done both in PCI core and some controller drivers.
- PCI core's cap search func requires PCI device and bus structs exist.
- Controller drivers cannot use PCI core's cap search func as they
need to find capabilities before they instantiated the PCI device & bus
structs.
- Move capability search into a macro so it can be reused where normal
PCI config space accessors cannot yet be used due to lack of the
instantiated PCI dev.
- Instead, give the config space reading function as an argument to the
new macro.
- Convert PCI core to use the new macro.
The macros now implement, parameterized by the config access method. The
PCI core functions are converted to utilize these macros with the standard
pci_bus_read_config accessors. Controller drivers can later use the same
macros with their early access mechanisms while maintaining the existing
protection against infinite loops through preserved TTL checks.
The ttl parameter was originally an additional safeguard to prevent
infinite loops in corrupted config space. However, the
PCI_FIND_NEXT_CAP_TTL macro already enforces a TTL limit internally.
Removing redundant ttl handling simplifies the interface while maintaining
the safety guarantee. This aligns with the macro's design intent of
encapsulating TTL management.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
drivers/pci/pci.c | 70 +++++---------------------------------
drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 95 insertions(+), 61 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e4d3719b653d..bef242d84ab4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -9,7 +9,6 @@
*/
#include <linux/acpi.h>
-#include <linux/align.h>
#include <linux/kernel.h>
#include <linux/delay.h>
#include <linux/dmi.h>
@@ -31,7 +30,6 @@
#include <asm/dma.h>
#include <linux/aer.h>
#include <linux/bitfield.h>
-#include <uapi/linux/pci_regs.h>
#include "pci.h"
DEFINE_MUTEX(pci_slot_mutex);
@@ -426,35 +424,16 @@ static int pci_dev_str_match(struct pci_dev *dev,
const char *p,
}
static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
- u8 pos, int cap, int *ttl)
+ u8 pos, int cap)
{
- u8 id;
- u16 ent;
-
- pci_bus_read_config_byte(bus, devfn, pos, &pos);
-
- while ((*ttl)--) {
- if (pos < PCI_STD_HEADER_SIZEOF)
- break;
- pos = ALIGN_DOWN(pos, 4);
- pci_bus_read_config_word(bus, devfn, pos, &ent);
-
- id = FIELD_GET(PCI_CAP_ID_MASK, ent);
- if (id == 0xff)
- break;
- if (id == cap)
- return pos;
- pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
- }
- return 0;
+ 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,
u8 pos, int cap)
{
- int ttl = PCI_FIND_CAP_TTL;
-
- return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
+ return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
}
u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
@@ -555,42 +534,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);
@@ -650,7 +598,7 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
{
- int rc, ttl = PCI_FIND_CAP_TTL;
+ int rc;
u8 cap, mask;
if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
@@ -659,7 +607,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
*dev, u8 pos, int ht_cap)
mask = HT_5BIT_CAP_MASK;
pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
- PCI_CAP_ID_HT, &ttl);
+ PCI_CAP_ID_HT);
while (pos) {
rc = pci_read_config_byte(dev, pos + 3, &cap);
if (rc != PCIBIOS_SUCCESSFUL)
@@ -670,7 +618,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
*dev, u8 pos, int ht_cap)
pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
pos + PCI_CAP_LIST_NEXT,
- PCI_CAP_ID_HT, &ttl);
+ PCI_CAP_ID_HT);
}
return 0;
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 6a7c88b9cd35..b204ebeeb1cf 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -2,7 +2,9 @@
#ifndef DRIVERS_PCI_H
#define DRIVERS_PCI_H
+#include <linux/align.h>
#include <linux/pci.h>
+#include <uapi/linux/pci_regs.h>
struct pcie_tlp_log;
@@ -91,6 +93,90 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
size,
u32 *val);
+/* Standard Capability finder */
+/**
+ * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search
+ * @cap: Capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Iterates through the capability list in PCI config space to find
+ * the specified capability. Implements TTL (time-to-live) protection
+ * against infinite loops.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
+({ \
+ int __ttl = PCI_FIND_CAP_TTL; \
+ u8 __id, __found_pos = 0; \
+ u8 __pos = (start); \
+ u16 __ent; \
+ \
+ 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 */
+/**
+ * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
+ * @read_cfg: Function pointer for reading PCI config space
+ * @start: Starting position to begin search (0 for initial search)
+ * @cap: Extended capability ID to find
+ * @args: Arguments to pass to read_cfg function
+ *
+ * Searches the extended capability space in PCI config registers
+ * for the specified capability. Implements TTL protection against
+ * infinite loops using a calculated maximum search count.
+ *
+ * Returns: Position of the capability if found, 0 otherwise.
+ */
+#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; \
+})
+
/* Functions internal to the PCI core code */
#ifdef CONFIG_DMI
Best regards,
Hans
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-03 16:35 ` Hans Zhang
@ 2025-04-07 17:03 ` Ilpo Järvinen
2025-04-08 12:19 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-07 17:03 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 12082 bytes --]
On Fri, 4 Apr 2025, Hans Zhang wrote:
>
>
> On 2025/4/3 20:24, Hans Zhang wrote:
> > > > > > }
> > > > > > EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
> > > > > > @@ -648,7 +614,6 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
> > > > > > static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos,
> > > > > > int
> > > > > > ht_cap)
> > > > > > {
> > > > > > - int rc, ttl = PCI_FIND_CAP_TTL;
> > > > > > u8 cap, mask;
> > > > > > if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap ==
> > > > > > HT_CAPTYPE_HOST)
> > > > > > @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
> > > > > > *dev,
> > > > > > u8 pos, int ht_cap)
> > > > > > mask = HT_5BIT_CAP_MASK;
> > > > > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> > > > > > - PCI_CAP_ID_HT, &ttl);
> > > > > > + PCI_CAP_ID_HT);
> > > > > > while (pos) {
> > > > > > rc = pci_read_config_byte(dev, pos + 3, &cap);
> > > > > > if (rc != PCIBIOS_SUCCESSFUL)
> > > > > > @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev
> > > > > > *dev,
> > > > > > u8 pos, int ht_cap)
> > > > > > pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> > > > > > pos + PCI_CAP_LIST_NEXT,
> > > > > > - PCI_CAP_ID_HT, &ttl);
> > > > > > + PCI_CAP_ID_HT);
> > > > >
> > > > > This function kind of had the idea to share the ttl but I suppose that
> > > > > was
> > > > > just a final safeguard to make sure the loop will always terminate in
> > > > > case
> > > > > the config space is corrupted so the unsharing is not a big issue.
> > > > >
> > > >
> > > > __pci_find_next_cap_ttl
> > > > // This macro definition already has ttl loop restrictions inside it.
> > > > PCI_FIND_NEXT_CAP_TTL
> > > >
> > > > Do I understand that you agree to remove ttl initialization and
> > > > parameter
> > > > passing?
> > >
> > > Yes, I agree with it but doing anything like this (although I'd mention
> > > the reasoning in the changelog myself).
> > >
> >
> > Ok, I see. I will give the reasons.
>
> Hi Ilpo,
>
> The [v9 3/6]patch I plan to submit is as follows, please review it.
>
> From 6da415d130e76b57ecf401f14bf0b66f20407839 Mon Sep 17 00:00:00 2001
> From: Hans Zhang <18255117159@163.com>
> Date: Fri, 4 Apr 2025 00:20:29 +0800
> Subject: [v9 3/6] PCI: Refactor capability search into common macros
>
> - Capability search is done both in PCI core and some controller drivers.
> - PCI core's cap search func requires PCI device and bus structs exist.
> - Controller drivers cannot use PCI core's cap search func as they
> need to find capabilities before they instantiated the PCI device & bus
> structs.
>
> - Move capability search into a macro so it can be reused where normal
> PCI config space accessors cannot yet be used due to lack of the
> instantiated PCI dev.
> - Instead, give the config space reading function as an argument to the
> new macro.
> - Convert PCI core to use the new macro.
None of these bullets are true lists so please write them as normal
English paragraphs. Also please extend some of shortened words lke "cap"
--> "Capability", "PCI dev" -> PCI Device (for terms, the capitalization
of the first letter, you should follow what the PCI specs use).
--
i.
>
> The macros now implement, parameterized by the config access method. The
> PCI core functions are converted to utilize these macros with the standard
> pci_bus_read_config accessors. Controller drivers can later use the same
> macros with their early access mechanisms while maintaining the existing
> protection against infinite loops through preserved TTL checks.
>
> The ttl parameter was originally an additional safeguard to prevent
> infinite loops in corrupted config space. However, the
> PCI_FIND_NEXT_CAP_TTL macro already enforces a TTL limit internally.
> Removing redundant ttl handling simplifies the interface while maintaining
> the safety guarantee. This aligns with the macro's design intent of
> encapsulating TTL management.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> drivers/pci/pci.c | 70 +++++---------------------------------
> drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 95 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e4d3719b653d..bef242d84ab4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -9,7 +9,6 @@
> */
>
> #include <linux/acpi.h>
> -#include <linux/align.h>
> #include <linux/kernel.h>
> #include <linux/delay.h>
> #include <linux/dmi.h>
> @@ -31,7 +30,6 @@
> #include <asm/dma.h>
> #include <linux/aer.h>
> #include <linux/bitfield.h>
> -#include <uapi/linux/pci_regs.h>
> #include "pci.h"
>
> DEFINE_MUTEX(pci_slot_mutex);
> @@ -426,35 +424,16 @@ static int pci_dev_str_match(struct pci_dev *dev, const
> char *p,
> }
>
> static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> - u8 pos, int cap, int *ttl)
> + u8 pos, int cap)
> {
> - u8 id;
> - u16 ent;
> -
> - pci_bus_read_config_byte(bus, devfn, pos, &pos);
> -
> - while ((*ttl)--) {
> - if (pos < PCI_STD_HEADER_SIZEOF)
> - break;
> - pos = ALIGN_DOWN(pos, 4);
> - pci_bus_read_config_word(bus, devfn, pos, &ent);
> -
> - id = FIELD_GET(PCI_CAP_ID_MASK, ent);
> - if (id == 0xff)
> - break;
> - if (id == cap)
> - return pos;
> - pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
> - }
> - return 0;
> + 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,
> u8 pos, int cap)
> {
> - int ttl = PCI_FIND_CAP_TTL;
> -
> - return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> + return __pci_find_next_cap_ttl(bus, devfn, pos, cap);
> }
>
> u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -555,42 +534,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);
>
> @@ -650,7 +598,7 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
>
> static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
> {
> - int rc, ttl = PCI_FIND_CAP_TTL;
> + int rc;
> u8 cap, mask;
>
> if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> @@ -659,7 +607,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8
> pos, int ht_cap)
> mask = HT_5BIT_CAP_MASK;
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> while (pos) {
> rc = pci_read_config_byte(dev, pos + 3, &cap);
> if (rc != PCIBIOS_SUCCESSFUL)
> @@ -670,7 +618,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8
> pos, int ht_cap)
>
> pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> pos + PCI_CAP_LIST_NEXT,
> - PCI_CAP_ID_HT, &ttl);
> + PCI_CAP_ID_HT);
> }
>
> return 0;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 6a7c88b9cd35..b204ebeeb1cf 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -2,7 +2,9 @@
> #ifndef DRIVERS_PCI_H
> #define DRIVERS_PCI_H
>
> +#include <linux/align.h>
> #include <linux/pci.h>
> +#include <uapi/linux/pci_regs.h>
>
> struct pcie_tlp_log;
>
> @@ -91,6 +93,90 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 size,
> u32 *val);
>
> +/* Standard Capability finder */
> +/**
> + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search
> + * @cap: Capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Iterates through the capability list in PCI config space to find
> + * the specified capability. Implements TTL (time-to-live) protection
> + * against infinite loops.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...) \
> +({ \
> + int __ttl = PCI_FIND_CAP_TTL; \
> + u8 __id, __found_pos = 0; \
> + u8 __pos = (start); \
> + u16 __ent; \
> + \
> + 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 */
> +/**
> + * PCI_FIND_NEXT_EXT_CAPABILITY - Find a PCI extended capability
> + * @read_cfg: Function pointer for reading PCI config space
> + * @start: Starting position to begin search (0 for initial search)
> + * @cap: Extended capability ID to find
> + * @args: Arguments to pass to read_cfg function
> + *
> + * Searches the extended capability space in PCI config registers
> + * for the specified capability. Implements TTL protection against
> + * infinite loops using a calculated maximum search count.
> + *
> + * Returns: Position of the capability if found, 0 otherwise.
> + */
> +#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;
> \
> +})
> +
> /* Functions internal to the PCI core code */
>
> #ifdef CONFIG_DMI
>
>
>
> Best regards,
> Hans
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-07 17:03 ` Ilpo Järvinen
@ 2025-04-08 12:19 ` Hans Zhang
2025-04-08 16:18 ` Ilpo Järvinen
0 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-08 12:19 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/8 01:03, Ilpo Järvinen wrote:
>> Hi Ilpo,
>>
>> The [v9 3/6]patch I plan to submit is as follows, please review it.
>>
>> From 6da415d130e76b57ecf401f14bf0b66f20407839 Mon Sep 17 00:00:00 2001
>> From: Hans Zhang<18255117159@163.com>
>> Date: Fri, 4 Apr 2025 00:20:29 +0800
>> Subject: [v9 3/6] PCI: Refactor capability search into common macros
>>
>> - Capability search is done both in PCI core and some controller drivers.
>> - PCI core's cap search func requires PCI device and bus structs exist.
>> - Controller drivers cannot use PCI core's cap search func as they
>> need to find capabilities before they instantiated the PCI device & bus
>> structs.
>>
>> - Move capability search into a macro so it can be reused where normal
>> PCI config space accessors cannot yet be used due to lack of the
>> instantiated PCI dev.
>> - Instead, give the config space reading function as an argument to the
>> new macro.
>> - Convert PCI core to use the new macro.
> None of these bullets are true lists so please write them as normal
> English paragraphs. Also please extend some of shortened words lke "cap"
> --> "Capability", "PCI dev" -> PCI Device (for terms, the capitalization
> of the first letter, you should follow what the PCI specs use).
>
Dear Ilpo,
Thank you very much for your reply. Is it OK to modify it like this?
The PCI Capability search functionality is duplicated across the PCI
core and several controller drivers. The core's current implementation
requires fully initialized PCI device and bus structures, which prevents
controller drivers from using it during early initialization phases
before these structures are available.
Move the Capability search logic into a header-based macro that accepts
a config space accessor function as an argument. This enables
controller drivers to perform Capability discovery using their early
access mechanisms prior to full device initialization while maintaining
the original search behavior.
Convert the existing PCI core Capability search implementation to use
this new macro, eliminating code duplication. The refactoring preserves
the original functionality without behavioral changes, while allowing
both the core and drivers to share common Capability discovery logic.
Best regards,
Hans
>> The macros now implement, parameterized by the config access method. The
>> PCI core functions are converted to utilize these macros with the standard
>> pci_bus_read_config accessors. Controller drivers can later use the same
>> macros with their early access mechanisms while maintaining the existing
>> protection against infinite loops through preserved TTL checks.
>>
>> The ttl parameter was originally an additional safeguard to prevent
>> infinite loops in corrupted config space. However, the
>> PCI_FIND_NEXT_CAP_TTL macro already enforces a TTL limit internally.
>> Removing redundant ttl handling simplifies the interface while maintaining
>> the safety guarantee. This aligns with the macro's design intent of
>> encapsulating TTL management.
>>
>> Signed-off-by: Hans Zhang<18255117159@163.com>
>> ---
>> drivers/pci/pci.c | 70 +++++---------------------------------
>> drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 95 insertions(+), 61 deletions(-)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-08 12:19 ` Hans Zhang
@ 2025-04-08 16:18 ` Ilpo Järvinen
2025-04-09 1:37 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: Ilpo Järvinen @ 2025-04-08 16:18 UTC (permalink / raw)
To: Hans Zhang
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
[-- Attachment #1: Type: text/plain, Size: 3803 bytes --]
On Tue, 8 Apr 2025, Hans Zhang wrote:
>
>
> On 2025/4/8 01:03, Ilpo Järvinen wrote:
> > > Hi Ilpo,
> > >
> > > The [v9 3/6]patch I plan to submit is as follows, please review it.
> > >
> > > From 6da415d130e76b57ecf401f14bf0b66f20407839 Mon Sep 17 00:00:00 2001
> > > From: Hans Zhang<18255117159@163.com>
> > > Date: Fri, 4 Apr 2025 00:20:29 +0800
> > > Subject: [v9 3/6] PCI: Refactor capability search into common macros
> > >
> > > - Capability search is done both in PCI core and some controller drivers.
> > > - PCI core's cap search func requires PCI device and bus structs exist.
> > > - Controller drivers cannot use PCI core's cap search func as they
> > > need to find capabilities before they instantiated the PCI device & bus
> > > structs.
> > >
> > > - Move capability search into a macro so it can be reused where normal
> > > PCI config space accessors cannot yet be used due to lack of the
> > > instantiated PCI dev.
> > > - Instead, give the config space reading function as an argument to the
> > > new macro.
> > > - Convert PCI core to use the new macro.
> > None of these bullets are true lists so please write them as normal
> > English paragraphs. Also please extend some of shortened words lke "cap"
> > --> "Capability", "PCI dev" -> PCI Device (for terms, the capitalization
> > of the first letter, you should follow what the PCI specs use).
> >
>
> Dear Ilpo,
>
> Thank you very much for your reply. Is it OK to modify it like this?
>
> The PCI Capability search functionality is duplicated across the PCI core and
> several controller drivers. The core's current implementation
> requires fully initialized PCI device and bus structures, which prevents
> controller drivers from using it during early initialization phases before
> these structures are available.
>
> Move the Capability search logic into a header-based macro that accepts a
> config space accessor function as an argument. This enables controller
> drivers to perform Capability discovery using their early access mechanisms
> prior to full device initialization while maintaining the original search
> behavior.
... while maintaining ... ->
... while sharing the Capability search code.
>
> Convert the existing PCI core Capability search implementation to use this new
> macro
I think the rest of this paragraph after this are unnecessary.
> , eliminating code duplication. The refactoring preserves the original
> functionality without behavioral changes, while allowing both the core and
> drivers to share common Capability discovery logic.
Other than that, it seemed good enough for me.
--
i.
> > > The macros now implement, parameterized by the config access method. The
> > > PCI core functions are converted to utilize these macros with the standard
> > > pci_bus_read_config accessors. Controller drivers can later use the same
> > > macros with their early access mechanisms while maintaining the existing
> > > protection against infinite loops through preserved TTL checks.
> > >
> > > The ttl parameter was originally an additional safeguard to prevent
> > > infinite loops in corrupted config space. However, the
> > > PCI_FIND_NEXT_CAP_TTL macro already enforces a TTL limit internally.
> > > Removing redundant ttl handling simplifies the interface while maintaining
> > > the safety guarantee. This aligns with the macro's design intent of
> > > encapsulating TTL management.
> > >
> > > Signed-off-by: Hans Zhang<18255117159@163.com>
> > > ---
> > > drivers/pci/pci.c | 70 +++++---------------------------------
> > > drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 95 insertions(+), 61 deletions(-)
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication
2025-04-08 16:18 ` Ilpo Järvinen
@ 2025-04-09 1:37 ` Hans Zhang
0 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-09 1:37 UTC (permalink / raw)
To: Ilpo Järvinen
Cc: lpieralisi, bhelgaas, kw, manivannan.sadhasivam, robh, jingoohan1,
thomas.richard, linux-pci, LKML
On 2025/4/9 00:18, Ilpo Järvinen wrote:
>> Move the Capability search logic into a header-based macro that accepts a
>> config space accessor function as an argument. This enables controller
>> drivers to perform Capability discovery using their early access mechanisms
>> prior to full device initialization while maintaining the original search
>> behavior.
>
> ... while maintaining ... ->
>
> ... while sharing the Capability search code.
>
Dear Ilpo,
Thank you very much for your reply. Will change.
>>
>> Convert the existing PCI core Capability search implementation to use this new
>> macro
>
> I think the rest of this paragraph after this are unnecessary.
>
Will delete.
>> , eliminating code duplication. The refactoring preserves the original
>> functionality without behavioral changes, while allowing both the core and
>> drivers to share common Capability discovery logic.
>
> Other than that, it seemed good enough for me.
>
Best regards,
Hans
^ permalink raw reply [flat|nested] 25+ messages in thread
* [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
2025-04-02 4:20 [v7 0/5] Refactor capability search into common macros Hans Zhang
2025-04-02 4:20 ` [v7 1/5] PCI: " Hans Zhang
2025-04-02 4:20 ` [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication Hans Zhang
@ 2025-04-02 4:20 ` Hans Zhang
2025-04-02 11:58 ` kernel test robot
2025-04-02 4:20 ` [v7 4/5] PCI: cadence: " Hans Zhang
2025-04-02 4:20 ` [v7 5/5] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode Hans Zhang
4 siblings, 1 reply; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 4:20 UTC (permalink / raw)
To: lpieralisi, bhelgaas
Cc: kw, manivannan.sadhasivam, ilpo.jarvinen, robh, jingoohan1,
thomas.richard, linux-pci, linux-kernel, Hans Zhang
Use the PCI core is now exposing generic macros for the host bridges to
search for the PCIe capabilities, make use of them in the DWC driver.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v6:
https://lore.kernel.org/linux-pci/20250323164852.430546-3-18255117159@163.com/
- The patch commit message were modified.
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-3-18255117159@163.com/
- Kconfig add "select PCI_HOST_HELPERS"
---
drivers/pci/controller/dwc/pcie-designware.c | 72 ++------------------
1 file changed, 7 insertions(+), 65 deletions(-)
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
2025-04-02 4:20 ` [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities Hans Zhang
@ 2025-04-02 11:58 ` kernel test robot
2025-04-02 12:18 ` Hans Zhang
0 siblings, 1 reply; 25+ messages in thread
From: kernel test robot @ 2025-04-02 11:58 UTC (permalink / raw)
To: Hans Zhang, lpieralisi, bhelgaas
Cc: oe-kbuild-all, kw, manivannan.sadhasivam, ilpo.jarvinen, robh,
jingoohan1, thomas.richard, linux-pci, linux-kernel, Hans Zhang
Hi Hans,
kernel test robot noticed the following build errors:
[auto build test ERROR on acb4f33713b9f6cadb6143f211714c343465411c]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Refactor-capability-search-into-common-macros/20250402-122544
base: acb4f33713b9f6cadb6143f211714c343465411c
patch link: https://lore.kernel.org/r/20250402042020.48681-4-18255117159%40163.com
patch subject: [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
config: loongarch-randconfig-001-20250402 (https://download.01.org/0day-ci/archive/20250402/202504021958.YeTPCsW1-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250402/202504021958.YeTPCsW1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202504021958.YeTPCsW1-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/pci/controller/dwc/pcie-designware.c:23:
drivers/pci/controller/dwc/pcie-designware.c: In function 'dw_pcie_find_capability':
>> drivers/pci/controller/dwc/pcie-designware.c:218:38: error: 'pcie' undeclared (first use in this function); did you mean 'pci'?
218 | pcie);
| ^~~~
drivers/pci/controller/dwc/../../pci.h:114:18: note: in definition of macro 'PCI_FIND_NEXT_CAP_TTL'
114 | read_cfg(args, __pos, 1, (u32 *)&__pos); \
| ^~~~
drivers/pci/controller/dwc/pcie-designware.c:218:38: note: each undeclared identifier is reported only once for each function it appears in
218 | pcie);
| ^~~~
drivers/pci/controller/dwc/../../pci.h:114:18: note: in definition of macro 'PCI_FIND_NEXT_CAP_TTL'
114 | read_cfg(args, __pos, 1, (u32 *)&__pos); \
| ^~~~
drivers/pci/controller/dwc/pcie-designware.c: In function 'dw_pcie_find_ext_capability':
drivers/pci/controller/dwc/pcie-designware.c:224:71: error: 'pcie' undeclared (first use in this function); did you mean 'pci'?
224 | return PCI_FIND_NEXT_EXT_CAPABILITY(dw_pcie_read_cfg, 0, cap, pcie);
| ^~~~
drivers/pci/controller/dwc/../../pci.h:156:34: note: in definition of macro 'PCI_FIND_NEXT_EXT_CAPABILITY'
156 | __ret = read_cfg(args, __pos, 4, &__header); \
| ^~~~
vim +218 drivers/pci/controller/dwc/pcie-designware.c
214
215 u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
216 {
217 return PCI_FIND_NEXT_CAP_TTL(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
> 218 pcie);
219 }
220 EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
221
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
2025-04-02 11:58 ` kernel test robot
@ 2025-04-02 12:18 ` Hans Zhang
0 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 12:18 UTC (permalink / raw)
To: kernel test robot, lpieralisi, bhelgaas
Cc: oe-kbuild-all, kw, manivannan.sadhasivam, ilpo.jarvinen, robh,
jingoohan1, thomas.richard, linux-pci, linux-kernel
On 2025/4/2 19:58, kernel test robot wrote:
> Hi Hans,
>
> kernel test robot noticed the following build errors:
>
> [auto build test ERROR on acb4f33713b9f6cadb6143f211714c343465411c]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Hans-Zhang/PCI-Refactor-capability-search-into-common-macros/20250402-122544
> base: acb4f33713b9f6cadb6143f211714c343465411c
> patch link: https://lore.kernel.org/r/20250402042020.48681-4-18255117159%40163.com
> patch subject: [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities
> config: loongarch-randconfig-001-20250402 (https://download.01.org/0day-ci/archive/20250402/202504021958.YeTPCsW1-lkp@intel.com/config)
> compiler: loongarch64-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250402/202504021958.YeTPCsW1-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202504021958.YeTPCsW1-lkp@intel.com/
>
> All errors (new ones prefixed by >>):
>
> In file included from drivers/pci/controller/dwc/pcie-designware.c:23:
> drivers/pci/controller/dwc/pcie-designware.c: In function 'dw_pcie_find_capability':
>>> drivers/pci/controller/dwc/pcie-designware.c:218:38: error: 'pcie' undeclared (first use in this function); did you mean 'pci'?
> 218 | pcie);
> | ^~~~
> drivers/pci/controller/dwc/../../pci.h:114:18: note: in definition of macro 'PCI_FIND_NEXT_CAP_TTL'
> 114 | read_cfg(args, __pos, 1, (u32 *)&__pos); \
> | ^~~~
> drivers/pci/controller/dwc/pcie-designware.c:218:38: note: each undeclared identifier is reported only once for each function it appears in
> 218 | pcie);
> | ^~~~
> drivers/pci/controller/dwc/../../pci.h:114:18: note: in definition of macro 'PCI_FIND_NEXT_CAP_TTL'
> 114 | read_cfg(args, __pos, 1, (u32 *)&__pos); \
> | ^~~~
> drivers/pci/controller/dwc/pcie-designware.c: In function 'dw_pcie_find_ext_capability':
> drivers/pci/controller/dwc/pcie-designware.c:224:71: error: 'pcie' undeclared (first use in this function); did you mean 'pci'?
> 224 | return PCI_FIND_NEXT_EXT_CAPABILITY(dw_pcie_read_cfg, 0, cap, pcie);
> | ^~~~
> drivers/pci/controller/dwc/../../pci.h:156:34: note: in definition of macro 'PCI_FIND_NEXT_EXT_CAPABILITY'
> 156 | __ret = read_cfg(args, __pos, 4, &__header); \
> | ^~~~
>
Copy the errors carried over. Will change.
Best regards,
Hans
>
> vim +218 drivers/pci/controller/dwc/pcie-designware.c
>
> 214
> 215 u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
> 216 {
> 217 return PCI_FIND_NEXT_CAP_TTL(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
> > 218 pcie);
> 219 }
> 220 EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
> 221
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [v7 4/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities
2025-04-02 4:20 [v7 0/5] Refactor capability search into common macros Hans Zhang
` (2 preceding siblings ...)
2025-04-02 4:20 ` [v7 3/5] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities Hans Zhang
@ 2025-04-02 4:20 ` Hans Zhang
2025-04-02 4:20 ` [v7 5/5] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode Hans Zhang
4 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 4:20 UTC (permalink / raw)
To: lpieralisi, bhelgaas
Cc: kw, manivannan.sadhasivam, ilpo.jarvinen, robh, jingoohan1,
thomas.richard, linux-pci, linux-kernel, Hans Zhang
Use the PCI core is now exposing generic macros for the host bridges to
search for the PCIe capabilities, make use of them in the CDNS driver.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v6:
https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@163.com/
- The patch commit message were modified.
Changes since v5:
https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
- Kconfig add "select PCI_HOST_HELPERS"
---
drivers/pci/controller/cadence/pcie-cadence.c | 28 +++++++++++++++++++
drivers/pci/controller/cadence/pcie-cadence.h | 13 +++++++++
2 files changed, 41 insertions(+)
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c
index 204e045aed8c..ca4a1a809fcb 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -7,6 +7,34 @@
#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);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_find_capability);
+
+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);
+}
+EXPORT_SYMBOL_GPL(cdns_pcie_find_ext_capability);
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,
--
2.25.1
^ permalink raw reply related [flat|nested] 25+ messages in thread* [v7 5/5] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode.
2025-04-02 4:20 [v7 0/5] Refactor capability search into common macros Hans Zhang
` (3 preceding siblings ...)
2025-04-02 4:20 ` [v7 4/5] PCI: cadence: " Hans Zhang
@ 2025-04-02 4:20 ` Hans Zhang
4 siblings, 0 replies; 25+ messages in thread
From: Hans Zhang @ 2025-04-02 4:20 UTC (permalink / raw)
To: lpieralisi, bhelgaas
Cc: kw, manivannan.sadhasivam, ilpo.jarvinen, robh, jingoohan1,
thomas.richard, linux-pci, linux-kernel, Hans Zhang
The PCIe capability/extended capability offsets are not guaranteed to be
the same across all SoCs integrating the Cadence PCIe IP. Hence, use the
cdns_pcie_find_{ext}_capability() APIs for finding them.
This avoids hardcoding the offsets in the driver.
Signed-off-by: Hans Zhang <18255117159@163.com>
---
Changes since v5~v6:
- None
Changes since v4:
https://lore.kernel.org/linux-pci/20250321101710.371480-5-18255117159@163.com/
- The patch subject and commit message were modified.
---
.../pci/controller/cadence/pcie-cadence-ep.c | 40 +++++++++++--------
drivers/pci/controller/cadence/pcie-cadence.h | 5 ---
2 files changed, 23 insertions(+), 22 deletions(-)
diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
index e0cc4560dfde..aea53ddcaf9b 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
@@ -19,12 +19,13 @@
static u8 cdns_pcie_get_fn_from_vfn(struct cdns_pcie *pcie, u8 fn, u8 vfn)
{
- u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
u32 first_vf_offset, stride;
+ u16 cap;
if (vfn == 0)
return fn;
+ cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
first_vf_offset = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_OFFSET);
stride = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_STRIDE);
fn = fn + first_vf_offset + ((vfn - 1) * stride);
@@ -36,10 +37,11 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
struct pci_epf_header *hdr)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
- u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
struct cdns_pcie *pcie = &ep->pcie;
u32 reg;
+ u16 cap;
+ cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
if (vfn > 1) {
dev_err(&epc->dev, "Only Virtual Function #1 has deviceID\n");
return -EINVAL;
@@ -224,9 +226,10 @@ static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
- u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
u16 flags;
+ u8 cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
/*
@@ -246,9 +249,10 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
- u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
u16 flags, mme;
+ u8 cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
/* Validate that the MSI feature is actually enabled. */
@@ -269,9 +273,10 @@ static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
- u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
u32 val, reg;
+ u8 cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
func_no = cdns_pcie_get_fn_from_vfn(pcie, func_no, vfunc_no);
reg = cap + PCI_MSIX_FLAGS;
@@ -290,9 +295,10 @@ static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u8 vfn,
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
struct cdns_pcie *pcie = &ep->pcie;
- u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
u32 val, reg;
+ u8 cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
reg = cap + PCI_MSIX_FLAGS;
@@ -379,11 +385,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
u8 interrupt_num)
{
struct cdns_pcie *pcie = &ep->pcie;
- u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
u16 flags, mme, data, data_mask;
- u8 msi_count;
u64 pci_addr, pci_addr_mask = 0xff;
+ u8 msi_count, cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -431,14 +437,14 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
u32 *msi_addr_offset)
{
struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
- u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
struct cdns_pcie *pcie = &ep->pcie;
u64 pci_addr, pci_addr_mask = 0xff;
u16 flags, mme, data, data_mask;
- u8 msi_count;
+ u8 msi_count, cap;
int ret;
int i;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
/* Check whether the MSI feature has been enabled by the PCI host. */
@@ -481,16 +487,16 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
u16 interrupt_num)
{
- u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
u32 tbl_offset, msg_data, reg;
struct cdns_pcie *pcie = &ep->pcie;
struct pci_epf_msix_tbl *msix_tbl;
struct cdns_pcie_epf *epf;
u64 pci_addr_mask = 0xff;
u64 msg_addr;
+ u8 bir, cap;
u16 flags;
- u8 bir;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
epf = &ep->epf[fn];
if (vfn > 0)
epf = &epf->epf[vfn - 1];
@@ -564,7 +570,9 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
int max_epfs = sizeof(epc->function_num_map) * 8;
int ret, epf, last_fn;
u32 reg, value;
+ u8 cap;
+ cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
/*
* BIT(0) is hardwired to 1, hence function 0 is always enabled
* and can't be disabled anyway.
@@ -588,12 +596,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
continue;
value = cdns_pcie_ep_fn_readl(pcie, epf,
- CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
- PCI_EXP_DEVCAP);
+ cap + PCI_EXP_DEVCAP);
value &= ~PCI_EXP_DEVCAP_FLR;
- cdns_pcie_ep_fn_writel(pcie, epf,
- CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
- PCI_EXP_DEVCAP, value);
+ cdns_pcie_ep_fn_writel(pcie, epf, cap + PCI_EXP_DEVCAP,
+ value);
}
}
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index dd7cb6b6b291..3cfef0b25988 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -125,11 +125,6 @@
*/
#define CDNS_PCIE_EP_FUNC_BASE(fn) (((fn) << 12) & GENMASK(19, 12))
-#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET 0x90
-#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET 0xb0
-#define CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET 0xc0
-#define CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET 0x200
-
/*
* Endpoint PF Registers
*/
--
2.25.1
^ permalink raw reply related [flat|nested] 25+ messages in thread