Message-ID: <fbc95b8020a152d487ada00e9751c2b3f602c11d.camel@linux.ibm.com>
Date: Thu, 14 Aug 2025 22:37:03 +0200
From: Niklas Schnelle <schnelle@...ux.ibm.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Hans Zhang <18255117159@....com>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
        jingoohan1@...il.com, mani@...nel.org, robh@...nel.org,
        ilpo.jarvinen@...ux.intel.com, gbayer@...ux.ibm.com, lukas@...ner.de,
        arnd@...nel.org, geert@...ux-m68k.org, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 0/6] Refactor capability search into common macros

On Thu, 2025-08-14 at 15:25 -0500, Bjorn Helgaas wrote:
> On Wed, Aug 13, 2025 at 10:45:23PM +0800, Hans Zhang wrote:
> > Dear Maintainers,
> > 
> > This patch series addresses long-standing code duplication in PCI
> > capability discovery logic across the PCI core and controller drivers.
> > The existing implementation ties capability search to fully initialized
> > PCI device structures, limiting its usability during early controller
> > initialization phases where device/bus structures may not yet be
> > available.
> > 
> > The primary goal is to decouple capability discovery from PCI device
> > dependencies by introducing a unified framework using config space
> > accessor-based macros. This enables:
> > 
> > 1. Early Capability Discovery: Host controllers (e.g., Cadence, DWC)
> > can now perform capability searches during pre-initialization stages
> > using their native config accessors.
> > 
> > 2. Code Consolidation: Common logic for standard and extended capability
> > searches is refactored into shared macros (`PCI_FIND_NEXT_CAP` and
> > `PCI_FIND_NEXT_EXT_CAP`), eliminating redundant implementations.
> > 
> > 3. Safety and Maintainability: TTL checks are centralized within the
> > macros to prevent infinite loops, while hardcoded offsets in drivers
> > are replaced with dynamic discovery, reducing fragility.
> > 
> > Key improvements include:  
> > - Driver Conversions: DesignWare and Cadence drivers are migrated to
> >   use the new macros, removing device-specific assumptions and ensuring
> >   consistent error handling.
> > 
> > - Enhanced Readability: Magic numbers are replaced with symbolic
> >   constants, and config space accessors are standardized for clarity.
> > 
> > - Backward Compatibility: Existing PCI core behavior remains unchanged.
> > 
> > ---
> > Dear Niklas and Gerd,
> > 
> > Can you test this series of patches on the s390?
> > 
> > Thank you very much.
> > ---
> > 
> > ---
> > Changes since v14:
> > - Delete the first patch in the v14 series.
> > - The functions that can obtain the values of the registers u8/u16/u32 are
> >   directly called in PCI_FIND_NEXT_CAP() and PCI_FIND_NEXT_EXT_CAP().
> >   Use the pci_bus_read_config_byte/word/dword function directly.
> > - Delete dw_pcie_read_cfg and redefine three functions: dw_pcie_read_cfg_byte/word/dword.
> > - Delete cdns_pcie_read_cfg and redefine three functions: cdns_pcie_read_cfg_byte/word/dword.
> > 
> > Changes since v13:
> > - Split patch 3/6 into two patches for searching standard and extended capability. (Bjorn)
> > - Optimize the code based on the review comments from Bjorn.
> > - Patch 5/7 and 6/7 use simplified macro definitions: PCI_FIND_NEXT_CAP(), PCI_FIND_NEXT_EXT_CAP().
> > - The other patches have not been modified.
> > 
> > Changes since v12:
> > - Modify some commit messages, code format issues, and optimize the function return values.
> > 
> > Changes since v11:
> > - Resolved some compilation warning.
> > - Add some include.
> > - Add the *** BLURB HERE *** description(Corrected by Mani and Krzysztof).
> > 
> > Changes since v10:
> > - The patch [v10 2/6] remove #include <uapi/linux/pci_regs.h> and add macro definition comments.
> > - The patch [v10 3/6] remove #include <uapi/linux/pci_regs.h> and commit message were modified.
> > - The other patches have not been modified.
> > 
> > Changes since v9:
> > - Resolved [v9 4/6] compilation error.
> >   The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses 
> >   dw_pcie_find_next_ext_capability.
> > - The other patches have not been modified.
> > 
> > Changes since v8:
> > - Split patch.
> > - The patch commit message were modified.
> > - Other patches(4/6, 5/6, 6/6) are unchanged.
> > 
> > Changes since v7:
> > - Patch 2/5 and 3/5 compilation error resolved.
> > - Other patches are unchanged.
> > 
> > Changes since v6:
> > - Refactor capability search into common macros.
> > - Delete pci-host-helpers.c and MAINTAINERS.
> > 
> > Changes since v5:
> > - If you put the helpers in drivers/pci/pci.c, they unnecessarily enlarge
> >   the kernel's .text section even if it's known already at compile time
> >   that they're never going to be used (e.g. on x86).
> > - Move the API for find capabilitys to a new file called
> >   pci-host-helpers.c.
> > - Add new patch for MAINTAINERS.
> > 
> > Changes since v4:
> > - Resolved [v4 1/4] compilation warning.
> > - The patch subject and commit message were modified.
> > 
> > Changes since v3:
> > - Resolved [v3 1/4] compilation error.
> > - Other patches are not modified.
> > 
> > Changes since v2:
> > - Add and split into a series of patches.
> > ---
> > 
> > Hans Zhang (6):
> >   PCI: Clean up __pci_find_next_cap_ttl() readability
> >   PCI: Refactor capability search into PCI_FIND_NEXT_CAP()
> >   PCI: Refactor extended capability search into PCI_FIND_NEXT_EXT_CAP()
> >   PCI: dwc: Use PCI core APIs to find capabilities
> >   PCI: cadence: Use PCI core APIs to find capabilities
> >   PCI: cadence: Use cdns_pcie_find_*capability() to avoid hardcoding
> >     offsets
> > 
> >  .../pci/controller/cadence/pcie-cadence-ep.c  | 38 +++++----
> >  drivers/pci/controller/cadence/pcie-cadence.c | 14 +++
> >  drivers/pci/controller/cadence/pcie-cadence.h | 39 +++++++--
> >  drivers/pci/controller/dwc/pcie-designware.c  | 77 ++---------------
> >  drivers/pci/controller/dwc/pcie-designware.h  | 21 +++++
> >  drivers/pci/pci.c                             | 76 +++--------------
> >  drivers/pci/pci.h                             | 85 +++++++++++++++++++
> >  include/uapi/linux/pci_regs.h                 |  3 +
> >  8 files changed, 194 insertions(+), 159 deletions(-)
> 
> I applied this on pci/capability-search for v6.18, thanks!
> 
> Niklas, I added your Tested-by, omitting the dwc and cadence patches
> because I think you tested s390 and probably didn't exercise dwc or
> cadence.  Thanks very much to you and Gerd for finding the issue and
> testing the resolution!
> 
> Bjorn

Thanks, yes leaving out dwc and cadence makes sense. Though I do often
also test on my private x86 systems this one was s390 only. Since I
have you here and as you applied this one now and Lukas PCI/ERR stuff
yesterday, is it possible that my series titled "PCI/ERR: s390/pci: Use
pci_uevent_ers() in PCI recovery" somehow fell through your mail
filters maybe due to not having any "PCI:" in a subject?

Thanks,
Niklas

Powered by blists - more mailing lists