Re: [PATCH net-next 3/5] net: libwx: Add msg task api

[Simon Horman <horms@kernel.org>]

On Thu, Mar 07, 2024 at 05:54:58PM +0800, Mengyuan Lou wrote:
> Implement ndo_set_vf_spoofchk and ndo_set_vf_link_state
> interfaces.
> Implement wx_msg_task which is used to process mailbox
> messages sent by vf.
> Reallocate queue and int resources when sriov is enabled.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>

Hi Mengyuan Lou,

some minor feedback from my side.

...

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

...

> +void wx_configure_virtualization(struct wx *wx)
> +{
> +	u16 pool = wx->num_rx_pools;
> +	u32 reg_offset, vf_shift;
> +	u32 i;
> +
> +	if (!test_bit(WX_FLAG_SRIOV_ENABLED, wx->flags))
> +		return;
> +
> +	wr32m(wx, WX_PSR_VM_CTL,
> +	      WX_PSR_VM_CTL_POOL_MASK | WX_PSR_VM_CTL_REPLEN,
> +	      FIELD_PREP(WX_PSR_VM_CTL_POOL_MASK, VMDQ_P(0)) |
> +	      WX_PSR_VM_CTL_REPLEN);
> +	while (pool--)
> +		wr32m(wx, WX_PSR_VM_L2CTL(i), WX_PSR_VM_L2CTL_AUPE, WX_PSR_VM_L2CTL_AUPE);

i is not initialised here.

Flagged by clang-17 W=1 build, and Smatch.

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

...

> +static int wx_vf_reset_msg(struct wx *wx, u16 vf)
> +{
> +	unsigned char *vf_mac = wx->vfinfo[vf].vf_mac_addr;
> +	struct net_device *dev = wx->netdev;
> +	u32 msgbuf[5] = {0, 0, 0, 0, 0};
> +	u8 *addr = (u8 *)(&msgbuf[1]);
> +	u32 reg, index, vf_bit;
> +	int pf_max_frame;
> +
> +	/* reset the filters for the device */
> +	wx_vf_reset_event(wx, vf);
> +
> +	/* set vf mac address */
> +	if (!is_zero_ether_addr(vf_mac))
> +		wx_set_vf_mac(wx, vf, vf_mac);
> +
> +	vf_bit = vf % 32;
> +	index = vf / 32;
> +
> +	/* force drop enable for all VF Rx queues */
> +	wx_write_qde(wx, vf, 1);
> +
> +	/* set transmit and receive for vf */
> +	wx_set_vf_rx_tx(wx, vf);
> +
> +	pf_max_frame = dev->mtu + ETH_HLEN;
> +
> +	if (pf_max_frame > ETH_FRAME_LEN)
> +		reg = BIT(vf_bit);

If the condition above is false then reg is used uninitialised below.

> +	wr32(wx, WX_RDM_VFRE_CLR(index), reg);
> +
> +	/* enable VF mailbox for further messages */
> +	wx->vfinfo[vf].clear_to_send = true;
> +
> +	/* reply to reset with ack and vf mac address */
> +	msgbuf[0] = WX_VF_RESET;
> +	if (!is_zero_ether_addr(vf_mac)) {
> +		msgbuf[0] |= WX_VT_MSGTYPE_ACK;
> +		memcpy(addr, vf_mac, ETH_ALEN);
> +	} else {
> +		msgbuf[0] |= WX_VT_MSGTYPE_NACK;
> +		wx_err(wx, "VF %d has no MAC address assigned", vf);
> +	}
> +
> +	/* Piggyback the multicast filter type so VF can compute the
> +	 * correct vectors
> +	 */
> +	msgbuf[3] = wx->mac.mc_filter_type;
> +	wx_write_mbx_pf(wx, msgbuf, WX_VF_PERMADDR_MSG_LEN, vf);
> +
> +	return 0;
> +}

...

> +static int wx_rcv_msg_from_vf(struct wx *wx, u16 vf)
> +{
> +	u16 mbx_size = WX_VXMAILBOX_SIZE;
> +	u32 msgbuf[WX_VXMAILBOX_SIZE];
> +	int retval;
> +
> +	retval = wx_read_mbx_pf(wx, msgbuf, mbx_size, vf);
> +	if (retval) {
> +		wx_err(wx, "Error receiving message from VF\n");
> +		return retval;
> +	}
> +
> +	/* this is a message we already processed, do nothing */
> +	if (msgbuf[0] & (WX_VT_MSGTYPE_ACK | WX_VT_MSGTYPE_NACK))

retval is 0 here. Should a negative error value be returned instead?

> +		return retval;
> +
> +	if (msgbuf[0] == WX_VF_RESET)
> +		return wx_vf_reset_msg(wx, vf);
> +
> +	/* until the vf completes a virtual function reset it should not be
> +	 * allowed to start any configuration.
> +	 */
> +	if (!wx->vfinfo[vf].clear_to_send) {
> +		msgbuf[0] |= WX_VT_MSGTYPE_NACK;
> +		wx_write_mbx_pf(wx, msgbuf, 1, vf);

Here too.

> +		return retval;
> +	}
> +
> +	switch ((msgbuf[0] & U16_MAX)) {
> +	case WX_VF_SET_MAC_ADDR:
> +		retval = wx_set_vf_mac_addr(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_SET_MULTICAST:
> +		retval = wx_set_vf_multicasts(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_SET_VLAN:
> +		retval = wx_set_vf_vlan_msg(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_SET_LPE:
> +		if (msgbuf[1] > WX_MAX_JUMBO_FRAME_SIZE) {
> +			wx_err(wx, "VF max_frame %d out of range\n", msgbuf[1]);
> +			return -EINVAL;
> +		}
> +		retval = wx_set_vf_lpe(wx, msgbuf[1], vf);
> +		break;
> +	case WX_VF_SET_MACVLAN:
> +		retval = wx_set_vf_macvlan_msg(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_API_NEGOTIATE:
> +		retval = wx_negotiate_vf_api(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_GET_QUEUES:
> +		retval = wx_get_vf_queues(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_GET_LINK_STATE:
> +		retval = wx_get_vf_link_state(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_GET_FW_VERSION:
> +		retval = wx_get_fw_version(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_UPDATE_XCAST_MODE:
> +		retval = wx_update_vf_xcast_mode(wx, msgbuf, vf);
> +		break;
> +	case WX_VF_BACKUP:
> +		break;
> +	default:
> +		wx_err(wx, "Unhandled Msg %8.8x\n", msgbuf[0]);
> +		retval = -EBUSY;
> +		break;
> +	}
> +
> +	/* notify the VF of the results of what it sent us */
> +	if (retval)
> +		msgbuf[0] |= WX_VT_MSGTYPE_NACK;
> +	else
> +		msgbuf[0] |= WX_VT_MSGTYPE_ACK;
> +
> +	msgbuf[0] |= WX_VT_MSGTYPE_CTS;
> +
> +	wx_write_mbx_pf(wx, msgbuf, mbx_size, vf);
> +
> +	return retval;
> +}

...