Re: [PATCH net-next v3 5/7] net: lan966x: Add lag support for lan966x.

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Jul 01, 2022 at 10:52:25PM +0200, Horatiu Vultur wrote:
> Add link aggregation hardware offload support for lan966x
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---

Superficially the code looks OK (maybe if you could only remove the
trailing "." from the commit message). A comment below.

> +int lan966x_lag_port_prechangeupper(struct net_device *dev,
> +				    struct netdev_notifier_changeupper_info *info)
> +{
> +	struct lan966x_port *port = netdev_priv(dev);
> +	struct lan966x *lan966x = port->lan966x;
> +	struct netdev_lag_upper_info *lui;
> +	struct netlink_ext_ack *extack;
> +
> +	extack = netdev_notifier_info_to_extack(&info->info);
> +	lui = info->upper_info;
> +	if (!lui)
> +		return NOTIFY_DONE;
> +
> +	if (lui->tx_type != NETDEV_LAG_TX_TYPE_HASH) {
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported Tx type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	switch (lui->hash_type) {
> +	case NETDEV_LAG_HASH_L2:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L34:
> +		lan_wr(ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_SIPDIP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);
> +		break;
> +	case NETDEV_LAG_HASH_L23:
> +		lan_wr(ANA_AGGR_CFG_AC_DMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_SMAC_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP6_TCPUDP_ENA_SET(1) |
> +		       ANA_AGGR_CFG_AC_IP4_TCPUDP_ENA_SET(1),
> +		       lan966x, ANA_AGGR_CFG);

ANA_AGGR_CFG is global to the switch, yet it constantly changes with the
hash_type of each new offloaded LAG. So a LAG that hashes on L3/L4 will
break the hash mode of an existing L2 LAG. I think you should implement
a system that disallows changes to ANA_AGGR_CFG as long as there are
users of a certain mode.

> +		break;
> +	default:
> +		NL_SET_ERR_MSG_MOD(extack,
> +				   "LAG device using unsupported hash type");
> +		return notifier_from_errno(-EINVAL);
> +	}
> +
> +	return NOTIFY_OK;
> +}