[PATCH net 0/6] Fixes on Microchip KSZ8795 DSA switch driver

[PATCH net 0/6] Fixes on Microchip KSZ8795 DSA switch driver

[Gilles DOFFE]

This patchset fixes various issues.
It mainly concerns VLANs support by fixing FID table management to
allow adding more than one VLAN.
It also fixes tag/untag behavior on ingress/egress packets.

Gilles DOFFE (6):
  net: dsa: ksz: fix FID management
  net: dsa: ksz: move tag/untag action
  net: dsa: ksz: insert tag on ks8795 ingress packets
  net: dsa: ksz: do not change tagging on del
  net: dsa: ksz: fix wrong pvid
  net: dsa: ksz: fix wrong read cast to u64

 drivers/net/dsa/microchip/ksz8795.c     | 71 +++++++++++++++++++++----
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 drivers/net/dsa/microchip/ksz_common.h  |  3 +-
 3 files changed, 63 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH net 1/6] net: dsa: ksz: fix FID management

[Gilles DOFFE]

The FID (Filter ID) is a 7 bits field used to link the VLAN table
to the static and dynamic mac address tables.
Until now the KSZ8795 driver could only add one VLAN as the FID was
always set to 1.
This commit allows setting a FID for each new active VLAN.
The FID list is stored in a static table dynamically allocated from
ks8795_fid structure.
Each newly activated VLAN is associated to the next available FID.
Only the VLAN 0 is not added to the list as it is a special VLAN.
As it has a special meaning, see IEEE 802.1q.
When a VLAN is no more used, the associated FID table entry is reset
to 0.

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz8795.c     | 59 +++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
 drivers/net/dsa/microchip/ksz_common.h  |  1 +
 3 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index c973db101b72..6962ba4ee125 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -648,7 +648,7 @@ static enum dsa_tag_protocol ksz8795_get_tag_protocol(struct dsa_switch *ds,
 						      int port,
 						      enum dsa_tag_protocol mp)
 {
-	return DSA_TAG_PROTO_KSZ8795;
+	return DSA_TAG_PROTO_NONE;
 }
 
 static void ksz8795_get_strings(struct dsa_switch *ds, int port,
@@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
+{
+	u8 i = 0;
+
+	if (!ksz_fid_table)
+		return;
+
+	for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+		if (ksz_fid_table[i] == vid) {
+			ksz_fid_table[i] = 0;
+			break;
+		}
+	}
+}
+
+static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
+{
+	u8 i = 0;
+	int ret = -EOVERFLOW;
+
+	if (!ksz_fid_table)
+		return ret;
+
+	for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
+		if (!ksz_fid_table[i]) {
+			ksz_fid_table[i] = vid;
+			*fid = i;
+			ret = 0;
+			break;
+		}
+	}
+
+	return ret;
+}
+
 static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 				  const struct switchdev_obj_port_vlan *vlan)
 {
@@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 	struct ksz_device *dev = ds->priv;
 	u16 data, vid, new_pvid = 0;
 	u8 fid, member, valid;
+	int ret;
 
 	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
+		if (vid == 0)
+			continue;
+
 		ksz8795_r_vlan_table(dev, vid, &data);
 		ksz8795_from_vlan(data, &fid, &member, &valid);
 
 		/* First time to setup the VLAN entry. */
 		if (!valid) {
-			/* Need to find a way to map VID to FID. */
-			fid = 1;
+			ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid, &fid);
+			if (ret) {
+				dev_err(ds->dev, "Switch FID table is full, cannot add");
+				return;
+			}
 			valid = 1;
 		}
 		member |= BIT(port);
@@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
 
 		/* Invalidate the entry if no more member. */
 		if (!member) {
-			fid = 0;
+			ksz8795_del_fid(dev->ksz_fid_table, vid);
 			valid = 0;
 		}
 
@@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
 	for (i = 0; i < (dev->num_vlans / 4); i++)
 		ksz8795_r_vlan_entries(dev, i);
 
+	/* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
+	dev->ksz_fid_table[0] = 1;
+
 	/* Setup STP address for STP operation. */
 	memset(&alu, 0, sizeof(alu));
 	ether_addr_copy(alu.mac, eth_stp_addr);
