diff mbox series

[RFC,v2,net-next,06/16] net: dsa: sync multicast router state when joining the bridge

Message ID 20210318231829.3892920-7-olteanv@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Better support for sandwiched LAGs with bridge and DSA | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit fail Errors and warnings before: 8 this patch: 8
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 22 lines checked
netdev/build_allmodconfig_warn fail Errors and warnings before: 8 this patch: 8
netdev/header_inline success Link

Commit Message

Vladimir Oltean March 18, 2021, 11:18 p.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

Make sure that the multicast router setting of the bridge is picked up
correctly by DSA when joining, regardless of whether there are
sandwiched interfaces or not. The SWITCHDEV_ATTR_ID_BRIDGE_MROUTER port
attribute is only emitted from br_mc_router_state_change.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/dsa/port.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Florian Fainelli March 19, 2021, 10:12 p.m. UTC | #1
On 3/18/2021 4:18 PM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> Make sure that the multicast router setting of the bridge is picked up
> correctly by DSA when joining, regardless of whether there are
> sandwiched interfaces or not. The SWITCHDEV_ATTR_ID_BRIDGE_MROUTER port
> attribute is only emitted from br_mc_router_state_change.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Tobias Waldekranz March 22, 2021, 11:17 a.m. UTC | #2
On Fri, Mar 19, 2021 at 01:18, Vladimir Oltean <olteanv@gmail.com> wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> Make sure that the multicast router setting of the bridge is picked up
> correctly by DSA when joining, regardless of whether there are
> sandwiched interfaces or not. The SWITCHDEV_ATTR_ID_BRIDGE_MROUTER port
> attribute is only emitted from br_mc_router_state_change.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  net/dsa/port.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/net/dsa/port.c b/net/dsa/port.c
> index ac1afe182c3b..8380509ee47c 100644
> --- a/net/dsa/port.c
> +++ b/net/dsa/port.c
> @@ -189,6 +189,10 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp,
>  	if (err && err != -EOPNOTSUPP)
>  		return err;
>  
> +	err = dsa_port_mrouter(dp->cpu_dp, br_multicast_router(br), extack);
> +	if (err && err != -EOPNOTSUPP)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -212,6 +216,12 @@ static void dsa_port_switchdev_unsync(struct dsa_port *dp)
>  	dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
>  
>  	/* VLAN filtering is handled by dsa_switch_bridge_leave */
> +
> +	/* Some drivers treat the notification for having a local multicast
> +	 * router by allowing multicast to be flooded to the CPU, so we should
> +	 * allow this in standalone mode too.
> +	 */
> +	dsa_port_mrouter(dp->cpu_dp, true, NULL);

Is this really for the DSA layer to decide? The driver has already been
notified that at least one port is now in standalone mode. So if that
particular driver then requires all multicast to be flooded towards the
CPU, it can make that decision on its own.

E.g. say that you implement standalone mode using a matchall TCAM rule
that maps all frames coming in on a particular port to the CPU. You
could still leave flooding of unknown multicast off in that case. Now
that driver has to figure out if the notification about a multicast
router on the CPU is a real router, or the DSA layer telling it
something that it can safely ignore.

Today I think that most (all?) DSA drivers treats mrouter in the same
way as the multicast flooding bridge flag. But AFAIK, the semantic
meaning of the setting is "flood IP multicast to this port because there
is a router behind it somewhere". This means unknown _IP_ multicast, but
also all known (IGMP/MLD) groups. As most smaller devices cannot
separate IP multicast from the non-IP variety, we flood everything. But
we should also make sure that the port in question receives all known
groups for the _bridge_ in question. Because this is really a bridge
setting, though that information is not carried over to the driver
today. So reusing it in this way feels like it could be problematic down
the road.

