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

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, Dec 16, 2020 at 09:09:58PM +0100, Tobias Waldekranz wrote:
> 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?

What I meant is that if you're just a casual reader carrying on with
your day and you see a function called dsa_switch_find, you have a vague
idea what it does just by reading the headline - it finds a DSA switch.
Whereas dsa_lag_map, I don't know, just doesn't speak to me, it's a very
uneventful name with a very uneventful implementation. If we hadn't had
the long discussion in the previous version about the linear array, I
would have honestly had to ask you what's the purpose of "mapping" and
"unmapping" a LAG. So I expect many of the readers ask themselves the
same thing, especially since the comments say that some drivers don't
use it.

I guess this would look more dynamic for me:

/* Helpers to add and remove a LAG net_device from this switch tree's
 * mapping towards a linear ID. These will do nothing for drivers which
 * don't populate ds->num_lag_ids, and therefore don't need the mapping.
 */
void dsa_lag_map(struct dsa_switch_tree *dst, struct net_device *lag)
{
	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. Calling dsa_lag_id will return an
	 * error when this device joins a 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;
		}
	}
}