@@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
 	/* set the real number of ports */
 	dev->ds->num_ports = dev->port_cnt;
 
+	dev->ksz_fid_table = devm_kzalloc(dev->dev,
+					  VLAN_TABLE_FID_SIZE * sizeof(u16),
+					  GFP_KERNEL);
+	if (!dev->ksz_fid_table)
+		return -ENOMEM;
+
 	return 0;
 }
 
diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h b/drivers/net/dsa/microchip/ksz8795_reg.h
index 40372047d40d..fe4c4f7fdd47 100644
--- a/drivers/net/dsa/microchip/ksz8795_reg.h
+++ b/drivers/net/dsa/microchip/ksz8795_reg.h
@@ -915,6 +915,7 @@
  */
 
 #define VLAN_TABLE_FID			0x007F
+#define VLAN_TABLE_FID_SIZE		128
 #define VLAN_TABLE_MEMBERSHIP		0x0F80
 #define VLAN_TABLE_VALID		0x1000
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 720f22275c84..44e97c55c2da 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -77,6 +77,7 @@ struct ksz_device {
 	bool synclko_125;
 
 	struct vlan_table *vlan_cache;
+	u16 *ksz_fid_table;
 
 	struct ksz_port *ports;
 	struct delayed_work mib_read;
-- 
2.25.1

Re: [PATCH net 1/6] net: dsa: ksz: fix FID management

[Gilles Doffe]

----- Le 13 Jan 21, à 13:45, Gilles Doffe gilles.doffe@savoirfairelinux.com a écrit :

> The FID (Filter ID) is a 7 bits field used to link the VLAN table
> to the static and dynamic mac address tables.
> Until now the KSZ8795 driver could only add one VLAN as the FID was
> always set to 1.
> This commit allows setting a FID for each new active VLAN.
> The FID list is stored in a static table dynamically allocated from
> ks8795_fid structure.
> Each newly activated VLAN is associated to the next available FID.
> Only the VLAN 0 is not added to the list as it is a special VLAN.
> As it has a special meaning, see IEEE 802.1q.
> When a VLAN is no more used, the associated FID table entry is reset
> to 0.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---
> drivers/net/dsa/microchip/ksz8795.c     | 59 +++++++++++++++++++++++--
> drivers/net/dsa/microchip/ksz8795_reg.h |  1 +
> drivers/net/dsa/microchip/ksz_common.h  |  1 +
> 3 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c
> b/drivers/net/dsa/microchip/ksz8795.c
> index c973db101b72..6962ba4ee125 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -648,7 +648,7 @@ static enum dsa_tag_protocol ksz8795_get_tag_protocol(struct
> dsa_switch *ds,
> 						      int port,
> 						      enum dsa_tag_protocol mp)
> {
> -	return DSA_TAG_PROTO_KSZ8795;
> +	return DSA_TAG_PROTO_NONE;

This is an oversight, will be removed in V2.

> }
> 
> static void ksz8795_get_strings(struct dsa_switch *ds, int port,
> @@ -796,6 +796,41 @@ static int ksz8795_port_vlan_filtering(struct dsa_switch
> *ds, int port,
> 	return 0;
> }
> 
> +static void ksz8795_del_fid(u16 *ksz_fid_table, u16 vid)
> +{
> +	u8 i = 0;
> +
> +	if (!ksz_fid_table)
> +		return;
> +
> +	for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> +		if (ksz_fid_table[i] == vid) {
> +			ksz_fid_table[i] = 0;
> +			break;
> +		}
> +	}
> +}
> +
> +static int ksz8795_get_next_fid(u16 *ksz_fid_table, u16 vid, u8 *fid)
> +{
> +	u8 i = 0;
> +	int ret = -EOVERFLOW;
> +
> +	if (!ksz_fid_table)
> +		return ret;
> +
> +	for (i = 0; i < VLAN_TABLE_FID_SIZE; i++) {
> +		if (!ksz_fid_table[i]) {
> +			ksz_fid_table[i] = vid;
> +			*fid = i;
> +			ret = 0;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
> 				  const struct switchdev_obj_port_vlan *vlan)
> {
> @@ -803,17 +838,24 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds,
> int port,
> 	struct ksz_device *dev = ds->priv;
> 	u16 data, vid, new_pvid = 0;
> 	u8 fid, member, valid;
> +	int ret;
> 
> 	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> 
> 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
> +		if (vid == 0)
> +			continue;
> +
> 		ksz8795_r_vlan_table(dev, vid, &data);
> 		ksz8795_from_vlan(data, &fid, &member, &valid);
> 
> 		/* First time to setup the VLAN entry. */
> 		if (!valid) {
> -			/* Need to find a way to map VID to FID. */
> -			fid = 1;
> +			ret = ksz8795_get_next_fid(dev->ksz_fid_table, vid, &fid);
> +			if (ret) {
> +				dev_err(ds->dev, "Switch FID table is full, cannot add");
> +				return;
> +			}
> 			valid = 1;
> 		}
> 		member |= BIT(port);
> @@ -855,7 +897,7 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int
> port,
> 
> 		/* Invalidate the entry if no more member. */
> 		if (!member) {
> -			fid = 0;
> +			ksz8795_del_fid(dev->ksz_fid_table, vid);
> 			valid = 0;
> 		}
> 
> @@ -1087,6 +1129,9 @@ static int ksz8795_setup(struct dsa_switch *ds)
> 	for (i = 0; i < (dev->num_vlans / 4); i++)
> 		ksz8795_r_vlan_entries(dev, i);
> 
> +	/* Assign first FID to VLAN 1 and always return FID=0 for this vlan */
> +	dev->ksz_fid_table[0] = 1;
> +
> 	/* Setup STP address for STP operation. */
> 	memset(&alu, 0, sizeof(alu));
> 	ether_addr_copy(alu.mac, eth_stp_addr);
> @@ -1261,6 +1306,12 @@ static int ksz8795_switch_init(struct ksz_device *dev)
> 	/* set the real number of ports */
> 	dev->ds->num_ports = dev->port_cnt;
> 
> +	dev->ksz_fid_table = devm_kzalloc(dev->dev,
> +					  VLAN_TABLE_FID_SIZE * sizeof(u16),
> +					  GFP_KERNEL);
> +	if (!dev->ksz_fid_table)
> +		return -ENOMEM;
> +
> 	return 0;
> }
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795_reg.h
> b/drivers/net/dsa/microchip/ksz8795_reg.h
> index 40372047d40d..fe4c4f7fdd47 100644
> --- a/drivers/net/dsa/microchip/ksz8795_reg.h
> +++ b/drivers/net/dsa/microchip/ksz8795_reg.h
> @@ -915,6 +915,7 @@
>  */
> 
> #define VLAN_TABLE_FID			0x007F
> +#define VLAN_TABLE_FID_SIZE		128
> #define VLAN_TABLE_MEMBERSHIP		0x0F80
> #define VLAN_TABLE_VALID		0x1000
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.h
> b/drivers/net/dsa/microchip/ksz_common.h
> index 720f22275c84..44e97c55c2da 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -77,6 +77,7 @@ struct ksz_device {
> 	bool synclko_125;
> 
> 	struct vlan_table *vlan_cache;
> +	u16 *ksz_fid_table;
> 
> 	struct ksz_port *ports;
> 	struct delayed_work mib_read;
> --
> 2.25.1

Re: [PATCH net 1/6] net: dsa: ksz: fix FID management

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:17PM +0100, Gilles DOFFE wrote:
> The FID (Filter ID) is a 7 bits field used to link the VLAN table
> to the static and dynamic mac address tables.
> Until now the KSZ8795 driver could only add one VLAN as the FID was
> always set to 1.

What do you mean the ksz8769 driver could only add one VLAN? That is
obviously a false statement.

All VLANs use the same FID of 1 means that the switch is currently
configured for shared address learning. Whereas each VLAN having a
separate FID would mean that it is configured for individual address
learning.

> This commit allows setting a FID for each new active VLAN.
> The FID list is stored in a static table dynamically allocated from
> ks8795_fid structure.
> Each newly activated VLAN is associated to the next available FID.
> Only the VLAN 0 is not added to the list as it is a special VLAN.
> As it has a special meaning, see IEEE 802.1q.
> When a VLAN is no more used, the associated FID table entry is reset
> to 0.

Why is this patch targeting the "net" tree? What is the problem that it
resolves?

[PATCH net 2/6] net: dsa: ksz: move tag/untag action

[Gilles DOFFE]

Move tag/untag action at the end of the function to avoid
tagging or untagging traffic if only vlan 0 is handled.

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 6962ba4ee125..4b060503b2e8 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -840,8 +840,6 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 	u8 fid, member, valid;
 	int ret;
 
-	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
-
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		if (vid == 0)
 			continue;
@@ -874,6 +872,8 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 		vid |= new_pvid;
 		ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid);
 	}
+
+	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
 }
 
 static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
-- 
2.25.1

Re: [PATCH net 2/6] net: dsa: ksz: move tag/untag action

[Florian Fainelli]

On 1/13/21 4:45 AM, Gilles DOFFE wrote:
> Move tag/untag action at the end of the function to avoid
> tagging or untagging traffic if only vlan 0 is handled.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 6962ba4ee125..4b060503b2e8 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -840,8 +840,6 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
>  	u8 fid, member, valid;
>  	int ret;
>  
> -	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> -
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		if (vid == 0)
>  			continue;
> @@ -874,6 +872,8 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
>  		vid |= new_pvid;
>  		ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid);
>  	}
> +
> +	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);