>  }
>  
>  int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,
> -- 
> 2.25.1
Vladimir Oltean March 22, 2021, 11:43 a.m. UTC | #3
On Mon, Mar 22, 2021 at 12:17:33PM +0100, Tobias Waldekranz wrote:
> On Fri, Mar 19, 2021 at 01:18, Vladimir Oltean <olteanv@gmail.com> wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > Make sure that the multicast router setting of the bridge is picked up
> > correctly by DSA when joining, regardless of whether there are
> > sandwiched interfaces or not. The SWITCHDEV_ATTR_ID_BRIDGE_MROUTER port
> > attribute is only emitted from br_mc_router_state_change.
> >
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  net/dsa/port.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/net/dsa/port.c b/net/dsa/port.c
> > index ac1afe182c3b..8380509ee47c 100644
> > --- a/net/dsa/port.c
> > +++ b/net/dsa/port.c
> > @@ -189,6 +189,10 @@ static int dsa_port_switchdev_sync(struct dsa_port *dp,
> >  	if (err && err != -EOPNOTSUPP)
> >  		return err;
> >  
> > +	err = dsa_port_mrouter(dp->cpu_dp, br_multicast_router(br), extack);
> > +	if (err && err != -EOPNOTSUPP)
> > +		return err;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -212,6 +216,12 @@ static void dsa_port_switchdev_unsync(struct dsa_port *dp)
> >  	dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
> >  
> >  	/* VLAN filtering is handled by dsa_switch_bridge_leave */
> > +
> > +	/* Some drivers treat the notification for having a local multicast
> > +	 * router by allowing multicast to be flooded to the CPU, so we should
> > +	 * allow this in standalone mode too.
> > +	 */
> > +	dsa_port_mrouter(dp->cpu_dp, true, NULL);
> 
> Is this really for the DSA layer to decide? The driver has already been
> notified that at least one port is now in standalone mode. So if that
> particular driver then requires all multicast to be flooded towards the
> CPU, it can make that decision on its own.
> 
> E.g. say that you implement standalone mode using a matchall TCAM rule
> that maps all frames coming in on a particular port to the CPU. You
> could still leave flooding of unknown multicast off in that case. Now
> that driver has to figure out if the notification about a multicast
> router on the CPU is a real router, or the DSA layer telling it
> something that it can safely ignore.
> 
> Today I think that most (all?) DSA drivers treats mrouter in the same
> way as the multicast flooding bridge flag. But AFAIK, the semantic
> meaning of the setting is "flood IP multicast to this port because there
> is a router behind it somewhere". This means unknown _IP_ multicast, but
> also all known (IGMP/MLD) groups. As most smaller devices cannot
> separate IP multicast from the non-IP variety, we flood everything. But
> we should also make sure that the port in question receives all known
> groups for the _bridge_ in question. Because this is really a bridge
> setting, though that information is not carried over to the driver
> today. So reusing it in this way feels like it could be problematic down
> the road.

I agree with your objections in principle, but somehow I would like to
make progress with this patch series which is not really about how we
deal with IP multicast flooding to the CPU port in standalone ports
mode, so I would like to not get bogged down too much into this for now.
Don't forget that up until recent commit a8b659e7ff75 ("net: dsa: act as
passthrough for bridge port flags"), DSA drivers had no real idea
whether multicast flooding was meant for IP or not. And in standalone
mode, the way things work now is that the CPU port should see all
traffic, so it isn't wrong to do what this patch does.
Unless you see a breaking change introduced by this patch, we can
revisit this discussion for the "RX filtering on DSA" series, where it
is more relevant.
diff mbox series

Patch

diff --git a/net/dsa/port.c b/net/dsa/port.c
index ac1afe182c3b..8380509ee47c 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -189,6 +189,10 @@  static int dsa_port_switchdev_sync(struct dsa_port *dp,
 	if (err && err != -EOPNOTSUPP)
 		return err;
 
+	err = dsa_port_mrouter(dp->cpu_dp, br_multicast_router(br), extack);
+	if (err && err != -EOPNOTSUPP)
+		return err;
+
 	return 0;
 }
 
@@ -212,6 +216,12 @@  static void dsa_port_switchdev_unsync(struct dsa_port *dp)
 	dsa_port_set_state_now(dp, BR_STATE_FORWARDING);
 
 	/* VLAN filtering is handled by dsa_switch_bridge_leave */
+
+	/* Some drivers treat the notification for having a local multicast
+	 * router by allowing multicast to be flooded to the CPU, so we should
+	 * allow this in standalone mode too.
+	 */
+	dsa_port_mrouter(dp->cpu_dp, true, NULL);
 }
 
 int dsa_port_bridge_join(struct dsa_port *dp, struct net_device *br,