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

[Florian Fainelli <f.fainelli@gmail.com>]

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@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 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