You should be giving an example how this fails, because it is not
immediately obvious why this fixes a problem and how it does fix it,
especially for VID == 0 given that the loop would be skipped over.
--
Florian

Re: [PATCH net 2/6] net: dsa: ksz: move tag/untag action

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:18PM +0100, Gilles DOFFE wrote:
> Move tag/untag action at the end of the function to avoid
> tagging or untagging traffic if only vlan 0 is handled.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---

No matter how much you move the assignment around, there's no escaping
the truth that the Tag Removal bit in the Port Registers affects all
VLANs that egress a port, whereas the BRIDGE_VLAN_INFO_UNTAGGED flag
controls egress VLAN stripping per VLAN. Sorry, if you work with broken
hardware, you might as well treat it accordingly too.

And as to why the moving around would make any difference in the first
place, you need to do a better job explaining that. There is nothing
that prevents PORT_REMOVE_TAG from being written to the port, regardless
of the order of operations. Unless the order matters?

[PATCH net 3/6] net: dsa: ksz: insert tag on ks8795 ingress packets

[Gilles DOFFE]

If 802.1q VLAN tag is removed from egress traffic, ingress
traffic should by logic be tagged.

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 4b060503b2e8..193f03ef9160 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 	}
 
 	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
+	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
 }
 
 static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
