Re: [PATCH net-next v4 4/6] net: libwx: Add msg task func

[Simon Horman <horms@kernel.org>]

On Tue, Jun 04, 2024 at 11:57:33PM +0800, Mengyuan Lou wrote:
> Implement wx_msg_task which is used to process mailbox
> messages sent by vf.
> 
> Signed-off-by: Mengyuan Lou <mengyuanlou@net-swift.com>

Hi Mengyuan Lou,

Some minor comments from my side.

...

> +static int wx_set_vf_mac_addr(struct wx *wx, u32 *msgbuf, u16 vf)
> +{
> +	u8 *new_mac = ((u8 *)(&msgbuf[1]));
> +
> +	if (!is_valid_ether_addr(new_mac)) {
> +		wx_err(wx, "VF %d attempted to set invalid mac\n", vf);
> +		return -EINVAL;
> +	}
> +
> +	if (wx->vfinfo[vf].pf_set_mac &&
> +	    memcmp(wx->vfinfo[vf].vf_mac_addr, new_mac, ETH_ALEN)) {
> +		wx_err(wx,
> +		       "VF %d attempted to set a MAC address but it already had a MAC address.",
> +		       vf);
> +		return -EBUSY;
> +	}
> +	return wx_set_vf_mac(wx, vf, new_mac) < 0;

This seems to return a bool - true on error, false otherwise.
But I think it would be more natural to consistently return
a negative error value on error - as is done above, and 0 on success.

So perhaps something like this (completely untested!):

	err = wx_set_vf_mac(wx, vf, index, new_mac);
	if (err)
		return err;

	return 0;

> +}

...

> +static int wx_set_vf_lpe(struct wx *wx, u32 max_frame, u32 vf)
> +{
> +	struct net_device *netdev = wx->netdev;
> +	u32 index, vf_bit, vfre;
> +	u32 max_frs, reg_val;
> +	int pf_max_frame;
> +	int err = 0;
> +
> +	pf_max_frame = netdev->mtu + ETH_HLEN +  ETH_FCS_LEN + VLAN_HLEN;
> +	switch (wx->vfinfo[vf].vf_api) {
> +	case wx_mbox_api_11 ... wx_mbox_api_13:
> +		/* Version 1.1 supports jumbo frames on VFs if PF has
> +		 * jumbo frames enabled which means legacy VFs are
> +		 * disabled
> +		 */
> +		if (pf_max_frame > ETH_FRAME_LEN)
> +			break;
> +		fallthrough;
> +	default:
> +		/* If the PF or VF are running w/ jumbo frames enabled
> +		 * we need to shut down the VF Rx path as we cannot
> +		 * support jumbo frames on legacy VFs
> +		 */
> +		if (pf_max_frame > ETH_FRAME_LEN ||
> +		    (max_frame > (ETH_FRAME_LEN + ETH_FCS_LEN + VLAN_HLEN)))
> +			err = -EINVAL;
> +		break;
> +	}
> +
> +	/* determine VF receive enable location */
> +	vf_bit = vf % 32;
> +	index = vf / 32;
> +
> +	/* enable or disable receive depending on error */
> +	vfre = rd32(wx, WX_RDM_VF_RE(index));
> +	if (err)
> +		vfre &= ~BIT(vf_bit);
> +	else
> +		vfre |= BIT(vf_bit);
> +	wr32(wx, WX_RDM_VF_RE(index), vfre);
> +
> +	if (err) {
> +		wx_err(wx, "VF max_frame %d out of range\n", max_frame);
> +		return err;
> +	}
> +	/* pull current max frame size from hardware */
> +	max_frs = DIV_ROUND_UP(max_frame, 1024);
> +	reg_val = rd32(wx, WX_MAC_WDG_TIMEOUT) & WX_MAC_WDG_TIMEOUT_WTO_MASK;
> +	if (max_frs > (reg_val + WX_MAC_WDG_TIMEOUT_WTO_DELTA))
> +		wr32(wx, WX_MAC_WDG_TIMEOUT, max_frs - WX_MAC_WDG_TIMEOUT_WTO_DELTA);

nit: This could trivially be line wrapped to <= 80 columns wide

		wr32(wx, WX_MAC_WDG_TIMEOUT,
		     max_frs - WX_MAC_WDG_TIMEOUT_WTO_DELTA);

     Flagged by checkpatch.pl --max-line-length=80

> +
> +	return 0;
> +}

