lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f4dfc405-1841-4254-95e9-2079c183277d@163.com>
Date: Mon, 4 Aug 2025 16:25:35 +0800
From: Hans Zhang <18255117159@....com>
To: Arnd Bergmann <arnd@...nel.org>, Gerd Bayer <gbayer@...ux.ibm.com>,
 Manivannan Sadhasivam <mani@...nel.org>, Hans Zhang <hans.zhang@...tech.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, 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 Uytterhoeven <geert@...ux-m68k.org>
Subject: Re: [PATCH] PCI: Fix endianness issues in pci_bus_read_config()



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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