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

[Wei Fang]

> 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.

Okay, I will.

> 
> > +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?
Yes.

> Why create full ops for something this trivial?

We add enetc_pf_hw_ops to implement different hardware ops
for different chips. So that they can be called in common functions.
Although the change is minor, it is consistent with the original
intention of adding enetc_pf_hw_ops.

> 
> > +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?

We will add ETHTOOL_G/SRXCLSXXX in the future, but both the hardware and
software implementation of ENETC4 are different from ENETC1, and we don't
want to mix them in one function, which would look a bit messy.

> 
> > @@ -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