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

Jacob Keller
2026-05-04T22:33:37Z

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?