netdev - RE: [Intel-wired-lan] [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: <CO1PR11MB508925AC362628515F74C747D693A@CO1PR11MB5089.namprd11.prod.outlook.com>
Date: Mon, 26 Jan 2026 20:00:47 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Simon Horman <horms@...nel.org>
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: [Intel-wired-lan] [PATCH iwl-next v2 1/2] ixgbe: e610: Convert
 ACI descriptor buffer to little endian



> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@...osl.org> On Behalf Of
> Simon Horman
> Sent: Monday, January 26, 2026 3:01 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>; 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: [Intel-wired-lan] [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.

True! Iff the structure is __le then we need to do this. The structure really should only be __le32 if it actually gets used that way somewhere else (something like a firmware AQ message?)