netdev - Re: [PATCH net-next 4/5] net: wangxun: add ethtool

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]

Date: Wed, 22 Nov 2023 17:35:51 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, linux@...linux.org.uk,
	horms@...nel.org, mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next 4/5] net: wangxun: add ethtool_ops for channel
 number

> +int wx_set_channels(struct net_device *dev,
> +		    struct ethtool_channels *ch)
> +{
> +	unsigned int count = ch->combined_count;
> +	struct wx *wx = netdev_priv(dev);
> +
> +	/* verify they are not requesting separate vectors */
> +	if (!count || ch->rx_count || ch->tx_count)
> +		return -EINVAL;

I think EOPNOTSUPP here. Its a but fuzzy when you use EINVAL and when
to use EINVAL. If its a feature you don't support at all, use
EOPNOTSUPP. If its a feature you do support, but the value is out of
range for what the hardware can do, then EINVAL.

So here, the configuration is asking for something you cannot support,
split RX and TX configuration. So EOPNOTSUPP.

> +
> +	/* verify other_count has not changed */
> +	if (ch->other_count != 1)
> +		return -EINVAL;
> +
> +	/* verify the number of channels does not exceed hardware limits */
> +	if (count > wx_max_channels(wx))
> +		return -EINVAL;

Here it is out of range, so EINVAL is correct.

Please think about this for the whole patchset, and the driver in
general.

> +
> +	wx->ring_feature[RING_F_RSS].limit = count;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(wx_set_channels);
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.h b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.h
> index 3a80f8e63719..5b5af3689c04 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_ethtool.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_ethtool.h
> @@ -25,4 +25,8 @@ int wx_set_coalesce(struct net_device *netdev,
>  		    struct ethtool_coalesce *ec,
>  		    struct kernel_ethtool_coalesce *kernel_coal,
>  		    struct netlink_ext_ack *extack);
> +void wx_get_channels(struct net_device *dev,
> +		     struct ethtool_channels *ch);
> +int wx_set_channels(struct net_device *dev,
> +		    struct ethtool_channels *ch);
>  #endif /* _WX_ETHTOOL_H_ */
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_hw.c b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> index 40897419a970..bad56bba26fc 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_hw.c
> @@ -1597,6 +1597,71 @@ static void wx_restore_vlan(struct wx *wx)
>  		wx_vlan_rx_add_vid(wx->netdev, htons(ETH_P_8021Q), vid);
>  }
>  
> +static void wx_store_reta(struct wx *wx)
> +{
> +	u8 *indir_tbl = wx->rss_indir_tbl;
> +	u32 reta = 0;
> +	u32 i;
> +
> +	/* Fill out the redirection table as follows:
> +	 *  - 8 bit wide entries containing 4 bit RSS index
> +	 */
> +	for (i = 0; i < 128; i++) {
> +		reta |= indir_tbl[i] << (i & 0x3) * 8;
> +		if ((i & 3) == 3) {
> +			wr32(wx, WX_RDB_RSSTBL(i >> 2), reta);
> +			reta = 0;
> +		}
> +	}
> +}
> +
> +static void wx_setup_reta(struct wx *wx)
> +{
> +	u16 rss_i = wx->ring_feature[RING_F_RSS].indices;
> +	u32 i, j;
> +
> +	/* Fill out hash function seeds */
> +	for (i = 0; i < 10; i++)
> +		wr32(wx, WX_RDB_RSSRK(i), wx->rss_key[i]);
> +
> +	/* Fill out redirection table */
> +	memset(wx->rss_indir_tbl, 0, sizeof(wx->rss_indir_tbl));
> +
> +	for (i = 0, j = 0; i < 128; i++, j++) {
> +		if (j == rss_i)
> +			j = 0;
> +
> +		wx->rss_indir_tbl[i] = j;
> +	}
> +
> +	wx_store_reta(wx);
> +}

There are a lot of magic numbers here, 10, 128 etc. It would be good
to add #define to document what they are.

> +/**
> + * wx_init_rss_key - Initialize wx RSS key
> + * @wx: device handle
> + *
> + * Allocates and initializes the RSS key if it is not allocated.
> + **/
> +static inline int wx_init_rss_key(struct wx *wx)

No inline functions in .c files. Let the compiler decide.

> +{
> +	u32 *rss_key;
> +
> +	if (!wx->rss_key) {
> +		rss_key = kzalloc(WX_RSS_KEY_SIZE, GFP_KERNEL);
> +		if (unlikely(!rss_key))
> +			return -ENOMEM;
> +
> +		netdev_rss_key_fill(rss_key, WX_RSS_KEY_SIZE);
> +		wx->rss_key = rss_key;
> +	}
> +
> +	return 0;
> +}
> +
>  int wx_sw_init(struct wx *wx)
>  {
>  	struct pci_dev *pdev = wx->pdev;
> @@ -1861,6 +1950,11 @@ int wx_sw_init(struct wx *wx)
>  		return -ENOMEM;
>  	}
>  
> +	if (wx_init_rss_key(wx)) {
> +		wx_err(wx, "rss key allocation failed\n");
> +		return -ENOMEM;

Return the error code wx_init_rss_key() returns.

       Andrew