On Sat, Feb 04, 2023 at 09:48:24AM +0100, netdev@kapio-technology.com wrote:

On 2023-02-04 09:12, Simon Horman wrote:

quoted

On Fri, Feb 03, 2023 at 10:44:22PM +0200, Vladimir Oltean wrote:

quoted

On Fri, Feb 03, 2023 at 09:20:22AM +0100, Simon Horman wrote:

quoted

quoted

else if (someflag)
        dosomething();

For now only one flag will actually be set and they are mutually exclusive,
as they will not make sense together with the potential flags I know, but
that can change at some time of course.

Yes, I see that is workable. I do feel that checking for other flags would
be a bit more robust. But as you say, there are none. So whichever
approach you prefer is fine by me.

The model we have for unsupported bits in the
SWITCHDEV_ATTR_ID_PORT_PRE_BRIDGE_FLAGS
and SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS handlers is essentially this:

	if (flags & ~(supported_flag_mask))
		return -EOPNOTSUPP;

	if (flags & supported_flag_1)
		...

	if (flags & supported_flag_2)
		...

I suppose applying this model here would address Simon's
extensibility concern.

Yes, that is the model I had in mind.

The only thing is that we actually need to return both 0 and -EOPNOTSUPP for
unsupported flags. The dynamic flag requires 0 when not supported (and
supported) AFAICS.
Setting a mask as 'supported' for a feature that is not really supported
defeats the notion of 'supported' IMHO.

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.

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.

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).