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