Re: [PATCH RESEND net-next v4 2/3] net: dsa: rzn1-a5psw: add support for .port_bridge_flags

["Clément Léger" <clement.leger@bootlin.com>]

Le Wed, 15 Mar 2023 01:08:21 +0200,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Tue, Mar 14, 2023 at 05:36:50PM +0100, Clément Léger wrote:
> > +static int a5psw_port_pre_bridge_flags(struct dsa_switch *ds, int port,
> > +				       struct switchdev_brport_flags flags,
> > +				       struct netlink_ext_ack *extack)
> > +{
> > +	if (flags.mask & ~(BR_LEARNING | BR_FLOOD | BR_MCAST_FLOOD |
> > +			   BR_BCAST_FLOOD))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +a5psw_port_bridge_flags(struct dsa_switch *ds, int port,
> > +			struct switchdev_brport_flags flags,
> > +			struct netlink_ext_ack *extack)
> > +{
> > +	struct a5psw *a5psw = ds->priv;
> > +	u32 val;
> > +
> > +	if (flags.mask & BR_LEARNING) {
> > +		val = flags.val & BR_LEARNING ? 0 : A5PSW_INPUT_LEARN_DIS(port);
> > +		a5psw_reg_rmw(a5psw, A5PSW_INPUT_LEARN,
> > +			      A5PSW_INPUT_LEARN_DIS(port), val);
> > +	}  
> 
> 2 issues.
> 
> 1: does this not get overwritten by a5psw_port_stp_state_set()?

Hum indeed. How is this kind of thing supposed to be handled ? Should I
remove the handling of BR_LEARNING to forbid modifying it ? Ot should I
allow it only if STP isn't enabled (which I'm not sure how to do it) ?

> 2: What is the hardware default value for A5PSW_INPUT_LEARN? Please make
>    sure that standalone ports have learning disabled by default, when
>    the driver probes.
> 
> > +
> > +	if (flags.mask & BR_FLOOD) {
> > +		val = flags.val & BR_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_UCAST_DEF_MASK, BIT(port), val);
> > +	}
> > +
> > +	if (flags.mask & BR_MCAST_FLOOD) {
> > +		val = flags.val & BR_MCAST_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_MCAST_DEF_MASK, BIT(port), val);
> > +	}
> > +
> > +	if (flags.mask & BR_BCAST_FLOOD) {
> > +		val = flags.val & BR_BCAST_FLOOD ? BIT(port) : 0;
> > +		a5psw_reg_rmw(a5psw, A5PSW_BCAST_DEF_MASK, BIT(port), val);
> > +	}  
> 
> Humm, there's a (huge) problem with this flooding mask.
> 
> a5psw_flooding_set_resolution() - called from a5psw_port_bridge_join()
> and a5psw_port_bridge_leave() - touches the same registers as
> a5psw_port_bridge_flags(). Which means that your bridge forwarding
> domain controls are the same as your flooding controls.
> 
> Which is bad news, because
> dsa_port_bridge_leave()
> -> dsa_port_switchdev_unsync_attrs()
>    -> dsa_port_clear_brport_flags()
>       -> dsa_port_bridge_flags()
>          -> a5psw_port_bridge_flags()  
> 
> enables flooding on the port after calling a5psw_port_bridge_leave().
> So the port which has left a bridge is standalone, but it still forwards
> packets to the other bridged ports!

Actually not this way because the port is configured in a specific mode
which only forward packet to the CPU ports. Indeed, we set a specific
rule using the PATTERN_CTRL register with the MGMTFWD bit set:
When set, the frame is forwarded to the management port only
(suppressing destination address lookup).

However, the port will received packets *from* the other ports (which is
wrong... I can handle that by not setting the flooding attributes if
the port is not in bridge. Doing so would definitely fix the various
problems that could happen.

BTW, the same goes with the learning bit that would be reenabled after
leaving the bridge and you mentionned it should be disabled for a
standalone port.

> 
> You should be able to see that this is the case, if you put the ports
> under a dummy bridge, then run tools/testing/selftests/drivers/net/dsa/no_forwarding.sh.

Yes, makes sense.

Thanks,

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com