-- 
2.25.1

Re: [PATCH net 3/6] net: dsa: ksz: insert tag on ks8795 ingress packets

[Florian Fainelli]

On 1/13/21 4:45 AM, Gilles DOFFE wrote:
> If 802.1q VLAN tag is removed from egress traffic, ingress
> traffic should by logic be tagged.

Which logic do you refer to? Software or hardware? What an user
configures with the "bridge vlan add ..." commands is the egress
tagging, but this also affects what egresses the CPU port, and therefore
what your Ethernet MAC used as a DSA master "sees", so I am not sure why
this is doing?

> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 4b060503b2e8..193f03ef9160 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
>  	}
>  
>  	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> +	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
>  }
>  
>  static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
> 


-- 
Florian

Re: [PATCH net 3/6] net: dsa: ksz: insert tag on ks8795 ingress packets

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:19PM +0100, Gilles DOFFE wrote:
> If 802.1q VLAN tag is removed from egress traffic, ingress
> traffic should by logic be tagged.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 4b060503b2e8..193f03ef9160 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -874,6 +874,7 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
>  	}
>  
>  	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> +	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_INSERT_TAG, !untagged);
>  }
>  
>  static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
> -- 
> 2.25.1
> 

KSZ8795 manual says:

TABLE 4-4: PORT REGISTERS
Bit 2: Tag insertion
1 = When packets are output on the port, the switch
will add 802.1q tags to packets without 802.1q tags
when received. The switch will not add tags to
packets already tagged. The tag inserted is the
ingress port’s “Port VID.”
0 = Disable tag insertion.

Bit 1: Tag Removal
1 = When packets are output on the port, the switch
will remove 802.1q tags from packets with 802.1q
tags when received. The switch will not modify
packets received without tags.
0 = Disable tag removal.

