[PATCH net-next v2 0/4] Introduce VLAN support in HSR

[PATCH net-next v2 0/4] Introduce VLAN support in HSR

[MD Danish Anwar]

This series adds VLAN support to HSR framework.
This series also adds VLAN support to HSR mode of ICSSG Ethernet driver.

Changes from v1 to v2:
*) Added patch 4/4 to add test script related to VLAN in HSR as asked by
Lukasz Majewski <lukma@denx.de>

v1 https://lore.kernel.org/all/20241004074715.791191-1-danishanwar@ti.com/

MD Danish Anwar (1):
  selftests: hsr: Add test for VLAN

Murali Karicheri (1):
  net: hsr: Add VLAN CTAG filter support

Ravi Gunasekaran (1):
  net: ti: icssg-prueth: Add VLAN support for HSR mode

WingMan Kwok (1):
  net: hsr: Add VLAN support

 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 45 +++++++++++-
 net/hsr/hsr_device.c                         | 76 ++++++++++++++++++--
 net/hsr/hsr_forward.c                        | 19 +++--
 tools/testing/selftests/net/hsr/config       |  1 +
 tools/testing/selftests/net/hsr/hsr_ping.sh  | 63 +++++++++++++++-
 5 files changed, 191 insertions(+), 13 deletions(-)


base-commit: 1bf70e6c3a5346966c25e0a1ff492945b25d3f80
-- 
2.34.1

[PATCH net-next v2 1/4] net: hsr: Add VLAN support

[MD Danish Anwar]

From: WingMan Kwok <w-kwok2@ti.com>

Add support for creating VLAN interfaces over HSR/PRP interface.

Signed-off-by: WingMan Kwok <w-kwok2@ti.com>
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 net/hsr/hsr_device.c  |  5 -----
 net/hsr/hsr_forward.c | 19 ++++++++++++++-----
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index ebdfd5b64e17..0ca47ebb01d3 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -572,11 +572,6 @@ void hsr_dev_setup(struct net_device *dev)
 			   NETIF_F_HW_VLAN_CTAG_TX;
 
 	dev->features = dev->hw_features;
-
-	/* VLAN on top of HSR needs testing and probably some work on
-	 * hsr_header_create() etc.
-	 */
-	dev->features |= NETIF_F_VLAN_CHALLENGED;
 }
 
 /* Return true if dev is a HSR master; return false otherwise.
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index b38060246e62..aa6acebc7c1e 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -280,6 +280,7 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
 				    struct hsr_port *port, u8 proto_version)
 {
 	struct hsr_ethhdr *hsr_ethhdr;
+	unsigned char *pc;
 	int lsdu_size;
 
 	/* pad to minimum packet size which is 60 + 6 (HSR tag) */
@@ -290,7 +291,18 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
 	if (frame->is_vlan)
 		lsdu_size -= 4;
 
-	hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
+	pc = skb_mac_header(skb);
+	if (frame->is_vlan)
+		/* This 4-byte shift (size of a vlan tag) does not
+		 * mean that the ethhdr starts there. But rather it
+		 * provides the proper environment for accessing
+		 * the fields, such as hsr_tag etc., just like
+		 * when the vlan tag is not there. This is because
+		 * the hsr tag is after the vlan tag.
+		 */
+		hsr_ethhdr = (struct hsr_ethhdr *)(pc + VLAN_HLEN);
+	else
+		hsr_ethhdr = (struct hsr_ethhdr *)pc;
 
 	hsr_set_path_id(hsr_ethhdr, port);
 	set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
@@ -368,7 +380,7 @@ struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
 		return skb_clone(frame->skb_std, GFP_ATOMIC);
 	}
 
-	skb = skb_copy_expand(frame->skb_std, 0,
+	skb = skb_copy_expand(frame->skb_std, skb_headroom(frame->skb_std),
 			      skb_tailroom(frame->skb_std) + HSR_HLEN,
 			      GFP_ATOMIC);
 	return prp_fill_rct(skb, frame, port);
@@ -690,9 +702,6 @@ static int fill_frame_info(struct hsr_frame_info *frame,
 	if (frame->is_vlan) {
 		vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
 		proto = vlan_hdr->vlanhdr.h_vlan_encapsulated_proto;
-		/* FIXME: */
-		netdev_warn_once(skb->dev, "VLAN not yet supported");
-		return -EINVAL;
 	}
 
 	frame->is_from_san = false;
-- 
2.34.1

[PATCH net-next v2 2/4] net: hsr: Add VLAN CTAG filter support

[MD Danish Anwar]

From: Murali Karicheri <m-karicheri2@ti.com>

This patch adds support for VLAN ctag based filtering at slave devices.
The slave ethernet device may be capable of filtering ethernet packets
based on VLAN ID. This requires that when the VLAN interface is created
over an HSR/PRP interface, it passes the VID information to the
associated slave ethernet devices so that it updates the hardware
filters to filter ethernet frames based on VID. This patch adds the
required functions to propagate the vid information to the slave
devices.

Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+), 1 deletion(-)

diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index 0ca47ebb01d3..ff586bdc2bde 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
 	}
 }
 
