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

Hans Zhang
2025-04-03T12:25:41Z

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