| Message ID | 20250402042020.48681-2-18255117159@163.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Refactor capability search into common macros | expand |
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 */ >
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 */ >> >
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 */ > > > > > >
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
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
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 */
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(+)