Simple pointer-casts to map byte and word reads from PCI config space
into dwords (i.e. u32) produce unintended results on big-endian systems.
Add the necessary adjustments under compile-time switch
CONFIG_CPU_BIG_ENDIAN.

pci_bus_read_config() was just introduced with
https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/

Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
---

Hi Hans, hi Bjorn,

Sorry to spill this endianness aware code into drivers/pci, feel free to
suggest a cleaner approach. This has fixed the issues seen on s390 systems
Otherwise it is just compile-tested for x86 and arm64.

Since this is still sitting in the a pull-request for upstream, I'm not sure if this
warrants a Fixes: tag.

Thanks,
Gerd
---
 drivers/pci/access.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba66f55d2524..77a73b772a28 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -89,15 +89,24 @@ 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)
-		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
+	} else if (size == 2) {
+		rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
+#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
+		*val = ((*val >> 16) & 0xffff);
+#endif
+	} else if (size == 4) {
+		rc = pci_bus_read_config_dword(bus, devfn, where, val);
+	} else {
+		rc =  PCIBIOS_BAD_REGISTER_NUMBER;
+	}
+	return rc;
 }
 
 int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
-- 
2.48.1

Hi Gerd,

On Thu, 31 Jul 2025 at 20:57, Gerd Bayer <gbayer@linux.ibm.com> wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
>
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
>
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>

Thanks for your patch!

> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,24 @@ 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)
> -               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

IMHO this looks ugly and error-prone.  In addition, it still relies
on the caller initializing the upper bits to zero on little-endian.

What about:

    u8 byte;

    rc = pci_bus_read_config_byte(bus, devfn, where, &byte);
    *val = byte;

> +       } else if (size == 2) {
> +               rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +               *val = ((*val >> 16) & 0xffff);
> +#endif

and:

    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;
> +       }
> +       return rc;
>  }
>
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
> 
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
> Sorry to spill this endianness aware code into drivers/pci, feel free to
> suggest a cleaner approach. This has fixed the issues seen on s390 systems

PCI is little-endian.  On big-endian systems, the endianness conversion
of Config Space accesses happens transparently in the struct pci_ops
->read() and ->write() callbacks.  E.g. on s390, zpci_cfg_load() and
zpci_cfg_store() call le64_to_cpu() and cpu_to_le64(), respectively.

We do not want to mess with endianness in the PCI core, so this isn't
a proper fix IMO.

A viable approach might be to turn pci_bus_read_config() into a macro
in include/linux/pci.h which calls the byte/word/dword variant based
on sizeof(*val) or something like that.

But at this point, with the merge window already open, it's probably
better to drop the pci/capability-search topic branch from the pull
request and retry in the next cycle.

> Since this is still sitting in the a pull-request for upstream,
> I'm not sure if this warrants a Fixes: tag.

In cases like this, do include a Fixes tag but no stable designation.

Thanks,

Lukas

[+cc Arnd]

On Thu, Jul 31, 2025 at 07:38:58PM +0200, Gerd Bayer wrote:
> Simple pointer-casts to map byte and word reads from PCI config space
> into dwords (i.e. u32) produce unintended results on big-endian systems.
> Add the necessary adjustments under compile-time switch
> CONFIG_CPU_BIG_ENDIAN.
> 
> pci_bus_read_config() was just introduced with
> https://lore.kernel.org/all/20250716161203.83823-2-18255117159@163.com/
> 
> Signed-off-by: Gerd Bayer <gbayer@linux.ibm.com>
> ---
> 
> Hi Hans, hi Bjorn,
> 
> Sorry to spill this endianness aware code into drivers/pci, feel free to
> suggest a cleaner approach. This has fixed the issues seen on s390 systems
> Otherwise it is just compile-tested for x86 and arm64.
> 
> Since this is still sitting in the a pull-request for upstream, I'm not sure if this
> warrants a Fixes: tag.
> 
> Thanks,
> Gerd
> ---
>  drivers/pci/access.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ba66f55d2524..77a73b772a28 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -89,15 +89,24 @@ 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)
> -		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?

> +	} else if (size == 2) {
> +		rc = pci_bus_read_config_word(bus, devfn, where, (u16 *)val);
> +#if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> +		*val = ((*val >> 16) & 0xffff);
> +#endif
> +	} else if (size == 4) {
> +		rc = pci_bus_read_config_dword(bus, devfn, where, val);
> +	} else {
> +		rc =  PCIBIOS_BAD_REGISTER_NUMBER;
> +	}
> +	return rc;
>  }
>  
>  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> -- 
> 2.48.1
>

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.

The pci_bus_read_config() interface itself may have been a
mistake, can't the callers just use the underlying helpers
directly?

      Arnd

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.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

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.

- Mani

> 
> 
> 
> 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
> 
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

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
>>
>>
>>
> 
> --
> மணிவண்ணன் சதாசிவம்

On Fri, Aug 01, 2025 at 06:06:16PM GMT, Hans Zhang wrote:
> 
> 
> 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.
> 

Sorry, my previous reply was not clear. I was opposed to returning 'u32 *val'
for a variable 'size' value. The API should only return 'val' of 'size' ie. if
size is 1, it should return 'u8 *val' and so on. It finally breaks down to
calling the underlying accessors. So I don't see a value in having this API.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:

<--- snip --->

> > > > > > 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.

no worries!

