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

[Jacob Keller <jacob.e.keller@intel.com>]

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:

> commit 198ec0389c757125d9079647a6c8aba34ba29048
> Author: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> 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?