Message-ID: <20250814202519.GA344690@bhelgaas>
Date: Thu, 14 Aug 2025 15:25:19 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: 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, schnelle@...ux.ibm.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 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

Powered by blists - more mailing lists