Re: [PATCH net-next 2/2] libie: log more info when virtchnl fails

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new "Non-zero virtchnl ret val" log in
  libie_ctlq_xn_process_recv()…

--

libie: log more info when virtchnl fails

This commit adds rate-limited error messages to the libie control queue
code. libie_ctlq_xn_process_recv() now logs when a received message has a
non-zero virtchnl return value. libie_ctlq_xn_send() now logs when a
synchronous transaction ends in a failed state.

> diff --git a/drivers/net/ethernet/intel/libie/controlq.c b/drivers/net/ethernet/intel/libie/controlq.c
> index 6214fc036ce5f..5fce6d9db1e28 100644
> --- a/drivers/net/ethernet/intel/libie/controlq.c
> +++ b/drivers/net/ethernet/intel/libie/controlq.c
> @@ -835,6 +835,13 @@ libie_ctlq_xn_process_recv(struct libie_ctlq_xn_recv_params *params,
>  		return false;
>  	}
>  
> +	if (ctlq_msg->chnl_retval) {
> +		dev_err_ratelimited(params->ctlq->dev,
> +				    "Non-zero virtchnl ret val %u (msg op: %u, data_len: %u); xn id: %u, cookie: %u\n",
> +				    ctlq_msg->chnl_retval, ctlq_msg->chnl_opcode,
> +				    ctlq_msg->data_len, xn->index, xn->cookie);
> +	}

[Severity: Low]
Does this message always show values the device actually reported? When
ctlq_msg reaches libie_ctlq_xn_process_recv(), libie_ctlq_fill_rx_msg()
may already have overwritten chnl_retval and data_len:

drivers/net/ethernet/intel/libie/controlq.c:libie_ctlq_fill_rx_msg() {
    ...
	if (likely(msg->data_len)) {
		if (unlikely(msg->data_len > LIBIE_CTLQ_MAX_BUF_LEN)) {
			msg->data_len = LIBIE_CTLQ_MAX_BUF_LEN;
			msg->chnl_retval = U32_MAX;
		}
    ...
}

Suppose a reply's descriptor has a DATA_LEN field larger than
LIBIE_CTLQ_MAX_BUF_LEN. This would print "Non-zero virtchnl ret val
4294967295" with data_len equal to LIBIE_CTLQ_MAX_BUF_LEN.

By then the firmware's real virtchnl retval (possibly 0) and the real
DATA_LEN are lost. The log would then make the driver's own length check
look like a firmware error.

The new "Transaction failed" message in libie_ctlq_xn_send() prints
xn->recv_mem.iov_len, which is the same capped length.

Also, neither message includes ctlq_msg->hw_retval. That is the
mailbox-level status that libie_ctlq_fill_rx_msg() decodes from the
descriptor.

Could the oversized-length case be logged separately in
libie_ctlq_fill_rx_msg() with the raw DATA_LEN? Could hw_retval also be
added to the new messages?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929231305.1515873-1-anthony.l.nguyen%40intel.com