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

[Hans Zhang <18255117159@163.com>]

On 2025/4/2 20:38, Ilpo Järvinen wrote:
> 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.
> 

Hi Ilpo,

Thank you very much for your guidance. Will change.


>>   {
>> -	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.
> 

Ok. I'll change it to the following. The rest I'll combine into a patch.

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index b123da16b63b..bb2e26c2eb81 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,6 +85,23 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
  EXPORT_SYMBOL(pci_bus_write_config_word);
  EXPORT_SYMBOL(pci_bus_write_config_dword);

+
+int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 
size,
+			u32 *val)
+{
+	struct pci_bus *bus = priv;
+	int ret;
+
+	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);
+
+	return ret;
+}
+
  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
  			    int where, int size, u32 *val)
  {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 2e9cf26a9ee9..6a7c88b9cd35 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -88,6 +88,8 @@ extern bool pci_early_dump;
  bool pcie_cap_has_lnkctl(const struct pci_dev *dev);
  bool pcie_cap_has_lnkctl2(const struct pci_dev *dev);
  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);

  /* Functions internal to the PCI core code */


>>   }
>>   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.
> 

__pci_find_next_cap_ttl
   // This macro definition already has ttl loop restrictions inside it.
   PCI_FIND_NEXT_CAP_TTL

Do I understand that you agree to remove ttl initialization and 
parameter passing?

Best regards,
Hans

>>   	}
>>   
>>   	return 0;
>>
>