What I understand from this is that the "Tag Removal" bit controls
whether the port will send all VLANs as egress-untagged or not.

Whereas the "Tag insertion" bit controls whether the pvid of the ingress
port will be sent as egress-tagged (if the insertion bit is 1), or as-is
(probably egress-untagged) (if the insertion bit is 0) on the egress
port.

I deduce that the "Tag Removal" bit overrules the "Tag insertion" bit of
a different port, if both are set. Example:

lan0:               lan1
pvid=20
Tag insertion=1     Tag removal=0

An untagged packet forwarded from lan0 to lan1 should be transmitted as
egress-tagged, because lan0 is configured to insert its pvid into the
frames.

But:

lan0:               lan1
pvid=20
Tag insertion=1     Tag removal=1

An untagged packet forwarded from lan0 to lan1 should be transmitted as
untagged, because even though lan0 inserted its pvid into the frame,
lan1 removed it.

Based on my interpretation of the manual, I believe you have a lot more
work to do than simply operating "by logic". You can test, but I don't
believe that the PORT_INSERT_TAG flag affects the port on which the
switchdev VLAN object is supposed to be offloading. On the contrary: it
affects every other port in the same bridge _except_ for that one.

[PATCH net 4/6] net: dsa: ksz: do not change tagging on del

[Gilles DOFFE]

