linux-kernel - Re: [v7 2/5] PCI: Refactor capability search functions to eliminate code duplication

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e4db2248-ed8b-d270-d417-9efdca947e8e@linux.intel.com>
Date: Tue, 8 Apr 2025 19:18:28 +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 2/5] PCI: Refactor capability search functions to eliminate
 code duplication

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@....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@....com>
> > > ---
> > >   drivers/pci/pci.c | 70 +++++---------------------------------
> > >   drivers/pci/pci.h | 86 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 95 insertions(+), 61 deletions(-)
>