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

Kwapulinski, Piotr
2026-05-05T06:11:50Z

>-----Original Message-----
>From: Keller, Jacob E <jacob.e.keller@intel.com>
>Sent: Tuesday, May 5, 2026 12:33 AM
>To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
>Cc: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; andrew+netdev@lunn.ch; davem@davemloft.net; edumazet@google.com; pabeni@redhat.com; Lobakin, Aleksander <aleksander.lobakin@intel.com>; horms@kernel.org; michal.swiatkowski@linux.intel.com; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nitka, Grzegorz <grzegorz.nitka@intel.com>; Vecera, Ivan <ivecera@redhat.com>; Kubalewski, Arkadiusz <arkadiusz.kubalewski@intel.com>; Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Wegrzyn, Stefan <stefan.wegrzyn@intel.com>; Kwapulinski, Piotr <piotr.kwapulinski@intel.com>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; pmenzel@molgen.mpg.de; Mekala, SunithaX D <sunithax.d.mekala@intel.com>
>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@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?