Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()

[Hans Zhang <hans.zhang@cixtech.com>]

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?




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