RE: [PATCH net-next 07/15] ixgbe: E610: add ACI dynamic debug

[Kwapulinski, Piotr]

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@xxxxxxxxx> 
>Sent: Tuesday, May 5, 2026 12:33 AM
>To: Kwapulinski, Piotr <piotr.kwapulinski@xxxxxxxxx>
>Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; Lobakin, Aleksander <aleksander.lobakin@xxxxxxxxx>; horms@xxxxxxxxxx; michal.swiatkowski@xxxxxxxxxxxxxxx; Jagielski, Jedrzej <jedrzej.jagielski@xxxxxxxxx>; Loktionov, Aleksandr <aleksandr.loktionov@xxxxxxxxx>; Nitka, Grzegorz <grzegorz.nitka@xxxxxxxxx>; Vecera, Ivan <ivecera@xxxxxxxxxx>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@xxxxxxxxx>; Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Wegrzyn, Stefan <stefan.wegrzyn@xxxxxxxxx>; Kwapulinski, Piotr <piotr.kwapulinski@xxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; pmenzel@xxxxxxxxxxxxx; Mekala, SunithaX D <sunithax.d.mekala@xxxxxxxxx>
>Subject: Re: [PATCH net-next 07/15] ixgbe: E610: add ACI dynamic debug
>
>On 5/2/2026 7:06 PM, Jakub Kicinski wrote:
>> This is an AI-generated review of your patch. The human sending this 
>> email has considered the AI review valid, or at least plausible.
>> ---
>> ixgbe: E610: add ACI dynamic debug
>> 
>> This commit enables dynamic debugging of the Admin Command Interface 
>> (ACI) for the e610 adapter, allowing ACI commands and descriptor 
>> details to be printed into the kernel log.
>> 
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c 
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> 
>> [ ... ]
>> 
>>> @@ -106,8 +185,12 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>>>  		if (desc->flags & cpu_to_le16(LIBIE_AQ_FLAG_RD)) {
>>>  			for (i = 0; i < buf_size / 4; i++)
>>>  				IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), ((u32 *)buf)[i]);
>>> -			if (buf_tail_size)
>>> +			ixgbe_aci_debug(hw, desc, buf, buf_size);
>>> +			if (buf_tail_size) {
>>>  				IXGBE_WRITE_REG(hw, IXGBE_PF_HIBA(i), buf_tail);
>>> +				ixgbe_aci_debug(hw, desc, &buf_tail,
>>> +						buf_tail_size);
>>> +			}
>>>  		}
>>>  	}
>> 
>> Does this result in redundant output being printed to the kernel log?
>> 
>> When buf_tail_size is non-zero, ixgbe_aci_debug() is called twice: 
>> once for the full buffer, and a second time for just the tail. Since 
>> ixgbe_aci_debug() prints the full CQ CMD descriptor details each time, 
>> this appears to print the descriptor headers twice and fragment the buffer output.
>> 
>> [ ... ]
>> 
>>> @@ -183,10 +278,13 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>>>  	if (valid_buf) {
>>>  		for (i = 0; i < buf_size / 4; i++)
>>>  			((u32 *)buf)[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>>> +		ixgbe_aci_debug(hw, raw_desc, buf, buf_size);
>>>  		if (buf_tail_size) {
>>>  			buf_tail = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>>>  			memcpy(buf + buf_size - buf_tail_size, &buf_tail,
>>>  			       buf_tail_size);
>>> +			ixgbe_aci_debug(hw, raw_desc, &buf_tail,
>>> +					buf_tail_size);
>>>  		}
>>>  	}
>> 
>> Can this read uninitialized memory from the buffer?
>> 
>> At the point ixgbe_aci_debug(..., buf, buf_size) is called, the loop 
>> has only populated the 4-byte aligned chunks of the buffer. The 
>> trailing bytes at the end of the buffer are not populated until the 
>> memcpy() occurs later in the block. Because ixgbe_aci_debug() consumes 
>> buf_size bytes, it looks like it will read and log uninitialized 
>> trailing memory (potentially from the kernel stack).
>> 
>> Also, similar to the transmit path, this calls ixgbe_aci_debug() a 
>> second time on &buf_tail, which will print the descriptor headers again.
>> 
>> Could we move a single ixgbe_aci_debug(..., buf, buf_size) call to the 
>> end of the block, after the entire buffer including the tail has been fully written?
>
>Piotr,
>
>Please review this report and submit a new version of this patch to Intel Wired LAN.
>
>While you are at it, please review the following report from a run of sashiko I did myself using claude:
Will update. Thank you.
Piotr

>
>> commit 198ec0389c757125d9079647a6c8aba34ba29048
>> Author: Piotr Kwapulinski <piotr.kwapulinski@xxxxxxxxx>
>> ixgbe: E610: add ACI dynamic debug
>> This patch enables dynamic debug of the Admin Command Interface (ACI) 
>> for the E610 adapter, adding descriptor and buffer hex dumps via the 
>> standard dyndbg infrastructure, plus hw_dbg() calls on error paths.
>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c 
>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>>> index 665a9813e251..b686636beb93 100644
>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
>> [ ... ]
>>> @@ -147,6 +230,7 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>>> 			raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA(i));
>>> 			raw_desc[i] = raw_desc[i];
>>> 		}
>>>+		ixgbe_aci_debug(hw, raw_desc, NULL, 0);
>>> 	}
>> [ ... ]
>>> 	/* Handle timeout and invalid state of HICR register */
>>>-	if (hicr & IXGBE_PF_HICR_C)
>>>+	if (hicr & IXGBE_PF_HICR_C) {
>>>+		hw_dbg(hw, "error: Admin Command 0x%X command timeout\n",
>>>+		       le16_to_cpu(desc->opcode));
>>> 		return -ETIME;
>>>+	}
>> Since raw_desc is declared as a cast of desc:
>>     u32 *raw_desc = (u32 *)desc;
>> and the sync response read loop overwrites raw_desc[] (and thus desc) 
>> with firmware response data, could le16_to_cpu(desc->opcode) here 
>> print the response opcode rather than the original command opcode?
>> The local variable opcode already holds the original value saved 
>> earlier via:
>>     opcode = le16_to_cpu(desc->opcode); Would it be more correct to 
>> use opcode directly in this hw_dbg() call (and the one in the 
>> invalid-state check below)?
>>> @@ -183,10 +278,13 @@ static int ixgbe_aci_send_cmd_execute(struct ixgbe_hw *hw,
>>> 	if (valid_buf) {
>>> 		for (i = 0; i < buf_size / 4; i++)
>>> 			((u32 *)buf)[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>>>+		ixgbe_aci_debug(hw, raw_desc, buf, buf_size);
>>> 		if (buf_tail_size) {
>>> 			buf_tail = IXGBE_READ_REG(hw, IXGBE_PF_HIBA(i));
>>> 			memcpy(buf + buf_size - buf_tail_size, &buf_tail,
>>> 			       buf_tail_size);
>>>+			ixgbe_aci_debug(hw, raw_desc, &buf_tail,
>>>+					buf_tail_size);
>>> 		}
>>> 	}
>> When buf_size is not 4-byte aligned, ixgbe_aci_debug() is called with 
>> the full buf_size before the tail bytes have been read from hardware 
>> and memcpy'd into buf. The hex dump will show stale content for the 
>> last 1-3 bytes of the buffer.
>> Should the ixgbe_aci_debug() call be moved after the tail memcpy so 
>> that it dumps the complete response?