Re: [PATCH net-next 0/4] net: dsa: b53: Clean up CPU/IMP ports

On Fri, Sep 17, 2021 at 02:21:16PM +0200, Andrew Lunn wrote:

quoted

quoted

That DSA_PORT_TYPE_UNUSED would probably require investigating DSA & b53
behaviour *and* discussing it with DSA maintainer to make sure we don't
abuse that.

How absent are these ports in hardware? For DSA_PORT_TYPE_UNUSED we do
register a devlink port, but if those ports are really not present in
hardware, I'm thinking maybe the easiest way would be to supply a
ds->disabled_port_mask before dsa_register_switch(), and DSA will simply
skip those ports when allocating the dp, the devlink_port etc. So you
will literally have nothing for them.

The basic idea seems O.K, we just need to be careful.

We have code like:

static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p)
{
	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_DSA;
}

static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
{
	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_USER;
}

dsa_to_port(ds, p) will return NULL, and then bad things will happen.

Maybe it would be safer to add DSA_PORT_TYPE_PHANTOM and do allocate
the dp?

I think DSA is not yet large enough for us to take the defeatist
position of inserting bits of code that simply protect from other bits
of code.

Especially after these patches:
https://patchwork.kernel.org/project/netdevbpf/cover/20210810161448.1879192-1-vladimir.oltean@nxp.com/
the DSA core should basically never iterate over ports using
	for (port = 0; port < ds->num_ports; port++)
but instead using
	dsa_switch_for_each_port(dp, ds)
which basically ensures that we never dereference dsa_to_port(ds, p) for
a p with no associated dp (the "dp" from "dsa_switch_for_each_port" is
taken from a list).

Conversely, driver code is "mostly" event-driven, so DSA should only call
dsa_switch_ops methods for ports where the dp structure does exist.
What we would need to audit are just the drivers which set ds->disabled_port_mask
(b53, maybe ksz switches?). I guess that would be on the developer who
makes the change in those respective drivers to ensure that the "dp"
which is searched for does in fact exist.

My 2 cents anyway, I might be missing some trickier cases.