On Wed, Dec 16, 2020 at 09:09:58PM +0100, Tobias Waldekranz wrote:
> On Wed, Dec 16, 2020 at 20:44, Vladimir Oltean <olte...@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;
}
}
}
