Linux Ethernet Bridging - Feb 2023 - [Bridge] [PATCH net-next 5/5] net: dsa: mv88e6xxx: implementation of dynamic ATU entries

On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman at corigine.com>
wrote:
>
> Just to clarify my suggestion one last time, it would be along the lines
> of the following (completely untested!). I feel that it robustly covers
> all cases for fdb_flags. And as a bonus doesn't need to be modified
> if other (unsupported) flags are added in future.
>
> 	if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> 		return -EOPNOTSUPP;
>
> 	is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC)
> 	if (is_dynamic)
> 		state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
>
>
> And perhaps for other drivers:
>
> 	if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> 		return -EOPNOTSUPP;
> 	if (fdb_flags)
> 		return 0;
>
> Perhaps a helper would be warranted for the above.
How would such a helper look? Inline function is not clean.
>
> But in writing this I think that, perhaps drivers could return -EOPNOTSUPP
> for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather tha
> propagate, -EOPNOTSUPP.
I looked at that, but changing the caller is also a bit ugly.
>
> Returning -EOPNOTSUPP is the normal way to drivers to respond to requests
> for unsupported hardware offloads. Sticking to that may be clearner
> in the long run. That said, I do agree your current patch is correct
> given the flag that is defined (by your patchset).

On Tue, Feb 14, 2023 at 10:14:55PM +0100, Hans Schultz
wrote:
> On Mon, Feb 06, 2023 at 17:02, Simon Horman <simon.horman at
corigine.com> wrote:
> >
> > Just to clarify my suggestion one last time, it would be along the
lines
> > of the following (completely untested!). I feel that it robustly
covers
> > all cases for fdb_flags. And as a bonus doesn't need to be
modified
> > if other (unsupported) flags are added in future.
> >
> > 	if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > 		return -EOPNOTSUPP;
> >
> > 	is_dynamic = !!(fdb_flags & DSA_FDB_FLAG_DYNAMIC)
> > 	if (is_dynamic)
> > 		state = MV88E6XXX_G1_ATU_DATA_STATE_UC_AGE_7_NEWEST;
> >
> >
> > And perhaps for other drivers:
> >
> > 	if (fdb_flags & ~(DSA_FDB_FLAG_DYNAMIC))
> > 		return -EOPNOTSUPP;
> > 	if (fdb_flags)
> > 		return 0;
> >
> > Perhaps a helper would be warranted for the above.
> 
> How would such a helper look? Inline function is not clean.
> 
> >
> > But in writing this I think that, perhaps drivers could return
-EOPNOTSUPP
> > for the DSA_FDB_FLAG_DYNAMIC case and the caller can handle, rather
tha
> > propagate, -EOPNOTSUPP.
> 
> I looked at that, but changing the caller is also a bit ugly.
Answering on behalf of Simon, and hoping he will agree.

You are missing a big opportunity to make the kernel avoid doing useless work.
The dsa_slave_fdb_event() function runs in atomic switchdev notifier context,
and schedules a deferred workqueue item - dsa_schedule_work() - to get sleepable
context to program hardware.

Only that scheduling a deferred work item is not exactly cheap, so we try to
avoid doing that unless we know that we'll end up doing something with that
FDB entry once the deferred work does get scheduled:

	/* Check early that we're not doing work in vain.
	 * Host addresses on LAG ports still require regular FDB ops,
	 * since the CPU port isn't in a LAG.
	 */
	if (dp->lag && !host_addr) {
		if (!ds->ops->lag_fdb_add || !ds->ops->lag_fdb_del)
			return -EOPNOTSUPP;
	} else {
		if (!ds->ops->port_fdb_add || !ds->ops->port_fdb_del)
			return -EOPNOTSUPP;
	}

What you should be doing is you should be using the pahole tool to find
a good place for a new unsigned long field in struct dsa_switch, and add
a new field ds->supported_fdb_flags. You should extend the early checking
from dsa_slave_fdb_event() and exit without doing anything if the
(fdb->flags & ~ds->supported_fdb_flags) expression is non-zero.

This way you would kill 2 birds with 1 stone, since individual drivers
would no longer need to check the flags; DSA would guarantee not calling
them with unsupported flags.

It would be also very good to reach an agreement with switchdev
maintainers regarding the naming of the is_static/is_dyn field.

It would also be excellent if you could rename "fdb_flags" to just
"flags" within DSA.