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

[Jakub Kicinski]

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?