diff mbox series

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

Message ID bc79946d1dafded91729ee1674c1b88a3beea110.1610540603.git.gilles.doffe@savoirfairelinux.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Fixes on Microchip KSZ8795 DSA switch driver | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present fail Series targets non-next tree, but doesn't contain any Fixes tags
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Gilles Doffe Jan. 13, 2021, 12:45 p.m. UTC
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(+)

Comments

Florian Fainelli Jan. 13, 2021, 11:41 p.m. UTC | #1
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,
>
Vladimir Oltean Jan. 14, 2021, 12:10 a.m. UTC | #2
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.
diff mbox series

Patch

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,