Hi Tristram,
On 12/17/18 7:31 PM, tristram...@microchip.com wrote:
> From: Tristram Ha <tristram...@microchip.com>
>
> Fix VLAN filtering operation in kernel 4.15 and later.
A better commit message should be added to this patch, something like:
When VLAN filtering on a bridge port is disabled, it is necessary to
create a VLAN entry with VID = 0 for that port in order for untagged
traffic to reach the bridge. Conversely, when turning on VLAN filtering,
a VLAN entry with the desired PVID value (typically VID = 1) must be
created.
Something along those lines.
>
> Fixes: b987e98e50ab90e5 ("dsa: add DSA switch driver for Microchip KSZ9477")
> Signed-off-by: Tristram Ha <tristram...@microchip.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;
Is not that breaking separation between bridged ports and non bridged
ports? Don't you want to just include the CPU and the port as specified
as argument to that function here?
> + if (ksz9477_set_vlan_table(dev, 0, vlan_table)) {
> + dev_dbg(dev->dev, "Failed to set vlan table\n");
> + return 0;
> + }
> 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;
Likewise here
> + if (ksz9477_set_vlan_table(dev, 1, vlan_table)) {
> + dev_dbg(dev->dev, "Failed to set vlan table\n");
> + return 0;
> + }
You are assuming that VID 1 is always used, but this may not be true, I
would read whichever was the last PVID value configured, which could
have been either at bridge creation time through the default_pvid
property, or later on through:
bridge vlan add vid <num> dev <interface> pvid untagged
and use that value as the VLAN entry to create.
> 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),
parenthesis should not be needed
> + 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));
>
> 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);
>
>
--
Florian
