ixgbe: Fix endian handling for ACI descriptor registers

[PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Dheeraj Reddy Jonnalagadda 7 months, 3 weeks ago

The ixgbe driver was missing proper endian conversion for ACI descriptor
register operations. Add the necessary conversions when reading and
writing to the registers.

Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
---
Changelog

v2:
	- Updated the patch to include suggested fix
	- Updated the commit message to describe the issue

 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index 683c668672d6..3b9017e72d0e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -113,7 +113,7 @@ 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), cpu_to_le32(raw_desc[i]));
 
 	/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
 	 * PF_HICR_EV
@@ -145,7 +145,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] = le32_to_cpu(raw_desc[i]);
 		}
 	}
 
@@ -153,7 +153,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] = le32_to_cpu(raw_desc[i]);
 		}
 	}
 
-- 
2.34.1

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by kernel test robot 7 months, 2 weeks ago

Hi Dheeraj,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Dheeraj-Reddy-Jonnalagadda/ixgbe-Fix-endian-handling-for-ACI-descriptor-registers/20250115-114330
base:   net-next/main
patch link:    https://lore.kernel.org/r/20250115034117.172999-1-dheeraj.linuxdev%40gmail.com
patch subject: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers
config: x86_64-randconfig-r133-20250118 (https://download.01.org/0day-ci/archive/20250118/202501182225.DicoE2L2-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250118/202501182225.DicoE2L2-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501182225.DicoE2L2-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:116:17: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected unsigned int [usertype] value @@     got restricted __le32 [usertype] @@
   drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:116:17: sparse:     expected unsigned int [usertype] value
   drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:116:17: sparse:     got restricted __le32 [usertype]
>> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:148:39: sparse: sparse: cast to restricted __le32
   drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c:156:39: sparse: sparse: cast to restricted __le32

vim +116 drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c

    35	
    36	/**
    37	 * ixgbe_aci_send_cmd_execute - execute sending FW Admin Command to FW Admin
    38	 * Command Interface
    39	 * @hw: pointer to the HW struct
    40	 * @desc: descriptor describing the command
    41	 * @buf: buffer to use for indirect commands (NULL for direct commands)
    42	 * @buf_size: size of buffer for indirect commands (0 for direct commands)
    43	 *
    44	 * Admin Command is sent using CSR by setting descriptor and buffer in specific
    45	 * registers.
    46	 *
    47	 * Return: the exit code of the operation.
    48	 * * - 0 - success.
    49	 * * - -EIO - CSR mechanism is not enabled.
    50	 * * - -EBUSY - CSR mechanism is busy.
    51	 * * - -EINVAL - buf_size is too big or
    52	 * invalid argument buf or buf_size.
    53	 * * - -ETIME - Admin Command X command timeout.
    54	 * * - -EIO - Admin Command X invalid state of HICR register or
    55	 * Admin Command failed because of bad opcode was returned or
    56	 * Admin Command failed with error Y.
    57	 */
    58	static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
    59					      struct ixgbe_aci_desc *desc,
    60					      void *buf, u16 buf_size)
    61	{
    62		u16 opcode, buf_tail_size = buf_size % 4;
    63		u32 *raw_desc = (u32 *)desc;
    64		u32 hicr, i, buf_tail = 0;
    65		bool valid_buf = false;
    66	
    67		hw->aci.last_status = IXGBE_ACI_RC_OK;
    68	
    69		/* It's necessary to check if mechanism is enabled */
    70		hicr = IXGBE_READ_REG(hw, IXGBE_PF_HICR);
    71	
    72		if (!(hicr & IXGBE_PF_HICR_EN))
    73			return -EIO;
    74	
    75		if (hicr & IXGBE_PF_HICR_C) {
    76			hw->aci.last_status = IXGBE_ACI_RC_EBUSY;
    77			return -EBUSY;
    78		}
    79	
    80		opcode = le16_to_cpu(desc->opcode);
    81	
    82		if (buf_size > IXGBE_ACI_MAX_BUFFER_SIZE)
    83			return -EINVAL;
    84	
    85		if (buf)
    86			desc->flags |= cpu_to_le16(IXGBE_ACI_FLAG_BUF);
    87	
    88		if (desc->flags & cpu_to_le16(IXGBE_ACI_FLAG_BUF)) {
    89			if ((buf && !buf_size) ||
    90			    (!buf && buf_size))
    91				return -EINVAL;
    92			if (buf && buf_size)
    93				valid_buf = true;
    94		}
    95	
    96		if (valid_buf) {
    97			if (buf_tail_size)
    98				memcpy(&buf_tail, buf + buf_size - buf_tail_size,
    99				       buf_tail_size);
   100	
   101			if (((buf_size + 3) & ~0x3) > IXGBE_ACI_LG_BUF)
   102				desc->flags |= cpu_to_le16(IXGBE_ACI_FLAG_LB);
   103	
   104			desc->datalen = cpu_to_le16(buf_size);
   105	
   106			if (desc->flags & cpu_to_le16(IXGBE_ACI_FLAG_RD)) {
   107				for (i = 0; i < buf_size / 4; i++)
   108					IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), ((u32 *)buf)[i]);
   109				if (buf_tail_size)
   110					IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), buf_tail);
   111			}
   112		}
   113	
   114		/* Descriptor is written to specific registers */
   115		for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
 > 116			IXGBE_WRITE_REG(hw, IXGBE_PF_HIDA(i), cpu_to_le32(raw_desc[i]));
   117	
   118		/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
   119		 * PF_HICR_EV
   120		 */
   121		hicr = (IXGBE_READ_REG(hw, IXGBE_PF_HICR) | IXGBE_PF_HICR_C) &
   122		       ~(IXGBE_PF_HICR_SV | IXGBE_PF_HICR_EV);
   123		IXGBE_WRITE_REG(hw, IXGBE_PF_HICR, hicr);
   124	
   125	#define MAX_SLEEP_RESP_US 1000
   126	#define MAX_TMOUT_RESP_SYNC_US 100000000
   127	
   128		/* Wait for sync Admin Command response */
   129		read_poll_timeout(IXGBE_READ_REG, hicr,
   130				  (hicr & IXGBE_PF_HICR_SV) ||
   131				  !(hicr & IXGBE_PF_HICR_C),
   132				  MAX_SLEEP_RESP_US, MAX_TMOUT_RESP_SYNC_US, true, hw,
   133				  IXGBE_PF_HICR);
   134	
   135	#define MAX_TMOUT_RESP_ASYNC_US 150000000
   136	
   137		/* Wait for async Admin Command response */
   138		read_poll_timeout(IXGBE_READ_REG, hicr,
   139				  (hicr & IXGBE_PF_HICR_EV) ||
   140				  !(hicr & IXGBE_PF_HICR_C),
   141				  MAX_SLEEP_RESP_US, MAX_TMOUT_RESP_ASYNC_US, true, hw,
   142				  IXGBE_PF_HICR);
   143	
   144		/* Read sync Admin Command response */
   145		if ((hicr & IXGBE_PF_HICR_SV)) {
   146			for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
   147				raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
 > 148				raw_desc[i] = le32_to_cpu(raw_desc[i]);
   149			}
   150		}
   151	
   152		/* Read async Admin Command response */
   153		if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C)) {
   154			for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++) {
   155				raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));
   156				raw_desc[i] = le32_to_cpu(raw_desc[i]);
   157			}
   158		}
   159	
   160		/* Handle timeout and invalid state of HICR register */
   161		if (hicr & IXGBE_PF_HICR_C)
   162			return -ETIME;
   163	
   164		if (!(hicr & IXGBE_PF_HICR_SV) && !(hicr & IXGBE_PF_HICR_EV))
   165			return -EIO;
   166	
   167		/* For every command other than 0x0014 treat opcode mismatch
   168		 * as an error. Response to 0x0014 command read from HIDA_2
   169		 * is a descriptor of an event which is expected to contain
   170		 * different opcode than the command.
   171		 */
   172		if (desc->opcode != cpu_to_le16(opcode) &&
   173		    opcode != ixgbe_aci_opc_get_fw_event)
   174			return -EIO;
   175	
   176		if (desc->retval) {
   177			hw->aci.last_status = (enum ixgbe_aci_err)
   178				le16_to_cpu(desc->retval);
   179			return -EIO;
   180		}
   181	
   182		/* Write a response values to a buf */
   183		if (valid_buf) {
   184			for (i = 0; i < buf_size / 4; i++)
   185				((u32 *)buf)[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
   186			if (buf_tail_size) {
   187				buf_tail = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
   188				memcpy(buf + buf_size - buf_tail_size, &buf_tail,
   189				       buf_tail_size);
   190			}
   191		}
   192	
   193		return 0;
   194	}
   195	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Simon Horman 7 months, 3 weeks ago

