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: <aXdJfu8Lml_dnjd3@horms.kernel.org>
Date: Mon, 26 Jan 2026 11:01:18 +0000
From: Simon Horman <horms@...nel.org>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>
Cc: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>,
	"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

On Fri, Jan 23, 2026 at 11:41:26PM +0000, Keller, Jacob E wrote:
> 
> 
> > -----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... 

Yes, sorry. I seem to have completely confused myself there.

> > >  	/* 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?

Sorry for not being clearer.

I'm suspecting that the issue that this patch tries to address
is that the backing structure, struct libie_aq_desc, is described in terms
of __le integers.

I haven't dug deep enough to be able to say if that is a good approach or not.
But if that data is __le, then I expect that some sort of conversion along
the lines of this patch makes sense.