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

["Jiawen Wu" <jiawenwu@trustnetic.com>]

On Wed, Jan 15, 2025 11:30 PM, Simon Horman wrote:
> 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...

Oooh, you are right, I'm too careless.

> 
> > +
> > +	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;
> > +}
> 
> ...
>