+static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
+				   __be16 proto, u16 vid)
+{
+	struct hsr_port *port;
+	struct hsr_priv *hsr;
+	int ret = 0;
+
+	hsr = netdev_priv(dev);
+
+	hsr_for_each_port(hsr, port) {
+		if (port->type == HSR_PT_MASTER)
+			continue;
+
+		ret = vlan_vid_add(port->dev, proto, vid);
+		switch (port->type) {
+		case HSR_PT_SLAVE_A:
+			if (ret) {
+				netdev_err(dev, "add vid failed for Slave-A\n");
+				return ret;
+			}
+			break;
+
+		case HSR_PT_SLAVE_B:
+			if (ret) {
+				/* clean up Slave-A */
+				netdev_err(dev, "add vid failed for Slave-B\n");
+				vlan_vid_del(port->dev, proto, vid);
+				return ret;
+			}
+			break;
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
+				    __be16 proto, u16 vid)
+{
+	struct hsr_port *port;
+	struct hsr_priv *hsr;
+
+	hsr = netdev_priv(dev);
+
+	hsr_for_each_port(hsr, port) {
+		if (port->type == HSR_PT_MASTER)
+			continue;
+		switch (port->type) {
+		case HSR_PT_SLAVE_A:
+		case HSR_PT_SLAVE_B:
+			vlan_vid_del(port->dev, proto, vid);
+			break;
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops hsr_device_ops = {
 	.ndo_change_mtu = hsr_dev_change_mtu,
 	.ndo_open = hsr_dev_open,
@@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = {
 	.ndo_change_rx_flags = hsr_change_rx_flags,
 	.ndo_fix_features = hsr_fix_features,
 	.ndo_set_rx_mode = hsr_set_rx_mode,
+	.ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid,
 };
 
 static const struct device_type hsr_type = {
@@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev)
 
 	dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
 			   NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
-			   NETIF_F_HW_VLAN_CTAG_TX;
+			   NETIF_F_HW_VLAN_CTAG_TX |
+			   NETIF_F_HW_VLAN_CTAG_FILTER;
 
 	dev->features = dev->hw_features;
 }
@@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
 	    (slave[1]->features & NETIF_F_HW_HSR_FWD))
 		hsr->fwd_offloaded = true;
 
+	if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
+	    (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER))
+		hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
+
 	res = register_netdevice(hsr_dev);
 	if (res)
 		goto err_unregister;
-- 
2.34.1

[PATCH net-next v2 3/4] net: ti: icssg-prueth: Add VLAN support for HSR mode

[MD Danish Anwar]

From: Ravi Gunasekaran <r-gunasekaran@ti.com>

Add support for VLAN addition/deletion in HSR mode.
In HSR mode, even if the host port is not a member of
the VLAN domain, the slave ports should simply forward the
frames. So allow forwarding of all VLAN frames in HSR mode.

Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 45 +++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 0556910938fa..b4d70c6e0cff 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -808,6 +808,47 @@ static netdev_features_t emac_ndo_fix_features(struct net_device *ndev,
 	return features;
 }
 
+static int emac_ndo_vlan_rx_add_vid(struct net_device *ndev,
+				    __be16 proto, u16 vid)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+	int untag_mask = 0;
+	int port_mask;
+
+	if (prueth->is_hsr_offload_mode) {
+		port_mask = BIT(PRUETH_PORT_HOST) | BIT(emac->port_id);
+		untag_mask = 0;
+
+		netdev_dbg(emac->ndev, "VID add vid:%u port_mask:%X untag_mask %X\n",
+			   vid, port_mask, untag_mask);
+
+		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, true);
+		icssg_set_pvid(emac->prueth, vid, emac->port_id);
+	}
+	return 0;
+}
+
+static int emac_ndo_vlan_rx_del_vid(struct net_device *ndev,
+				    __be16 proto, u16 vid)
+{
+	struct prueth_emac *emac = netdev_priv(ndev);
+	struct prueth *prueth = emac->prueth;
+	int untag_mask = 0;
+	int port_mask;
+
+	if (prueth->is_hsr_offload_mode) {
+		port_mask = BIT(PRUETH_PORT_HOST);
+		untag_mask = 0;
+
+		netdev_dbg(emac->ndev, "VID del vid:%u port_mask:%X untag_mask  %X\n",
+			   vid, port_mask, untag_mask);
+
+		icssg_vtbl_modify(emac, vid, port_mask, untag_mask, false);
+	}
+	return 0;
+}
+
 static const struct net_device_ops emac_netdev_ops = {
 	.ndo_open = emac_ndo_open,
 	.ndo_stop = emac_ndo_stop,
@@ -820,6 +861,8 @@ static const struct net_device_ops emac_netdev_ops = {
 	.ndo_get_stats64 = icssg_ndo_get_stats64,
 	.ndo_get_phys_port_name = icssg_ndo_get_phys_port_name,
 	.ndo_fix_features = emac_ndo_fix_features,
+	.ndo_vlan_rx_add_vid = emac_ndo_vlan_rx_add_vid,
+	.ndo_vlan_rx_kill_vid = emac_ndo_vlan_rx_del_vid,
 };
 
 static int prueth_netdev_init(struct prueth *prueth,
@@ -947,7 +990,7 @@ static int prueth_netdev_init(struct prueth *prueth,
 	ndev->netdev_ops = &emac_netdev_ops;
 	ndev->ethtool_ops = &icssg_ethtool_ops;
 	ndev->hw_features = NETIF_F_SG;
-	ndev->features = ndev->hw_features;
+	ndev->features = ndev->hw_features | NETIF_F_HW_VLAN_CTAG_FILTER;
 	ndev->hw_features |= NETIF_PRUETH_HSR_OFFLOAD_FEATURES;
 
 	netif_napi_add(ndev, &emac->napi_rx, icssg_napi_rx_poll);
-- 
2.34.1

[PATCH net-next v2 4/4] selftests: hsr: Add test for VLAN

[MD Danish Anwar]

Add test for VLAN ping for HSR. The test adds vlan interfaces to the hsr
interface and then verifies if ping to them works.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 tools/testing/selftests/net/hsr/config      |  1 +
 tools/testing/selftests/net/hsr/hsr_ping.sh | 63 ++++++++++++++++++++-
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/hsr/config b/tools/testing/selftests/net/hsr/config
index 241542441c51..555a868743f0 100644
--- a/tools/testing/selftests/net/hsr/config
+++ b/tools/testing/selftests/net/hsr/config
@@ -3,3 +3,4 @@ CONFIG_NET_SCH_NETEM=m
 CONFIG_HSR=y
 CONFIG_VETH=y
 CONFIG_BRIDGE=y
+CONFIG_VLAN_8021Q=m
diff --git a/tools/testing/selftests/net/hsr/hsr_ping.sh b/tools/testing/selftests/net/hsr/hsr_ping.sh
index f5d207fc770a..fb7c7d3fb6c7 100755
--- a/tools/testing/selftests/net/hsr/hsr_ping.sh
+++ b/tools/testing/selftests/net/hsr/hsr_ping.sh
@@ -2,13 +2,15 @@
 # SPDX-License-Identifier: GPL-2.0
 
 ipv6=true
+vlan=false
 
 source ./hsr_common.sh
 
-optstring="h4"
+optstring="h4v"
 usage() {
 	echo "Usage: $0 [OPTION]"
 	echo -e "\t-4: IPv4 only: disable IPv6 tests (default: test both IPv4 and IPv6)"
+	echo -e "\t-v: Enable VLAN tests"
 }
 
 while getopts "$optstring" option;do
@@ -20,6 +22,9 @@ while getopts "$optstring" option;do
 	"4")
 		ipv6=false
 		;;
+	"v")
+		vlan=true
+		;;
 	"?")
 		usage $0
 		exit 1
@@ -175,6 +180,50 @@ setup_hsr_interfaces()
 	ip -net "$ns3" link set hsr3 up
 }
 
