| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <36e74f93-0800-70bb-3d30-2fac1db092fd@gmail.com>
Date: Mon, 13 Sep 2021 10:29:01 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>,
Andrew Lunn <andrew@...n.ch>,
Vivien Didelot <vivien.didelot@...il.com>,
Vladimir Oltean <olteanv@...il.com>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Mauri Sandberg <sandberg@...lfence.com>,
Alvin Šipraga <alsi@...g-olufsen.dk>,
DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH net-next 3/8] net: dsa: rtl8366rb: Rewrite weird VLAN
filering enablement
On 9/13/2021 7:42 AM, 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, describe these and activate both.
>
> Cc: Vladimir Oltean <olteanv@...il.com>
> Cc: Mauri Sandberg <sandberg@...lfence.com>
> Cc: Alvin Šipraga <alsi@...g-olufsen.dk>
> Cc: Florian Fainelli <f.fainelli@...il.com>
> Cc: DENG Qingfang <dqfext@...il.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> 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 | 36 +++++++++++++++++++++++++-----
> 3 files changed, 30 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..6c35e1ed49aa 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,26 @@ 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;
> + int ret;
> +
> + dev_dbg(smi->dev, "port %d: %s VLAN filtering\n", port,
> + vlan_filtering ? "enable" : "disable");
> +
> + /* Any incoming frames without VID (untagged) will be dropped */
But this is not exactly what you want though, an untagged frame for
which there is a VLAN entry should be accepted, think about the bridge's
default_pvid. Is the code wrong or the comment wrong?
> + ret = regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
> + BIT(port), vlan_filtering ? BIT(port) : 0);
> + if (ret)
> + return ret;
> + /* If the port is not in the member set, the frame will be dropped */
OK that part does make sense and is how it should behave.
> + 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 +1461,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 +1618,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,
>
--
Florian
Powered by blists - more mailing lists