If a VLAN is removed, the tagging policy should not be changed as
still active VLANs could be impacted.

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index 193f03ef9160..b55fb2761993 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -880,7 +880,6 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
 				 const struct switchdev_obj_port_vlan *vlan)
 {
-	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
 	struct ksz_device *dev = ds->priv;
 	u16 data, vid, pvid, new_pvid = 0;
 	u8 fid, member, valid;
@@ -888,8 +887,6 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
 	ksz_pread16(dev, port, REG_PORT_CTRL_VID, &pvid);
 	pvid = pvid & 0xFFF;
 
-	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
-
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
 		ksz8795_r_vlan_table(dev, vid, &data);
 		ksz8795_from_vlan(data, &fid, &member, &valid);
-- 
2.25.1

Re: [PATCH net 4/6] net: dsa: ksz: do not change tagging on del

[Florian Fainelli]

On 1/13/21 4:45 AM, Gilles DOFFE wrote:
> If a VLAN is removed, the tagging policy should not be changed as
> still active VLANs could be impacted.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>

It sounds like you need to consolidate the patches dealing with
tagging/untagged of a VLAN/port into a single patch, that may be clearer
and easier to review rather than doing this piecemeal.
-- 
Florian

Re: [PATCH net 4/6] net: dsa: ksz: do not change tagging on del

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:20PM +0100, Gilles DOFFE wrote:
> If a VLAN is removed, the tagging policy should not be changed as
> still active VLANs could be impacted.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---
>  drivers/net/dsa/microchip/ksz8795.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
> index 193f03ef9160..b55fb2761993 100644
> --- a/drivers/net/dsa/microchip/ksz8795.c
> +++ b/drivers/net/dsa/microchip/ksz8795.c
> @@ -880,7 +880,6 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
>  static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
>  				 const struct switchdev_obj_port_vlan *vlan)
>  {
> -	bool untagged = vlan->flags & BRIDGE_VLAN_INFO_UNTAGGED;
>  	struct ksz_device *dev = ds->priv;
>  	u16 data, vid, pvid, new_pvid = 0;
>  	u8 fid, member, valid;
> @@ -888,8 +887,6 @@ static int ksz8795_port_vlan_del(struct dsa_switch *ds, int port,
>  	ksz_pread16(dev, port, REG_PORT_CTRL_VID, &pvid);
>  	pvid = pvid & 0xFFF;
>  
> -	ksz_port_cfg(dev, port, P_TAG_CTRL, PORT_REMOVE_TAG, untagged);
> -
>  	for (vid = vlan->vid_begin; vid <= vlan->vid_end; vid++) {
>  		ksz8795_r_vlan_table(dev, vid, &data);
>  		ksz8795_from_vlan(data, &fid, &member, &valid);
> -- 
> 2.25.1
> 

What do you mean the tagging policy "should not be changed". Nothing is
changed, the write to PORT_REMOVE_TAG is identical to the one done on
.port_vlan_add. If anything, the egress untagging policy is reinforced
on delete, not changed...

What's the actual problem (beside for the fact that the driver is
obviously a lot more broken than you can fix through patches to "net")?

[PATCH net 5/6] net: dsa: ksz: fix wrong pvid

[Gilles DOFFE]

A logical 'or' was performed until now.
So if vlan 1 is the current pvid and vlan 20 is set as the new one,
vlan 21 is the new pvid.
This commit fixes this by setting the right mask to set the new pvid.

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz8795.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index b55fb2761993..44306a7e297a 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c
@@ -868,8 +868,8 @@ static void ksz8795_port_vlan_add(struct dsa_switch *ds, int port,
 
 	if (new_pvid) {
 		ksz_pread16(dev, port, REG_PORT_CTRL_VID, &vid);
-		vid &= 0xfff;
-		vid |= new_pvid;
+		vid &= ~0xfff;
+		vid |= (new_pvid & 0xfff);
 		ksz_pwrite16(dev, port, REG_PORT_CTRL_VID, vid);
 	}
 
-- 
2.25.1

Re: [PATCH net 5/6] net: dsa: ksz: fix wrong pvid

[Florian Fainelli]

On 1/13/21 4:45 AM, Gilles DOFFE wrote:
> A logical 'or' was performed until now.
> So if vlan 1 is the current pvid and vlan 20 is set as the new one,
> vlan 21 is the new pvid.
> This commit fixes this by setting the right mask to set the new pvid.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>

Looks about right, can you provide a Fixes: tag for this change?
-- 
Florian

Re: [PATCH net 5/6] net: dsa: ksz: fix wrong pvid

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:21PM +0100, Gilles DOFFE wrote:
> A logical 'or' was performed until now.
> So if vlan 1 is the current pvid and vlan 20 is set as the new one,
> vlan 21 is the new pvid.
> This commit fixes this by setting the right mask to set the new pvid.
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net 6/6] net: dsa: ksz: fix wrong read cast to u64

[Gilles DOFFE]

'(u64)*value' casts a u32 to a u64. So depending on endianness,
LSB or MSB is lost.
The pointer needs to be cast to read the full u64:
'*((u64 *)value)'

Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
---
 drivers/net/dsa/microchip/ksz_common.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 44e97c55c2da..492dcb2011f1 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -213,7 +213,7 @@ static inline int ksz_read64(struct ksz_device *dev, u32 reg, u64 *val)
 		/* Ick! ToDo: Add 64bit R/W to regmap on 32bit systems */
 		value[0] = swab32(value[0]);
 		value[1] = swab32(value[1]);
-		*val = swab64((u64)*value);
+		*val = swab64((((u64)value[1]) << 32) | value[0]);
 	}
 
 	return ret;
-- 
2.25.1

Re: [PATCH net 6/6] net: dsa: ksz: fix wrong read cast to u64

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:22PM +0100, Gilles DOFFE wrote:
> '(u64)*value' casts a u32 to a u64. So depending on endianness,
> LSB or MSB is lost.
> The pointer needs to be cast to read the full u64:
> '*((u64 *)value)'
> 
> Signed-off-by: Gilles DOFFE <gilles.doffe@savoirfairelinux.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net 0/6] Fixes on Microchip KSZ8795 DSA switch driver

[Vladimir Oltean]

On Wed, Jan 13, 2021 at 01:45:16PM +0100, Gilles DOFFE wrote:
> 
> This patchset fixes various issues.
> It mainly concerns VLANs support by fixing FID table management to
> allow adding more than one VLAN.
> It also fixes tag/untag behavior on ingress/egress packets.

As far as I understand the series, it "fixes" something that was not
broken (but which nonetheless could take some improvement), and does not
fix something that was broken, because it was too broken.

Good thing you brought up the bugs now, because FYI a tsunami is coming
soon and it will cause major conflicts once Jakub merges net back into
net-next. Had you waited a little bit longer, and the bug fixes sent to
"net" would have not been backported too far down the line due to the
API rework.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=b7a9e0da2d1c954b7c38217a29e002528b90d174

You should try to find a reasonable balance between bugs due to an
oversight, and "bugs" due to code which was put there as a joke more
than anything else. Then you should send the fixes for the former to net
and for the latter to net-next.

Good luck.