Re: [PATCH v4 net-next 3/5] net: dsa: Link aggregation support

[Tobias Waldekranz <tobias@waldekranz.com>]

On Wed, Dec 16, 2020 at 20:44, Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Dec 16, 2020 at 05:00:54PM +0100, Tobias Waldekranz wrote:
>> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
>> index 183003e45762..deee4c0ecb31 100644
>> --- a/net/dsa/dsa2.c
>> +++ b/net/dsa/dsa2.c
>> @@ -21,6 +21,46 @@
>>  static DEFINE_MUTEX(dsa2_mutex);
>>  LIST_HEAD(dsa_tree_list);
>>
>> +void dsa_lag_map(struct dsa_switch_tree *dst, struct net_device *lag)
>
> Maybe a small comment here and in dsa_lag_unmap, describing what they're
> for? They look a bit bland. Just a few words about the linear array will
> suffice.

Not sure I understand why these two are "bland" whereas dsa_switch_find
just below it is not. But sure, I will add a comment. You want a block
comment before each function?

>> +{
>> +	unsigned int id;
>> +
>> +	if (dsa_lag_id(dst, lag) >= 0)
>> +		/* Already mapped */
>> +		return;
>> +
>> +	for (id = 0; id < dst->lags_len; id++) {
>> +		if (!dsa_lag_dev(dst, id)) {
>> +			dst->lags[id] = lag;
>> +			return;
>> +		}
>> +	}
>> +
>> +	/* No IDs left, which is OK. Some drivers do not need it. The
>> +	 * ones that do, e.g. mv88e6xxx, will discover that
>> +	 * dsa_tree_lag_id returns an error for this device when
>> +	 * joining the LAG. The driver can then return -EOPNOTSUPP
>> +	 * back to DSA, which will fall back to a software LAG.
>> +	 */
>> +}
>> +
>> +void dsa_lag_unmap(struct dsa_switch_tree *dst, struct net_device *lag)
>> +{
>> +	struct dsa_port *dp;
>> +	unsigned int id;
>> +
>> +	dsa_lag_foreach_port(dp, dst, lag)
>> +		/* There are remaining users of this mapping */
>> +		return;
>> +
>> +	dsa_lags_foreach_id(id, dst) {
>> +		if (dsa_lag_dev(dst, id) == lag) {
>> +			dst->lags[id] = NULL;
>> +			break;
>> +		}
>> +	}
>> +}
>> diff --git a/net/dsa/port.c b/net/dsa/port.c
>> index 73569c9af3cc..121e5044dbe7 100644
>> --- a/net/dsa/port.c
>> +++ b/net/dsa/port.c
>> @@ -193,6 +193,85 @@ void dsa_port_bridge_leave(struct dsa_port *dp, struct net_device *br)
>>  	dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
>>  }
>>
>> +int dsa_port_lag_change(struct dsa_port *dp,
>> +			struct netdev_lag_lower_state_info *linfo)
>> +{
>> +	struct dsa_notifier_lag_info info = {
>> +		.sw_index = dp->ds->index,
>> +		.port = dp->index,
>> +	};
>> +	bool tx_enabled;
>> +
>> +	if (!dp->lag_dev)
>> +		return 0;
>> +
>> +	/* On statically configured aggregates (e.g. loadbalance
>> +	 * without LACP) ports will always be tx_enabled, even if the
>> +	 * link is down. Thus we require both link_up and tx_enabled
>> +	 * in order to include it in the tx set.
>> +	 */
>> +	tx_enabled = linfo->link_up && linfo->tx_enabled;
>> +
>> +	if (tx_enabled == dp->lag_tx_enabled)
>> +		return 0;
>
> Why would we get a NETDEV_CHANGELOWERSTATE notification if tx_enabled ==
> dp->lag_tx_enabled? What is it that changed?

A typical scenario would be:

1. Link goes down: linfo->link_up=false linfo->tx_enabled=false
   => tx_enabled=false

2. Link comes up: linfo->link_up=true linfo->tx_enabled=false
   => tx_enabled=false

3. LACP peers: linfo->link_up=true linfo->tx_enabled=true
   => tx_enabled=true

We get three events, but we only go to the hardware for (1) and (3).

>> +
>> +	dp->lag_tx_enabled = tx_enabled;
>> +
>> +	return dsa_port_notify(dp, DSA_NOTIFIER_LAG_CHANGE, &info);
>> +}
>
> I am very happy with how simple this turned out. Thanks for the patience.
> You can add these tags when you resend once net-next opens.
>
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> Tested-by: Vladimir Oltean <olteanv@gmail.com>

Thank you. Yeah I also like the way it ended up.