Re: [PATCH net-next v3 1/2] net: txgbe: Add basic support for new AML devices

[Simon Horman <horms@kernel.org>]

On Wed, Jan 15, 2025 at 06:24:07PM +0800, Jiawen Wu wrote:
> There is a new 40/25/10 Gigabit Ethernet device.
> 
> To support basic functions, PHYLINK is temporarily skipped as it is
> intended to implement these configurations in the firmware. And the
> associated link IRQ is also skipped.
> 
> And Implement the new SW-FW interaction interface, which use 64 Byte
> message buffer.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>

...

> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c

...

> +static bool wx_poll_fw_reply(struct wx *wx, u32 *buffer,
> +			     struct wx_hic_hdr *recv_hdr, u8 send_cmd)
> +{
> +	u32 dword_len = sizeof(struct wx_hic_hdr) >> 2;
> +	u32 i;
> +
> +	/* read hdr */
> +	for (i = 0; i < dword_len; i++) {
> +		buffer[i] = rd32a(wx, WX_FW2SW_MBOX, i);
> +		le32_to_cpus(&buffer[i]);
> +	}
> +
> +	/* check hdr */
> +	recv_hdr = (struct wx_hic_hdr *)buffer;
> +	if (recv_hdr->cmd == send_cmd &&
> +	    recv_hdr->index == wx->swfw_index)
> +		return true;

Hi Jiawen Wu,

Maybe I am misreading this but, given the way that recv_hdr is
passed to this function, it seems that the same result would
he achieved if recv_hdr was a local variable...

> +
> +	return false;
> +}
> +
> +static int wx_host_interface_command_r(struct wx *wx, u32 *buffer,
> +				       u32 length, u32 timeout, bool return_data)
> +{
> +	struct wx_hic_hdr *send_hdr = (struct wx_hic_hdr *)buffer;
> +	u32 hdr_size = sizeof(struct wx_hic_hdr);
> +	struct wx_hic_hdr *recv_hdr;
> +	bool busy, reply;
> +	u32 dword_len;
> +	u16 buf_len;
> +	int err = 0;
> +	u8 send_cmd;
> +	u32 i;
> +
> +	/* wait to get lock */
> +	might_sleep();
> +	err = read_poll_timeout(test_and_set_bit, busy, !busy, 1000, timeout * 1000,
> +				false, WX_STATE_SWFW_BUSY, wx->state);
> +	if (err)
> +		return err;
> +
> +	/* index to unique seq id for each mbox message */
> +	send_hdr->index = wx->swfw_index;
> +	send_cmd = send_hdr->cmd;
> +
> +	dword_len = length >> 2;
> +	/* write data to SW-FW mbox array */
> +	for (i = 0; i < dword_len; i++) {
> +		wr32a(wx, WX_SW2FW_MBOX, i, (__force u32)cpu_to_le32(buffer[i]));
> +		/* write flush */
> +		rd32a(wx, WX_SW2FW_MBOX, i);
> +	}
> +
> +	/* generate interrupt to notify FW */
> +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, 0);
> +	wr32m(wx, WX_SW2FW_MBOX_CMD, WX_SW2FW_MBOX_CMD_VLD, WX_SW2FW_MBOX_CMD_VLD);
> +
> +	/* polling reply from FW */
> +	err = read_poll_timeout(wx_poll_fw_reply, reply, reply, 1000, 50000,
> +				true, wx, buffer, recv_hdr, send_cmd);
> +	if (err) {
> +		wx_err(wx, "Polling from FW messages timeout, cmd: 0x%x, index: %d\n",
> +		       send_cmd, wx->swfw_index);
> +		goto rel_out;
> +	}
> +
> +	/* expect no reply from FW then return */
> +	if (!return_data)
> +		goto rel_out;
> +
> +	/* If there is any thing in data position pull it in */
> +	buf_len = recv_hdr->buf_len;

... and most likely related, recv_hdr appears to be uninitialised here.

This part is flagged by W=1 builds with clang-19, and my Smatch.

> +	if (buf_len == 0)
> +		goto rel_out;
> +
> +	if (length < buf_len + hdr_size) {
> +		wx_err(wx, "Buffer not large enough for reply message.\n");
> +		err = -EFAULT;
> +		goto rel_out;
> +	}
> +
> +	/* Calculate length in DWORDs, add 3 for odd lengths */
> +	dword_len = (buf_len + 3) >> 2;
> +	for (i = hdr_size >> 2; i <= dword_len; i++) {
> +		buffer[i] = rd32a(wx, WX_FW2SW_MBOX, i);
> +		le32_to_cpus(&buffer[i]);
> +	}
> +
> +rel_out:
> +	/* index++, index replace wx_hic_hdr.checksum */
> +	if (send_hdr->index == WX_HIC_HDR_INDEX_MAX)
> +		wx->swfw_index = 0;
> +	else
> +		wx->swfw_index = send_hdr->index + 1;
> +
> +	clear_bit(WX_STATE_SWFW_BUSY, wx->state);
> +	return err;
> +}

...