This commit switches delay loop to read_poll_timeout macro durring
Arbiter empty check in adjust link function.
As Russel King suggested:
"This [change] avoids the issue that on the last iteration, the code reads
the register, test it, find the condition that's being waiting for is
false, _then_ waits and end up printing the error message - that last
wait is rather useless, and as the arbiter state isn't checked after
waiting, it could be that we had success during the last wait."
It also remove one short msleep delay.
Suggested-by: Russell King <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v2:
- introduced patch
drivers/net/dsa/vitesse-vsc73xx-core.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index ae55167ce0a6..bea5ec7a89fd 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -780,7 +780,7 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
* after a PHY or the CPU port comes up or down.
*/
if (!phydev->link) {
- int maxloop = 10;
+ int ret, err;
dev_dbg(vsc->dev, "port %d: went down\n",
port);
@@ -795,19 +795,16 @@ static void vsc73xx_adjust_link(struct dsa_switch *ds, int port,
VSC73XX_ARBDISC, BIT(port), BIT(port));
/* Wait until queue is empty */
- vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- while (!(val & BIT(port))) {
- msleep(1);
- vsc73xx_read(vsc, VSC73XX_BLOCK_ARBITER, 0,
- VSC73XX_ARBEMPTY, &val);
- if (--maxloop == 0) {
- dev_err(vsc->dev,
- "timeout waiting for block arbiter\n");
- /* Continue anyway */
- break;
- }
- }
+ ret = read_poll_timeout(vsc73xx_read, err,
+ err < 0 || (val & BIT(port)),
+ 1000, 10000, false,
+ vsc, VSC73XX_BLOCK_ARBITER, 0,
+ VSC73XX_ARBEMPTY, &val);
+ if (ret)
+ dev_err(vsc->dev,
+ "timeout waiting for block arbiter\n");
+ else if (err < 0)
+ dev_err(vsc->dev, "error reading arbiter\n");
/* Put this port into reset */
vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_MAC_CFG,
--
2.34.1
Switch in MAXLEN register store maximum size of data frame.
MTU size is 18 bytes smaller than frame size.
Current settings causes problems with packet forwarding.
This patch fix MTU settings to proper values.
Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v2:
- fix commit message style issue
drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index c946464489ab..59bb3dbe780a 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
struct vsc73xx *vsc = ds->priv;
return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
- VSC73XX_MAXLEN, new_mtu);
+ VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
}
/* According to application not "VSC7398 Jumbo Frames" setting
- * up the MTU to 9.6 KB does not affect the performance on standard
+ * up the frame size to 9.6 KB does not affect the performance on standard
* frames. It is clear from the application note that
* "9.6 kilobytes" == 9600 bytes.
*/
static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
{
- return 9600;
+ /* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
+ return 9582;
}
static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
--
2.34.1
This patch implement vlan filtering for vsc73xx driver.
After vlan filtering start, switch is reconfigured from QinQ to simple
vlan aware mode. It's required, because VSC73XX chips haven't support
for inner vlan tag filter.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Pawel Dembicki <[email protected]>
---
v2:
- no changes done
drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
index 457eb7fddf4c..c946464489ab 100644
--- a/drivers/net/dsa/vitesse-vsc73xx-core.c
+++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
@@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
return ret;
}
+static int
+vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
+ bool vlan_filtering, struct netlink_ext_ack *extack)
+{
+ int ret, i;
+
+ if (vlan_filtering) {
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
+ } else {
+ if (port == CPU_PORT)
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
+ else
+ vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
+ }
+
+ for (i = 0; i <= 3072; i++) {
+ ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
bool port_vlan)
{
@@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
return 0;
}
+static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan,
+ struct netlink_ext_ack *extack)
+{
+ bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
+ bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
+ int ret;
+
+ /* Be sure to deny alterations to the configuration done by tag_8021q.
+ */
+ if (vid_is_dsa_8021q(vlan->vid)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Range 3072-4095 reserved for dsa_8021q operation");
+ return -EBUSY;
+ }
+
+ if (untagged && port != CPU_PORT) {
+ ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ }
+ if (pvid && port != CPU_PORT) {
+ ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
+ if (ret)
+ return ret;
+ }
+
+ ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
+
+ return ret;
+}
+
+static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
+ const struct switchdev_obj_port_vlan *vlan)
+{
+ struct vsc73xx *vsc = ds->priv;
+ u16 vlan_no;
+ int ret;
+ u32 val;
+
+ ret =
+ vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
+ if (ret)
+ return ret;
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
+
+ if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG, &val);
+ vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
+ if (vlan_no == vlan->vid) {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
+ 0);
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_TXUPDCFG,
+ VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
+ }
+ }
+
+ vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
+ vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
+ if (vlan_no && vlan_no == vlan->vid) {
+ vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
+ VSC73XX_CAT_PORT_VLAN,
+ VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
+ }
+
+ return 0;
+}
+
static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
{
int i;
@@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
.port_change_mtu = vsc73xx_change_mtu,
.port_max_mtu = vsc73xx_get_max_mtu,
.port_stp_state_set = vsc73xx_port_stp_state_set,
+ .port_vlan_filtering = vsc73xx_port_vlan_filtering,
+ .port_vlan_add = vsc73xx_port_vlan_add,
+ .port_vlan_del = vsc73xx_port_vlan_del,
.tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
.tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
};
--
2.34.1
On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote:
> This patch implement vlan filtering for vsc73xx driver.
>
> After vlan filtering start, switch is reconfigured from QinQ to simple
> vlan aware mode. It's required, because VSC73XX chips haven't support
> for inner vlan tag filter.
>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
> v2:
> - no changes done
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
> 1 file changed, 101 insertions(+)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index 457eb7fddf4c..c946464489ab 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> return ret;
> }
>
> +static int
> +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> + bool vlan_filtering, struct netlink_ext_ack *extack)
> +{
> + int ret, i;
> +
> + if (vlan_filtering) {
> + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
> + } else {
> + if (port == CPU_PORT)
> + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> + else
> + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
> + }
Why do you need ports to be double VLAN aware when vlan_filtering=0?
Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
incoming packets, and set up the PVIDs of user ports as egress-tagged on
the CPU port?
> +
> + for (i = 0; i <= 3072; i++) {
> + ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> + if (ret)
> + return ret;
> + }
What is the purpose of this?
> +
> + return ret;
> +}
> +
> static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> bool port_vlan)
> {
> @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> return 0;
> }
>
> +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan,
> + struct netlink_ext_ack *extack)
> +{
> + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> + int ret;
> +
> + /* Be sure to deny alterations to the configuration done by tag_8021q.
> + */
> + if (vid_is_dsa_8021q(vlan->vid)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Range 3072-4095 reserved for dsa_8021q operation");
> + return -EBUSY;
> + }
> +
> + if (untagged && port != CPU_PORT) {
> + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> + if (ret)
> + return ret;
> + }
> + if (pvid && port != CPU_PORT) {
Missing logic to change hardware PVID only while VLAN-aware, and to
restore the tag_8021q PVID when the bridge VLAN awareness gets disabled.
DSA does not resolve the conflicts on resources between .port_vlan_add()
and .tag_8021q_vlan_add(), the driver must do that.
> + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> + if (ret)
> + return ret;
> + }
> +
> + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
> +
> + return ret;
Style: return vsc73xx_port_update_vlan_table(...)
> +}
> +
> +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> + const struct switchdev_obj_port_vlan *vlan)
> +{
> + struct vsc73xx *vsc = ds->priv;
> + u16 vlan_no;
> + int ret;
> + u32 val;
> +
> + ret =
> + vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
Style: single line
> + if (ret)
> + return ret;
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> +
> + if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG, &val);
> + vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> + if (vlan_no == vlan->vid) {
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> + 0);
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_TXUPDCFG,
> + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> + }
> + }
> +
> + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> + vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> + if (vlan_no && vlan_no == vlan->vid) {
> + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> + VSC73XX_CAT_PORT_VLAN,
> + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
As documented in Documentation/networking/switchdev.rst:
When the bridge has VLAN filtering enabled and a PVID is not configured on the
ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
priority-tagged packets must be accepted and forwarded according to the
bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
disabled, the presence/lack of a PVID should not influence the packet
forwarding decision.
Setting the hardware PVID to 0 when the bridge PVID is deleted sounds
like it accomplishes none of those.
> + }
> +
> + return 0;
> +}
> +
> static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> {
> int i;
> @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> .port_change_mtu = vsc73xx_change_mtu,
> .port_max_mtu = vsc73xx_get_max_mtu,
> .port_stp_state_set = vsc73xx_port_stp_state_set,
> + .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> + .port_vlan_add = vsc73xx_port_vlan_add,
> + .port_vlan_del = vsc73xx_port_vlan_del,
> .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
> };
> --
> 2.34.1
>
On Sun, Jun 25, 2023 at 01:53:36PM +0200, Pawel Dembicki wrote:
> This commit switches delay loop to read_poll_timeout macro durring
> Arbiter empty check in adjust link function.
>
> As Russel King suggested:
>
> "This [change] avoids the issue that on the last iteration, the code reads
> the register, test it, find the condition that's being waiting for is
> false, _then_ waits and end up printing the error message - that last
> wait is rather useless, and as the arbiter state isn't checked after
> waiting, it could be that we had success during the last wait."
>
> It also remove one short msleep delay.
>
> Suggested-by: Russell King <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Sun, Jun 25, 2023 at 01:53:42PM +0200, Pawel Dembicki wrote:
> Switch in MAXLEN register store maximum size of data frame.
> MTU size is 18 bytes smaller than frame size.
>
> Current settings causes problems with packet forwarding.
> This patch fix MTU settings to proper values.
>
> Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Pawel Dembicki <[email protected]>
> ---
Please split this patch from the rest of the series and re-target it
towards net.git.
> v2:
> - fix commit message style issue
>
> drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> index c946464489ab..59bb3dbe780a 100644
> --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> @@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> struct vsc73xx *vsc = ds->priv;
>
> return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
> - VSC73XX_MAXLEN, new_mtu);
> + VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
> }
>
> /* According to application not "VSC7398 Jumbo Frames" setting
> - * up the MTU to 9.6 KB does not affect the performance on standard
> + * up the frame size to 9.6 KB does not affect the performance on standard
> * frames. It is clear from the application note that
> * "9.6 kilobytes" == 9600 bytes.
> */
> static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> {
> - return 9600;
> + /* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
> + return 9582;
This can also be:
return 9600 - ETH_HLEN - ETH_FCS_LEN;
since the arithmetic is on constants, it can be evaluated at compile
time and it results in the same generated code, but the comment is no
longer necessary.
> }
>
> static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> --
> 2.34.1
>
niedz., 25 cze 2023 o 16:54 Vladimir Oltean <[email protected]> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:42PM +0200, Pawel Dembicki wrote:
> > Switch in MAXLEN register store maximum size of data frame.
> > MTU size is 18 bytes smaller than frame size.
> >
> > Current settings causes problems with packet forwarding.
> > This patch fix MTU settings to proper values.
> >
> > Fixes: fb77ffc6ec86 ("net: dsa: vsc73xx: make the MTU configurable")
> > Reviewed-by: Linus Walleij <[email protected]>
> > Signed-off-by: Pawel Dembicki <[email protected]>
> > ---
>
> Please split this patch from the rest of the series and re-target it
> towards net.git.
>
I resend it.
https://lore.kernel.org/netdev/[email protected]/
--
Pawel Dembicki
> > v2:
> > - fix commit message style issue
> >
> > drivers/net/dsa/vitesse-vsc73xx-core.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index c946464489ab..59bb3dbe780a 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -979,17 +979,18 @@ static int vsc73xx_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> > struct vsc73xx *vsc = ds->priv;
> >
> > return vsc73xx_write(vsc, VSC73XX_BLOCK_MAC, port,
> > - VSC73XX_MAXLEN, new_mtu);
> > + VSC73XX_MAXLEN, new_mtu + ETH_HLEN + ETH_FCS_LEN);
> > }
> >
> > /* According to application not "VSC7398 Jumbo Frames" setting
> > - * up the MTU to 9.6 KB does not affect the performance on standard
> > + * up the frame size to 9.6 KB does not affect the performance on standard
> > * frames. It is clear from the application note that
> > * "9.6 kilobytes" == 9600 bytes.
> > */
> > static int vsc73xx_get_max_mtu(struct dsa_switch *ds, int port)
> > {
> > - return 9600;
> > + /* max mtu = 9600 - ETH_HLEN - ETH_FCS_LEN */
> > + return 9582;
>
> This can also be:
>
> return 9600 - ETH_HLEN - ETH_FCS_LEN;
>
> since the arithmetic is on constants, it can be evaluated at compile
> time and it results in the same generated code, but the comment is no
> longer necessary.
>
> > }
> >
> > static void vsc73xx_port_stp_state_set(struct dsa_switch *ds, int port,
> > --
> > 2.34.1
> >
>
niedz., 25 cze 2023 o 17:05 Vladimir Oltean <[email protected]> napisał(a):
>
> On Sun, Jun 25, 2023 at 01:53:41PM +0200, Pawel Dembicki wrote:
> > This patch implement vlan filtering for vsc73xx driver.
> >
> > After vlan filtering start, switch is reconfigured from QinQ to simple
> > vlan aware mode. It's required, because VSC73XX chips haven't support
> > for inner vlan tag filter.
> >
> > Reviewed-by: Linus Walleij <[email protected]>
> > Signed-off-by: Pawel Dembicki <[email protected]>
> > ---
> > v2:
> > - no changes done
> >
> > drivers/net/dsa/vitesse-vsc73xx-core.c | 101 +++++++++++++++++++++++++
> > 1 file changed, 101 insertions(+)
> >
> > diff --git a/drivers/net/dsa/vitesse-vsc73xx-core.c b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > index 457eb7fddf4c..c946464489ab 100644
> > --- a/drivers/net/dsa/vitesse-vsc73xx-core.c
> > +++ b/drivers/net/dsa/vitesse-vsc73xx-core.c
> > @@ -1226,6 +1226,30 @@ static int vsc73xx_port_set_double_vlan_aware(struct dsa_switch *ds, int port)
> > return ret;
> > }
> >
> > +static int
> > +vsc73xx_port_vlan_filtering(struct dsa_switch *ds, int port,
> > + bool vlan_filtering, struct netlink_ext_ack *extack)
> > +{
> > + int ret, i;
> > +
> > + if (vlan_filtering) {
> > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_VLAN_AWARE);
> > + } else {
> > + if (port == CPU_PORT)
> > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_CPU_AWARE);
> > + else
> > + vsc73xx_port_set_vlan_conf(ds, port, VSC73XX_DOUBLE_VLAN_AWARE);
> > + }
>
> Why do you need ports to be double VLAN aware when vlan_filtering=0?
> Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> incoming packets, and set up the PVIDs of user ports as egress-tagged on
> the CPU port?
>
Because I want to forward tagged and untagged frames when
vlan_filtering is off. If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
all (tagged and untagged) traffic into the outer vlan, called by the
datasheet as "MAN space". In QinQ mode, it is possible to ignore what
goes from a particular port but it is possible to separate traffic
from different ports.
> > +
> > + for (i = 0; i <= 3072; i++) {
> > + ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > + if (ret)
> > + return ret;
> > + }
>
> What is the purpose of this?
I want to be sure that the table is cleared when vlan awareness is changed.
>
> > +
> > + return ret;
> > +}
> > +
> > static int vsc73xx_vlan_set_untagged(struct dsa_switch *ds, int port, u16 vid,
> > bool port_vlan)
> > {
> > @@ -1304,6 +1328,80 @@ static int vsc73xx_vlan_set_pvid(struct dsa_switch *ds, int port, u16 vid,
> > return 0;
> > }
> >
> > +static int vsc73xx_port_vlan_add(struct dsa_switch *ds, int port,
> > + const struct switchdev_obj_port_vlan *vlan,
> > + struct netlink_ext_ack *extack)
> > +{
> > + bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> > + bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
> > + int ret;
> > +
> > + /* Be sure to deny alterations to the configuration done by tag_8021q.
> > + */
> > + if (vid_is_dsa_8021q(vlan->vid)) {
> > + NL_SET_ERR_MSG_MOD(extack,
> > + "Range 3072-4095 reserved for dsa_8021q operation");
> > + return -EBUSY;
> > + }
> > +
> > + if (untagged && port != CPU_PORT) {
> > + ret = vsc73xx_vlan_set_untagged(ds, port, vlan->vid, true);
> > + if (ret)
> > + return ret;
> > + }
> > + if (pvid && port != CPU_PORT) {
>
> Missing logic to change hardware PVID only while VLAN-aware, and to
> restore the tag_8021q PVID when the bridge VLAN awareness gets disabled.
> DSA does not resolve the conflicts on resources between .port_vlan_add()
> and .tag_8021q_vlan_add(), the driver must do that.
>
> > + ret = vsc73xx_vlan_set_pvid(ds, port, vlan->vid, true);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + ret = vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 1);
> > +
> > + return ret;
>
> Style: return vsc73xx_port_update_vlan_table(...)
>
> > +}
> > +
> > +static int vsc73xx_port_vlan_del(struct dsa_switch *ds, int port,
> > + const struct switchdev_obj_port_vlan *vlan)
> > +{
> > + struct vsc73xx *vsc = ds->priv;
> > + u16 vlan_no;
> > + int ret;
> > + u32 val;
> > +
> > + ret =
> > + vsc73xx_port_update_vlan_table(ds, port, vlan->vid, 0);
>
> Style: single line
>
> > + if (ret)
> > + return ret;
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_TXUPDCFG, &val);
> > +
> > + if (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA) {
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_TXUPDCFG, &val);
> > + vlan_no = (val & VSC73XX_TXUPDCFG_TX_UNTAGGED_VID) >>
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_SHIFT;
> > + if (vlan_no == vlan->vid) {
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID_ENA,
> > + 0);
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_TXUPDCFG,
> > + VSC73XX_TXUPDCFG_TX_UNTAGGED_VID, 0);
> > + }
> > + }
> > +
> > + vsc73xx_read(vsc, VSC73XX_BLOCK_MAC, port, VSC73XX_CAT_PORT_VLAN, &val);
> > + vlan_no = val & VSC73XX_CAT_PORT_VLAN_VLAN_VID;
> > + if (vlan_no && vlan_no == vlan->vid) {
> > + vsc73xx_update_bits(vsc, VSC73XX_BLOCK_MAC, port,
> > + VSC73XX_CAT_PORT_VLAN,
> > + VSC73XX_CAT_PORT_VLAN_VLAN_VID, 0);
>
> As documented in Documentation/networking/switchdev.rst:
>
> When the bridge has VLAN filtering enabled and a PVID is not configured on the
> ingress port, untagged and 802.1p tagged packets must be dropped. When the bridge
> has VLAN filtering enabled and a PVID exists on the ingress port, untagged and
> priority-tagged packets must be accepted and forwarded according to the
> bridge's port membership of the PVID VLAN. When the bridge has VLAN filtering
> disabled, the presence/lack of a PVID should not influence the packet
> forwarding decision.
>
> Setting the hardware PVID to 0 when the bridge PVID is deleted sounds
> like it accomplishes none of those.
>
My bad. I should just set VSC73XX_CAT_DROP_UNTAGGED_ENA here.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static void vsc73xx_update_forwarding_map(struct vsc73xx *vsc)
> > {
> > int i;
> > @@ -1524,6 +1622,9 @@ static const struct dsa_switch_ops vsc73xx_ds_ops = {
> > .port_change_mtu = vsc73xx_change_mtu,
> > .port_max_mtu = vsc73xx_get_max_mtu,
> > .port_stp_state_set = vsc73xx_port_stp_state_set,
> > + .port_vlan_filtering = vsc73xx_port_vlan_filtering,
> > + .port_vlan_add = vsc73xx_port_vlan_add,
> > + .port_vlan_del = vsc73xx_port_vlan_del,
> > .tag_8021q_vlan_add = vsc73xx_tag_8021q_vlan_add,
> > .tag_8021q_vlan_del = vsc73xx_tag_8021q_vlan_del,
> > };
> > --
> > 2.34.1
> >
>
Thank you for such detailed responses and clarifying for me many issues.
--
Pawel Dembicki
On Thu, Jun 29, 2023 at 10:18:08PM +0200, Paweł Dembicki wrote:
> niedz., 25 cze 2023 o 17:05 Vladimir Oltean <[email protected]> napisał(a):
> > Why do you need ports to be double VLAN aware when vlan_filtering=0?
> > Isn't VLAN_TCI_IGNORE_ENA sufficient to ignore the 802.1Q header from
> > incoming packets, and set up the PVIDs of user ports as egress-tagged on
> > the CPU port?
>
> Because I want to forward tagged and untagged frames when
> vlan_filtering is off. If I set VSC73XX_DOUBLE_VLAN_AWARE, I can put
> all (tagged and untagged) traffic into the outer vlan, called by the
> datasheet as "MAN space". In QinQ mode, it is possible to ignore what
> goes from a particular port but it is possible to separate traffic
> from different ports.
I think we may have some problem in finding common terminology.
Opening the manual and seeing the table "Customer Port Sample Configuration",
I think it's indeed what you need. But I wouldn't call it "double VLAN aware".
The port is actually configured to be VLAN *unaware* from the perspective of
classification, and always encapsulate all packets in one more VLAN (the
port PVID).
This switch's analyzer is always aware only of the outer VLAN header, and
that's not "double VLAN aware" (it can perform no action based on the
inner VLAN, if that exists), but it's fine for what is needed of it.
You might be mixing these with MAC_CFG::VLAN_AWR and MAC_CFG::VLAN_DBLAWR,
which essentially are only there to allow single- and double-VLAN-tagged
frames to be longer by 4 and 8 bytes, respectively, than the max frame
size. I don't think that these 2 fields have any reason to depend upon
the bridge VLAN awareness state of the port. They can be unconditionally
enabled. After all, Linux only cares about MTU, and that is the size of
the L2 payload, excluding any VLAN headers, if present.
I would suggest that if you exclude the MAC_CFG registers from
vsc73xx_port_set_vlan_conf(), you end up with not as many VLAN awareness
modes as you think. 2, to be precise: on or off. So you don't need the
enum.
Also, AFAIU, I don't see a reason to modify CAT_VLAN_MISC::VLAN_KEEP_TAG_ENA
from the value of 1 at all. You could always keep frames in the queue
system with the VID attached, and strip that VID on egress, if necessary,
via TXUPDCFG.
Not sure if you're noticed this, but drivers/net/ethernet/mscc/ and
drivers/net/dsa/ocelot/ contain a driver for a newer generation of
hardware than the VSC73xx, but many of the concepts apply. Maybe you
can take a look at how some things were done there.
> > > +
> > > + for (i = 0; i <= 3072; i++) {
> > > + ret = vsc73xx_port_update_vlan_table(ds, port, i, 0);
> > > + if (ret)
> > > + return ret;
> > > + }
> >
> > What is the purpose of this?
>
> I want to be sure that the table is cleared when vlan awareness is changed.
Yes, but why? That should specifically not be done, since there is no
code in the kernel to replay the port_vlan_add() and tag_8021q_vlan_add()
calls for you when the VLAN awareness state changes. If you delete the
VLANs, they're gone, even though in software they're still there.