[PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Piotr Kwapulinski]

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@intel.com>
Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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]));
 
 	/* 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];
+			raw_desc[i] = cpu_to_le32(raw_desc[i]);
 		}
 	}
 
@@ -153,7 +154,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
 	if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
 		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
 			raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
-			raw_desc[i] = raw_desc[i];
+			raw_desc[i] = cpu_to_le32(raw_desc[i]);
 		}
 	}
 
-- 
2.47.1

Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Simon Horman]

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@intel.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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.

>  
>  	/* 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]);

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.

>  		}
>  	}
>  
> @@ -153,7 +154,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>  	if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
>  		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
>  			raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
> -			raw_desc[i] = raw_desc[i];
> +			raw_desc[i] = cpu_to_le32(raw_desc[i]);
>  		}
>  	}
>  
> -- 
> 2.47.1
> 

RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Keller, Jacob E]

> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Friday, January 23, 2026 12:07 PM
> To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.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@intel.com>
> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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... 

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

Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Simon Horman]

On Fri, Jan 23, 2026 at 11:41:26PM +0000, Keller, Jacob E wrote:
> 
> 
> > -----Original Message-----
> > From: Simon Horman <horms@kernel.org>
> > Sent: Friday, January 23, 2026 12:07 PM
> > To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> > dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr
> > <aleksandr.loktionov@intel.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@intel.com>
> > > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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.

RE: [Intel-wired-lan] [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Keller, Jacob E]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Simon Horman
> Sent: Monday, January 26, 2026 3:01 AM
> To: Keller, Jacob E <jacob.e.keller@intel.com>
> Cc: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>; intel-wired-
> lan@lists.osuosl.org; netdev@vger.kernel.org; dan.carpenter@linaro.org;
> pmenzel@molgen.mpg.de; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.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@kernel.org>
> > > Sent: Friday, January 23, 2026 12:07 PM
> > > To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
> > > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org;
> > > dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov,
> Aleksandr
> > > <aleksandr.loktionov@intel.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@intel.com>
> > > > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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?)

RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@intel.com> 
>Sent: Saturday, January 24, 2026 12:41 AM
>To: Simon Horman <horms@kernel.org>; Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
>Subject: RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian
>
>
>
>> -----Original Message-----
>> From: Simon Horman <horms@kernel.org>
>> Sent: Friday, January 23, 2026 12:07 PM
>> To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; 
>> dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr 
>> <aleksandr.loktionov@intel.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@intel.com>
>> > Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.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... 
>
>> >
>> >  	/* 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

[...]

Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Tony Nguyen]

On 1/27/2026 3:31 AM, Kwapulinski, Piotr wrote:

...

>>>> @@ -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.

...
> will drop this patch,

Could we remove the raw_desc redundant assignment though? I think 
there's one other instance nearby. You can send it separately and I'll 
make it part of the same series as the other patch when it goes to netdev.

Thanks,
Tony

RE: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Nguyen, Anthony L <anthony.l.nguyen@intel.com>
>Sent: Tuesday, January 27, 2026 7:51 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Simon Horman <horms@kernel.org>
>Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; dan.carpenter@linaro.org; pmenzel@molgen.mpg.de; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>
>Subject: Re: [PATCH iwl-next v2 1/2] ixgbe: e610: Convert ACI descriptor buffer to little endian
>
>
>
>On 1/27/2026 3:31 AM, Kwapulinski, Piotr wrote:
>
>...
>
>>>>> @@ -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.
>
>...
>> will drop this patch,
>
>Could we remove the raw_desc redundant assignment though? I think there's one other instance nearby. You can send it separately and I'll make it part of the same series as the other patch when it goes to netdev.
That's the plan, will do.
Piotr

>Thanks,
>Tony