Messages in this thread |  | | Date | Mon, 4 May 2026 15:33:25 -0700 | | Subject | Re: [PATCH net-next 07/15] ixgbe: E610: add ACI dynamic debug | | From | Jacob Keller <> |
| |
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?
|  |