+setup_vlan_interfaces() {
+	ip link add link hsr1 name hsr1.2 type vlan id 2
+	ip link add link hsr1 name hsr1.3 type vlan id 3
+	ip link add link hsr1 name hsr1.4 type vlan id 4
+	ip link add link hsr1 name hsr1.5 type vlan id 5
+
+	ip link add link hsr2 name hsr2.2 type vlan id 2
+	ip link add link hsr2 name hsr2.3 type vlan id 3
+	ip link add link hsr2 name hsr2.4 type vlan id 4
+	ip link add link hsr2 name hsr2.5 type vlan id 5
+
+	ip link add link hsr3 name hsr3.2 type vlan id 2
+	ip link add link hsr3 name hsr3.3 type vlan id 3
+	ip link add link hsr3 name hsr3.4 type vlan id 4
+	ip link add link hsr3 name hsr3.5 type vlan id 5
+
+	ip -net "$ns1" addr add 100.64.2.1/24 dev hsr1.2
+	ip -net "$ns1" addr add 100.64.3.1/24 dev hsr1.3
+	ip -net "$ns1" addr add 100.64.4.1/24 dev hsr1.4
+	ip -net "$ns1" addr add 100.64.5.1/24 dev hsr1.5
+
+	ip -net "$ns2" addr add 100.64.2.2/24 dev hsr2.2
+	ip -net "$ns2" addr add 100.64.3.2/24 dev hsr2.3
+	ip -net "$ns2" addr add 100.64.4.2/24 dev hsr2.4
+	ip -net "$ns2" addr add 100.64.5.2/24 dev hsr2.5
+
+	ip -net "$ns3" addr add 100.64.2.3/24 dev hsr3.2
+	ip -net "$ns3" addr add 100.64.3.3/24 dev hsr3.3
+	ip -net "$ns3" addr add 100.64.4.3/24 dev hsr3.4
+	ip -net "$ns3" addr add 100.64.5.3/24 dev hsr3.5
+}
+
+hsr_vlan_ping() {
+	do_ping "$ns2" 100.64.2.1
+	do_ping "$ns2" 100.64.3.1
+	do_ping "$ns2" 100.64.4.1
+	do_ping "$ns2" 100.64.5.1
+
+	do_ping "$ns2" 100.64.2.3
+	do_ping "$ns2" 100.64.3.3
+	do_ping "$ns2" 100.64.4.3
+	do_ping "$ns2" 100.64.5.3
+}
+
 check_prerequisites
 setup_ns ns1 ns2 ns3
 