...

> +static int wx_set_vf_macvlan_msg(struct wx *wx, u32 *msgbuf, u16 vf)
> +{
> +	int index = (msgbuf[0] & WX_VT_MSGINFO_MASK) >>
> +		    WX_VT_MSGINFO_SHIFT;
> +	u8 *new_mac = ((u8 *)(&msgbuf[1]));
> +	int err;
> +
> +	if (wx->vfinfo[vf].pf_set_mac && index > 0) {
> +		wx_err(wx, "VF %d requested MACVLAN filter but is administratively denied\n", vf);
> +		return -EINVAL;
> +	}
> +
> +	/* An non-zero index indicates the VF is setting a filter */
> +	if (index) {
> +		if (!is_valid_ether_addr(new_mac)) {
> +			wx_err(wx, "VF %d attempted to set invalid mac\n", vf);
> +			return -EINVAL;
> +		}
> +		/* If the VF is allowed to set MAC filters then turn off
> +		 * anti-spoofing to avoid false positives.
> +		 */
> +		if (wx->vfinfo[vf].spoofchk_enabled)
> +			wx_set_vf_spoofchk(wx->netdev, vf, false);
> +	}
> +
> +	err = wx_set_vf_macvlan(wx, vf, index, new_mac);
> +	if (err == -ENOSPC)
> +		wx_err(wx,
> +		       "VF %d has requested a MACVLAN filter but there is no space for it\n",
> +		       vf);
> +
> +	return err < 0;

As per my comment on wx_set_vf_mac_addr(),
this return scheme seems a little unorthodox.

I'd suggest something like this (completely untested!):

	err = wx_set_vf_macvlan(wx, vf, index, new_mac);
	if (err == -ENOSPC)
		wx_err(...)
	if (err)
		return err;

	return 0;

> +}

...

> +static int wx_update_vf_xcast_mode(struct wx *wx, u32 *msgbuf, u32 vf)
> +{
> +	int xcast_mode = msgbuf[1];
> +	u32 vmolr, disable, enable;
> +
> +	/* verify the PF is supporting the correct APIs */
> +	switch (wx->vfinfo[vf].vf_api) {
> +	case wx_mbox_api_12:
> +		/* promisc introduced in 1.3 version */
> +		if (xcast_mode == WXVF_XCAST_MODE_PROMISC)
> +			return -EOPNOTSUPP;
> +		fallthrough;
> +	case wx_mbox_api_13:
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	if (wx->vfinfo[vf].xcast_mode == xcast_mode)
> +		goto out;
> +
> +	switch (xcast_mode) {
> +	case WXVF_XCAST_MODE_NONE:
> +		disable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE |
> +			  WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;

nit: This could also be trivially line-wrapped to less than 80 columns wide.
     Likewise a few times below.

> +		enable = 0;
> +		break;
> +	case WXVF_XCAST_MODE_MULTI:
> +		disable = WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> +		enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE;
> +		break;
> +	case WXVF_XCAST_MODE_ALLMULTI:
> +		disable = WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> +		enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE | WX_PSR_VM_L2CTL_MPE;
> +		break;
> +	case WXVF_XCAST_MODE_PROMISC:
> +		disable = 0;
> +		enable = WX_PSR_VM_L2CTL_BAM | WX_PSR_VM_L2CTL_ROMPE |
> +			 WX_PSR_VM_L2CTL_MPE | WX_PSR_VM_L2CTL_UPE | WX_PSR_VM_L2CTL_VPE;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	vmolr = rd32(wx, WX_PSR_VM_L2CTL(vf));
> +	vmolr &= ~disable;
> +	vmolr |= enable;
> +	wr32(wx, WX_PSR_VM_L2CTL(vf), vmolr);
> +
> +	wx->vfinfo[vf].xcast_mode = xcast_mode;
> +out:
> +	msgbuf[1] = xcast_mode;
> +
> +	return 0;
> +}

...