On Wed, Jan 15, 2025 at 09:11:17AM +0530, Dheeraj Reddy Jonnalagadda wrote:
> The ixgbe driver was missing proper endian conversion for ACI descriptor
> register operations. Add the necessary conversions when reading and
> writing to the registers.
> 
> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>

Hi Dheeraj,

It seems that Sparse is not very happy about __le32 values appearing
where u32 ones are expected. I wonder if something like what is below
(compile tested only!) would both address the problem at hand and
keep Sparse happy (even if negting much of it's usefulness by using casts).

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
index 6639069ad528..8b3787837128 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
@@ -150,6 +150,9 @@ static inline void ixgbe_write_reg(struct ixgbe_hw *hw, u32 reg, u32 value)
 }
 #define IXGBE_WRITE_REG(a, reg, value) ixgbe_write_reg((a), (reg), (value))
 
+#define IXGBE_WRITE_REG_LE32(a, reg, value) \
+	ixgbe_write_reg((a), (reg), (u32 __force)cpu_to_le32(value))
+
 #ifndef writeq
 #define writeq writeq
 static inline void writeq(u64 val, void __iomem *addr)
@@ -172,6 +175,9 @@ static inline void ixgbe_write_reg64(struct ixgbe_hw *hw, u32 reg, u64 value)
 u32 ixgbe_read_reg(struct ixgbe_hw *hw, u32 reg);
 #define IXGBE_READ_REG(a, reg) ixgbe_read_reg((a), (reg))
 