> > > > 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.
> > > > 

I found Geert's proposal intriguing for a quick resolution of the
issue. Yet, I have not tried that proposal, though.

Instead I spent some more cycles on Lukas' and Mani's question about
the value of the pci_bus_read_config() helper. So I changed
PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
of read_cfg accessor functions like this:

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index ac954584d991..9e2f75ede95f 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
devfn, int where, u32 size,
 ({                                                                   
\
        int __ttl = PCI_FIND_CAP_TTL;                                 
\
        u8 __id, __found_pos = 0;                                     
\
-       u32 __pos = (start);                                          
\
-       u32 __ent;                                                    
\
+       u8 __pos = (start);                                           
\
+       u16 __ent;                                                    
\
                                                                      
\
-       read_cfg(args, __pos, 1, &__pos);                             
\
+       read_cfg##_byte(args, __pos, &__pos);                         
\
                                                                      
\
        while (__ttl--) {                                             
\
                if (__pos < PCI_STD_HEADER_SIZEOF)                    
\
                        break;                                        
\
                                                                      
\
                __pos = ALIGN_DOWN(__pos, 4);                         
\
-               read_cfg(args, __pos, 2, &__ent);                     
\
+               read_cfg##_word(args, __pos, &__ent);                 
\
                                                                      
\
                __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);             
\
                if (__id == 0xff)                                     
\
@@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
devfn, int where, u32 size,
                                                                      
\
        __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;    
\
        while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {          
\
-               __ret = read_cfg(args, __pos, 4, &__header);          
\
+               __ret = read_cfg##_dword(args, __pos, &__header);     
\
                if (__ret != PCIBIOS_SUCCESSFUL)                      
\
                        break;                                        
\
                                                                      
\


This fixes the issue for s390's use-cases. With that
pci_bus_read_config() becomes unused - and could be removed in further
refinements.
                                                                      
However, this probably breaks your dwc and cdns use-cases. I think,
with the accessor functions for dwc and cadence changed to follow the
{_byte|_word|_dword} naming pattern they could be used straight out of
PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
cdns_pcie_read_cfg become obsolete as well.

Thoughts?

On 2025/8/1 19:30, Gerd Bayer wrote:
> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> 
> <--- snip --->
> 
>>>>>>> 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.
> 
> no worries!
> 
>>>>> 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.
>>>>>
> 
> I found Geert's proposal intriguing for a quick resolution of the
> issue. Yet, I have not tried that proposal, though.
> 

Hi Gerd,

As I mentioned in my reply to Mani's email, the data ultimately read 
here is also a forced type conversion.

#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
{									\
	unsigned long flags;						\
	u32 data = 0;							\
	int res;							\
									\
	if (PCI_##size##_BAD)						\
		return PCIBIOS_BAD_REGISTER_NUMBER;			\
									\
	pci_lock_config(flags);						\
	res = bus->ops->read(bus, devfn, pos, len, &data);		\
	if (res)							\
		PCI_SET_ERROR_RESPONSE(value);				\
	else								\
		*value = (type)data;					\
	pci_unlock_config(flags);					\
									\
	return res;							\
}

And this function. Could it be that I misunderstood something?

int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
			    int where, int size, u32 *val)
{
	void __iomem *addr;

	addr = bus->ops->map_bus(bus, devfn, where);
	if (!addr)
		return PCIBIOS_DEVICE_NOT_FOUND;

	if (size == 1)
		*val = readb(addr);
	else if (size == 2)
		*val = readw(addr);
	else
		*val = readl(addr);

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(pci_generic_config_read);

> Instead I spent some more cycles on Lukas' and Mani's question about
> the value of the pci_bus_read_config() helper. So I changed
> PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
> of read_cfg accessor functions like this:
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ac954584d991..9e2f75ede95f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>   ({
> \
>          int __ttl = PCI_FIND_CAP_TTL;
> \
>          u8 __id, __found_pos = 0;
> \
> -       u32 __pos = (start);
> \
> -       u32 __ent;
> \
> +       u8 __pos = (start);
> \
> +       u16 __ent;
> \
>                                                                        
> \
> -       read_cfg(args, __pos, 1, &__pos);
> \
> +       read_cfg##_byte(args, __pos, &__pos);
> \
>                                                                        
> \
>          while (__ttl--) {
> \
>                  if (__pos < PCI_STD_HEADER_SIZEOF)
> \
>                          break;
> \
>                                                                        
> \
>                  __pos = ALIGN_DOWN(__pos, 4);
> \
> -               read_cfg(args, __pos, 2, &__ent);
> \
> +               read_cfg##_word(args, __pos, &__ent);
> \
>                                                                        
> \
>                  __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);
> \
>                  if (__id == 0xff)
> \
> @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>                                                                        
> \
>          __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> \
>          while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {
> \
> -               __ret = read_cfg(args, __pos, 4, &__header);
> \
> +               __ret = read_cfg##_dword(args, __pos, &__header);
> \
>                  if (__ret != PCIBIOS_SUCCESSFUL)
> \
>                          break;
> \
>                                                                        
> \
> 
> 
> This fixes the issue for s390's use-cases. With that
> pci_bus_read_config() becomes unused - and could be removed in further
> refinements.
>                                                                        
> However, this probably breaks your dwc and cdns use-cases. I think,
> with the accessor functions for dwc and cadence changed to follow the
> {_byte|_word|_dword} naming pattern they could be used straight out of
> PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
> cdns_pcie_read_cfg become obsolete as well.
> 
> Thoughts?

In my opinion, it's no problem. I can provide the corresponding function 
of {_byte / _word / _dword}. But it is necessary to know Bjorn, Mani, 
Arnd, Lukas... Their viewpoints or suggestions.

Best regards,
Hans

On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
> As I mentioned in my reply to Mani's email, the data ultimately read here is
> also a forced type conversion.
> 
> #define PCI_OP_READ(size, type, len) \
> int noinline pci_bus_read_config_##size \
> 	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
> {									\
> 	unsigned long flags;						\
> 	u32 data = 0;							\
> 	int res;							\
> 									\
> 	if (PCI_##size##_BAD)						\
> 		return PCIBIOS_BAD_REGISTER_NUMBER;			\
> 									\
> 	pci_lock_config(flags);						\
> 	res = bus->ops->read(bus, devfn, pos, len, &data);		\
> 	if (res)							\
> 		PCI_SET_ERROR_RESPONSE(value);				\
> 	else								\
> 		*value = (type)data;					\
> 	pci_unlock_config(flags);					\
> 									\
> 	return res;							\
> }
> 
> And this function. Could it be that I misunderstood something?

The above macro retains the caller's type for "value". If the caller
passes a "u8 *", the value is deferenced as a u8.

The function below promotes everything to a u32 pointer and deferences
it as such regardless of what type the user passed in.
 
> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
> 			    int where, int size, u32 *val)
> {
> 	void __iomem *addr;
> 
> 	addr = bus->ops->map_bus(bus, devfn, where);
> 	if (!addr)
> 		return PCIBIOS_DEVICE_NOT_FOUND;
> 
> 	if (size == 1)
> 		*val = readb(addr);
> 	else if (size == 2)
> 		*val = readw(addr);
> 	else
> 		*val = readl(addr);
> 
> 	return PCIBIOS_SUCCESSFUL;
> }

On 2025/8/2 02:08, Keith Busch wrote:
> On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
>> As I mentioned in my reply to Mani's email, the data ultimately read here is
>> also a forced type conversion.
>>
>> #define PCI_OP_READ(size, type, len) \
>> int noinline pci_bus_read_config_##size \
>> 	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
>> {									\
>> 	unsigned long flags;						\
>> 	u32 data = 0;							\
>> 	int res;							\
>> 									\
>> 	if (PCI_##size##_BAD)						\
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;			\
>> 									\
>> 	pci_lock_config(flags);						\
>> 	res = bus->ops->read(bus, devfn, pos, len, &data);		\
>> 	if (res)							\
>> 		PCI_SET_ERROR_RESPONSE(value);				\
>> 	else								\
>> 		*value = (type)data;					\
>> 	pci_unlock_config(flags);					\
>> 									\
>> 	return res;							\
>> }
>>
>> And this function. Could it be that I misunderstood something?
> 
> The above macro retains the caller's type for "value". If the caller
> passes a "u8 *", the value is deferenced as a u8.

Dear Keith,

In this macro definition, bus->ops->read needs to ensure the byte order 
of the read, as Lukas mentioned; otherwise, there is also a big-endian 
issue at this location.

> 
> The function below promotes everything to a u32 pointer and deferences
> it as such regardless of what type the user passed in.

I searched and learned that readb/readw/readl automatically handle byte 
order, so there is no big-endian order issue.

>   
>> int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
>> 			    int where, int size, u32 *val)
>> {
>> 	void __iomem *addr;
>>
>> 	addr = bus->ops->map_bus(bus, devfn, where);
>> 	if (!addr)
>> 		return PCIBIOS_DEVICE_NOT_FOUND;
>>
>> 	if (size == 1)
>> 		*val = readb(addr);
>> 	else if (size == 2)
>> 		*val = readw(addr);
>> 	else
>> 		*val = readl(addr);
>>
>> 	return PCIBIOS_SUCCESSFUL;
>> }


Best regards,
Hans

On Sat, Aug 2, 2025, at 17:23, Hans Zhang wrote:
> On 2025/8/2 02:08, Keith Busch wrote:
>> On Sat, Aug 02, 2025 at 12:54:27AM +0800, Hans Zhang wrote:
>>>
>>> 		*value = (type)data;					\
>>>
>>> And this function. Could it be that I misunderstood something?
>> 
>> The above macro retains the caller's type for "value". If the caller
>> passes a "u8 *", the value is deferenced as a u8.
>
> In this macro definition, bus->ops->read needs to ensure the byte order 
> of the read, as Lukas mentioned; otherwise, there is also a big-endian 
> issue at this location.

No, there is no endianness problem here, the problem with casting
the pointer type like

      u32 *value;
      *(type *)value = data;

or any variation of that is is that it only writes to the first
few bytes of *value, and that introduces both the observed endianess
problem and possibly worse uninitialized data usage or out-of-bounds
stack access.

      Arnd

On 2025/8/1 19:30, Gerd Bayer wrote:
> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> 
> <--- snip --->
> 
>>>>>>> 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.
> 
> no worries!
> 
>>>>> 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.
>>>>>
> 
> I found Geert's proposal intriguing for a quick resolution of the
> issue. Yet, I have not tried that proposal, though.
> 
> Instead I spent some more cycles on Lukas' and Mani's question about
> the value of the pci_bus_read_config() helper. So I changed
> PCI_FIND_NEXT_CAP and PCI_FIND_NEXT_EXT_CAP to use size-aware versions
> of read_cfg accessor functions like this:
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ac954584d991..9e2f75ede95f 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -109,17 +109,17 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>   ({
> \
>          int __ttl = PCI_FIND_CAP_TTL;
> \
>          u8 __id, __found_pos = 0;
> \
> -       u32 __pos = (start);
> \
> -       u32 __ent;
> \
> +       u8 __pos = (start);
> \
> +       u16 __ent;
> \
>                                                                        
> \
> -       read_cfg(args, __pos, 1, &__pos);
> \
> +       read_cfg##_byte(args, __pos, &__pos);
> \
>                                                                        
> \
>          while (__ttl--) {
> \
>                  if (__pos < PCI_STD_HEADER_SIZEOF)
> \
>                          break;
> \
>                                                                        
> \
>                  __pos = ALIGN_DOWN(__pos, 4);
> \
> -               read_cfg(args, __pos, 2, &__ent);
> \
> +               read_cfg##_word(args, __pos, &__ent);
> \
>                                                                        
> \
>                  __id = FIELD_GET(PCI_CAP_ID_MASK, __ent);
> \
>                  if (__id == 0xff)
> \
> @@ -158,7 +158,7 @@ int pci_bus_read_config(void *priv, unsigned int
> devfn, int where, u32 size,
>                                                                        
> \
>          __ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
> \
>          while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {
> \
> -               __ret = read_cfg(args, __pos, 4, &__header);
> \
> +               __ret = read_cfg##_dword(args, __pos, &__header);
> \
>                  if (__ret != PCIBIOS_SUCCESSFUL)
> \
>                          break;
> \
>                                                                        
> \
> 
> 
> This fixes the issue for s390's use-cases. With that
> pci_bus_read_config() becomes unused - and could be removed in further
> refinements.
>                                                                        
> However, this probably breaks your dwc and cdns use-cases. I think,
> with the accessor functions for dwc and cadence changed to follow the
> {_byte|_word|_dword} naming pattern they could be used straight out of
> PCI_FIND_NEXT_{EXT_}CAP, too. Then, dw_pcie_read_cfg() and
> cdns_pcie_read_cfg become obsolete as well.
> 
> Thoughts?

Dear all,

According to the issue mentioned by Lukas and Mani. Gerd has already 
been tested on the s390. I have tested it on the RK3588 and it works 
fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our 
company's is based on Cadence's PCIe 4.0 IP, and the test function is 
normal. All the platforms I tested were based on ARM.

The following is the patch based on the capability-search branch. May I 
ask everyone, do you have any more questions?

Gerd, if there's no problem, I'll add your Tested-by label.

Branch: 
ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search


Patch:
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ba66f55d2524..b123da16b63b 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -85,21 +85,6 @@ EXPORT_SYMBOL(pci_bus_write_config_byte);
  EXPORT_SYMBOL(pci_bus_write_config_word);
  EXPORT_SYMBOL(pci_bus_write_config_dword);

-int pci_bus_read_config(void *priv, unsigned int devfn, int where, u32 
size,
-			u32 *val)
-{
-	struct pci_bus *bus = priv;
-
-	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;
-}
-
  int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
  			    int where, int size, u32 *val)
  {
diff --git a/drivers/pci/controller/cadence/pcie-cadence.c 
b/drivers/pci/controller/cadence/pcie-cadence.c
index 7b2955e4fafb..c45585ae1746 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.c
+++ b/drivers/pci/controller/cadence/pcie-cadence.c
@@ -10,22 +10,6 @@
  #include "pcie-cadence.h"
  #include "../../pci.h"

-static int cdns_pcie_read_cfg(void *priv, int where, int size, u32 *val)
-{
-	struct cdns_pcie *pcie = priv;
-
-	if (size == 4)
-		*val = cdns_pcie_readl(pcie, where);
-	else if (size == 2)
-		*val = cdns_pcie_readw(pcie, where);
-	else if (size == 1)
-		*val = cdns_pcie_readb(pcie, where);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
  u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
  {
  	return PCI_FIND_NEXT_CAP(cdns_pcie_read_cfg, PCI_CAPABILITY_LIST,
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h 
b/drivers/pci/controller/cadence/pcie-cadence.h
index f0fdeb3863f1..4ad874e68783 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -392,6 +392,26 @@ static inline u8 cdns_pcie_readb(struct cdns_pcie 
*pcie, u32 reg)
  	return readb(pcie->reg_base + reg);
  }

+#define CDNS_PCI_OP_READ(size, type, len)		\
+static inline int cdns_pcie_read_cfg_##size		\
+	(struct cdns_pcie *pcie, int where, type *val)	\
+{							\
+	if (len == 4)					\
+		*val = cdns_pcie_readl(pcie, where);	\
+	else if (len == 2)				\
+		*val = cdns_pcie_readw(pcie, where);	\
+	else if (len == 1)				\
+		*val = cdns_pcie_readb(pcie, where);	\
+	else						\
+		return PCIBIOS_BAD_REGISTER_NUMBER;	\
+							\
+	return PCIBIOS_SUCCESSFUL;			\
+}
+
+CDNS_PCI_OP_READ(byte, u8, 1)
+CDNS_PCI_OP_READ(word, u16, 2)
+CDNS_PCI_OP_READ(dword, u32, 4)
+
  static inline u32 cdns_pcie_read_sz(void __iomem *addr, int size)
  {
  	void __iomem *aligned_addr = PTR_ALIGN_DOWN(addr, 0x4);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c 
b/drivers/pci/controller/dwc/pcie-designware.c
index b503cb4bcb28..befb9df3123f 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -213,22 +213,6 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
  		pci->type = ver;
  }

-static int dw_pcie_read_cfg(void *priv, int where, int size, u32 *val)
-{
-	struct dw_pcie *pci = priv;
-
-	if (size == 4)
-		*val = dw_pcie_readl_dbi(pci, where);
-	else if (size == 2)
-		*val = dw_pcie_readw_dbi(pci, where);
-	else if (size == 1)
-		*val = dw_pcie_readb_dbi(pci, where);
-	else
-		return PCIBIOS_BAD_REGISTER_NUMBER;
-
-	return PCIBIOS_SUCCESSFUL;
-}
-
  u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
  {
  	return PCI_FIND_NEXT_CAP(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
diff --git a/drivers/pci/controller/dwc/pcie-designware.h 
b/drivers/pci/controller/dwc/pcie-designware.h
index ce9e18554e42..3b429a8ade70 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -614,6 +614,26 @@ static inline void dw_pcie_writel_dbi2(struct 
dw_pcie *pci, u32 reg, u32 val)
  	dw_pcie_write_dbi2(pci, reg, 0x4, val);
  }

+#define DW_PCI_OP_READ(size, type, len)			\
+static inline int dw_pcie_read_cfg_##size		\
+	(struct dw_pcie *pci, int where, type *val)	\
+{							\
+	if (len == 4)					\
+		*val = dw_pcie_readl_dbi(pci, where);	\
+	else if (len == 2)				\
+		*val = dw_pcie_readw_dbi(pci, where);	\
+	else if (len == 1)				\
+		*val = dw_pcie_readb_dbi(pci, where);	\
+	else						\
+		return PCIBIOS_BAD_REGISTER_NUMBER;	\
+							\
+	return PCIBIOS_SUCCESSFUL;			\
+}
+
+DW_PCI_OP_READ(byte, u8, 1)
+DW_PCI_OP_READ(word, u16, 2)
+DW_PCI_OP_READ(dword, u32, 4)
+
  static inline unsigned int dw_pcie_ep_get_dbi_offset(struct dw_pcie_ep 
*ep,
  						     u8 func_no)
  {
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index e53706d1d806..9c410e47e19a 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -92,8 +92,6 @@ 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);

  /* Standard Capability finder */
  /**
@@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int 
devfn, int where, u32 size,
  ({									\
  	int __ttl = PCI_FIND_CAP_TTL;					\
  	u8 __id, __found_pos = 0;					\
-	u32 __pos = (start);						\
-	u32 __ent;							\
+	u8 __pos = (start);						\
+	u16 __ent;							\
  									\
-	read_cfg(args, __pos, 1, &__pos);				\
+	read_cfg##_byte(args, __pos, &__pos);				\
  									\
  	while (__ttl--) {						\
  		if (__pos < PCI_STD_HEADER_SIZEOF)			\
  			break;						\
  									\
  		__pos = ALIGN_DOWN(__pos, 4);				\
-		read_cfg(args, __pos, 2, &__ent);			\
+		read_cfg##_word(args, __pos, &__ent);			\
  									\
  		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
  		if (__id == 0xff)					\
@@ -161,7 +159,7 @@ int pci_bus_read_config(void *priv, unsigned int 
devfn, int where, u32 size,
  									\
  	__ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;	\
  	while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {		\
-		__ret = read_cfg(args, __pos, 4, &__header);		\
+		__ret = read_cfg##_dword(args, __pos, &__header);	\
  		if (__ret != PCIBIOS_SUCCESSFUL)			\
  			break;						\
  									\



Best regards,
Hans

On Mon, Aug 4, 2025, at 05:06, Hans Zhang wrote:
> On 2025/8/1 19:30, Gerd Bayer wrote:
>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>   }
>
> +#define CDNS_PCI_OP_READ(size, type, len)		\
> +static inline int cdns_pcie_read_cfg_##size		\
> +	(struct cdns_pcie *pcie, int where, type *val)	\
> +{							\
> +	if (len == 4)					\
> +		*val = cdns_pcie_readl(pcie, where);	\
> +	else if (len == 2)				\
> +		*val = cdns_pcie_readw(pcie, where);	\
> +	else if (len == 1)				\
> +		*val = cdns_pcie_readb(pcie, where);	\
> +	else						\
> +		return PCIBIOS_BAD_REGISTER_NUMBER;	\
> +							\
> +	return PCIBIOS_SUCCESSFUL;			\
> +}
> +
> +CDNS_PCI_OP_READ(byte, u8, 1)
> +CDNS_PCI_OP_READ(word, u16, 2)
> +CDNS_PCI_OP_READ(dword, u32, 4)
> +

This is fine for the calling conventions, but the use of a macro
doesn't really help readability, so I'd suggest just having
separate inline functions if they are even needed.

> @@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int 
> devfn, int where, u32 size,
>   ({									\
>   	int __ttl = PCI_FIND_CAP_TTL;					\
>   	u8 __id, __found_pos = 0;					\
> -	u32 __pos = (start);						\
> -	u32 __ent;							\
> +	u8 __pos = (start);						\
> +	u16 __ent;							\
>   									\
> -	read_cfg(args, __pos, 1, &__pos);				\
> +	read_cfg##_byte(args, __pos, &__pos);				\
>   									\
>   	while (__ttl--) {						\
>   		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>   			break;						\
>   									\
>   		__pos = ALIGN_DOWN(__pos, 4);				\
> -		read_cfg(args, __pos, 2, &__ent);			\
> +		read_cfg##_word(args, __pos, &__ent);			\
>   									\
>   		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>   		if (__id == 0xff)					\

I still don't feel great about this macro either, and suspect
we should have a better abstraction with fixed types and a
global function to do it, but I don't see anything obviously
wrong here either.

     Arnd

On 2025/8/4 16:03, Arnd Bergmann wrote:
> On Mon, Aug 4, 2025, at 05:06, Hans Zhang wrote:
>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>    }
>>
>> +#define CDNS_PCI_OP_READ(size, type, len)		\
>> +static inline int cdns_pcie_read_cfg_##size		\
>> +	(struct cdns_pcie *pcie, int where, type *val)	\
>> +{							\
>> +	if (len == 4)					\
>> +		*val = cdns_pcie_readl(pcie, where);	\
>> +	else if (len == 2)				\
>> +		*val = cdns_pcie_readw(pcie, where);	\
>> +	else if (len == 1)				\
>> +		*val = cdns_pcie_readb(pcie, where);	\
>> +	else						\
>> +		return PCIBIOS_BAD_REGISTER_NUMBER;	\
>> +							\
>> +	return PCIBIOS_SUCCESSFUL;			\
>> +}
>> +
>> +CDNS_PCI_OP_READ(byte, u8, 1)
>> +CDNS_PCI_OP_READ(word, u16, 2)
>> +CDNS_PCI_OP_READ(dword, u32, 4)
>> +
> 
> This is fine for the calling conventions, but the use of a macro
> doesn't really help readability, so I'd suggest just having
> separate inline functions if they are even needed.
> 

Dear Arnd,

Thank you very much for your reply.

Will change.

>> @@ -112,17 +110,17 @@ int pci_bus_read_config(void *priv, unsigned int
>> devfn, int where, u32 size,
>>    ({									\
>>    	int __ttl = PCI_FIND_CAP_TTL;					\
>>    	u8 __id, __found_pos = 0;					\
>> -	u32 __pos = (start);						\
>> -	u32 __ent;							\
>> +	u8 __pos = (start);						\
>> +	u16 __ent;							\
>>    									\
>> -	read_cfg(args, __pos, 1, &__pos);				\
>> +	read_cfg##_byte(args, __pos, &__pos);				\
>>    									\
>>    	while (__ttl--) {						\
>>    		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>>    			break;						\
>>    									\
>>    		__pos = ALIGN_DOWN(__pos, 4);				\
>> -		read_cfg(args, __pos, 2, &__ent);			\
>> +		read_cfg##_word(args, __pos, &__ent);			\
>>    									\
>>    		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>>    		if (__id == 0xff)					\
> 
> I still don't feel great about this macro either, and suspect
> we should have a better abstraction with fixed types and a
> global function to do it, but I don't see anything obviously
> wrong here either.


Here is the history of communication with Bjorn and Ilpo. Because 
variable parameters need to be used, otherwise it will be very difficult 
to unify. I'll also think about your proposal again.


Bjorn:
https://lore.kernel.org/linux-pci/20250715224705.GA2504490@bhelgaas/
 > > I would like this a lot better if it could be implemented as a
 > > function, but I assume it has to be a macro for some varargs reason.
 > >
 >
Hans:
https://lore.kernel.org/linux-pci/903ea9c4-87d6-45a8-9825-4a0d45ec608f@163.com/
 > Yes. The macro definitions used in combination with the previous review
 > opinions of Ilpo.

Ilpo:
https://lore.kernel.org/linux-pci/d59fde6e-3e4a-248a-ae56-c35b4c6ec44c@linux.intel.com/
The other option would be to standardize the accessor interface signature
and pass the function as a pointer. One might immediately think of generic
PCI core accessors making it sound simpler than it is but here the
complication is the controller drivers want to pass some internal
structure due to lack of pci_dev so it would need to be void
*read_cfg_data. Then we'd need to deal with those voids also in some
generic PCI accessors which is a bit ugly.


Best regards,
Hans

On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
> 
> On 2025/8/1 19:30, Gerd Bayer wrote:
> > On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> > 
> > <--- snip --->
> > 
> > > > > 
> 
> Dear all,
> 
> According to the issue mentioned by Lukas and Mani. Gerd has already 
> been tested on the s390. I have tested it on the RK3588 and it works 
> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our 
> company's is based on Cadence's PCIe 4.0 IP, and the test function is 
> normal. All the platforms I tested were based on ARM.
> 
> The following is the patch based on the capability-search branch. May I 
> ask everyone, do you have any more questions?
> 
> Gerd, if there's no problem, I'll add your Tested-by label.

Before you add that I'd like to re-test with the "final" patch.

> Branch: 
> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> 
> Patch:

<--- snip --->

Please bear with me while I'm working on that.

Thanks, Gerd

On 2025/8/4 18:09, Gerd Bayer wrote:
> On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
>>
>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>>
>>> <--- snip --->
>>>
>>>>>>
>>
>> Dear all,
>>
>> According to the issue mentioned by Lukas and Mani. Gerd has already
>> been tested on the s390. I have tested it on the RK3588 and it works
>> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
>> company's is based on Cadence's PCIe 4.0 IP, and the test function is
>> normal. All the platforms I tested were based on ARM.
>>
>> The following is the patch based on the capability-search branch. May I
>> ask everyone, do you have any more questions?
>>
>> Gerd, if there's no problem, I'll add your Tested-by label.
> 
> Before you add that I'd like to re-test with the "final" patch.
> 
>> Branch:
>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
>>
>> Patch:
> 
> <--- snip --->
> 
> Please bear with me while I'm working on that.


Dear Gerd,

May I ask if there is any update?



I plan to submit the v15 version of my series based on v6.17-rc1.
The modification method is like the previous comment:
https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/

Best regards,
Hans

On Tue, 2025-08-12 at 22:44 +0800, Hans Zhang wrote:
> 
> On 2025/8/4 18:09, Gerd Bayer wrote:
> > On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
> > > 
> > > On 2025/8/1 19:30, Gerd Bayer wrote:
> > > > On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
> > > > 
> > > > <--- snip --->
> > > > 
> > > > > > > 
> > > 
> > > Dear all,
> > > 
> > > According to the issue mentioned by Lukas and Mani. Gerd has already
> > > been tested on the s390. I have tested it on the RK3588 and it works
> > > fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
> > > company's is based on Cadence's PCIe 4.0 IP, and the test function is
> > > normal. All the platforms I tested were based on ARM.
> > > 
> > > The following is the patch based on the capability-search branch. May I
> > > ask everyone, do you have any more questions?
> > > 
> > > Gerd, if there's no problem, I'll add your Tested-by label.
> > 
> > Before you add that I'd like to re-test with the "final" patch.
> > 
> > > Branch:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> > > 
> > > Patch:
> > 
> > <--- snip --->
> > 
> > Please bear with me while I'm working on that.
> 
> 
> Dear Gerd,
> 
> May I ask if there is any update?
> 
> 
> 
> I plan to submit the v15 version of my series based on v6.17-rc1.
> The modification method is like the previous comment:
> https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/
> 
> Best regards,
> Hans
> 

Hi Hans,

Gerd is currently out so I just gave the patch you provided against
capability-search-v14 a try on s390. Didn't see any issues with the
previously broken device probing. As I understand it Bjorn asked you to
send a complete v15 and then for people to test that. I like that
approach and would prefer to provide a Tested-by for v15 rather than
via a patch on top. Gerd should be back next week too. Does that work
for you?

Thanks,
Niklas Schnelle

On 2025/8/13 15:47, Niklas Schnelle wrote:
> On Tue, 2025-08-12 at 22:44 +0800, Hans Zhang wrote:
>>
>> On 2025/8/4 18:09, Gerd Bayer wrote:
>>> On Mon, 2025-08-04 at 11:06 +0800, Hans Zhang wrote:
>>>>
>>>> On 2025/8/1 19:30, Gerd Bayer wrote:
>>>>> On Fri, 2025-08-01 at 16:24 +0530, Manivannan Sadhasivam wrote:
>>>>>
>>>>> <--- snip --->
>>>>>
>>>>>>>>
>>>>
>>>> Dear all,
>>>>
>>>> According to the issue mentioned by Lukas and Mani. Gerd has already
>>>> been tested on the s390. I have tested it on the RK3588 and it works
>>>> fine. RK3588 uses Synopsys' PCIe IP, that is, the DWC driver. Our
>>>> company's is based on Cadence's PCIe 4.0 IP, and the test function is
>>>> normal. All the platforms I tested were based on ARM.
>>>>
>>>> The following is the patch based on the capability-search branch. May I
>>>> ask everyone, do you have any more questions?
>>>>
>>>> Gerd, if there's no problem, I'll add your Tested-by label.
>>>
>>> Before you add that I'd like to re-test with the "final" patch.
>>>
>>>> Branch:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
>>>>
>>>> Patch:
>>>
>>> <--- snip --->
>>>
>>> Please bear with me while I'm working on that.
>>
>>
>> Dear Gerd,
>>
>> May I ask if there is any update?
>>
>>
>>
>> I plan to submit the v15 version of my series based on v6.17-rc1.
>> The modification method is like the previous comment:
>> https://lore.kernel.org/linux-pci/06012cc6-824d-4a7d-85c9-9995ec915382@163.com/
>>
>> Best regards,
>> Hans
>>
> 
> Hi Hans,
> 
> Gerd is currently out so I just gave the patch you provided against
> capability-search-v14 a try on s390. Didn't see any issues with the
> previously broken device probing. As I understand it Bjorn asked you to
> send a complete v15 and then for people to test that. I like that
> approach and would prefer to provide a Tested-by for v15 rather than
> via a patch on top. Gerd should be back next week too. Does that work
> for you?
> 

Hi Niklas,

Ok, no problem. I'm starting to prepare the patch for V15 now.

Best regards,
Hans

On Mon, Aug 04, 2025 at 11:06:36AM +0800, Hans Zhang wrote:
> ...

> According to the issue mentioned by Lukas and Mani. Gerd has already been
> tested on the s390. I have tested it on the RK3588 and it works fine. RK3588
> uses Synopsys' PCIe IP, that is, the DWC driver. Our company's is based on
> Cadence's PCIe 4.0 IP, and the test function is normal. All the platforms I
> tested were based on ARM.
> 
> The following is the patch based on the capability-search branch. May I ask
> everyone, do you have any more questions?
> 
> Gerd, if there's no problem, I'll add your Tested-by label.
> 
> Branch: ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search

Since this series will now target v6.18, I'll watch for a complete v15
series based on v6.17-rc1, with this fix and any typo or other fixes
from pci/capability-search fully integrated.

Then that series can be tested and completely replace the current
pci/capability-search branch.

Bjorn

On 2025/8/4 22:33, Bjorn Helgaas wrote:
> On Mon, Aug 04, 2025 at 11:06:36AM +0800, Hans Zhang wrote:
>> ...
> 
>> According to the issue mentioned by Lukas and Mani. Gerd has already been
>> tested on the s390. I have tested it on the RK3588 and it works fine. RK3588
>> uses Synopsys' PCIe IP, that is, the DWC driver. Our company's is based on
>> Cadence's PCIe 4.0 IP, and the test function is normal. All the platforms I
>> tested were based on ARM.
>>
>> The following is the patch based on the capability-search branch. May I ask
>> everyone, do you have any more questions?
>>
>> Gerd, if there's no problem, I'll add your Tested-by label.
>>
>> Branch: ttps://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=capability-search
> 
> Since this series will now target v6.18, I'll watch for a complete v15
> series based on v6.17-rc1, with this fix and any typo or other fixes
> from pci/capability-search fully integrated.
> 
> Then that series can be tested and completely replace the current
> pci/capability-search branch.
> 

Dear Bjorn,

Here is the patch based on pci/capability-search branch. It's to discuss 
clearly how the final v15 should be modified. The final plan has been 
confirmed. I will submit the v15 version of this series based on v6.17-rc1.

Best regards,
Hans

On 2025/8/1 18:54, Manivannan Sadhasivam wrote:
> On Fri, Aug 01, 2025 at 06:06:16PM GMT, Hans Zhang wrote:
>>
>>
>> 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.
>>
> 
> Sorry, my previous reply was not clear. I was opposed to returning 'u32 *val'
> for a variable 'size' value. The API should only return 'val' of 'size' ie. if
> size is 1, it should return 'u8 *val' and so on. It finally breaks down to
> calling the underlying accessors. So I don't see a value in having this API.

Dear Mani,

In this series, I had similar confusion before.
https://lore.kernel.org/linux-pci/4d77e199-8df8-4510-ad49-9a452a29c923@163.com/


I think there are a few pieces of code that stand out, such as:

Forced type conversion is also used here. (*value = (type)data;)


drivers/pci/access.c
#define PCI_OP_READ(size, type, len) \
int noinline pci_bus_read_config_##size \
	(struct pci_bus *bus, unsigned int devfn, int pos, type *value)	\
{									\
	unsigned long flags;						\
	u32 data = 0;							\
	int res;							\
									\
	if (PCI_##size##_BAD)						\
		return PCIBIOS_BAD_REGISTER_NUMBER;			\
									\
	pci_lock_config(flags);						\
	res = bus->ops->read(bus, devfn, pos, len, &data);		\
	if (res)							\
		PCI_SET_ERROR_RESPONSE(value);				\
	else								\
		*value = (type)data;					\
	pci_unlock_config(flags);					\
									\
	return res;							\
}


This function also uses u32 *val as its return value.

int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn,
			    int where, int size, u32 *val)
{
	void __iomem *addr;

	addr = bus->ops->map_bus(bus, devfn, where);
	if (!addr)
		return PCIBIOS_DEVICE_NOT_FOUND;

	if (size == 1)
		*val = readb(addr);
	else if (size == 2)
		*val = readw(addr);
	else
		*val = readl(addr);

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(pci_generic_config_read);


And it's the same here.
drivers/pci/controller/dwc/pcie-designware.c
int dw_pcie_read(void __iomem *addr, int size, u32 *val)
{
	if (!IS_ALIGNED((uintptr_t)addr, size)) {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	if (size == 4) {
		*val = readl(addr);
	} else if (size == 2) {
		*val = readw(addr);
	} else if (size == 1) {
		*val = readb(addr);
	} else {
		*val = 0;
		return PCIBIOS_BAD_REGISTER_NUMBER;
	}

	return PCIBIOS_SUCCESSFUL;
}
EXPORT_SYMBOL_GPL(dw_pcie_read);


Mani, I'm not here to refute you. I just want to ask if there are bugs 
everywhere here?

I think it's a good idea as mentioned in Gerd's latest reply email. For 
dw_pcie_read_cfg() and cdns_pcie_read_cfg, I can delete it and provide 
the macro definition function of {_byte/_word/_dword}.

Similar to this macro definition:
PCI_OP_READ(byte, u8, 1)
PCI_OP_READ(word, u16, 2)
PCI_OP_READ(dword, u32, 4)
https://lore.kernel.org/linux-pci/06f16b1a55eede3dc3e0bf31ff14eca89ab6f009.camel@linux.ibm.com/


Best regards,
Hans