>-----Original Message----- >From: Keller, Jacob E <[email protected]> >Sent: Saturday, January 24, 2026 12:41 AM >To: Simon Horman <[email protected]>; 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 > > > >> -----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? There were similar concerns before, will drop this patch, Thanks, Piotr
[...]
