Message-ID: <f28be780-445e-4823-a0c5-44c61241d93f@163.com>
Date: Sat, 2 Aug 2025 00:47:55 +0800
From: Hans Zhang <18255117159@....com>
To: Manivannan Sadhasivam <mani@...nel.org>,
 Hans Zhang <hans.zhang@...tech.com>
Cc: Arnd Bergmann <arnd@...nel.org>, Bjorn Helgaas <helgaas@...nel.org>,
 Gerd Bayer <gbayer@...ux.ibm.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 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

Powered by blists - more mailing lists