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

[Hans Zhang <18255117159@163.com>]

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