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

[DENG Qingfang <dqfext@gmail.com>]

On Sun, Sep 26, 2021 at 12:59:26AM +0200, Linus Walleij wrote:
> Implement the correct callback: we have two registers dealing
> with filtering on the RTL9366RB, so we implement an ASIC-specific

Typo

> 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

You can use dsa_port_is_vlan_filtering() instead.

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

Can standalone ports or VLAN-unaware bridges send/receive VLAN-tagged frames as
is? If not, please take a look at rtl8368s_setAsicVlanKeepCtagFormat function
in the vendor driver. Set the registers to all 1's in setup function, and
change them here.

> +
> +	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.

Typo

> +	 */
> +	if (vlan_filtering)
> +		ret = rtl8366rb_drop_untagged(smi, port, !rb->pvid_enabled[port]);
> +	else
> +		ret = rtl8366rb_drop_untagged(smi, port, false);

Can be simplified as
rtl8366rb_drop_untagged(smi, port, vlan_filtering && !rb->pvid_enabled[port]);

> +
> +	return ret;
> +}