@@ -183,9 +232,21 @@ trap cleanup_all_ns EXIT
 setup_hsr_interfaces 0
 do_complete_ping_test
 
+# Run VLAN Test
+if $vlan; then
+	setup_vlan_interfaces
+	hsr_vlan_ping
+fi
+
 setup_ns ns1 ns2 ns3
 
 setup_hsr_interfaces 1
 do_complete_ping_test
 
+# Run VLAN Test
+if $vlan; then
+	setup_vlan_interfaces
+	hsr_vlan_ping
+fi
+
 exit $ret
-- 
2.34.1

Re: [PATCH net-next v2 2/4] net: hsr: Add VLAN CTAG filter support

[Vadim Fedorenko]

On 24/10/2024 11:30, MD Danish Anwar wrote:
> From: Murali Karicheri <m-karicheri2@ti.com>
> 
> This patch adds support for VLAN ctag based filtering at slave devices.
> The slave ethernet device may be capable of filtering ethernet packets
> based on VLAN ID. This requires that when the VLAN interface is created
> over an HSR/PRP interface, it passes the VID information to the
> associated slave ethernet devices so that it updates the hardware
> filters to filter ethernet frames based on VID. This patch adds the
> required functions to propagate the vid information to the slave
> devices.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>   net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 0ca47ebb01d3..ff586bdc2bde 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
>   	}
>   }
>   
> +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
> +				   __be16 proto, u16 vid)
> +{
> +	struct hsr_port *port;
> +	struct hsr_priv *hsr;
> +	int ret = 0;
> +
> +	hsr = netdev_priv(dev);
> +
> +	hsr_for_each_port(hsr, port) {
> +		if (port->type == HSR_PT_MASTER)
> +			continue;
> +
> +		ret = vlan_vid_add(port->dev, proto, vid);
> +		switch (port->type) {
> +		case HSR_PT_SLAVE_A:
> +			if (ret) {
> +				netdev_err(dev, "add vid failed for Slave-A\n");
> +				return ret;
> +			}
> +			break;
> +
> +		case HSR_PT_SLAVE_B:
> +			if (ret) {
> +				/* clean up Slave-A */
> +				netdev_err(dev, "add vid failed for Slave-B\n");
> +				vlan_vid_del(port->dev, proto, vid);
> +				return ret;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

This function doesn't match with hsr_ndo_vlan_rx_kill_vid().
vlan_vid_add() can potentially be executed for port->type
equals to HSR_PT_INTERLINK, but the result will be ignored. And
the vlan_vid_del() will never happen in this case. Is it desired
behavior? Maybe it's better to synchronize add/del code and refactor
error path to avoid coping the code?

> +
> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
> +				    __be16 proto, u16 vid)
> +{
> +	struct hsr_port *port;
> +	struct hsr_priv *hsr;
> +
> +	hsr = netdev_priv(dev);
> +
> +	hsr_for_each_port(hsr, port) {
> +		if (port->type == HSR_PT_MASTER)
> +			continue;
> +		switch (port->type) {
> +		case HSR_PT_SLAVE_A:
> +		case HSR_PT_SLAVE_B:
> +			vlan_vid_del(port->dev, proto, vid);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct net_device_ops hsr_device_ops = {
>   	.ndo_change_mtu = hsr_dev_change_mtu,
>   	.ndo_open = hsr_dev_open,
> @@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = {
>   	.ndo_change_rx_flags = hsr_change_rx_flags,
>   	.ndo_fix_features = hsr_fix_features,
>   	.ndo_set_rx_mode = hsr_set_rx_mode,
> +	.ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid,
> +	.ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid,
>   };
>   
>   static const struct device_type hsr_type = {
> @@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev)
>   
>   	dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST | NETIF_F_HIGHDMA |
>   			   NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
> -			   NETIF_F_HW_VLAN_CTAG_TX;
> +			   NETIF_F_HW_VLAN_CTAG_TX |
> +			   NETIF_F_HW_VLAN_CTAG_FILTER;
>   
>   	dev->features = dev->hw_features;
>   }
> @@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev, struct net_device *slave[2],
>   	    (slave[1]->features & NETIF_F_HW_HSR_FWD))
>   		hsr->fwd_offloaded = true;
>   
> +	if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
> +	    (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER))
> +		hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> +
>   	res = register_netdevice(hsr_dev);
>   	if (res)
>   		goto err_unregister;

Re: [PATCH net-next v2 2/4] net: hsr: Add VLAN CTAG filter support

[Anwar, Md Danish]

Hi Vadim,

On 10/24/2024 7:06 PM, Vadim Fedorenko wrote:
> On 24/10/2024 11:30, MD Danish Anwar wrote:
>> From: Murali Karicheri <m-karicheri2@ti.com>
>>
>> This patch adds support for VLAN ctag based filtering at slave devices.
>> The slave ethernet device may be capable of filtering ethernet packets
>> based on VLAN ID. This requires that when the VLAN interface is created
>> over an HSR/PRP interface, it passes the VID information to the
>> associated slave ethernet devices so that it updates the hardware
>> filters to filter ethernet frames based on VID. This patch adds the
>> required functions to propagate the vid information to the slave
>> devices.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>   net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>> index 0ca47ebb01d3..ff586bdc2bde 100644
>> --- a/net/hsr/hsr_device.c
>> +++ b/net/hsr/hsr_device.c
>> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device
>> *dev, int change)
>>       }
>>   }
>>   +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>> +                   __be16 proto, u16 vid)
>> +{
>> +    struct hsr_port *port;
>> +    struct hsr_priv *hsr;
>> +    int ret = 0;
>> +
>> +    hsr = netdev_priv(dev);
>> +
>> +    hsr_for_each_port(hsr, port) {
>> +        if (port->type == HSR_PT_MASTER)
>> +            continue;
>> +
>> +        ret = vlan_vid_add(port->dev, proto, vid);
>> +        switch (port->type) {
>> +        case HSR_PT_SLAVE_A:
>> +            if (ret) {
>> +                netdev_err(dev, "add vid failed for Slave-A\n");
>> +                return ret;
>> +            }
>> +            break;
>> +
>> +        case HSR_PT_SLAVE_B:
>> +            if (ret) {
>> +                /* clean up Slave-A */
>> +                netdev_err(dev, "add vid failed for Slave-B\n");
>> +                vlan_vid_del(port->dev, proto, vid);
>> +                return ret;
>> +            }
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
> 
> This function doesn't match with hsr_ndo_vlan_rx_kill_vid().
> vlan_vid_add() can potentially be executed for port->type
> equals to HSR_PT_INTERLINK, but the result will be ignored. And
> the vlan_vid_del() will never happen in this case. Is it desired
> behavior? Maybe it's better to synchronize add/del code and refactor
> error path to avoid coping the code?
> 

The kill_vid / add_vid is not similar because during add_vid, if
vlan_vid_add() succeeds for one port but fails for other, we need to
delete it for the earlier port. We can only continue if vlan_vid_add()
succeeds for both ports. That's the reason the switch case handling of
add_vid can not match the same for kill_vid. Since cleanup of port is
needed, it's not possible to synchronize add/kill code

We only care about HSR_PT_SLAVE_A and HSR_PT_SLAVE_B here. So it's okay
to ignore HSR_PT_INTERLINK. It's a desired behaviour here.

>> +
>> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
>> +                    __be16 proto, u16 vid)
>> +{
>> +    struct hsr_port *port;
>> +    struct hsr_priv *hsr;
>> +
>> +    hsr = netdev_priv(dev);
>> +
>> +    hsr_for_each_port(hsr, port) {
>> +        if (port->type == HSR_PT_MASTER)
>> +            continue;
>> +        switch (port->type) {
>> +        case HSR_PT_SLAVE_A:
>> +        case HSR_PT_SLAVE_B:
>> +            vlan_vid_del(port->dev, proto, vid);
>> +            break;
>> +        default:
>> +            break;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct net_device_ops hsr_device_ops = {
>>       .ndo_change_mtu = hsr_dev_change_mtu,
>>       .ndo_open = hsr_dev_open,
>> @@ -523,6 +585,8 @@ static const struct net_device_ops hsr_device_ops = {
>>       .ndo_change_rx_flags = hsr_change_rx_flags,
>>       .ndo_fix_features = hsr_fix_features,
>>       .ndo_set_rx_mode = hsr_set_rx_mode,
>> +    .ndo_vlan_rx_add_vid = hsr_ndo_vlan_rx_add_vid,
>> +    .ndo_vlan_rx_kill_vid = hsr_ndo_vlan_rx_kill_vid,
>>   };
>>     static const struct device_type hsr_type = {
>> @@ -569,7 +633,8 @@ void hsr_dev_setup(struct net_device *dev)
>>         dev->hw_features = NETIF_F_SG | NETIF_F_FRAGLIST |
>> NETIF_F_HIGHDMA |
>>                  NETIF_F_GSO_MASK | NETIF_F_HW_CSUM |
>> -               NETIF_F_HW_VLAN_CTAG_TX;
>> +               NETIF_F_HW_VLAN_CTAG_TX |
>> +               NETIF_F_HW_VLAN_CTAG_FILTER;
>>         dev->features = dev->hw_features;
>>   }
>> @@ -647,6 +712,10 @@ int hsr_dev_finalize(struct net_device *hsr_dev,
>> struct net_device *slave[2],
>>           (slave[1]->features & NETIF_F_HW_HSR_FWD))
>>           hsr->fwd_offloaded = true;
>>   +    if ((slave[0]->features & NETIF_F_HW_VLAN_CTAG_FILTER) &&
>> +        (slave[1]->features & NETIF_F_HW_VLAN_CTAG_FILTER))
>> +        hsr_dev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
>> +
>>       res = register_netdevice(hsr_dev);
>>       if (res)
>>           goto err_unregister;
> 

-- 
Thanks and Regards,
Md Danish Anwar

Re: [PATCH net-next v2 2/4] net: hsr: Add VLAN CTAG filter support

[Paolo Abeni]

On 10/24/24 12:30, MD Danish Anwar wrote:
> From: Murali Karicheri <m-karicheri2@ti.com>
> 
> This patch adds support for VLAN ctag based filtering at slave devices.
> The slave ethernet device may be capable of filtering ethernet packets
> based on VLAN ID. This requires that when the VLAN interface is created
> over an HSR/PRP interface, it passes the VID information to the
> associated slave ethernet devices so that it updates the hardware
> filters to filter ethernet frames based on VID. This patch adds the
> required functions to propagate the vid information to the slave
> devices.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 1 deletion(-)
> 
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 0ca47ebb01d3..ff586bdc2bde 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
>  	}
>  }
>  
> +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
> +				   __be16 proto, u16 vid)
> +{
> +	struct hsr_port *port;
> +	struct hsr_priv *hsr;
> +	int ret = 0;
> +
> +	hsr = netdev_priv(dev);
> +
> +	hsr_for_each_port(hsr, port) {
> +		if (port->type == HSR_PT_MASTER)
> +			continue;

If the desired behavior is to ignore INTERLINK port, I think you should
explicitly skip them here, otherwise you will end-up in a
nondeterministic state.

> +		ret = vlan_vid_add(port->dev, proto, vid);
> +		switch (port->type) {
> +		case HSR_PT_SLAVE_A:
> +			if (ret) {
> +				netdev_err(dev, "add vid failed for Slave-A\n");
> +				return ret;
> +			}
> +			break;
> +
> +		case HSR_PT_SLAVE_B:
> +			if (ret) {
> +				/* clean up Slave-A */
> +				netdev_err(dev, "add vid failed for Slave-B\n");
> +				vlan_vid_del(port->dev, proto, vid);

This code relies on a specific port_list order - which is actually
respected at list creation time. Still such assumption looks fragile and
may lead to long term bugs.

I think would be better to refactor the above loop handling arbitrary
HSR_PT_SLAVE_A, HSR_PT_SLAVE_B order. Guestimate is that the complexity
will not increase measurably.

> +				return ret;
> +			}
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
> +				    __be16 proto, u16 vid)
> +{
> +	struct hsr_port *port;
> +	struct hsr_priv *hsr;
> +
> +	hsr = netdev_priv(dev);
> +
> +	hsr_for_each_port(hsr, port) {
> +		if (port->type == HSR_PT_MASTER)
> +			continue;

I think it would be more consistent just removing the above statement...

> +		switch (port->type) {
> +		case HSR_PT_SLAVE_A:
> +		case HSR_PT_SLAVE_B:
> +			vlan_vid_del(port->dev, proto, vid);
> +			break;
> +		default:> +			break;

... MASTER and INTERLINK port will be ignored anyway.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct net_device_ops hsr_device_ops = {
>  	.ndo_change_mtu = hsr_dev_change_mtu,
>  	.ndo_open = hsr_dev_open,

Cheers,

Paolo

Re: [PATCH net-next v2 4/4] selftests: hsr: Add test for VLAN

[Paolo Abeni]

On 10/24/24 12:30, MD Danish Anwar wrote:
> @@ -183,9 +232,21 @@ trap cleanup_all_ns EXIT
>  setup_hsr_interfaces 0
>  do_complete_ping_test
>  
> +# Run VLAN Test
> +if $vlan; then
> +	setup_vlan_interfaces
> +	hsr_vlan_ping
> +fi
> +
>  setup_ns ns1 ns2 ns3
>  
>  setup_hsr_interfaces 1
>  do_complete_ping_test
>  
> +# Run VLAN Test
> +if $vlan; then
> +	setup_vlan_interfaces
> +	hsr_vlan_ping
> +fi

The new tests should be enabled by default. Indeed ideally the test
script should be able to run successfully on kernel not supporting such
feature; you could cope with that looking for the hsr exposed feature
and skipping the vlan test when the relevant feature is not present.

Cheers,

Paolo

Re: [PATCH net-next v2 2/4] net: hsr: Add VLAN CTAG filter support

[MD Danish Anwar]

Hi Paolo,

On 31/10/24 8:07 pm, Paolo Abeni wrote:
> 
> 
> On 10/24/24 12:30, MD Danish Anwar wrote:
>> From: Murali Karicheri <m-karicheri2@ti.com>
>>
>> This patch adds support for VLAN ctag based filtering at slave devices.
>> The slave ethernet device may be capable of filtering ethernet packets
>> based on VLAN ID. This requires that when the VLAN interface is created
>> over an HSR/PRP interface, it passes the VID information to the
>> associated slave ethernet devices so that it updates the hardware
>> filters to filter ethernet frames based on VID. This patch adds the
>> required functions to propagate the vid information to the slave
>> devices.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  net/hsr/hsr_device.c | 71 +++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>> index 0ca47ebb01d3..ff586bdc2bde 100644
>> --- a/net/hsr/hsr_device.c
>> +++ b/net/hsr/hsr_device.c
>> @@ -515,6 +515,68 @@ static void hsr_change_rx_flags(struct net_device *dev, int change)
>>  	}
>>  }
>>  
>> +static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
>> +				   __be16 proto, u16 vid)
>> +{
>> +	struct hsr_port *port;
>> +	struct hsr_priv *hsr;
>> +	int ret = 0;
>> +
>> +	hsr = netdev_priv(dev);
>> +
>> +	hsr_for_each_port(hsr, port) {
>> +		if (port->type == HSR_PT_MASTER)
>> +			continue;
> 
> If the desired behavior is to ignore INTERLINK port, I think you should
> explicitly skip them here, otherwise you will end-up in a
> nondeterministic state.
> 

Sure, I will change this to

	hsr_for_each_port(hsr, port) {
		if (port->type == HSR_PT_MASTER ||
		    port->type = HSR_PT_INTERLINK)
			continue;

>> +		ret = vlan_vid_add(port->dev, proto, vid);
>> +		switch (port->type) {
>> +		case HSR_PT_SLAVE_A:
>> +			if (ret) {
>> +				netdev_err(dev, "add vid failed for Slave-A\n");
>> +				return ret;
>> +			}
>> +			break;
>> +
>> +		case HSR_PT_SLAVE_B:
>> +			if (ret) {
>> +				/* clean up Slave-A */
>> +				netdev_err(dev, "add vid failed for Slave-B\n");
>> +				vlan_vid_del(port->dev, proto, vid);
> 
> This code relies on a specific port_list order - which is actually
> respected at list creation time. Still such assumption looks fragile and
> may lead to long term bugs.
> 

Agreed. The code is expecting HSR_PT_SLAVE_A to come first and add vid
for it. Then vid is added for HSR_PT_SLAVE_B. if vlan_vid_add fails for
HSR_PT_SLAVE_B, vid is deleted for HSR_PT_SLAVE_A before returning.

> I think would be better to refactor the above loop handling arbitrary
> HSR_PT_SLAVE_A, HSR_PT_SLAVE_B order. Guestimate is that the complexity
> will not increase measurably.
> 

I understand this will be better. But how would I figure out which port
came first.

The current implementation is to add vid for first port. If it fails it
returns. If it passes it adds vid for second port. If it fails it clears
vid of first port and returns. If it passes function returns 0.

Now how do I keep this behavior and also handle ports arbitrary. If the
same order is not guaranteed to be preserved, how would I know which
port came first so that it can be deleted if second port fails?

One idea I have is to keep two boolean flags is_slave_a_added,
is_slave_b_added. And based on these flags we can determine if cleanup
is needed or not.

The add function will then look like this,

static int hsr_ndo_vlan_rx_add_vid(struct net_device *dev,
				   __be16 proto, u16 vid)
{
	bool is_slave_a_added, is_slave_b_added;
	struct hsr_port *port;
	struct hsr_priv *hsr;
	int ret = 0;

	hsr = netdev_priv(dev);

	hsr_for_each_port(hsr, port) {
		if (port->type == HSR_PT_MASTER ||
		    port->type = HSR_PT_INTERLINK)
			continue;

		ret = vlan_vid_add(port->dev, proto, vid);
		switch (port->type) {
		case HSR_PT_SLAVE_A:
			if (ret) {
				/* clean up Slave-B */
				netdev_err(dev, "add vid failed for Slave-A\n");
				if (is_slave_b_added)
					vlan_vid_del(port->dev, proto, vid);
				return ret;
			} else {
				is_slave_a_added = true;
			}
			break;

		case HSR_PT_SLAVE_B:
			if (ret) {
				/* clean up Slave-A */
				netdev_err(dev, "add vid failed for Slave-B\n");
				if (is_slave_a_added)
					vlan_vid_del(port->dev, proto, vid);
				return ret;
			} else {
				is_slave_b_added = true;
			}
			break;
		default:
			break;
		}
	}

	return 0;
}

Let me know if this is OK. Or if you have some other method to handle
the ports arbitrary.

>> +				return ret;
>> +			}
>> +			break;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int hsr_ndo_vlan_rx_kill_vid(struct net_device *dev,
>> +				    __be16 proto, u16 vid)
>> +{
>> +	struct hsr_port *port;
>> +	struct hsr_priv *hsr;
>> +
>> +	hsr = netdev_priv(dev);
>> +
>> +	hsr_for_each_port(hsr, port) {
>> +		if (port->type == HSR_PT_MASTER)
>> +			continue;
> 
> I think it would be more consistent just removing the above statement...
> 

Sure. I'll do it.

>> +		switch (port->type) {
>> +		case HSR_PT_SLAVE_A:
>> +		case HSR_PT_SLAVE_B:
>> +			vlan_vid_del(port->dev, proto, vid);
>> +			break;
>> +		default:> +			break;
> 
> ... MASTER and INTERLINK port will be ignored anyway.
> 

Sure.

>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static const struct net_device_ops hsr_device_ops = {
>>  	.ndo_change_mtu = hsr_dev_change_mtu,
>>  	.ndo_open = hsr_dev_open,
> 
> Cheers,
> 
> Paolo
> 

-- 
Thanks and Regards,
Danish

Re: [PATCH net-next v2 4/4] selftests: hsr: Add test for VLAN

[MD Danish Anwar]

On 31/10/24 8:11 pm, Paolo Abeni wrote:
> On 10/24/24 12:30, MD Danish Anwar wrote:
>> @@ -183,9 +232,21 @@ trap cleanup_all_ns EXIT
>>  setup_hsr_interfaces 0
>>  do_complete_ping_test
>>  
>> +# Run VLAN Test
>> +if $vlan; then
>> +	setup_vlan_interfaces
>> +	hsr_vlan_ping
>> +fi
>> +
>>  setup_ns ns1 ns2 ns3
>>  
>>  setup_hsr_interfaces 1
>>  do_complete_ping_test
>>  
>> +# Run VLAN Test
>> +if $vlan; then
>> +	setup_vlan_interfaces
>> +	hsr_vlan_ping
>> +fi
> 
> The new tests should be enabled by default. Indeed ideally the test
> script should be able to run successfully on kernel not supporting such
> feature; you could cope with that looking for the hsr exposed feature
> and skipping the vlan test when the relevant feature is not present.
> 

Sure Paolo, I will make the new tests enabled by default. During the
test I will check the exposed feature `ethtool -k hsr1 | grep
"vlan-challenged"` and if vlan is not supported, skip the vlan test.

Below will be my API to run VLAN tests,

run_vlan_tests() {
	vlan_challenged_hsr1=$(ip net exec "$ns1" ethtool -k hsr1 | grep
"vlan-challenged" | awk '{print $2}')
	vlan_challenged_hsr2=$(ip net exec "$ns1" ethtool -k hsr2 | grep
"vlan-challenged" | awk '{print $2}')
	vlan_challenged_hsr3=$(ip net exec "$ns1" ethtool -k hsr3 | grep
"vlan-challenged" | awk '{print $2}')

	if [[ "$vlan_challenged_hsr1" = "off" || "$vlan_challenged_hsr2" =
"off" || "$vlan_challenged_hsr3" = "off" ]]; then
		setup_vlan_interfaces
		hsr_vlan_ping
	else
		echo "INFO: Not Running VLAN tests as the device does not support VLAN"
	fi
}

I will call this function after the ping test.
Let me know if this looks okay to you.

Thanks for the review.

> Cheers,
> 
> Paolo
> 

-- 
Thanks and Regards,
Danish