Message-ID: <d59fde6e-3e4a-248a-ae56-c35b4c6ec44c@linux.intel.com>
Date: Mon, 21 Jul 2025 11:20:46 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Hans Zhang <18255117159@....com>
cc: Bjorn Helgaas <helgaas@...nel.org>, lpieralisi@...nel.org, 
    bhelgaas@...gle.com, mani@...nel.org, kwilczynski@...nel.org, 
    robh@...nel.org, jingoohan1@...il.com, linux-pci@...r.kernel.org, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v13 3/6] PCI: Refactor capability search into common
 macros

On Thu, 17 Jul 2025, Hans Zhang wrote:

> 
> 
> On 2025/7/16 06:47, Bjorn Helgaas wrote:
> > On Sun, Jun 08, 2025 at 12:14:02AM +0800, Hans Zhang wrote:
> > > 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 sharing the
> > > Capability search code.
> > > 
> > > Convert the existing PCI core Capability search implementation to use this
> > > new macro. 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.
> > 
> > This is a big gulp, but I think I like it :)  It really enables some
> > nice cleanups later.
> > 
> > > -static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int
> > > devfn,
> > > -				  u8 pos, int cap, int *ttl)
> > > -{
> > > -	u8 id;
> > > -	u16 ent;
> > > -
> > > -	pci_bus_read_config_byte(bus, devfn, pos, &pos);
> > > -
> > > -	while ((*ttl)--) {
> > > -		if (pos < PCI_STD_HEADER_SIZEOF)
> > > -			break;
> > > -		pos = ALIGN_DOWN(pos, 4);
> > > -		pci_bus_read_config_word(bus, devfn, pos, &ent);
> > > -
> > > -		id = FIELD_GET(PCI_CAP_ID_MASK, ent);
> > > -		if (id == 0xff)
> > > -			break;
> > > -		if (id == cap)
> > > -			return pos;
> > > -		pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, ent);
> > > -	}
> > > -	return 0;
> > > -}
> > > -
> > >   static u8 __pci_find_next_cap(struct pci_bus *bus, unsigned int devfn,
> > >   			      u8 pos, int cap)
> > >   {
> > > -	int ttl = PCI_FIND_CAP_TTL;
> > > -
> > > -	return __pci_find_next_cap_ttl(bus, devfn, pos, cap, &ttl);
> > > +	return PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config, pos, cap, bus,
> > > devfn);
> > >   }
> > >     u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> > > @@ -554,42 +527,11 @@ EXPORT_SYMBOL(pci_bus_find_capability);
> > >    */
> > >   u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int
> > > cap)
> > >   {
> > > -	u32 header;
> > > -	int ttl;
> > > -	u16 pos = PCI_CFG_SPACE_SIZE;
> > > -
> > > -	/* minimum 8 bytes per capability */
> > > -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> > > -
> > >   	if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
> > >   		return 0;
> > >   -	if (start)
> > > -		pos = start;
> > > -
> > > -	if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
> > > -		return 0;
> > > -
> > > -	/*
> > > -	 * If we have no capabilities, this is indicated by cap ID,
> > > -	 * cap version and next pointer all being 0.
> > > -	 */
> > > -	if (header == 0)
> > > -		return 0;
> > > -
> > > -	while (ttl-- > 0) {
> > > -		if (PCI_EXT_CAP_ID(header) == cap && pos != start)
> > > -			return pos;
> > > -
> > > -		pos = PCI_EXT_CAP_NEXT(header);
> > > -		if (pos < PCI_CFG_SPACE_SIZE)
> > > -			break;
> > > -
> > > -		if (pci_read_config_dword(dev, pos, &header) !=
> > > PCIBIOS_SUCCESSFUL)
> > > -			break;
> > > -	}
> > > -
> > > -	return 0;
> > > +	return PCI_FIND_NEXT_EXT_CAPABILITY(pci_bus_read_config, start, cap,
> > > +					    dev->bus, dev->devfn);
> > >   }
> > >   EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
> > >   @@ -649,7 +591,7 @@ EXPORT_SYMBOL_GPL(pci_get_dsn);
> > >     static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int
> > > ht_cap)
> > >   {
> > > -	int rc, ttl = PCI_FIND_CAP_TTL;
> > > +	int rc;
> > >   	u8 cap, mask;
> > >     	if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> > > @@ -657,8 +599,8 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > >   	else
> > >   		mask = HT_5BIT_CAP_MASK;
> > >   -	pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> > > -				      PCI_CAP_ID_HT, &ttl);
> > > +	pos = PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config, pos,
> > > +				    PCI_CAP_ID_HT, dev->bus, dev->devfn);
> > >   	while (pos) {
> > >   		rc = pci_read_config_byte(dev, pos + 3, &cap);
> > >   		if (rc != PCIBIOS_SUCCESSFUL)
> > > @@ -667,9 +609,10 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev,
> > > u8 pos, int ht_cap)
> > >   		if ((cap & mask) == ht_cap)
> > >   			return pos;
> > >   -		pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
> > > -					      pos + PCI_CAP_LIST_NEXT,
> > > -					      PCI_CAP_ID_HT, &ttl);
> > > +		pos = PCI_FIND_NEXT_CAP_TTL(pci_bus_read_config,
> > > +					    pos + PCI_CAP_LIST_NEXT,
> > > +					    PCI_CAP_ID_HT, dev->bus,
> > > +					    dev->devfn);
> > >   	}
> > >     	return 0;
> > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > > index e7d31ed56731..46fb6b5a854e 100644
> > > --- a/drivers/pci/pci.h
> > > +++ b/drivers/pci/pci.h
> > > @@ -2,6 +2,8 @@
> > >   #ifndef DRIVERS_PCI_H
> > >   #define DRIVERS_PCI_H
> > >   +#include <linux/align.h>
> > > +#include <linux/bitfield.h>
> > >   #include <linux/pci.h>
> > >     struct pcie_tlp_log;
> > > @@ -93,6 +95,89 @@ bool pcie_cap_has_rtctl(const struct pci_dev *dev);
> > >   int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32
> > > size,
> > >   			u32 *val);
> > >   +/* Standard Capability finder */
> > > +/**
> > > + * PCI_FIND_NEXT_CAP_TTL - Find a PCI standard capability
> > 
> > I don't think "_TTL" is relevant in the macro name here.  I see it
> > copied the previous __pci_find_next_cap_ttl() name; "ttl" *was*
> > relevant there, but it isn't anymore.
> > 
> 
> Dear Bjorn,
> 
> Thank you very much for your reply.
> 
> The _TTL suffix will be removed.
> 
> 
> > I would like this a lot better if it could be implemented as a
> > function, but I assume it has to be a macro for some varargs reason.
> > 
> 
> Yes. The macro definitions used in combination with the previous review
> opinions of Ilpo.

The other option would be to standardize the accessor interface signature 
and pass the function as a pointer. One might immediately think of generic 
PCI core accessors making it sound simpler than it is but here the 
complication is the controller drivers want to pass some internal 
structure due to lack of pci_dev so it would need to be void 
*read_cfg_data. Then we'd need to deal with those voids also in some 
generic PCI accessors which is a bit ugly.

> > > + * @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
> > > + * @cap. 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...)
> > > \
> > > +({									\
> > > +	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;						\
> > 
> > I guess this is just lifted directly from __pci_find_next_cap_ttl(),
> > but wow, I wish it weren't quite *so* subtle.  Totally fine though.
> > 
> > Maybe this could be split into one patch for standard capabilities and
> > a second for extended capabilities, just so the connection between
> > this and __pci_find_next_cap_ttl() would be clearer.
> > 
> 
> I will split the two patches.
> 
> > > +									\
> > > +		__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;							\
> > > +})


-- 
 i.

Powered by blists - more mailing lists