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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Wed, 2 Apr 2025, Hans Zhang wrote:

> Refactor the PCI capability and extended capability search functions
> by consolidating common code patterns into reusable macros
> (PCI_FIND_NEXT_CAP_TTL and PCI_FIND_NEXT_EXT_CAPABILITY). The main
> changes include:
> 
> 1. Introducing a unified config space read helper (__pci_bus_read_config).
> 2. Removing duplicate search logic from __pci_find_next_cap_ttl and
>    pci_find_next_ext_capability.
> 3. Implementing consistent capability discovery using the new macros.
> 4. Simplifying HyperTransport capability lookup by leveraging the
>    refactored code.
> 
> The refactoring maintains existing functionality while reducing code
> duplication and improving maintainability. By centralizing the search
> logic, we achieve better code consistency and make future updates easier.
> 
> This change has been verified to maintain backward compatibility with
> existing capability discovery patterns through thorough testing of PCI
> device enumeration and capability probing.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/pci.c | 79 +++++++++++++----------------------------------
>  1 file changed, 22 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..521096c73686 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -423,36 +423,33 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
>  	return 1;
>  }
>  
> -static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> -				  u8 pos, int cap, int *ttl)
> +static int __pci_bus_read_config(void *priv, unsigned int devfn, int where,
> +				 u32 size, u32 *val)

This probably should be where the other accessors are so in access.c. I'd 
put its prototype into drivers/pci/pci.h only for now.

>  {
> -	u8 id;
> -	u16 ent;
> +	struct pci_bus *bus = priv;
> +	int ret;
>  
> -	pci_bus_read_config_byte(bus, devfn, pos, &pos);
> +	if (size == 1)
> +		ret = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
> +	else if (size == 2)
> +		ret = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> +	else
> +		ret = pci_bus_read_config_dword(bus, devfn, where, val);
>  
> -	while ((*ttl)--) {
> -		if (pos < 0x40)
> -			break;
> -		pos &= ~3;
> -		pci_bus_read_config_word(bus, devfn, pos, &ent);
> +	return ret;
> +}
>  
> -		id = ent & 0xff;
> -		if (id == 0xff)
> -			break;
> -		if (id == cap)
> -			return pos;
> -		pos = (ent >> 8);
> -	}
> -	return 0;
> +static u8 __pci_find_next_cap_ttl(struct pci_bus *bus, unsigned int devfn,
> +				  u8 pos, int cap)
> +{
> +	return PCI_FIND_NEXT_CAP_TTL(__pci_bus_read_config, pos, cap, bus,
> +				     devfn);
>  }
>  
>  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(bus, devfn, pos, cap);
>  }
>  
>  u8 pci_find_next_capability(struct pci_dev *dev, u8 pos, int cap)
> @@ -553,42 +550,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);

I don't like how 1 & 2 patches are split into two. IMO, they mostly belong 
together. However, (IMO) you can introduce the new all-size config space 
accessor in a separate patch before the combined patch.

>  }
>  EXPORT_SYMBOL_GPL(pci_find_next_ext_capability);
>  
> @@ -648,7 +614,6 @@ 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;
>  	u8 cap, mask;
>  
>  	if (ht_cap == HT_CAPTYPE_SLAVE || ht_cap == HT_CAPTYPE_HOST)
> @@ -657,7 +622,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>  		mask = HT_5BIT_CAP_MASK;
>  
>  	pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn, pos,
> -				      PCI_CAP_ID_HT, &ttl);
> +				      PCI_CAP_ID_HT);
>  	while (pos) {
>  		rc = pci_read_config_byte(dev, pos + 3, &cap);
>  		if (rc != PCIBIOS_SUCCESSFUL)
> @@ -668,7 +633,7 @@ static u8 __pci_find_next_ht_cap(struct pci_dev *dev, u8 pos, int ht_cap)
>  
>  		pos = __pci_find_next_cap_ttl(dev->bus, dev->devfn,
>  					      pos + PCI_CAP_LIST_NEXT,
> -					      PCI_CAP_ID_HT, &ttl);
> +					      PCI_CAP_ID_HT);

This function kind of had the idea to share the ttl but I suppose that was
just a final safeguard to make sure the loop will always terminate in case 
the config space is corrupted so the unsharing is not a big issue.

>  	}
>  
>  	return 0;
> 

-- 
 i.