Re: [PATCH net-next 3/6 v7] net: dsa: rtl8366rb: Rewrite weird VLAN filering enablement

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

On 9/26/21 12:59 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 and implement filering using the register bit that makes
> the switch drop frames if the port is not in the VLAN member set.
> 
> The DSA documentation Documentation/networking/switchdev.rst states:
> 
>    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.
> 
> To comply with this, we add two arrays of bool in the RTL8366RB
> state that keeps track of if filtering and PVID is enabled or
> not for each port. We then add code such that whenever filtering
> or PVID changes, we update the filter according to the
> specification.
> 
> 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>
> ---

Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk>

> ChangeLog v6->v7:
> - Add comments to the register definitions.
> - Rewrite the code to keep track on whether filtering and
>    PVID is enabled on each port, and use this information to
>    selectively drop untagged and C-tagged (802.1p) frames.
> - Add a hook in the PVID setup to always check the filtering
>    of untagged and C-tagged frames when changing the PVID
>    setting.
> 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        | 102 +++++++++++++++++++++++++++--
>   3 files changed, 95 insertions(+), 44 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..01d794f94156 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -143,6 +143,21 @@
>   #define RTL8366RB_PHY_NO_OFFSET			9
>   #define RTL8366RB_PHY_NO_MASK			(0x1f << 9)
>   
> +/* VLAN Ingress Control Register 1, one bit per port.
> + * bit 0 .. 5 will make the switch drop ingress frames without
> + * VID such as untagged or priority-tagged frames for respective
> + * port.
> + * bit 6 .. 11 will make the switch drop ingress frames carrying
> + * a C-tag with VID != 0 for respective port.
> + */
> +#define RTL8366RB_VLAN_INGRESS_CTRL1_REG	0x037E
> +#define RTL8366RB_VLAN_INGRESS_CTRL1_DROP(port)	(BIT((port)) | BIT((port) + 6))
> +
> +/* VLAN Ingress Control Register 2, one bit per port.
> + * bit0 .. bit5 will make the switch drop all ingress frames with
> + * a VLAN classification that does not include the port is in its
> + * member set.
> + */
>   #define RTL8366RB_VLAN_INGRESS_CTRL2_REG	0x037f
>   
>   /* LED control registers */
> @@ -321,9 +336,13 @@
>   /**
>    * struct rtl8366rb - RTL8366RB-specific data
>    * @max_mtu: per-port max MTU setting
> + * @pvid_enabled: if PVID is set for respective port
> + * @vlan_filtering: if VLAN filtering is enabled for respective port
>    */
>   struct rtl8366rb {
>   	unsigned int max_mtu[RTL8366RB_NUM_PORTS];
> +	bool pvid_enabled[RTL8366RB_NUM_PORTS];
> +	bool vlan_filtering[RTL8366RB_NUM_PORTS];
>   };
>   
>   static struct rtl8366_mib_counter rtl8366rb_mib_counters[] = {
> @@ -933,11 +952,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 +1230,53 @@ rtl8366rb_port_bridge_leave(struct dsa_switch *ds, int port,
>   			   RTL8366RB_PORT_ISO_PORTS(port_bitmap), 0);
>   }
>   
> +/**
> + * rtl8366rb_drop_untagged() - make the switch drop untagged and C-tagged frames
> + * @smi: SMI state container
> + * @port: the port to drop untagged and C-tagged frames on
> + * @drop: whether to drop or pass untagged and C-tagged frames
> + */
> +static int rtl8366rb_drop_untagged(struct realtek_smi *smi, int port, bool drop)
> +{
> +	return regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL1_REG,
> +				  RTL8366RB_VLAN_INGRESS_CTRL1_DROP(port),
> +				  drop ? RTL8366RB_VLAN_INGRESS_CTRL1_DROP(port) : 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;
> +	struct rtl8366rb *rb;
> +	int ret;
> +
> +	rb = smi->chip_data;
> +
> +	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 */
> +	ret = regmap_update_bits(smi->map, RTL8366RB_VLAN_INGRESS_CTRL2_REG,
> +				 BIT(port), vlan_filtering ? BIT(port) : 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Keep track if filtering is enabled on each port */
> +	rb->vlan_filtering[port] = vlan_filtering;
> +
> +	/* If VLAN filtering is enabled and PVID is also enabled, we must
> +	 * not drop any untagged or C-tagged frames. If we turn off VLAN
> +	 * filtering on a port, we need ti accept any frames.
> +	 */
> +	if (vlan_filtering)
> +		ret = rtl8366rb_drop_untagged(smi, port, !rb->pvid_enabled[port]);
> +	else
> +		ret = rtl8366rb_drop_untagged(smi, port, false);
> +
> +	return ret;
> +}
> +
>   static int rtl8366rb_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
>   {
>   	struct realtek_smi *smi = ds->priv;
> @@ -1420,14 +1488,34 @@ static int rtl8366rb_get_mc_index(struct realtek_smi *smi, int port, int *val)
>   
>   static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
>   {
> +	struct rtl8366rb *rb;
> +	bool pvid_enabled;
> +	int ret;
> +
> +	rb = smi->chip_data;
> +	pvid_enabled = !!index;
> +
>   	if (port >= smi->num_ports || index >= RTL8366RB_NUM_VLANS)
>   		return -EINVAL;
>   
> -	return regmap_update_bits(smi->map, RTL8366RB_PORT_VLAN_CTRL_REG(port),
> +	ret = regmap_update_bits(smi->map, RTL8366RB_PORT_VLAN_CTRL_REG(port),
>   				RTL8366RB_PORT_VLAN_CTRL_MASK <<
>   					RTL8366RB_PORT_VLAN_CTRL_SHIFT(port),
>   				(index & RTL8366RB_PORT_VLAN_CTRL_MASK) <<
>   					RTL8366RB_PORT_VLAN_CTRL_SHIFT(port));
> +	if (ret)
> +		return ret;
> +
> +	rb->pvid_enabled[port] = pvid_enabled;
> +
> +	/* If VLAN filtering is enabled and PVID is also enabled, we must
> +	 * not drop any untagged or C-tagged frames. Make sure to update the
> +	 * filtering setting.
> +	 */
> +	if (rb->vlan_filtering[port])
> +		ret = rtl8366rb_drop_untagged(smi, port, !pvid_enabled);
> +
> +	return ret;
>   }
>   
>   static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
> @@ -1437,7 +1525,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 +1682,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,
>