Re: [RFC PATCH net-next 12/16] net: mscc: ocelot: drop the use of the "lags" array

[Vladimir Oltean <olteanv@gmail.com>]

On Tue, Dec 08, 2020 at 02:07:58PM +0200, Vladimir Oltean wrote:
> We can now simplify the implementation by always using ocelot_get_bond_mask
> to look up the other ports that are offloading the same bonding interface
> as us.
> 
> In ocelot_set_aggr_pgids, the code had a way to uniquely iterate through
> LAGs. We need to achieve the same behavior by marking each LAG as visited,
> which we do now by temporarily allocating an array of pointers to bonding
> uppers of each port, and marking each bonding upper as NULL once it has
> been treated by the first port that is a member. And because we now do
> some dynamic allocation, we need to propagate errors from
> ocelot_set_aggr_pgid all the way to ocelot_port_lag_leave.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c     | 104 ++++++++++---------------
>  drivers/net/ethernet/mscc/ocelot.h     |   4 +-
>  drivers/net/ethernet/mscc/ocelot_net.c |   4 +-
>  include/soc/mscc/ocelot.h              |   2 -
>  4 files changed, 47 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 1a98c24af056..d4dbba66aa65 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -909,21 +909,17 @@ static void ocelot_apply_bridge_fwd_mask(struct ocelot *ocelot)
>  	 * source port's forwarding mask.
>  	 */
>  	for (port = 0; port < ocelot->num_phys_ports; port++) {
> -		if (ocelot->bridge_fwd_mask & BIT(port)) {
> -			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> -			int lag;
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
>  
> -			for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
> -				unsigned long bond_mask = ocelot->lags[lag];
> +		if (!ocelot_port)
> +			continue;
>  
> -				if (!bond_mask)
> -					continue;
> +		if (ocelot->bridge_fwd_mask & BIT(port)) {
> +			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
> +			struct net_device *bond = ocelot_port->bond;
>  
> -				if (bond_mask & BIT(port)) {
> -					mask &= ~bond_mask;
> -					break;
> -				}
> -			}
> +			if (bond)
> +				mask &= ~ocelot_get_bond_mask(ocelot, bond);
>  
>  			ocelot_write_rix(ocelot, mask,
>  					 ANA_PGID_PGID, PGID_SRC + port);
> @@ -1238,10 +1234,16 @@ int ocelot_port_bridge_leave(struct ocelot *ocelot, int port,
>  }
>  EXPORT_SYMBOL(ocelot_port_bridge_leave);
>  
> -static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
> +static int ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  {
> +	struct net_device **bonds;
>  	int i, port, lag;
>  
> +	bonds = kcalloc(ocelot->num_phys_ports, sizeof(struct net_device *),
> +			GFP_KERNEL);
> +	if (!bonds)
> +		return -ENOMEM;
> +

I remember somebody complaining about the temporary memory allocation
done here, but I can't seem to find that email for some reason.

Is it ok if I still keep the dynamic allocation there, though? Felix has
up to 5 user ports, Seville has up to 9, Ocelot up to 11. I would like
to not hardcode anything, in case (who knows!) more switches get added.

>  	/* Reset destination and aggregation PGIDS */
>  	for_each_unicast_dest_pgid(ocelot, port)
>  		ocelot_write_rix(ocelot, BIT(port), ANA_PGID_PGID, port);
> @@ -1250,16 +1252,26 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  		ocelot_write_rix(ocelot, GENMASK(ocelot->num_phys_ports - 1, 0),
>  				 ANA_PGID_PGID, i);
>  
> +	for (port = 0; port < ocelot->num_phys_ports; port++) {
> +		struct ocelot_port *ocelot_port = ocelot->ports[port];
> +
> +		if (!ocelot_port)
> +			continue;
> +
> +		bonds[port] = ocelot_port->bond;
> +	}
> +
>  	/* Now, set PGIDs for each LAG */
>  	for (lag = 0; lag < ocelot->num_phys_ports; lag++) {
>  		unsigned long bond_mask;
>  		int aggr_count = 0;
>  		u8 aggr_idx[16];
>  
> -		bond_mask = ocelot->lags[lag];
> -		if (!bond_mask)
> +		if (!bonds[lag])
>  			continue;
>  
> +		bond_mask = ocelot_get_bond_mask(ocelot, bonds[lag]);
> +
>  		for_each_set_bit(port, &bond_mask, ocelot->num_phys_ports) {
>  			// Destination mask
>  			ocelot_write_rix(ocelot, bond_mask,
> @@ -1276,7 +1288,19 @@ static void ocelot_set_aggr_pgids(struct ocelot *ocelot)
>  			ac |= BIT(aggr_idx[i % aggr_count]);
>  			ocelot_write_rix(ocelot, ac, ANA_PGID_PGID, i);
>  		}
> +
> +		/* Mark the bonding interface as visited to avoid applying
> +		 * the same config again
> +		 */
> +		for (i = lag + 1; i < ocelot->num_phys_ports; i++)
> +			if (bonds[i] == bonds[lag])
> +				bonds[i] = NULL;
> +
> +		bonds[lag] = NULL;
>  	}
> +
> +	kfree(bonds);
> +	return 0;
>  }
>  
>  /* When offloading a bonding interface, the switch ports configured under the
> @@ -1315,59 +1339,22 @@ static void ocelot_setup_logical_port_ids(struct ocelot *ocelot)
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond)
>  {
> -	u32 bond_mask = 0;
> -	int lag;
> -
>  	ocelot->ports[port]->bond = bond;
>  
> -	bond_mask = ocelot_get_bond_mask(ocelot, bond);
> -
> -	lag = __ffs(bond_mask);
> -
> -	/* If the new port is the lowest one, use it as the logical port from
> -	 * now on
> -	 */
> -	if (port == lag) {
> -		ocelot->lags[port] = bond_mask;
> -		bond_mask &= ~BIT(port);
> -		if (bond_mask)
> -			ocelot->lags[__ffs(bond_mask)] = 0;
> -	} else {
> -		ocelot->lags[lag] |= BIT(port);
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> -
> -	return 0;
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_join);
>  
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond)
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond)
>  {
> -	int i;
> -
>  	ocelot->ports[port]->bond = NULL;
>  
> -	/* Remove port from any lag */
> -	for (i = 0; i < ocelot->num_phys_ports; i++)
> -		ocelot->lags[i] &= ~BIT(port);
> -
> -	/* if it was the logical port of the lag, move the lag config to the
> -	 * next port
> -	 */
> -	if (ocelot->lags[port]) {
> -		int n = __ffs(ocelot->lags[port]);
> -
> -		ocelot->lags[n] = ocelot->lags[port];
> -		ocelot->lags[port] = 0;
> -	}
> -
>  	ocelot_setup_logical_port_ids(ocelot);
>  	ocelot_apply_bridge_fwd_mask(ocelot);
> -	ocelot_set_aggr_pgids(ocelot);
> +	return ocelot_set_aggr_pgids(ocelot);
>  }
>  EXPORT_SYMBOL(ocelot_port_lag_leave);
>  
> @@ -1543,11 +1530,6 @@ int ocelot_init(struct ocelot *ocelot)
>  		}
>  	}
>  
> -	ocelot->lags = devm_kcalloc(ocelot->dev, ocelot->num_phys_ports,
> -				    sizeof(u32), GFP_KERNEL);
> -	if (!ocelot->lags)
> -		return -ENOMEM;
> -
>  	ocelot->stats = devm_kcalloc(ocelot->dev,
>  				     ocelot->num_phys_ports * ocelot->num_stats,
>  				     sizeof(u64), GFP_KERNEL);
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index 739bd201d951..bef8d5f8e6e5 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -114,8 +114,8 @@ int ocelot_mact_forget(struct ocelot *ocelot,
>  		       const unsigned char mac[ETH_ALEN], unsigned int vid);
>  int ocelot_port_lag_join(struct ocelot *ocelot, int port,
>  			 struct net_device *bond);
> -void ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> -			   struct net_device *bond);
> +int ocelot_port_lag_leave(struct ocelot *ocelot, int port,
> +			  struct net_device *bond);
>  struct net_device *ocelot_port_to_netdev(struct ocelot *ocelot, int port);
>  int ocelot_netdev_to_port(struct net_device *dev);
>  
> diff --git a/drivers/net/ethernet/mscc/ocelot_net.c b/drivers/net/ethernet/mscc/ocelot_net.c
> index 77957328722a..93aaa631e347 100644
> --- a/drivers/net/ethernet/mscc/ocelot_net.c
> +++ b/drivers/net/ethernet/mscc/ocelot_net.c
> @@ -1035,8 +1035,8 @@ static int ocelot_netdevice_changeupper(struct net_device *dev,
>  			err = ocelot_port_lag_join(ocelot, port,
>  						   info->upper_dev);
>  		else
> -			ocelot_port_lag_leave(ocelot, port,
> -					      info->upper_dev);
> +			err = ocelot_port_lag_leave(ocelot, port,
> +						    info->upper_dev);
>  	}
>  
>  	return notifier_from_errno(err);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index b812bdff1da1..0cd45659430f 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -639,8 +639,6 @@ struct ocelot {
>  	enum ocelot_tag_prefix		inj_prefix;
>  	enum ocelot_tag_prefix		xtr_prefix;
>  
> -	u32				*lags;
> -
>  	struct list_head		multicast;
>  	struct list_head		pgids;
>  
> -- 
> 2.25.1
>