| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9e9a68b1-8c3a-6132-d4fc-9f7b0b2d3e3a@linux.intel.com>
Date: Thu, 3 Apr 2025 12:10:41 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: lpieralisi@...nel.org, bhelgaas@...gle.com, kw@...ux.com,
manivannan.sadhasivam@...aro.org, robh@...nel.org, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v7 1/5] PCI: Refactor capability search into common macros
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@....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 */
> > >
> >
>
Powered by blists - more mailing lists