+#define IXGBE_READ_REG_LE32(a, reg) \
+	le32_to_cpu((__le32 __force)ixgbe_read_reg((a), (reg)))
+
 #define IXGBE_WRITE_REG_ARRAY(a, reg, offset, value) \
 		ixgbe_write_reg((a), (reg) + ((offset) << 2), (value))
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
index 3b9017e72d0e..8d9b91375584 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
@@ -113,7 +113,7 @@ 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), cpu_to_le32(raw_desc[i]));
+		IXGBE_WRITE_REG_LE32(hw, IXGBE_PF_HIDA(i), raw_desc[i]);
 
 	/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
 	 * PF_HICR_EV
@@ -145,7 +145,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] = le32_to_cpu(raw_desc[i]);
+			raw_desc[i] = IXGBE_READ_REG_LE32(hw, IXGBE_PF_HIDA(i));
 		}
 	}
 
@@ -153,7 +153,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] = le32_to_cpu(raw_desc[i]);
+			raw_desc[i] = IXGBE_READ_REG_LE32(hw, IXGBE_PF_HIDA_2(i));
 		}
 	}

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Przemek Kitszel 7 months, 3 weeks ago

On 1/16/25 17:21, Simon Horman wrote:
> On Wed, Jan 15, 2025 at 09:11:17AM +0530, Dheeraj Reddy Jonnalagadda wrote:
>> The ixgbe driver was missing proper endian conversion for ACI descriptor
>> register operations. Add the necessary conversions when reading and
>> writing to the registers.
>>
>> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
>> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
>> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> 
> Hi Dheeraj,
> 
> It seems that Sparse is not very happy about __le32 values appearing
> where u32 ones are expected. I wonder if something like what is below
> (compile tested only!) would both address the problem at hand and
> keep Sparse happy (even if negting much of it's usefulness by using casts).
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> index 6639069ad528..8b3787837128 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> @@ -150,6 +150,9 @@ static inline void ixgbe_write_reg(struct ixgbe_hw *hw, u32 reg, u32 value)
>   }
>   #define IXGBE_WRITE_REG(a, reg, value) ixgbe_write_reg((a), (reg), (value))

Simon,

