netdev - Re: [PATCH net 5/5] net: txgbe: fix to control VLAN strip

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240418195458.GR3975545@kernel.org>
Date: Thu, 18 Apr 2024 20:54:58 +0100
From: Simon Horman <horms@...nel.org>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, rmk+kernel@...linux.org.uk, andrew@...n.ch,
	netdev@...r.kernel.org, mengyuanlou@...-swift.com,
	duanqiangwen@...-swift.com
Subject: Re: [PATCH net 5/5] net: txgbe: fix to control VLAN strip

On Tue, Apr 16, 2024 at 02:29:52PM +0800, Jiawen Wu wrote:
> When VLAN tag strip is changed to enable or disable, the hardware requires
> the Rx ring to be in a disabled state, otherwise the feature cannot be
> changed.
> 
> Fixes: f3b03c655f67 ("net: wangxun: Implement vlan add and kill functions")
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---
>  .../net/ethernet/wangxun/txgbe/txgbe_main.c   | 58 ++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> index af0c548e52b0..2a6b35036fce 100644
> --- a/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c
> +++ b/drivers/net/ethernet/wangxun/txgbe/txgbe_main.c

...

> +static int txgbe_set_features(struct net_device *netdev, netdev_features_t features)

There seems to be a lot of overlap between this function and
wx_set_features(). To aid maintenance, the common code be
factored out into a shared function?

> +{
> +	netdev_features_t changed = netdev->features ^ features;
> +	struct wx *wx = netdev_priv(netdev);
> +
> +	if (features & NETIF_F_RXHASH) {
> +		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN,
> +		      WX_RDB_RA_CTL_RSS_EN);
> +		wx->rss_enabled = true;
> +	} else {
> +		wr32m(wx, WX_RDB_RA_CTL, WX_RDB_RA_CTL_RSS_EN, 0);
> +		wx->rss_enabled = false;
> +	}
> +
> +	netdev->features = features;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_RX)
> +		txgbe_do_reset(netdev);
> +	else if (changed & NETIF_F_HW_VLAN_CTAG_FILTER)
> +		wx_set_rx_mode(netdev);

I see the following in wx_set_features():

        if (changed &
	    (NETIF_F_HW_VLAN_CTAG_RX |
	     NETIF_F_HW_VLAN_STAG_RX))
		wx_set_rx_mode(netdev);

Should NETIF_F_HW_VLAN_STAG_RX and NETIF_F_HW_VLAN_STAG_FILTER
also be checked in this function?

> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops txgbe_netdev_ops = {
>  	.ndo_open               = txgbe_open,
>  	.ndo_stop               = txgbe_close,
>  	.ndo_change_mtu         = wx_change_mtu,
>  	.ndo_start_xmit         = wx_xmit_frame,
>  	.ndo_set_rx_mode        = wx_set_rx_mode,
> -	.ndo_set_features       = wx_set_features,
> +	.ndo_set_features       = txgbe_set_features,
>  	.ndo_fix_features       = wx_fix_features,
>  	.ndo_validate_addr      = eth_validate_addr,
>  	.ndo_set_mac_address    = wx_set_mac,
> -- 
> 2.27.0
> 
>