From: Hans Zhang <hans.zhang@cixtech•com>
To: Manivannan Sadhasivam <mani@kernel•org>
Cc: "Arnd Bergmann" <arnd@kernel•org>,
"Bjorn Helgaas" <helgaas@kernel•org>,
"Gerd Bayer" <gbayer@linux•ibm.com>,
"Hans Zhang" <18255117159@163•com>,
bhelgaas@google•com, "Alexander Gordeev" <agordeev@linux•ibm.com>,
"Christian Borntraeger" <borntraeger@linux•ibm.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux•intel.com>,
jingoohan1@gmail•com,
"Krzysztof Wilczyński" <kwilczynski@kernel•org>,
linux-kernel@vger•kernel.org, linux-s390@vger•kernel.org,
linux-next <linux-next@vger•kernel.org>,
linux-pci@vger•kernel.org,
"Lorenzo Pieralisi" <lpieralisi@kernel•org>,
"Rob Herring" <robh@kernel•org>,
"Niklas Schnelle" <schnelle@linux•ibm.com>,
geert@linux-m68k•org
Subject: Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()
Date: Fri, 1 Aug 2025 18:06:16 +0800 [thread overview]
Message-ID: <c8ffdd21-9000-40c2-9f4d-4d6318e730b5@cixtech.com> (raw)
In-Reply-To: <ofsbfhor5ah3yzvkc5g5kb4fpjlzoqkkzukctmr3f6ur4vl2e7@7zvudt63ucbk>
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
>>
>>
>>
>
> --
> மணிவண்ணன் சதாசிவம்
next prev parent reply other threads:[~2025-08-01 10:06 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20250716231121.GA2564572@bhelgaas>
2025-07-31 7:32 ` [REGRESSION] next/master: suspect endianness issue in common PCI capability search macro Gerd Bayer
2025-07-31 17:38 ` [PATCH] PCI: Fix endianness issues in pci_bus_read_config() Gerd Bayer
2025-07-31 18:39 ` Bjorn Helgaas
2025-07-31 19:01 ` Arnd Bergmann
2025-08-01 8:18 ` Manivannan Sadhasivam
2025-08-01 9:25 ` Hans Zhang
2025-08-01 9:47 ` Manivannan Sadhasivam
2025-08-01 10:06 ` Hans Zhang [this message]
2025-08-01 10:54 ` Manivannan Sadhasivam
2025-08-01 11:30 ` Gerd Bayer
2025-08-01 16:54 ` Hans Zhang
2025-08-01 18:08 ` Keith Busch
2025-08-02 15:23 ` Hans Zhang
2025-08-02 15:40 ` Arnd Bergmann
2025-08-04 3:06 ` Hans Zhang
2025-08-04 8:03 ` Arnd Bergmann
2025-08-04 8:25 ` Hans Zhang
2025-08-04 10:09 ` Gerd Bayer
2025-08-12 14:44 ` Hans Zhang
2025-08-13 7:47 ` Niklas Schnelle
2025-08-13 7:50 ` Hans Zhang
2025-08-04 14:33 ` Bjorn Helgaas
2025-08-04 15:04 ` Hans Zhang
2025-08-01 16:47 ` Hans Zhang
2025-07-31 18:53 ` Lukas Wunner
2025-08-01 7:52 ` Geert Uytterhoeven
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c8ffdd21-9000-40c2-9f4d-4d6318e730b5@cixtech.com \
--to=hans.zhang@cixtech$(echo .)com \
--cc=18255117159@163$(echo .)com \
--cc=agordeev@linux$(echo .)ibm.com \
--cc=arnd@kernel$(echo .)org \
--cc=bhelgaas@google$(echo .)com \
--cc=borntraeger@linux$(echo .)ibm.com \
--cc=gbayer@linux$(echo .)ibm.com \
--cc=geert@linux-m68k$(echo .)org \
--cc=helgaas@kernel$(echo .)org \
--cc=ilpo.jarvinen@linux$(echo .)intel.com \
--cc=jingoohan1@gmail$(echo .)com \
--cc=kwilczynski@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-next@vger$(echo .)kernel.org \
--cc=linux-pci@vger$(echo .)kernel.org \
--cc=linux-s390@vger$(echo .)kernel.org \
--cc=lpieralisi@kernel$(echo .)org \
--cc=mani@kernel$(echo .)org \
--cc=robh@kernel$(echo .)org \
--cc=schnelle@linux$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox