| Message ID | 20210925132311.2040272-4-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | RTL8366(RB) cleanups part 1 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 8 of 8 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 | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 21 this patch: 21 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 110 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 9/25/21 3:23 PM, Linus Walleij wrote: > While we were defining one VLAN per port for isolating the ports > the port_vlan_filtering() callback was implemented to enable a > VLAN on the port + 1. This function makes no sense, not only is > it incomplete as it only enables the VLAN, it doesn't do what > the callback is supposed to do, which is to selectively enable > and disable filtering on a certain port. > > Implement the correct callback: we have two registers dealing > with filtering on the RTL9366RB, so we implement an ASIC-specific > callback and implement filering using the register bit that makes > the switch drop frames if the port is not in the VLAN member set. > > Cc: Vladimir Oltean <olteanv@gmail.com> > Cc: Mauri Sandberg <sandberg@mailfence.com> > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: DENG Qingfang <dqfext@gmail.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v5->v6: > - Drop unused leftover variable "ret" > ChangeLog v4->v5: > - Drop the code dropping frames without VID, after Florian > described that this is not expected semantics for this > callback. > ChangeLog v1->v4: > - New patch after discovering that this weirdness of mine is > causing problems. > --- > drivers/net/dsa/realtek-smi-core.h | 2 -- > drivers/net/dsa/rtl8366.c | 35 ------------------------------ > drivers/net/dsa/rtl8366rb.c | 30 ++++++++++++++++++++----- > 3 files changed, 24 insertions(+), 43 deletions(-) > > diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h > index c8fbd7b9fd0b..214f710d7dd5 100644 > --- a/drivers/net/dsa/realtek-smi-core.h > +++ b/drivers/net/dsa/realtek-smi-core.h > @@ -129,8 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port, > int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable); > int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable); > int rtl8366_reset_vlan(struct realtek_smi *smi); > -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering, > - struct netlink_ext_ack *extack); > int rtl8366_vlan_add(struct dsa_switch *ds, int port, > const struct switchdev_obj_port_vlan *vlan, > struct netlink_ext_ack *extack); > diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c > index 59c5bc4f7b71..0672dd56c698 100644 > --- a/drivers/net/dsa/rtl8366.c > +++ b/drivers/net/dsa/rtl8366.c > @@ -292,41 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi) > } > EXPORT_SYMBOL_GPL(rtl8366_reset_vlan); > > -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering, > - struct netlink_ext_ack *extack) > -{ > - struct realtek_smi *smi = ds->priv; > - struct rtl8366_vlan_4k vlan4k; > - int ret; > - > - /* Use VLAN nr port + 1 since VLAN0 is not valid */ > - if (!smi->ops->is_vlan_valid(smi, port + 1)) > - return -EINVAL; > - > - dev_info(smi->dev, "%s filtering on port %d\n", > - vlan_filtering ? "enable" : "disable", > - port); > - > - /* TODO: > - * The hardware support filter ID (FID) 0..7, I have no clue how to > - * support this in the driver when the callback only says on/off. > - */ > - ret = smi->ops->get_vlan_4k(smi, port + 1, &vlan4k); > - if (ret) > - return ret; > - > - /* Just set the filter to FID 1 for now then */ > - ret = rtl8366_set_vlan(smi, port + 1, > - vlan4k.member, > - vlan4k.untag, > - 1); > - if (ret) > - return ret; > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(rtl8366_vlan_filtering); > - > int rtl8366_vlan_add(struct dsa_switch *ds, int port, > const struct switchdev_obj_port_vlan *vlan, > struct netlink_ext_ack *extack) > diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c > index a5b7d7ff8884..2c66a0c2ee50 100644 > --- a/drivers/net/dsa/rtl8366rb.c > +++ b/drivers/net/dsa/rtl8366rb.c > @@ -143,6 +143,8 @@ > #define RTL8366RB_PHY_NO_OFFSET 9 > #define RTL8366RB_PHY_NO_MASK (0x1f << 9) > > +/* VLAN Ingress Control Register */ > +#define RTL8366RB_VLAN_INGRESS_CTRL1_REG 0x037E > #define RTL8366RB_VLAN_INGRESS_CTRL2_REG 0x037f > > /* LED control registers */ > @@ -933,11 +935,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds) > if (ret) > return ret; > > - /* Discard VLAN tagged packets if the port is not a member of > - * the VLAN with which the packets is associated. > - */ > + /* Accept all packets by default, we enable filtering on-demand */ > + ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG, > + 0); From the previous version of this patch I understood that CTRL1_REG is for controlling whether or not to accept untagged frames, and CTRL2 for accepting tagged frames that don't match the port member set. Is that correct? Do you know whether DSA _always_ wants ports to accept untagged frames, or only if the port has PVID set? I'm also asking for my own understanding. In the latter case I think you might have to set CTRL1 in rtl8366rb_vlan_filtering() (depending on whether the port has a PVID), as well as whenever a PVID is set or unset. Of course it depends on the switch semantics - maybe it ignores CTRL1(port) == 1 if the port has a PVID, in which case your previous version of the patch would be OK. > + if (ret) > + return ret; > ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG, > - RTL8366RB_PORT_ALL); > + 0); > if (ret) > return ret; > > @@ -1209,6 +1213,20 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port, > RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0); > } > > +static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port, > + bool vlan_filtering, > + struct netlink_ext_ack *extack) > +{ > + struct realtek_smi *smi = ds->priv; > + > + dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port, > + vlan_filtering ? "enable" : "disable"); > + > + /* If the port is not in the member set, the frame will be dropped */ > + return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG, > + BIT(port), vlan_filtering ? BIT(port) : 0); > +} > + > static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > { > struct realtek_smi *smi = ds->priv; > @@ -1437,7 +1455,7 @@ static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan) > if (smi->vlan4k_enabled) > max = RTL8366RB_NUM_VIDS - 1; > > - if (vlan == 0 || vlan > max) > + if (vlan > max) > return false; > > return true; > @@ -1594,7 +1612,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = { > .get_sset_count = rtl8366_get_sset_count, > .port_bridge_join = rtl8366rb_port_bridge_join, > .port_bridge_leave = rtl8366rb_port_bridge_leave, > - .port_vlan_filtering = rtl8366_vlan_filtering, > + .port_vlan_filtering = rtl8366rb_vlan_filtering, > .port_vlan_add = rtl8366_vlan_add, > .port_vlan_del = rtl8366_vlan_del, > .port_enable = rtl8366rb_port_enable, >
On Sat, Sep 25, 2021 at 05:12:24PM +0000, Alvin Šipraga wrote: > From the previous version of this patch I understood that CTRL1_REG is > for controlling whether or not to accept untagged frames, and CTRL2 for > accepting tagged frames that don't match the port member set. Is that > correct? It might not hurt to add comments which explain what these registers do. > Do you know whether DSA _always_ wants ports to accept untagged frames, > or only if the port has PVID set? I'm also asking for my own > understanding. In the latter case I think you might have to set CTRL1 in > rtl8366rb_vlan_filtering() (depending on whether the port has a PVID), > as well as whenever a PVID is set or unset. Of course it depends on the > switch semantics - maybe it ignores CTRL1(port) == 1 if the port has a > PVID, in which case your previous version of the patch would be OK. Documentation/networking/switchdev.rst says: 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. Anyway, I suppose Linus can make that adjustment after the fact too, if everything else is ok in the other patches.
On Sat, Sep 25, 2021 at 8:38 PM Vladimir Oltean <olteanv@gmail.com> wrote: > Documentation/networking/switchdev.rst says: Aha now I get this in context, thanks a lot! > Anyway, I suppose Linus can make that adjustment after the fact too, if > everything else is ok in the other patches. I better get this right and explain it to myself while I still have it in my head, I'm sending a new version of this patch (the whole set) that fixes it according to spec (I hope). Yours, Linus Walleij
diff --git a/drivers/net/dsa/realtek-smi-core.h b/drivers/net/dsa/realtek-smi-core.h index c8fbd7b9fd0b..214f710d7dd5 100644 --- a/drivers/net/dsa/realtek-smi-core.h +++ b/drivers/net/dsa/realtek-smi-core.h @@ -129,8 +129,6 @@ int rtl8366_set_pvid(struct realtek_smi *smi, unsigned int port, int rtl8366_enable_vlan4k(struct realtek_smi *smi, bool enable); int rtl8366_enable_vlan(struct realtek_smi *smi, bool enable); int rtl8366_reset_vlan(struct realtek_smi *smi); -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering, - struct netlink_ext_ack *extack); int rtl8366_vlan_add(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan, struct netlink_ext_ack *extack); diff --git a/drivers/net/dsa/rtl8366.c b/drivers/net/dsa/rtl8366.c index 59c5bc4f7b71..0672dd56c698 100644 --- a/drivers/net/dsa/rtl8366.c +++ b/drivers/net/dsa/rtl8366.c @@ -292,41 +292,6 @@ int rtl8366_reset_vlan(struct realtek_smi *smi) } EXPORT_SYMBOL_GPL(rtl8366_reset_vlan); -int rtl8366_vlan_filtering(struct dsa_switch *ds, int port, bool vlan_filtering, - struct netlink_ext_ack *extack) -{ - struct realtek_smi *smi = ds->priv; - struct rtl8366_vlan_4k vlan4k; - int ret; - - /* Use VLAN nr port + 1 since VLAN0 is not valid */ - if (!smi->ops->is_vlan_valid(smi, port + 1)) - return -EINVAL; - - dev_info(smi->dev, "%s filtering on port %d\n", - vlan_filtering ? "enable" : "disable", - port); - - /* TODO: - * The hardware support filter ID (FID) 0..7, I have no clue how to - * support this in the driver when the callback only says on/off. - */ - ret = smi->ops->get_vlan_4k(smi, port + 1, &vlan4k); - if (ret) - return ret; - - /* Just set the filter to FID 1 for now then */ - ret = rtl8366_set_vlan(smi, port + 1, - vlan4k.member, - vlan4k.untag, - 1); - if (ret) - return ret; - - return 0; -} -EXPORT_SYMBOL_GPL(rtl8366_vlan_filtering); - int rtl8366_vlan_add(struct dsa_switch *ds, int port, const struct switchdev_obj_port_vlan *vlan, struct netlink_ext_ack *extack) diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c index a5b7d7ff8884..2c66a0c2ee50 100644 --- a/drivers/net/dsa/rtl8366rb.c +++ b/drivers/net/dsa/rtl8366rb.c @@ -143,6 +143,8 @@ #define RTL8366RB_PHY_NO_OFFSET 9 #define RTL8366RB_PHY_NO_MASK (0x1f << 9) +/* VLAN Ingress Control Register */ +#define RTL8366RB_VLAN_INGRESS_CTRL1_REG 0x037E #define RTL8366RB_VLAN_INGRESS_CTRL2_REG 0x037f /* LED control registers */ @@ -933,11 +935,13 @@ static int rtl8366rb_setup(struct dsa_switch *ds) if (ret) return ret; - /* Discard VLAN tagged packets if the port is not a member of - * the VLAN with which the packets is associated. - */ + /* Accept all packets by default, we enable filtering on-demand */ + ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG, + 0); + if (ret) + return ret; ret = regmap_write(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG, - RTL8366RB_PORT_ALL); + 0); if (ret) return ret; @@ -1209,6 +1213,20 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port, RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0); } +static int rtl8366rb_vlan_filtering(struct dsa_switch *ds, int port, + bool vlan_filtering, + struct netlink_ext_ack *extack) +{ + struct realtek_smi *smi = ds->priv; + + dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port, + vlan_filtering ? "enable" : "disable"); + + /* If the port is not in the member set, the frame will be dropped */ + return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG, + BIT(port), vlan_filtering ? BIT(port) : 0); +} + static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu) { struct realtek_smi *smi = ds->priv; @@ -1437,7 +1455,7 @@ static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan) if (smi->vlan4k_enabled) max = RTL8366RB_NUM_VIDS - 1; - if (vlan == 0 || vlan > max) + if (vlan > max) return false; return true; @@ -1594,7 +1612,7 @@ static const struct dsa_switch_ops rtl8366rb_switch_ops = { .get_sset_count = rtl8366_get_sset_count, .port_bridge_join = rtl8366rb_port_bridge_join, .port_bridge_leave = rtl8366rb_port_bridge_leave, - .port_vlan_filtering = rtl8366_vlan_filtering, + .port_vlan_filtering = rtl8366rb_vlan_filtering, .port_vlan_add = rtl8366_vlan_add, .port_vlan_del = rtl8366_vlan_del, .port_enable = rtl8366rb_port_enable,
While we were defining one VLAN per port for isolating the ports the port_vlan_filtering() callback was implemented to enable a VLAN on the port + 1. This function makes no sense, not only is it incomplete as it only enables the VLAN, it doesn't do what the callback is supposed to do, which is to selectively enable and disable filtering on a certain port. Implement the correct callback: we have two registers dealing with filtering on the RTL9366RB, so we implement an ASIC-specific callback and implement filering using the register bit that makes the switch drop frames if the port is not in the VLAN member set. Cc: Vladimir Oltean <olteanv@gmail.com> Cc: Mauri Sandberg <sandberg@mailfence.com> Cc: Alvin Šipraga <alsi@bang-olufsen.dk> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: DENG Qingfang <dqfext@gmail.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v5->v6: - Drop unused leftover variable "ret" ChangeLog v4->v5: - Drop the code dropping frames without VID, after Florian described that this is not expected semantics for this callback. ChangeLog v1->v4: - New patch after discovering that this weirdness of mine is causing problems. --- drivers/net/dsa/realtek-smi-core.h | 2 -- drivers/net/dsa/rtl8366.c | 35 ------------------------------ drivers/net/dsa/rtl8366rb.c | 30 ++++++++++++++++++++----- 3 files changed, 24 insertions(+), 43 deletions(-)