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

Vladimir Oltean
2022-07-02T14:12:28Z

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;
> +}