| Message ID | 20201028074221.29326-3-kurt@linutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | Hirschmann Hellcreek DSA driver | expand |
On Wed, Oct 28, 2020 at 08:42:15AM +0100, Kurt Kanzenbach wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Some switches rely on unique pvids to ensure port separation in > standalone mode, because they don't have a port forwarding matrix > configurable in hardware. So, setups like a group of 2 uppers with the > same VLAN, swp0.100 and swp1.100, will cause traffic tagged with VLAN > 100 to be autonomously forwarded between these switch ports, in spite > of there being no bridge between swp0 and swp1. > > These drivers need to prevent this from happening. They need to have > VLAN filtering enabled in standalone mode (so they'll drop frames tagged > with unknown VLANs) and they can only accept an 8021q upper on a port as > long as it isn't installed on any other port too. So give them the > chance to veto bad user requests. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- In case reviewers have doubts about this new DSA operation in general. I would expect that when LAG support is merged, some drivers will support it, but not any tx_type, but e.g. just NETDEV_LAG_TX_TYPE_HASH. So it would also be helpful in that case, so they could veto other types of bond interfaces cleanly. So I do see the need for a generic "prechangeupper" operation given to drivers.
On Wed, Oct 28, 2020 at 12:43:44PM +0200, Vladimir Oltean wrote: > On Wed, Oct 28, 2020 at 08:42:15AM +0100, Kurt Kanzenbach wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Some switches rely on unique pvids to ensure port separation in > > standalone mode, because they don't have a port forwarding matrix > > configurable in hardware. So, setups like a group of 2 uppers with the > > same VLAN, swp0.100 and swp1.100, will cause traffic tagged with VLAN > > 100 to be autonomously forwarded between these switch ports, in spite > > of there being no bridge between swp0 and swp1. > > > > These drivers need to prevent this from happening. They need to have > > VLAN filtering enabled in standalone mode (so they'll drop frames tagged > > with unknown VLANs) and they can only accept an 8021q upper on a port as > > long as it isn't installed on any other port too. So give them the > > chance to veto bad user requests. > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > > --- > > In case reviewers have doubts about this new DSA operation in general. > I would expect that when LAG support is merged, some drivers will > support it, but not any tx_type, but e.g. just NETDEV_LAG_TX_TYPE_HASH. > So it would also be helpful in that case, so they could veto other types > of bond interfaces cleanly. So I do see the need for a generic > "prechangeupper" operation given to drivers. There is always the interesting question, do we want to veto, or simply not accelerate it? We will want to consider that case by case. Andrew
On 10/28/2020 12:42 AM, Kurt Kanzenbach wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Some switches rely on unique pvids to ensure port separation in > standalone mode, because they don't have a port forwarding matrix > configurable in hardware. So, setups like a group of 2 uppers with the > same VLAN, swp0.100 and swp1.100, will cause traffic tagged with VLAN > 100 to be autonomously forwarded between these switch ports, in spite > of there being no bridge between swp0 and swp1. > > These drivers need to prevent this from happening. They need to have > VLAN filtering enabled in standalone mode (so they'll drop frames tagged > with unknown VLANs) and they can only accept an 8021q upper on a port as > long as it isn't installed on any other port too. So give them the > chance to veto bad user requests. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > include/net/dsa.h | 6 ++++++ > net/dsa/slave.c | 12 ++++++++++++ > 2 files changed, 18 insertions(+) > > diff --git a/include/net/dsa.h b/include/net/dsa.h > index 04e93bafb7bd..4e60d2610f20 100644 > --- a/include/net/dsa.h > +++ b/include/net/dsa.h > @@ -536,6 +536,12 @@ struct dsa_switch_ops { > void (*get_regs)(struct dsa_switch *ds, int port, > struct ethtool_regs *regs, void *p); > > + /* > + * Upper device tracking. > + */ > + int (*port_prechangeupper)(struct dsa_switch *ds, int port, > + struct netdev_notifier_changeupper_info *info); > + > /* > * Bridge integration > */ > diff --git a/net/dsa/slave.c b/net/dsa/slave.c > index 3bc5ca40c9fb..1919a025c06f 100644 > --- a/net/dsa/slave.c > +++ b/net/dsa/slave.c > @@ -1987,10 +1987,22 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, > switch (event) { > case NETDEV_PRECHANGEUPPER: { > struct netdev_notifier_changeupper_info *info = ptr; > + struct dsa_switch *ds; > + struct dsa_port *dp; > + int err; > > if (!dsa_slave_dev_check(dev)) > return dsa_prevent_bridging_8021q_upper(dev, ptr); > > + dp = dsa_slave_to_port(dev); > + ds = dp->ds; > + > + if (ds->ops->port_prechangeupper) { > + err = ds->ops->port_prechangeupper(ds, dp->index, ptr); I would pass 'info' instead of 'ptr' here even if there is no functional difference, this would be clearer. Not a reason to resubmit if everything else is fine in this series: Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> -- Florian
diff --git a/include/net/dsa.h b/include/net/dsa.h index 04e93bafb7bd..4e60d2610f20 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -536,6 +536,12 @@ struct dsa_switch_ops { void (*get_regs)(struct dsa_switch *ds, int port, struct ethtool_regs *regs, void *p); + /* + * Upper device tracking. + */ + int (*port_prechangeupper)(struct dsa_switch *ds, int port, + struct netdev_notifier_changeupper_info *info); + /* * Bridge integration */ diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 3bc5ca40c9fb..1919a025c06f 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1987,10 +1987,22 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb, switch (event) { case NETDEV_PRECHANGEUPPER: { struct netdev_notifier_changeupper_info *info = ptr; + struct dsa_switch *ds; + struct dsa_port *dp; + int err; if (!dsa_slave_dev_check(dev)) return dsa_prevent_bridging_8021q_upper(dev, ptr); + dp = dsa_slave_to_port(dev); + ds = dp->ds; + + if (ds->ops->port_prechangeupper) { + err = ds->ops->port_prechangeupper(ds, dp->index, ptr); + if (err) + return notifier_from_errno(err); + } + if (is_vlan_dev(info->upper_dev)) return dsa_slave_check_8021q_upper(dev, ptr); break;