As all ixgbe registers are LE, it would be beneficial to change
ixgbe_write_reg(), as @value should be __le32, (perhaps @reg too).
Similar for 64b.

This clearly would not be a "fix" material, as all call sites should be
examined to check if they conform.

>   
> +#define IXGBE_WRITE_REG_LE32(a, reg, value) \
> +	ixgbe_write_reg((a), (reg), (u32 __force)cpu_to_le32(value))
> +

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Simon Horman 7 months, 3 weeks ago

On Fri, Jan 17, 2025 at 11:01:22AM +0100, Przemek Kitszel wrote:
> On 1/16/25 17:21, Simon Horman wrote:
> > On Wed, Jan 15, 2025 at 09:11:17AM +0530, Dheeraj Reddy Jonnalagadda wrote:
> > > The ixgbe driver was missing proper endian conversion for ACI descriptor
> > > register operations. Add the necessary conversions when reading and
> > > writing to the registers.
> > > 
> > > Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
> > > Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
> > > Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> > 
> > Hi Dheeraj,
> > 
> > It seems that Sparse is not very happy about __le32 values appearing
> > where u32 ones are expected. I wonder if something like what is below
> > (compile tested only!) would both address the problem at hand and
> > keep Sparse happy (even if negting much of it's usefulness by using casts).
> > 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> > index 6639069ad528..8b3787837128 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
> > @@ -150,6 +150,9 @@ static inline void ixgbe_write_reg(struct ixgbe_hw *hw, u32 reg, u32 value)
> >   }
> >   #define IXGBE_WRITE_REG(a, reg, value) ixgbe_write_reg((a), (reg), (value))
> 
> Simon,
> 
> As all ixgbe registers are LE, it would be beneficial to change
> ixgbe_write_reg(), as @value should be __le32, (perhaps @reg too).
> Similar for 64b.

Understood, sounds good to me.

> This clearly would not be a "fix" material, as all call sites should be
> examined to check if they conform.

Sure, that also seems reasonable.
But do you also think a more minimal fix is in order?

> 
> > +#define IXGBE_WRITE_REG_LE32(a, reg, value) \
> > +	ixgbe_write_reg((a), (reg), (u32 __force)cpu_to_le32(value))
> > +
>

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Przemek Kitszel 7 months, 2 weeks ago

On 1/17/25 19:09, Simon Horman wrote:
> On Fri, Jan 17, 2025 at 11:01:22AM +0100, Przemek Kitszel wrote:
>> On 1/16/25 17:21, Simon Horman wrote:
>>> On Wed, Jan 15, 2025 at 09:11:17AM +0530, Dheeraj Reddy Jonnalagadda wrote:
>>>> The ixgbe driver was missing proper endian conversion for ACI descriptor
>>>> register operations. Add the necessary conversions when reading and
>>>> writing to the registers.
>>>>
>>>> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
>>>> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
>>>> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
>>>
>>> Hi Dheeraj,
>>>
>>> It seems that Sparse is not very happy about __le32 values appearing
>>> where u32 ones are expected. I wonder if something like what is below
>>> (compile tested only!) would both address the problem at hand and
>>> keep Sparse happy (even if negting much of it's usefulness by using casts).
>>>
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> index 6639069ad528..8b3787837128 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.h
>>> @@ -150,6 +150,9 @@ static inline void ixgbe_write_reg(struct ixgbe_hw *hw, u32 reg, u32 value)
>>>    }
>>>    #define IXGBE_WRITE_REG(a, reg, value) ixgbe_write_reg((a), (reg), (value))
>>
>> Simon,
>>
>> As all ixgbe registers are LE, it would be beneficial to change
>> ixgbe_write_reg(), as @value should be __le32, (perhaps @reg too).
>> Similar for 64b.
> 
> Understood, sounds good to me.
> 
>> This clearly would not be a "fix" material, as all call sites should be
>> examined to check if they conform.
> 
> Sure, that also seems reasonable.
> But do you also think a more minimal fix is in order?
> 

@Dheeraj, do you want to hone your minimal fix to avoid sparse warnings?

follow up question: do you want to proceed with a full conversion?

@Michal is going to send patches that depend on this "full conversion"
next month, so he could also take care of the "full conversion".

