netdev - Re: [PATCH v2 net-next 07/13] net: enetc: add RSS support for i.MX95 ENETC PF

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: <20250115140042.63b99c4f@kernel.org>
Date: Wed, 15 Jan 2025 14:00:42 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: Wei Fang <wei.fang@....com>
Cc: claudiu.manoil@....com, vladimir.oltean@....com, xiaoning.wang@....com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 pabeni@...hat.com, christophe.leroy@...roup.eu, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
 linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev
Subject: Re: [PATCH v2 net-next 07/13] net: enetc: add RSS support for
 i.MX95 ENETC PF

On Mon, 13 Jan 2025 16:22:39 +0800 Wei Fang wrote:
> Add Receive side scaling (RSS) support for i.MX95 ENETC PF to improve the
> network performance and balance the CPU loading. In addition, since both
> ENETC v1 and ENETC v4 only support the toeplitz algorithm, so a check for
> hfunc was added.

This and previous commits are a bi hard to follow. You plumb some
stuff thru in the previous commit. In this one you reshuffle things,
again. Try to separate code movement / restructuring in one commit. 
And new additions more clearly in the next.

> +static void enetc4_set_rss_key(struct enetc_hw *hw, const u8 *key)
> +{
> +	int i;
> +
> +	for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++)
> +		enetc_port_wr(hw, ENETC4_PRSSKR(i), ((u32 *)key)[i]);
> +}
> +
> +static void enetc4_get_rss_key(struct enetc_hw *hw, u8 *key)
> +{
> +	int i;
> +
> +	for (i = 0; i < ENETC_RSSHASH_KEY_SIZE / 4; i++)
> +		((u32 *)key)[i] = enetc_port_rd(hw, ENETC4_PRSSKR(i));
> +}

Isn't the only difference between the chips the register offset?
Why create full ops for something this trivial?

> +static int enetc4_get_rxnfc(struct net_device *ndev, struct ethtool_rxnfc *rxnfc,
> +			    u32 *rule_locs)
> +{
> +	struct enetc_ndev_priv *priv = netdev_priv(ndev);
> +
> +	switch (rxnfc->cmd) {
> +	case ETHTOOL_GRXRINGS:
> +		rxnfc->data = priv->num_rx_rings;
> +		break;
> +	case ETHTOOL_GRXFH:
> +		return enetc_get_rsshash(rxnfc);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}

Why add a new function instead of returning EOPNOTSUPP for new chips 
in the existing one?

> @@ -712,6 +730,12 @@ static int enetc_set_rxfh(struct net_device *ndev,
>  	struct enetc_hw *hw = &si->hw;
>  	int err = 0;
>  
> +	if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
> +	    rxfh->hfunc != ETH_RSS_HASH_TOP) {
> +		netdev_err(ndev, "Only toeplitz hash function is supported\n");
> +		return -EOPNOTSUPP;

Should be a separate commit.
-- 
pw-bot: cr