netdev - Re: [PATCH net-next] net: dsa: microchip: fix VLAN filtering operation

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c6a851c6-854b-96e3-0b00-83b3fc66356d@denx.de>
Date:   Tue, 18 Dec 2018 19:03:10 +0100
From:   Marek Vasut <marex@...x.de>
To:     Tristram.Ha@...rochip.com,
        Sergio Paracuellos <sergio.paracuellos@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Pavel Machek <pavel@....cz>
Cc:     vivien.didelot@...oirfairelinux.com, UNGLinuxDriver@...rochip.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: microchip: fix VLAN filtering
 operation

On 12/18/2018 04:31 AM, Tristram.Ha@...rochip.com wrote:
> From: Tristram Ha <Tristram.Ha@...rochip.com>
> 
> Fix VLAN filtering operation in kernel 4.15 and later.

What was broken with it ? :-)

Please write a more verbose commit message.

> Fixes: b987e98e50ab90e5 ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Tristram Ha <Tristram.Ha@...rochip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c | 44 ++++++++++++++++++++++++++++++-------
>  1 file changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 57a146a..28e3fa5 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -496,19 +496,33 @@ static int ksz9477_port_vlan_filtering(struct dsa_switch *ds, int port,
>  				       bool flag)
>  {
>  	struct ksz_device *dev = ds->priv;
> +	u32 vlan_table[3];
>  
>  	if (flag) {
> +		vlan_table[0] = VLAN_VALID | 0;
> +		vlan_table[1] = 0;
> +		vlan_table[2] = dev->port_mask;
> +		if (ksz9477_set_vlan_table(dev, 0, vlan_table)) {
> +			dev_dbg(dev->dev, "Failed to set vlan table\n");
> +			return 0;

Shouldn't failure return -Exx value ?

> +		}
>  		ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> -			     PORT_VLAN_LOOKUP_VID_0, true);
> -		ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> -			      true);
> +			     (PORT_VLAN_LOOKUP_VID_0 | PORT_INGRESS_FILTER),
> +			     true);
>  		ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, true);
>  	} else {
> +		/* VLAN 1 entry is required to run properly. */
> +		vlan_table[0] = VLAN_VALID | 0;
> +		vlan_table[1] = 0;
> +		vlan_table[2] = dev->port_mask;
> +		if (ksz9477_set_vlan_table(dev, 1, vlan_table)) {
> +			dev_dbg(dev->dev, "Failed to set vlan table\n");
> +			return 0;
> +		}
>  		ksz_cfg(dev, REG_SW_LUE_CTRL_0, SW_VLAN_ENABLE, false);
> -		ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> -			      false);
>  		ksz_port_cfg(dev, port, REG_PORT_LUE_CTRL,
> -			     PORT_VLAN_LOOKUP_VID_0, false);
> +			     (PORT_VLAN_LOOKUP_VID_0 | PORT_INGRESS_FILTER),
> +			     false);
>  	}
>  
>  	return 0;
> @@ -521,6 +535,7 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  	u32 vlan_table[3];
>  	u16 vid;
>  	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
> +	bool pvid = vlan->flags & BRIDGE_VLAN_INFO_PVID;
>  
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		if (ksz9477_get_vlan_table(dev, vid, vlan_table)) {
> @@ -533,7 +548,12 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  			vlan_table[1] |= BIT(port);
>  		else
>  			vlan_table[1] &= ~BIT(port);
> -		vlan_table[1] &= ~(BIT(dev->cpu_port));
> +
> +		/* Keep host port untagged when setting pvid. */
> +		if (untagged && pvid)
> +			vlan_table[1] |= BIT(dev->cpu_port);
> +		else
> +			vlan_table[1] &= ~(BIT(dev->cpu_port));

Drop extra parenthesis around BIT()

>  
>  		vlan_table[2] |= BIT(port) | BIT(dev->cpu_port);
>  
> @@ -543,7 +563,7 @@ static void ksz9477_port_vlan_add(struct dsa_switch *ds, int port,
>  		}
>  
>  		/* change PVID */
> -		if (vlan->flags & BRIDGE_VLAN_INFO_PVID)
> +		if (pvid)
>  			ksz_pwrite16(dev, port, REG_PORT_DEFAULT_VID, vid);
>  	}
>  }
> @@ -568,6 +588,10 @@ static int ksz9477_port_vlan_del(struct dsa_switch *ds, int port,
>  
>  		vlan_table[2] &= ~BIT(port);
>  
> +		/* Remove VLAN entry if no longer used. */
> +		if (vid > 1 && !(vlan_table[2] & ~dev->host_mask))
> +			vlan_table[0] &= ~VLAN_VALID;
> +
>  		if (pvid == vid)
>  			pvid = 1;
>  
> @@ -1130,6 +1154,10 @@ static int ksz9477_setup(struct dsa_switch *ds)
>  		return ret;
>  	}
>  
> +	/* Required for port partitioning. */
> +	ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY,
> +		      true);
> +
>  	/* accept packet up to 2000bytes */
>  	ksz_cfg(dev, REG_SW_MAC_CTRL_1, SW_LEGAL_PACKET_DISABLE, true);
>  
> 


-- 
Best regards,
Marek Vasut