diff mbox series

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

Message ID 20201208120802.1268708-13-vladimir.oltean@nxp.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series LAG offload for Ocelot DSA switches | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 4 this patch: 4
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: line length of 82 exceeds 80 columns
netdev/build_allmodconfig_warn fail Errors and warnings before: 4 this patch: 4
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Vladimir Oltean Dec. 8, 2020, 12:07 p.m. UTC
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(-)

Comments

Alexandre Belloni Dec. 16, 2020, 9:29 p.m. UTC | #1
On 08/12/2020 14:07:58+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.

I would say that this allocation never fails but wouldn't it be better
to simply put that on the stack? It is just a bunch of pointers, that
would be 10 on ocelot, maximum would be 64 on jaguar if we ever use the
same driver. This seems reasonable to me, the main issue is that you
have to avoid VLAs.

> 
> 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;
> +
>  	/* 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
>
Vladimir Oltean Jan. 15, 2021, 11:05 a.m. UTC | #2
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
>
Alexandre Belloni Jan. 15, 2021, 12:02 p.m. UTC | #3
On 15/01/2021 13:05:47+0200, Vladimir Oltean wrote:
> > -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.
> 

I was probably the one, the main reason being that this make this
function able to fail. Removing the dynamic allocation would ensure it
never fails. However, I didn't suggest any other solution so I'm fine if
you keep it.
diff mbox series

Patch

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;
+
 	/* 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;