Message-ID: <c8ffdd21-9000-40c2-9f4d-4d6318e730b5@cixtech.com>
Date: Fri, 1 Aug 2025 18:06:16 +0800
From: Hans Zhang <hans.zhang@...tech.com>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: Arnd Bergmann <arnd@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>,
 Gerd Bayer <gbayer@...ux.ibm.com>, Hans Zhang <18255117159@....com>,
 bhelgaas@...gle.com, Alexander Gordeev <agordeev@...ux.ibm.com>,
 Christian Borntraeger <borntraeger@...ux.ibm.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 jingoohan1@...il.com, Krzysztof Wilczyński
 <kwilczynski@...nel.org>, linux-kernel@...r.kernel.org,
 linux-s390@...r.kernel.org, linux-next <linux-next@...r.kernel.org>,
 linux-pci@...r.kernel.org, Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Rob Herring <robh@...nel.org>, Niklas Schnelle <schnelle@...ux.ibm.com>,
 geert@...ux-m68k.org
Subject: Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()



On 2025/8/1 17:47, Manivannan Sadhasivam wrote:
> EXTERNAL EMAIL
> 
> On Fri, Aug 01, 2025 at 05:25:51PM GMT, Hans Zhang wrote:
>>
>>
>> On 2025/8/1 16:18, Manivannan Sadhasivam wrote:
>>> EXTERNAL EMAIL
>>>
>>> On Thu, Jul 31, 2025 at 09:01:17PM GMT, Arnd Bergmann wrote:
>>>> On Thu, Jul 31, 2025, at 20:39, Bjorn Helgaas wrote:
>>>>> On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
>>>>>>
>>>>>> -  if (size == 1)
>>>>>> -          return pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>> -  else if (size == 2)
>>>>>> -          return pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
>>>>>> -  else if (size == 4)
>>>>>> -          return pci_bus_read_config_dword(bus, devfn, where, val);
>>>>>> -  else
>>>>>> -          return PCIBIOS_BAD_REGISTER_NUMBER;
>>>>>> +  if (size == 1) {
>>>>>> +          rc = pci_bus_read_config_byte(bus, devfn, where, (u8 *)val);
>>>>>> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>>>>>> +          *val = ((*val >> 24) & 0xff);
>>>>>> +#endif
>>>>>
>>>>> Yeah, this is all pretty ugly.  Obviously the previous code in
>>>>> __pci_find_next_cap_ttl() didn't need this.  My guess is that was
>>>>> because the destination for the read data was always the correct type
>>>>> (u8/u16/u32), but here we always use a u32 and cast it to the
>>>>> appropriate type.  Maybe we can use the correct types here instead of
>>>>> the casts?
>>>>
>>>> Agreed, the casts here just add more potential for bugs.
>>>>
>>>
>>> Ack. Missed the obvious issue during review.
>>>
>>>> The pci_bus_read_config() interface itself may have been a
>>>> mistake, can't the callers just use the underlying helpers
>>>> directly?
>>>>
>>>
>>> They can! Since the callers of this API is mostly the macros, we can easily
>>> implement the logic to call relevant accessors based on the requested size.
>>>
>>> Hans, could you please respin the series based the feedback since the series is
>>> dropped for 6.17.
>>>
>>
>> Dear all,
>>
>> I am once again deeply sorry for the problems that occurred in this series.
>> I only test pulling the ARM platform.
>>
>> Thank you very much, Gerd, for reporting the problem.
>>
>> Thank you all for your discussions and suggestions for revision.
>>
>> Hi Mani,
>>
>> Geert provided a solution. My patch based on this is as follows. Please
>> check if there are any problems.
>> https://lore.kernel.org/linux-pci/CAMuHMdVwFeV46oCid_sMHjXfP+yyGTpBfs9t3uaa=wRxNcSOAQ@mail.gmail.com/
>>
>> Also, please ask Gerd to help test whether it works properly. Thank you very
>> much.
>>
>>
>> If there are no issues, am I sending the new version? Can this series of
>> pacth 0001 be directly replaced?
>>
> 
> What benefit does this helper provide if it simply invokes the accessors based
> on the requested size? IMO, the API should not return 'int' sized value if the
> caller has explicitly requested to read variable size from config space.
> 

Dear Mani,

This newly added macro definition PCI_FIND_NEXT_CAP is derived from 
__pci_find_next_cap_ttl. Another newly added macro definition, 
PCI_FIND_NEXT_EXT_CAP, is derived from pci_find_next_ext_capability. The 
first one has no return value judgment, while the second one has a 
judgment return value. So, pci_bus_read_config is defined as having an 
int return value.

Best regards,
Hans

> 
>>
>>
>>
>> The patch is as follows:
>> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
>> index ba66f55d2524..2bfd8fc1c0f5 100644
>> --- a/drivers/pci/access.c
>> +++ b/drivers/pci/access.c
>> @@ -89,15 +89,25 @@ int pci_bus_read_config(void *priv, unsigned int devfn,
>> int where, u32 size,
>>                          u32 *val)
>>   {
>>          struct pci_bus *bus = priv;
>> +       int rc;
>> +
>> +       if (size == 1) {
>> +               u8 byte;
>> +
>> +               rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
>> +               *val = byte;
>> +       } else if (size == 2) {
>> +               u16 word;
>> +
>> +               rc = pci_bus_read_config_word(bus, devfn, where, &word);
>> +               *val = word;
>> +       } else if (size == 4) {
>> +               rc = pci_bus_read_config_dword(bus, devfn, where, val);
>> +       } else {
>> +               rc = PCIBIOS_BAD_REGISTER_NUMBER;
>> +       }
>>
>> -       if (size == 1)
>> -               return pci_bus_read_config_byte(bus, devfn, where, (u8
>> *)val);
>> -       else if (size == 2)
>> -               return pci_bus_read_config_word(bus, devfn, where, (u16
>> *)val);
>> -       else if (size == 4)
>> -               return pci_bus_read_config_dword(bus, devfn, where, val);
>> -       else
>> -               return PCIBIOS_BAD_REGISTER_NUMBER;
>> +       return rc;
>>   }
>>
>>   int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>>
>>
>>
>> Best regards,
>> Hans
>>
>>
>>
> 
> --
> மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists