| Message ID | 20200513153717.15599-1-dqfext@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: dsa: mt7530: set CPU port to fallback mode | expand |
On 5/13/2020 8:37 AM, DENG Qingfang wrote: > Currently, setting a bridge's self PVID to other value and deleting > the default VID 1 renders untagged ports of that VLAN unable to talk to > the CPU port: > > bridge vlan add dev br0 vid 2 pvid untagged self > bridge vlan del dev br0 vid 1 self > bridge vlan add dev sw0p0 vid 2 pvid untagged > bridge vlan del dev sw0p0 vid 1 > # br0 cannot send untagged frames out of sw0p0 anymore > > That is because the CPU port is set to security mode and its PVID is > still 1, and untagged frames are dropped due to VLAN member violation. > > Set the CPU port to fallback mode so untagged frames can pass through. How about if the bridge has vlan_filtering=1? The use case you present seems to be valid to me, that is, you may create a VLAN just for the user ports and not have the CPU port be part of it at all. > > Fixes: 83163f7dca56 ("net: dsa: mediatek: add VLAN support for MT7530") > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > drivers/net/dsa/mt7530.c | 11 ++++++++--- > drivers/net/dsa/mt7530.h | 6 ++++++ > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > index 5c444cd722bd..a063d914c23f 100644 > --- a/drivers/net/dsa/mt7530.c > +++ b/drivers/net/dsa/mt7530.c > @@ -810,10 +810,15 @@ mt7530_port_set_vlan_aware(struct dsa_switch *ds, int port) > PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS)); > > /* Trapped into security mode allows packet forwarding through VLAN > - * table lookup. > + * table lookup. CPU port is set to fallback mode to let untagged > + * frames pass through. > */ > - mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > - MT7530_PORT_SECURITY_MODE); > + if (dsa_is_cpu_port(ds, port)) > + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > + MT7530_PORT_FALLBACK_MODE); > + else > + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > + MT7530_PORT_SECURITY_MODE); > > /* Set the port as a user port which is to be able to recognize VID > * from incoming packets before fetching entry within the VLAN table. > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > index 979bb6374678..d45eb7540703 100644 > --- a/drivers/net/dsa/mt7530.h > +++ b/drivers/net/dsa/mt7530.h > @@ -152,6 +152,12 @@ enum mt7530_port_mode { > /* Port Matrix Mode: Frames are forwarded by the PCR_MATRIX members. */ > MT7530_PORT_MATRIX_MODE = PORT_VLAN(0), > > + /* Fallback Mode: Forward received frames with ingress ports that do > + * not belong to the VLAN member. Frames whose VID is not listed on > + * the VLAN table are forwarded by the PCR_MATRIX members. > + */ > + MT7530_PORT_FALLBACK_MODE = PORT_VLAN(1), > + > /* Security Mode: Discard any frame due to ingress membership > * violation or VID missed on the VLAN table. > */ >
Hi Florian On Wed, May 13, 2020 at 11:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 5/13/2020 8:37 AM, DENG Qingfang wrote: > > Currently, setting a bridge's self PVID to other value and deleting > > the default VID 1 renders untagged ports of that VLAN unable to talk to > > the CPU port: > > > > bridge vlan add dev br0 vid 2 pvid untagged self > > bridge vlan del dev br0 vid 1 self > > bridge vlan add dev sw0p0 vid 2 pvid untagged > > bridge vlan del dev sw0p0 vid 1 > > # br0 cannot send untagged frames out of sw0p0 anymore > > > > That is because the CPU port is set to security mode and its PVID is > > still 1, and untagged frames are dropped due to VLAN member violation. > > > > Set the CPU port to fallback mode so untagged frames can pass through. > > How about if the bridge has vlan_filtering=1? The use case you present > seems to be valid to me, that is, you may create a VLAN just for the > user ports and not have the CPU port be part of it at all. I forgot to mention that this is ONLY for vlan_filtering=1 `bridge vlan` simply won't do anything if VLAN filtering is disabled. > > > > > Fixes: 83163f7dca56 ("net: dsa: mediatek: add VLAN support for MT7530") > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > > --- > > drivers/net/dsa/mt7530.c | 11 ++++++++--- > > drivers/net/dsa/mt7530.h | 6 ++++++ > > 2 files changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c > > index 5c444cd722bd..a063d914c23f 100644 > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -810,10 +810,15 @@ mt7530_port_set_vlan_aware(struct dsa_switch *ds, int port) > > PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS)); > > > > /* Trapped into security mode allows packet forwarding through VLAN > > - * table lookup. > > + * table lookup. CPU port is set to fallback mode to let untagged > > + * frames pass through. > > */ > > - mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > > - MT7530_PORT_SECURITY_MODE); > > + if (dsa_is_cpu_port(ds, port)) > > + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > > + MT7530_PORT_FALLBACK_MODE); > > + else > > + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, > > + MT7530_PORT_SECURITY_MODE); > > > > /* Set the port as a user port which is to be able to recognize VID > > * from incoming packets before fetching entry within the VLAN table. > > diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h > > index 979bb6374678..d45eb7540703 100644 > > --- a/drivers/net/dsa/mt7530.h > > +++ b/drivers/net/dsa/mt7530.h > > @@ -152,6 +152,12 @@ enum mt7530_port_mode { > > /* Port Matrix Mode: Frames are forwarded by the PCR_MATRIX members. */ > > MT7530_PORT_MATRIX_MODE = PORT_VLAN(0), > > > > + /* Fallback Mode: Forward received frames with ingress ports that do > > + * not belong to the VLAN member. Frames whose VID is not listed on > > + * the VLAN table are forwarded by the PCR_MATRIX members. > > + */ > > + MT7530_PORT_FALLBACK_MODE = PORT_VLAN(1), > > + > > /* Security Mode: Discard any frame due to ingress membership > > * violation or VID missed on the VLAN table. > > */ > > > > -- > Florian
On 5/13/2020 8:56 AM, DENG Qingfang wrote: > Hi Florian > > On Wed, May 13, 2020 at 11:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 5/13/2020 8:37 AM, DENG Qingfang wrote: >>> Currently, setting a bridge's self PVID to other value and deleting >>> the default VID 1 renders untagged ports of that VLAN unable to talk to >>> the CPU port: >>> >>> bridge vlan add dev br0 vid 2 pvid untagged self >>> bridge vlan del dev br0 vid 1 self >>> bridge vlan add dev sw0p0 vid 2 pvid untagged >>> bridge vlan del dev sw0p0 vid 1 >>> # br0 cannot send untagged frames out of sw0p0 anymore >>> >>> That is because the CPU port is set to security mode and its PVID is >>> still 1, and untagged frames are dropped due to VLAN member violation. >>> >>> Set the CPU port to fallback mode so untagged frames can pass through. >> >> How about if the bridge has vlan_filtering=1? The use case you present >> seems to be valid to me, that is, you may create a VLAN just for the >> user ports and not have the CPU port be part of it at all. > > I forgot to mention that this is ONLY for vlan_filtering=1 > `bridge vlan` simply won't do anything if VLAN filtering is disabled. It depends now as of this patch: https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=54a0ed0df49609f4e3f098f8943e38e389dc2e15 and sorry I misunderstood your use case, you are changing the default VLAN for the CPU port through the bridge's master device and you still want it to be in the same VLAN membership as sw0p0, so yes that looks correct.
On Wed, 13 May 2020 at 18:49, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > On 5/13/2020 8:37 AM, DENG Qingfang wrote: > > Currently, setting a bridge's self PVID to other value and deleting > > the default VID 1 renders untagged ports of that VLAN unable to talk to > > the CPU port: > > > > bridge vlan add dev br0 vid 2 pvid untagged self > > bridge vlan del dev br0 vid 1 self > > bridge vlan add dev sw0p0 vid 2 pvid untagged > > bridge vlan del dev sw0p0 vid 1 > > # br0 cannot send untagged frames out of sw0p0 anymore > > > > That is because the CPU port is set to security mode and its PVID is > > still 1, and untagged frames are dropped due to VLAN member violation. > > > > Set the CPU port to fallback mode so untagged frames can pass through. > > How about if the bridge has vlan_filtering=1? The use case you present > seems to be valid to me, that is, you may create a VLAN just for the > user ports and not have the CPU port be part of it at all. > What Qingfang is doing is in effect (but not by intention) removing the front panel port sw0p0 from the membership list of the CPU port's pvid. What you seem to be thinking of (VLAN of which the CPU is not a member of) does not seem to be supported in DSA at the moment. As a fix, there's nothing wrong with the patch actually, I don't even know how it would work otherwise. DSA doesn't change the pvid of the CPU port when the pvid of a slave changes, because 4 slave ports could have 4 different pvids and the CPU port pvid would keep changing. Fallback mode should only apply on ingress from CPU, so there's no danger really. Thanks, -Vladimir
On 5/13/2020 9:17 AM, Vladimir Oltean wrote: > On Wed, 13 May 2020 at 18:49, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> >> >> On 5/13/2020 8:37 AM, DENG Qingfang wrote: >>> Currently, setting a bridge's self PVID to other value and deleting >>> the default VID 1 renders untagged ports of that VLAN unable to talk to >>> the CPU port: >>> >>> bridge vlan add dev br0 vid 2 pvid untagged self >>> bridge vlan del dev br0 vid 1 self >>> bridge vlan add dev sw0p0 vid 2 pvid untagged >>> bridge vlan del dev sw0p0 vid 1 >>> # br0 cannot send untagged frames out of sw0p0 anymore >>> >>> That is because the CPU port is set to security mode and its PVID is >>> still 1, and untagged frames are dropped due to VLAN member violation. >>> >>> Set the CPU port to fallback mode so untagged frames can pass through. >> >> How about if the bridge has vlan_filtering=1? The use case you present >> seems to be valid to me, that is, you may create a VLAN just for the >> user ports and not have the CPU port be part of it at all. >> > > What Qingfang is doing is in effect (but not by intention) removing > the front panel port sw0p0 from the membership list of the CPU port's > pvid. What you seem to be thinking of (VLAN of which the CPU is not a > member of) does not seem to be supported in DSA at the moment. Indeed and I replied to Qingfang already that I had misunderstood his patch originally. > > As a fix, there's nothing wrong with the patch actually, I don't even > know how it would work otherwise. DSA doesn't change the pvid of the > CPU port when the pvid of a slave changes, because 4 slave ports could > have 4 different pvids and the CPU port pvid would keep changing. > Fallback mode should only apply on ingress from CPU, so there's no > danger really. Agreed. Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
From: DENG Qingfang <dqfext@gmail.com> Date: Wed, 13 May 2020 23:37:17 +0800 > Currently, setting a bridge's self PVID to other value and deleting > the default VID 1 renders untagged ports of that VLAN unable to talk to > the CPU port: > > bridge vlan add dev br0 vid 2 pvid untagged self > bridge vlan del dev br0 vid 1 self > bridge vlan add dev sw0p0 vid 2 pvid untagged > bridge vlan del dev sw0p0 vid 1 > # br0 cannot send untagged frames out of sw0p0 anymore > > That is because the CPU port is set to security mode and its PVID is > still 1, and untagged frames are dropped due to VLAN member violation. > > Set the CPU port to fallback mode so untagged frames can pass through. > > Fixes: 83163f7dca56 ("net: dsa: mediatek: add VLAN support for MT7530") > Signed-off-by: DENG Qingfang <dqfext@gmail.com> Applied to net-next, thanks.
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 5c444cd722bd..a063d914c23f 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -810,10 +810,15 @@ mt7530_port_set_vlan_aware(struct dsa_switch *ds, int port) PCR_MATRIX_MASK, PCR_MATRIX(MT7530_ALL_MEMBERS)); /* Trapped into security mode allows packet forwarding through VLAN - * table lookup. + * table lookup. CPU port is set to fallback mode to let untagged + * frames pass through. */ - mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, - MT7530_PORT_SECURITY_MODE); + if (dsa_is_cpu_port(ds, port)) + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, + MT7530_PORT_FALLBACK_MODE); + else + mt7530_rmw(priv, MT7530_PCR_P(port), PCR_PORT_VLAN_MASK, + MT7530_PORT_SECURITY_MODE); /* Set the port as a user port which is to be able to recognize VID * from incoming packets before fetching entry within the VLAN table. diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index 979bb6374678..d45eb7540703 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -152,6 +152,12 @@ enum mt7530_port_mode { /* Port Matrix Mode: Frames are forwarded by the PCR_MATRIX members. */ MT7530_PORT_MATRIX_MODE = PORT_VLAN(0), + /* Fallback Mode: Forward received frames with ingress ports that do + * not belong to the VLAN member. Frames whose VID is not listed on + * the VLAN table are forwarded by the PCR_MATRIX members. + */ + MT7530_PORT_FALLBACK_MODE = PORT_VLAN(1), + /* Security Mode: Discard any frame due to ingress membership * violation or VID missed on the VLAN table. */
Currently, setting a bridge's self PVID to other value and deleting the default VID 1 renders untagged ports of that VLAN unable to talk to the CPU port: bridge vlan add dev br0 vid 2 pvid untagged self bridge vlan del dev br0 vid 1 self bridge vlan add dev sw0p0 vid 2 pvid untagged bridge vlan del dev sw0p0 vid 1 # br0 cannot send untagged frames out of sw0p0 anymore That is because the CPU port is set to security mode and its PVID is still 1, and untagged frames are dropped due to VLAN member violation. Set the CPU port to fallback mode so untagged frames can pass through. Fixes: 83163f7dca56 ("net: dsa: mediatek: add VLAN support for MT7530") Signed-off-by: DENG Qingfang <dqfext@gmail.com> --- drivers/net/dsa/mt7530.c | 11 ++++++++--- drivers/net/dsa/mt7530.h | 6 ++++++ 2 files changed, 14 insertions(+), 3 deletions(-)