Messages in this thread |  | | From | "Kwapulinski, Piotr" <> | | Subject | RE: [PATCH net-next 07/15] ixgbe: E610: add ACI dynamic debug | | Date | Tue, 5 May 2026 06:11:44 +0000 |
| |
>-----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?
|  |