> -----Original Message-----
> From: Intel-wired-lan <[email protected]> On Behalf Of
> Simon Horman
> Sent: Monday, January 26, 2026 3:01 AM
> To: Keller, Jacob E <[email protected]>
> Cc: Kwapulinski, Piotr <[email protected]>; intel-wired-
> [email protected]; [email protected]; [email protected];
> [email protected]; Loktionov, Aleksandr
> <[email protected]>
> 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 <[email protected]>
> > > Sent: Friday, January 23, 2026 12:07 PM
> > > To: Kwapulinski, Piotr <[email protected]>
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; Loktionov,
> Aleksandr
> > > <[email protected]>
> > > 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 <[email protected]>
> > > > Signed-off-by: Piotr Kwapulinski <[email protected]>
> > > > ---
> > > > 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?)