>>
>>> +#define IXGBE_WRITE_REG_LE32(a, reg, value) \
>>> +	ixgbe_write_reg((a), (reg), (u32 __force)cpu_to_le32(value))
>>> +
>>

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Dheeraj Reddy Jonnalagadda 7 months, 2 weeks ago

> 
> @Dheeraj, do you want to hone your minimal fix to avoid sparse warnings?

Sure, I will update the patch to avoid sparse warnings.

> follow up question: do you want to proceed with a full conversion?
> 
> @Michal is going to send patches that depend on this "full conversion"
> next month, so he could also take care of the "full conversion".

 I don't mind sending a patch with the full conversion. Although that
 would have quite a few changes. To clarify

 1. Are we updating both @reg and @value to be __le32 ?
 2. Should we also update ixgbe_read_reg() to also handle and return
    __le32 values?

-Dheeraj

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Przemek Kitszel 7 months, 1 week ago

On 1/21/25 14:50, Dheeraj Reddy Jonnalagadda wrote:
>>
>> @Dheeraj, do you want to hone your minimal fix to avoid sparse warnings?
> 
> Sure, I will update the patch to avoid sparse warnings.
> 
>> follow up question: do you want to proceed with a full conversion?
>>
>> @Michal is going to send patches that depend on this "full conversion"
>> next month, so he could also take care of the "full conversion".
> 
>   I don't mind sending a patch with the full conversion. Although that
>   would have quite a few changes. To clarify
> 
>   1. Are we updating both @reg and @value to be __le32 ?
>   2. Should we also update ixgbe_read_reg() to also handle and return
>      __le32 values?
> 
> -Dheeraj

Hi, noone objected, so the answer is 2 x Yes

RE: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Kwapulinski, Piotr 7 months, 3 weeks ago

>-----Original Message-----
>From: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> 
>Sent: Wednesday, January 15, 2025 4:41 AM
>To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>Cc: andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; michal.swiatkowski@linux.intel.com; intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
>Subject: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers
>
>The ixgbe driver was missing proper endian conversion for ACI descriptor register operations. Add the necessary conversions when reading and writing to the registers.
>
>Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
>Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
>Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
>---
>Changelog
>
>v2:
>	- Updated the patch to include suggested fix
>	- Updated the commit message to describe the issue
>
> drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>index 683c668672d6..3b9017e72d0e 100644
>--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>@@ -113,7 +113,7 @@ 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), cpu_to_le32(raw_desc[i]));
> 
> 	/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
> 	 * PF_HICR_EV
>@@ -145,7 +145,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] = le32_to_cpu(raw_desc[i]);
> 		}
> 	}
> 
>@@ -153,7 +153,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] = le32_to_cpu(raw_desc[i]);
> 		}
> 	}
> 
>--
>2.34.1
>
Please avoid the closed external links in the future. Thank you.
Reviewed-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>

Re: [PATCH v2 net-next] ixgbe: Fix endian handling for ACI descriptor registers

Posted by Michal Swiatkowski 7 months, 3 weeks ago

On Wed, Jan 15, 2025 at 09:11:17AM +0530, Dheeraj Reddy Jonnalagadda wrote:
> The ixgbe driver was missing proper endian conversion for ACI descriptor
> register operations. Add the necessary conversions when reading and
> writing to the registers.
> 
> Fixes: 46761fd52a88 ("ixgbe: Add support for E610 FW Admin Command Interface")
> Closes: https://scan7.scan.coverity.com/#/project-view/52337/11354?selectedIssue=1602757
> Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com>
> ---
> Changelog
> 
> v2:
> 	- Updated the patch to include suggested fix
> 	- Updated the commit message to describe the issue
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> index 683c668672d6..3b9017e72d0e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> @@ -113,7 +113,7 @@ 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), cpu_to_le32(raw_desc[i]));
>  
>  	/* SW has to set PF_HICR.C bit and clear PF_HICR.SV and
>  	 * PF_HICR_EV
> @@ -145,7 +145,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] = le32_to_cpu(raw_desc[i]);
>  		}
>  	}
>  
> @@ -153,7 +153,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] = le32_to_cpu(raw_desc[i]);
>  		}
>  	}
>  
> -- 
> 2.34.1
> 
Thanks for fixing it
Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>