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

[Hans Zhang <18255117159@163.com>]

On 2025/4/3 17:15, Ilpo Järvinen wrote:
>>> 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);
>>
>> +
> 
> Extra newline
> 

Hi Ilpo,

Thanks your for reply. Will delete.

>> +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?
> 
> Yes, I agree with it but doing anything like this (although I'd mention
> the reasoning in the changelog myself).
> 

Ok, I see. I will give the reasons.

Best regards,
Hans