netdev - RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL1PR11MB597993D5BC80645017BF6F60F390A@BL1PR11MB5979.namprd11.prod.outlook.com>
Date: Tue, 27 Jan 2026 11:31:25 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Simon Horman
	<horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "dan.carpenter@...aro.org"
	<dan.carpenter@...aro.org>, "pmenzel@...gen.mpg.de" <pmenzel@...gen.mpg.de>,
	"Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
Subject: RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor
 buffer to little endian

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@...el.com> 
>Sent: Saturday, January 24, 2026 12:41 AM
>To: Simon Horman <horms@...nel.org>; Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; dan.carpenter@...aro.org; pmenzel@...gen.mpg.de; Loktionov, Aleksandr <aleksandr.loktionov@...el.com>
>Subject: RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian
>
>
>
>> -----Original Message-----
>> From: Simon Horman <horms@...nel.org>
>> Sent: Friday, January 23, 2026 12:07 PM
>> To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>> Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; 
>> dan.carpenter@...aro.org; pmenzel@...gen.mpg.de; Loktionov, Aleksandr 
>> <aleksandr.loktionov@...el.com>
>> Subject: Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI 
>> descriptor buffer to little endian
>> 
>> On Thu, Jan 22, 2026 at 05:46:32PM +0100, Piotr Kwapulinski wrote:
>> > The ixgbe device registers/descriptors expect little-endian 
>> > ordering. Make the code aware that the e610 adapter operates on data 
>> > with little endian ordering. The extra conversion is required on 
>> > big-endian hosts. In most scenarios this conversion is not required.
>> >
>> > Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command
>> Interface")
>> > Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
>> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>> > ---
>> >  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 7 ++++---
>> >  1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> > index c2f8189..f494e90 100644
>> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> > @@ -113,7 +113,8 @@ static int ixgbe_aci_send_cmd_execute(struct
>> ixgbe_hw *hw,
>> >
>> >  	/* Descriptor is written to specific registers */
>> >  	for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
>> > -		IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), raw_desc[i]);
>> > +		IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i),
>> > +				le32_to_cpu(raw_desc[i]));
>> 
>> IXGBE_WRITE_REG is backed by writel.  And my understanding is that 
>> writel converts values from host byte order to little endian.  So I'm 
>> confused about where this is going.
>> 
>
>Yes, it should. In this case, the raw_desc value is being converted *to* CPU order to work with writel... 
>
>> >
>> >  	/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
>> >  	 * PF_HICR_EV
>> > @@ -145,7 +146,7 @@ static int ixgbe_aci_send_cmd_execute(struct
>> ixgbe_hw *hw,
>> >  	if ((hicr & IXGBE_PF_HICR_SV)) {
>> >  		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
>> >  			raw_desc[i] = IXGBE_READ_REG(hw,
>> IXGBE_PF_HIDA(i));
>> > -			raw_desc[i] = raw_desc[i];
>> 
>> I'm also curious to know what the intent (if any) of the line above was/is.
>> 
>> > +			raw_desc[i] = cpu_to_le32(raw_desc[i]);
>
>
>It's being converted to LE32 order here. But if nothing else touches raw_desc is there any reason to convert??
>
>> 
>> Please don't use the same variable to store both host byte order and 
>> little endian values.  In this case I'd use another local variable, 
>> say scoped to within this block, to store the intermediate value.
>> 
>> And if raw_desc will be used to hold __le32 values, it's type should 
>> be updated.
>> 
>
>If I understand Simon's comments correctly, this whole change is a no-op, and unnecessary. Writel and readl already handle conversion to CPU format, so unless you have some issue because raw_desc is assumed to be LE32 somewhere else, I think this patch should be dropped. If you do have a real case where something was wrong, can you please provide details?
There were similar concerns before, will drop this patch,
Thanks,
Piotr

[...]