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 for Android: free password hash cracker in your pocket

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

Message-ID: <CO1PR11MB508945D598B6284773B6E9E4D694A@CO1PR11MB5089.namprd11.prod.outlook.com>
Date: Fri, 23 Jan 2026 23:41:26 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>, "Kwapulinski, Piotr"
	<piotr.kwapulinski@...el.com>
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: 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?

Thanks,
Jake