* [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
@ 2020-09-01 19:54 Murali Karicheri
2020-09-01 19:54 ` [PATCH net-next 1/1] net: hsr/prp: add vlan support Murali Karicheri
2020-09-02 16:14 ` [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
0 siblings, 2 replies; 12+ messages in thread
From: Murali Karicheri @ 2020-09-01 19:54 UTC (permalink / raw)
To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko
This series add support for creating VLAN interface over HSR or
PRP interface. Typically industrial networks uses VLAN in
deployment and this capability is needed to support these
networks.
This is tested using two TI AM572x IDK boards connected back
to back over CPSW ports (eth0 and eth1).
Following is the setup
Physical Setup
++++++++++++++
_______________ (CPSW) _______________
| |----eth0-----| |
|TI AM572x IDK1| | TI AM572x IDK2|
|______________|----eth1-----|_______________|
Network Topolgy
+++++++++++++++
TI AM571x IDK TI AM572x IDK
192.168.100.10 CPSW ports 192.168.100.20
IDK-1 IDK-2
hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
|----hsr0/prp0--| |---hsr0/prp0--|
hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
192.168.101.10 192.168.101.20
Following tests:-
- create hsr or prp interface and ping the interface IP address
and verify ping is successful.
- Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
101). Ping between the IP address of the VLAN interfaces
- Do iperf UDP traffic test with server on one IDK and client on the
other. Do this using 100 and 101 subnet IP addresses
- Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
and received at these interfaces.
- Delete the vlan and hsr/prp interface and verify interfaces are
removed cleanly.
Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
Murali Karicheri (1):
net: hsr/prp: add vlan support
net/hsr/hsr_device.c | 4 ----
net/hsr/hsr_forward.c | 16 +++++++++++++---
2 files changed, 13 insertions(+), 7 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net-next 1/1] net: hsr/prp: add vlan support
2020-09-01 19:54 [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
@ 2020-09-01 19:54 ` Murali Karicheri
2020-09-04 15:45 ` Willem de Bruijn
2020-09-02 16:14 ` [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
1 sibling, 1 reply; 12+ messages in thread
From: Murali Karicheri @ 2020-09-01 19:54 UTC (permalink / raw)
To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko
This patch add support for creating vlan interfaces
over hsr/prp interface.
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
---
net/hsr/hsr_device.c | 4 ----
net/hsr/hsr_forward.c | 16 +++++++++++++---
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
index ab953a1a0d6c..e1951579a3ad 100644
--- a/net/hsr/hsr_device.c
+++ b/net/hsr/hsr_device.c
@@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
/* Prevent recursive tx locking */
dev->features |= NETIF_F_LLTX;
- /* VLAN on top of HSR needs testing and probably some work on
- * hsr_header_create() etc.
- */
- dev->features |= NETIF_F_VLAN_CHALLENGED;
/* Not sure about this. Taken from bridge code. netdev_features.h says
* it means "Does not change network namespaces".
*/
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index cadfccd7876e..de21df30b0d9 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -208,6 +208,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) */
@@ -218,7 +219,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);
@@ -511,8 +523,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");
}
frame->is_from_san = false;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-01 19:54 [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
2020-09-01 19:54 ` [PATCH net-next 1/1] net: hsr/prp: add vlan support Murali Karicheri
@ 2020-09-02 16:14 ` Murali Karicheri
2020-09-02 22:29 ` Murali Karicheri
1 sibling, 1 reply; 12+ messages in thread
From: Murali Karicheri @ 2020-09-02 16:14 UTC (permalink / raw)
To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko
All,
On 9/1/20 3:54 PM, Murali Karicheri wrote:
> This series add support for creating VLAN interface over HSR or
> PRP interface. Typically industrial networks uses VLAN in
> deployment and this capability is needed to support these
> networks.
>
> This is tested using two TI AM572x IDK boards connected back
> to back over CPSW ports (eth0 and eth1).
>
> Following is the setup
>
> Physical Setup
> ++++++++++++++
>
> _______________ (CPSW) _______________
> | |----eth0-----| |
> |TI AM572x IDK1| | TI AM572x IDK2|
> |______________|----eth1-----|_______________|
>
>
> Network Topolgy
> +++++++++++++++
>
> TI AM571x IDK TI AM572x IDK
>
>
> 192.168.100.10 CPSW ports 192.168.100.20
> IDK-1 IDK-2
> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
> |----hsr0/prp0--| |---hsr0/prp0--|
> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
>
> 192.168.101.10 192.168.101.20
>
> Following tests:-
> - create hsr or prp interface and ping the interface IP address
> and verify ping is successful.
> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
> 101). Ping between the IP address of the VLAN interfaces
> - Do iperf UDP traffic test with server on one IDK and client on the
> other. Do this using 100 and 101 subnet IP addresses
> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
> and received at these interfaces.
> - Delete the vlan and hsr/prp interface and verify interfaces are
> removed cleanly.
>
> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
>
> Murali Karicheri (1):
> net: hsr/prp: add vlan support
>
> net/hsr/hsr_device.c | 4 ----
> net/hsr/hsr_forward.c | 16 +++++++++++++---
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
I am not sure if the packet flow is right for this?
VLAN over HSR frame format is like this.
<Start of Frame><VLAN tag><HSR Tag><IP><CRC>
My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
frames.
So I did a WARN_ON() in HSR driver before frame is forwarded to upper
layer.
a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index de21df30b0d9..545a3cd8c71b 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
*frame)
}
skb->dev = port->dev;
- if (port->type == HSR_PT_MASTER)
+ if (port->type == HSR_PT_MASTER) {
+ if (skb_vlan_tag_present(skb))
+ WARN_ON(1);
hsr_deliver_master(skb, port->dev,
frame->node_src);
- else
+ } else
hsr_xmit(skb, port, frame);
}
}
And I get the trace shown below.
[ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
hsr_forward_skb+0x460/0x564
[ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
[ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
5.9.0-rc1-00658-g473e463812c2-dirty #8
[ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
(show_stack+0x10/0x14)
[ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
(dump_stack+0xc4/0xe4)
[ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
(__warn+0xc0/0xf4)
[ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
(warn_slowpath_fmt+0x58/0xb8)
[ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
(hsr_forward_skb+0x460/0x564)
[ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
(hsr_handle_frame+0x15c/0x190)
[ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
(__netif_receive_skb_core+0x23c/0xc88)
[ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
(__netif_receive_skb_one_core+0x30/0x74)
[ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
[<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
[ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
(cpsw_rx_handler+0x230/0x308)
[ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
(__cpdma_chan_process+0xf4/0x188)
[ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
(cpdma_chan_process+0x3c/0x5c)
[ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
(cpsw_rx_mq_poll+0x44/0x98)
[ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
(net_rx_action+0xf0/0x400)
[ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
(__do_softirq+0xf0/0x3ac)
[ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
(irq_exit+0xa8/0xe4)
[ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
(__handle_domain_irq+0x6c/0xe0)
[ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
(gic_handle_irq+0x4c/0xa8)
[ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
(__irq_svc+0x6c/0x90)
[ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
Shouldn't it show vlan_do_receive() ?
if (skb_vlan_tag_present(skb)) {
if (pt_prev) {
ret = deliver_skb(skb, pt_prev, orig_dev);
pt_prev = NULL;
}
if (vlan_do_receive(&skb))
goto another_round;
else if (unlikely(!skb))
goto out;
}
Thanks
--
Murali Karicheri
Texas Instruments
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-02 16:14 ` [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
@ 2020-09-02 22:29 ` Murali Karicheri
2020-09-04 15:52 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Murali Karicheri @ 2020-09-02 22:29 UTC (permalink / raw)
To: davem, kuba, netdev, linux-kernel, nsekhar, grygorii.strashko
All,
On 9/2/20 12:14 PM, Murali Karicheri wrote:
> All,
>
> On 9/1/20 3:54 PM, Murali Karicheri wrote:
>> This series add support for creating VLAN interface over HSR or
>> PRP interface. Typically industrial networks uses VLAN in
>> deployment and this capability is needed to support these
>> networks.
>>
>> This is tested using two TI AM572x IDK boards connected back
>> to back over CPSW ports (eth0 and eth1).
>>
>> Following is the setup
>>
>> Physical Setup
>> ++++++++++++++
>> _______________ (CPSW) _______________
>> | |----eth0-----| |
>> |TI AM572x IDK1| | TI AM572x IDK2|
>> |______________|----eth1-----|_______________|
>>
>>
>> Network Topolgy
>> +++++++++++++++
>>
>> TI AM571x IDK TI AM572x IDK
>>
>> 192.168.100.10 CPSW ports 192.168.100.20
>> IDK-1 IDK-2
>> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
>> |----hsr0/prp0--| |---hsr0/prp0--|
>> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
>>
>> 192.168.101.10 192.168.101.20
>>
>> Following tests:-
>> - create hsr or prp interface and ping the interface IP address
>> and verify ping is successful.
>> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
>> 101). Ping between the IP address of the VLAN interfaces
>> - Do iperf UDP traffic test with server on one IDK and client on the
>> other. Do this using 100 and 101 subnet IP addresses
>> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
>> and received at these interfaces.
>> - Delete the vlan and hsr/prp interface and verify interfaces are
>> removed cleanly.
>>
>> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
>> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
>>
>> Murali Karicheri (1):
>> net: hsr/prp: add vlan support
>>
>> net/hsr/hsr_device.c | 4 ----
>> net/hsr/hsr_forward.c | 16 +++++++++++++---
>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>
> I am not sure if the packet flow is right for this?
>
> VLAN over HSR frame format is like this.
>
> <Start of Frame><VLAN tag><HSR Tag><IP><CRC>
>
> My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
> frames.
>
> So I did a WARN_ON() in HSR driver before frame is forwarded to upper
> layer.
>
> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index de21df30b0d9..545a3cd8c71b 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
> *frame)
> }
>
> skb->dev = port->dev;
> - if (port->type == HSR_PT_MASTER)
> + if (port->type == HSR_PT_MASTER) {
> + if (skb_vlan_tag_present(skb))
> + WARN_ON(1);
> hsr_deliver_master(skb, port->dev,
> frame->node_src);
> - else
> + } else
> hsr_xmit(skb, port, frame);
> }
> }
>
> And I get the trace shown below.
>
> [ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
> hsr_forward_skb+0x460/0x564
> [ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
> snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
> [ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
> 5.9.0-rc1-00658-g473e463812c2-dirty #8
> [ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
> [ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
> (show_stack+0x10/0x14)
> [ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
> (dump_stack+0xc4/0xe4)
> [ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
> (__warn+0xc0/0xf4)
> [ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
> (warn_slowpath_fmt+0x58/0xb8)
> [ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
> (hsr_forward_skb+0x460/0x564)
> [ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
> (hsr_handle_frame+0x15c/0x190)
> [ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
> (__netif_receive_skb_core+0x23c/0xc88)
> [ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
> (__netif_receive_skb_one_core+0x30/0x74)
> [ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
> [<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
> [ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
> (cpsw_rx_handler+0x230/0x308)
> [ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
> (__cpdma_chan_process+0xf4/0x188)
> [ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
> (cpdma_chan_process+0x3c/0x5c)
> [ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
> (cpsw_rx_mq_poll+0x44/0x98)
> [ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
> (net_rx_action+0xf0/0x400)
> [ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
> (__do_softirq+0xf0/0x3ac)
> [ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
> (irq_exit+0xa8/0xe4)
> [ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
> (__handle_domain_irq+0x6c/0xe0)
> [ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
> (gic_handle_irq+0x4c/0xa8)
> [ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
> (__irq_svc+0x6c/0x90)
> [ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
>
> Shouldn't it show vlan_do_receive() ?
>
> if (skb_vlan_tag_present(skb)) {
> if (pt_prev) {
> ret = deliver_skb(skb, pt_prev, orig_dev);
> pt_prev = NULL;
> }
> if (vlan_do_receive(&skb))
> goto another_round;
> else if (unlikely(!skb))
> goto out;
> }
>
> Thanks
>
I did an ftrace today and I find vlan_do_receive() is called for the
incoming frames before passing SKB to hsr_handle_frame(). If someone
can review this, it will help. Thanks.
https://pastebin.ubuntu.com/p/CbRzXjwjR5/
--
Murali Karicheri
Texas Instruments
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support
2020-09-01 19:54 ` [PATCH net-next 1/1] net: hsr/prp: add vlan support Murali Karicheri
@ 2020-09-04 15:45 ` Willem de Bruijn
2020-09-08 16:38 ` Murali Karicheri
0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2020-09-04 15:45 UTC (permalink / raw)
To: Murali Karicheri
Cc: David Miller, Jakub Kicinski, Network Development, linux-kernel,
nsekhar, Grygorii Strashko
On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> This patch add support for creating vlan interfaces
> over hsr/prp interface.
>
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> ---
> net/hsr/hsr_device.c | 4 ----
> net/hsr/hsr_forward.c | 16 +++++++++++++---
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index ab953a1a0d6c..e1951579a3ad 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
>
> /* Prevent recursive tx locking */
> dev->features |= NETIF_F_LLTX;
> - /* VLAN on top of HSR needs testing and probably some work on
> - * hsr_header_create() etc.
> - */
> - dev->features |= NETIF_F_VLAN_CHALLENGED;
> /* Not sure about this. Taken from bridge code. netdev_features.h says
> * it means "Does not change network namespaces".
> */
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index cadfccd7876e..de21df30b0d9 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -208,6 +208,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) */
> @@ -218,7 +219,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;
Instead, I would pass the header from the caller, which knows the
offset because it moves the previous headers to make space.
Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
which means code should parse the headers instead of hardcoding
VLAN_HLEN.
> hsr_set_path_id(hsr_ethhdr, port);
> set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
> @@ -511,8 +523,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");
> }
>
> frame->is_from_san = false;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-02 22:29 ` Murali Karicheri
@ 2020-09-04 15:52 ` Willem de Bruijn
2020-09-08 16:55 ` Murali Karicheri
0 siblings, 1 reply; 12+ messages in thread
From: Willem de Bruijn @ 2020-09-04 15:52 UTC (permalink / raw)
To: Murali Karicheri
Cc: David Miller, Jakub Kicinski, Network Development, linux-kernel,
nsekhar, Grygorii Strashko
On Thu, Sep 3, 2020 at 12:30 AM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> All,
>
> On 9/2/20 12:14 PM, Murali Karicheri wrote:
> > All,
> >
> > On 9/1/20 3:54 PM, Murali Karicheri wrote:
> >> This series add support for creating VLAN interface over HSR or
> >> PRP interface. Typically industrial networks uses VLAN in
> >> deployment and this capability is needed to support these
> >> networks.
> >>
> >> This is tested using two TI AM572x IDK boards connected back
> >> to back over CPSW ports (eth0 and eth1).
> >>
> >> Following is the setup
> >>
> >> Physical Setup
> >> ++++++++++++++
> >> _______________ (CPSW) _______________
> >> | |----eth0-----| |
> >> |TI AM572x IDK1| | TI AM572x IDK2|
> >> |______________|----eth1-----|_______________|
> >>
> >>
> >> Network Topolgy
> >> +++++++++++++++
> >>
> >> TI AM571x IDK TI AM572x IDK
> >>
> >> 192.168.100.10 CPSW ports 192.168.100.20
> >> IDK-1 IDK-2
> >> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
> >> |----hsr0/prp0--| |---hsr0/prp0--|
> >> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
> >>
> >> 192.168.101.10 192.168.101.20
> >>
> >> Following tests:-
> >> - create hsr or prp interface and ping the interface IP address
> >> and verify ping is successful.
> >> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
> >> 101). Ping between the IP address of the VLAN interfaces
> >> - Do iperf UDP traffic test with server on one IDK and client on the
> >> other. Do this using 100 and 101 subnet IP addresses
> >> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
> >> and received at these interfaces.
> >> - Delete the vlan and hsr/prp interface and verify interfaces are
> >> removed cleanly.
> >>
> >> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
> >> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
> >>
> >> Murali Karicheri (1):
> >> net: hsr/prp: add vlan support
> >>
> >> net/hsr/hsr_device.c | 4 ----
> >> net/hsr/hsr_forward.c | 16 +++++++++++++---
> >> 2 files changed, 13 insertions(+), 7 deletions(-)
> >>
> > I am not sure if the packet flow is right for this?
> >
> > VLAN over HSR frame format is like this.
> >
> > <Start of Frame><VLAN tag><HSR Tag><IP><CRC>
> >
> > My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
> > frames.
> >
> > So I did a WARN_ON() in HSR driver before frame is forwarded to upper
> > layer.
> >
> > a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index de21df30b0d9..545a3cd8c71b 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> > @@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
> > *frame)
> > }
> >
> > skb->dev = port->dev;
> > - if (port->type == HSR_PT_MASTER)
> > + if (port->type == HSR_PT_MASTER) {
> > + if (skb_vlan_tag_present(skb))
> > + WARN_ON(1);
> > hsr_deliver_master(skb, port->dev,
> > frame->node_src);
> > - else
> > + } else
> > hsr_xmit(skb, port, frame);
> > }
> > }
> >
> > And I get the trace shown below.
> >
> > [ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
> > hsr_forward_skb+0x460/0x564
> > [ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
> > snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
> > [ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
> > 5.9.0-rc1-00658-g473e463812c2-dirty #8
> > [ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
> > [ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
> > (show_stack+0x10/0x14)
> > [ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
> > (dump_stack+0xc4/0xe4)
> > [ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
> > (__warn+0xc0/0xf4)
> > [ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
> > (warn_slowpath_fmt+0x58/0xb8)
> > [ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
> > (hsr_forward_skb+0x460/0x564)
> > [ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
> > (hsr_handle_frame+0x15c/0x190)
> > [ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
> > (__netif_receive_skb_core+0x23c/0xc88)
> > [ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
> > (__netif_receive_skb_one_core+0x30/0x74)
> > [ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
> > [<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
> > [ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
> > (cpsw_rx_handler+0x230/0x308)
> > [ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
> > (__cpdma_chan_process+0xf4/0x188)
> > [ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
> > (cpdma_chan_process+0x3c/0x5c)
> > [ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
> > (cpsw_rx_mq_poll+0x44/0x98)
> > [ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
> > (net_rx_action+0xf0/0x400)
> > [ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
> > (__do_softirq+0xf0/0x3ac)
> > [ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
> > (irq_exit+0xa8/0xe4)
> > [ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
> > (__handle_domain_irq+0x6c/0xe0)
> > [ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
> > (gic_handle_irq+0x4c/0xa8)
> > [ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
> > (__irq_svc+0x6c/0x90)
> > [ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
> >
> > Shouldn't it show vlan_do_receive() ?
> >
> > if (skb_vlan_tag_present(skb)) {
> > if (pt_prev) {
> > ret = deliver_skb(skb, pt_prev, orig_dev);
> > pt_prev = NULL;
> > }
> > if (vlan_do_receive(&skb))
> > goto another_round;
> > else if (unlikely(!skb))
> > goto out;
> > }
> >
> > Thanks
> >
>
> I did an ftrace today and I find vlan_do_receive() is called for the
> incoming frames before passing SKB to hsr_handle_frame(). If someone
> can review this, it will help. Thanks.
>
> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
hsr_handle_frame is an rx_handler called after
__netif_receive_skb_core called vlan_do_receive and jumped back to
another_round.
That's how it should work right?
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support
2020-09-04 15:45 ` Willem de Bruijn
@ 2020-09-08 16:38 ` Murali Karicheri
2020-09-08 17:34 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Murali Karicheri @ 2020-09-08 16:38 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David Miller, Jakub Kicinski, Network Development, linux-kernel,
nsekhar, Grygorii Strashko
Hi Willem,
Thanks for the response!
On 9/4/20 11:45 AM, Willem de Bruijn wrote:
> On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>>
>> This patch add support for creating vlan interfaces
>> over hsr/prp interface.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> ---
>> net/hsr/hsr_device.c | 4 ----
>> net/hsr/hsr_forward.c | 16 +++++++++++++---
>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
>> index ab953a1a0d6c..e1951579a3ad 100644
>> --- a/net/hsr/hsr_device.c
>> +++ b/net/hsr/hsr_device.c
>> @@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
>>
>> /* Prevent recursive tx locking */
>> dev->features |= NETIF_F_LLTX;
>> - /* VLAN on top of HSR needs testing and probably some work on
>> - * hsr_header_create() etc.
>> - */
>> - dev->features |= NETIF_F_VLAN_CHALLENGED;
>> /* Not sure about this. Taken from bridge code. netdev_features.h says
>> * it means "Does not change network namespaces".
>> */
>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>> index cadfccd7876e..de21df30b0d9 100644
>> --- a/net/hsr/hsr_forward.c
>> +++ b/net/hsr/hsr_forward.c
>> @@ -208,6 +208,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) */
>> @@ -218,7 +219,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;
>
> Instead, I would pass the header from the caller, which knows the
> offset because it moves the previous headers to make space.
>
So if I understood you correctly a diff for the above would like this
where pass dst + movelen as struct hsr_ethhdr *hsr_ethhdr
pointer to hsr_fill_tag(), right?
a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
index de21df30b0d9..4d9192c8bcf8 100644
--- a/net/hsr/hsr_forward.c
+++ b/net/hsr/hsr_forward.c
@@ -204,11 +204,10 @@ static void hsr_set_path_id(struct hsr_ethhdr
*hsr_ethhdr,
}
static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
+ struct hsr_ethhdr *hsr_ethhdr,
struct hsr_frame_info *frame,
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) */
@@ -219,19 +218,6 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff
*skb,
if (frame->is_vlan)
lsdu_size -= 4;
- 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);
hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
@@ -280,10 +266,12 @@ struct sk_buff *hsr_create_tagged_frame(struct
hsr_frame_info *frame,
memmove(dst, src, movelen);
skb_reset_mac_header(skb);
- /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
- * that case
+ /* dst point to the start of hsr tag. So pass it to fill the
+ * hsr info. Also skb_put_padto free skb on error and hsr_fill_tag
+ * returns NULL in that case.
*/
- return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
+ return hsr_fill_tag(skb, (struct hsr_ethhdr *)(dst + movelen),
+ frame, port, port->hsr->prot_version);
}
> Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
> which means code should parse the headers instead of hardcoding
> VLAN_HLEN.
>
iec-62439-3 standard only talks about VLAN (TPID 0x8100), not about
QinQ. So what I could do is to check and bail out if 802.1ad frame is
received at the interface from upper layer. Something like below and
frame will get dropped.
@@ -519,6 +507,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
if (proto == htons(ETH_P_8021Q))
frame->is_vlan = true;
+ else if (proto == htons(ETH_P_8021AD))
+ return -1; /* Don't support 802.1ad */
if (frame->is_vlan) {
vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
What do you think?
Thanks.
>> hsr_set_path_id(hsr_ethhdr, port);
>> set_hsr_tag_LSDU_size(&hsr_ethhdr->hsr_tag, lsdu_size);
>> @@ -511,8 +523,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");
>> }
>>
>> frame->is_from_san = false;
>> --
>> 2.17.1
>>
--
Murali Karicheri
Texas Instruments
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-04 15:52 ` Willem de Bruijn
@ 2020-09-08 16:55 ` Murali Karicheri
2020-09-08 17:51 ` Willem de Bruijn
0 siblings, 1 reply; 12+ messages in thread
From: Murali Karicheri @ 2020-09-08 16:55 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David Miller, Jakub Kicinski, Network Development, linux-kernel,
nsekhar, Grygorii Strashko
Hi Willem,
On 9/4/20 11:52 AM, Willem de Bruijn wrote:
> On Thu, Sep 3, 2020 at 12:30 AM Murali Karicheri <m-karicheri2@ti.com> wrote:
>>
>> All,
>>
>> On 9/2/20 12:14 PM, Murali Karicheri wrote:
>>> All,
>>>
>>> On 9/1/20 3:54 PM, Murali Karicheri wrote:
>>>> This series add support for creating VLAN interface over HSR or
>>>> PRP interface. Typically industrial networks uses VLAN in
>>>> deployment and this capability is needed to support these
>>>> networks.
>>>>
>>>> This is tested using two TI AM572x IDK boards connected back
>>>> to back over CPSW ports (eth0 and eth1).
>>>>
>>>> Following is the setup
>>>>
>>>> Physical Setup
>>>> ++++++++++++++
>>>> _______________ (CPSW) _______________
>>>> | |----eth0-----| |
>>>> |TI AM572x IDK1| | TI AM572x IDK2|
>>>> |______________|----eth1-----|_______________|
>>>>
>>>>
>>>> Network Topolgy
>>>> +++++++++++++++
>>>>
>>>> TI AM571x IDK TI AM572x IDK
>>>>
>>>> 192.168.100.10 CPSW ports 192.168.100.20
>>>> IDK-1 IDK-2
>>>> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
>>>> |----hsr0/prp0--| |---hsr0/prp0--|
>>>> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
>>>>
>>>> 192.168.101.10 192.168.101.20
>>>>
>>>> Following tests:-
>>>> - create hsr or prp interface and ping the interface IP address
>>>> and verify ping is successful.
>>>> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
>>>> 101). Ping between the IP address of the VLAN interfaces
>>>> - Do iperf UDP traffic test with server on one IDK and client on the
>>>> other. Do this using 100 and 101 subnet IP addresses
>>>> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
>>>> and received at these interfaces.
>>>> - Delete the vlan and hsr/prp interface and verify interfaces are
>>>> removed cleanly.
>>>>
>>>> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
>>>> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
>>>>
>>>> Murali Karicheri (1):
>>>> net: hsr/prp: add vlan support
>>>>
>>>> net/hsr/hsr_device.c | 4 ----
>>>> net/hsr/hsr_forward.c | 16 +++++++++++++---
>>>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>>>
>>> I am not sure if the packet flow is right for this?
>>>
>>> VLAN over HSR frame format is like this.
>>>
>>> <Start of Frame><VLAN tag><HSR Tag><IP><CRC>
>>>
>>> My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
>>> frames.
>>>
>>> So I did a WARN_ON() in HSR driver before frame is forwarded to upper
>>> layer.
>>>
>>> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
>>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>>> index de21df30b0d9..545a3cd8c71b 100644
>>> --- a/net/hsr/hsr_forward.c
>>> +++ b/net/hsr/hsr_forward.c
>>> @@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
>>> *frame)
>>> }
>>>
>>> skb->dev = port->dev;
>>> - if (port->type == HSR_PT_MASTER)
>>> + if (port->type == HSR_PT_MASTER) {
>>> + if (skb_vlan_tag_present(skb))
>>> + WARN_ON(1);
>>> hsr_deliver_master(skb, port->dev,
>>> frame->node_src);
>>> - else
>>> + } else
>>> hsr_xmit(skb, port, frame);
>>> }
>>> }
>>>
>>> And I get the trace shown below.
>>>
>>> [ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
>>> hsr_forward_skb+0x460/0x564
>>> [ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
>>> snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
>>> [ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
>>> 5.9.0-rc1-00658-g473e463812c2-dirty #8
>>> [ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
>>> [ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
>>> (show_stack+0x10/0x14)
>>> [ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
>>> (dump_stack+0xc4/0xe4)
>>> [ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
>>> (__warn+0xc0/0xf4)
>>> [ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
>>> (warn_slowpath_fmt+0x58/0xb8)
>>> [ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
>>> (hsr_forward_skb+0x460/0x564)
>>> [ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
>>> (hsr_handle_frame+0x15c/0x190)
>>> [ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
>>> (__netif_receive_skb_core+0x23c/0xc88)
>>> [ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
>>> (__netif_receive_skb_one_core+0x30/0x74)
>>> [ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
>>> [<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
>>> [ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
>>> (cpsw_rx_handler+0x230/0x308)
>>> [ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
>>> (__cpdma_chan_process+0xf4/0x188)
>>> [ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
>>> (cpdma_chan_process+0x3c/0x5c)
>>> [ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
>>> (cpsw_rx_mq_poll+0x44/0x98)
>>> [ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
>>> (net_rx_action+0xf0/0x400)
>>> [ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
>>> (__do_softirq+0xf0/0x3ac)
>>> [ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
>>> (irq_exit+0xa8/0xe4)
>>> [ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
>>> (__handle_domain_irq+0x6c/0xe0)
>>> [ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
>>> (gic_handle_irq+0x4c/0xa8)
>>> [ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
>>> (__irq_svc+0x6c/0x90)
>>> [ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
>>>
>>> Shouldn't it show vlan_do_receive() ?
>>>
>>> if (skb_vlan_tag_present(skb)) {
>>> if (pt_prev) {
>>> ret = deliver_skb(skb, pt_prev, orig_dev);
>>> pt_prev = NULL;
>>> }
>>> if (vlan_do_receive(&skb))
>>> goto another_round;
>>> else if (unlikely(!skb))
>>> goto out;
>>> }
>>>
>>> Thanks
>>>
>>
>> I did an ftrace today and I find vlan_do_receive() is called for the
>> incoming frames before passing SKB to hsr_handle_frame(). If someone
>> can review this, it will help. Thanks.
>>
>> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
>
> hsr_handle_frame is an rx_handler called after
> __netif_receive_skb_core called vlan_do_receive and jumped back to
> another_round.
Yes. hsr_handle_frame() is a rx_handler() after the above code that
does vlan_do_receive(). The ftrace shows vlan_do_receive() is called
followed by call to hsr_handle_frame(). From ifconfig I can see both
hsr and vlan interface stats increments by same count. So I assume,
vlan_do_receive() is called initially and it removes the tag, update
stats and then return true and go for another round. Do you think that
is the case?
vlan_do_receive() calls vlan_find_dev(skb->dev, vlan_proto, vlan_id)
to retrieve the real netdevice (real device). However VLAN device is
attached to hsr device (real device), but SKB will have HSR slave
Ethernet netdevice (in our case it is cpsw device) and vlan_find_dev()
would have failed since there is no vlan_info in cpsw netdev struct. So
below code in vlan_do_receive() should have failed and return false.
vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
if (!vlan_dev)
return false;
So how does it goes for another_round ? May be vlan_find_dev is
finding the hsr netdevice?
I am not an expert and so the question. Probably I can put a
traceprintk() to confirm this, but if someone can clarify this
it will be great. But for that, I will spin v2 with the above comments
addressed as in my reply and post.
Thanks
>
> That's how it should work right?
>
--
Murali Karicheri
Texas Instruments
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 1/1] net: hsr/prp: add vlan support
2020-09-08 16:38 ` Murali Karicheri
@ 2020-09-08 17:34 ` Willem de Bruijn
0 siblings, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2020-09-08 17:34 UTC (permalink / raw)
To: Murali Karicheri
Cc: Willem de Bruijn, David Miller, Jakub Kicinski,
Network Development, linux-kernel, nsekhar, Grygorii Strashko
On Tue, Sep 8, 2020 at 6:39 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> Hi Willem,
>
> Thanks for the response!
> On 9/4/20 11:45 AM, Willem de Bruijn wrote:
> > On Tue, Sep 1, 2020 at 9:54 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
> >>
> >> This patch add support for creating vlan interfaces
> >> over hsr/prp interface.
> >>
> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> >> ---
> >> net/hsr/hsr_device.c | 4 ----
> >> net/hsr/hsr_forward.c | 16 +++++++++++++---
> >> 2 files changed, 13 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> >> index ab953a1a0d6c..e1951579a3ad 100644
> >> --- a/net/hsr/hsr_device.c
> >> +++ b/net/hsr/hsr_device.c
> >> @@ -477,10 +477,6 @@ void hsr_dev_setup(struct net_device *dev)
> >>
> >> /* Prevent recursive tx locking */
> >> dev->features |= NETIF_F_LLTX;
> >> - /* VLAN on top of HSR needs testing and probably some work on
> >> - * hsr_header_create() etc.
> >> - */
> >> - dev->features |= NETIF_F_VLAN_CHALLENGED;
> >> /* Not sure about this. Taken from bridge code. netdev_features.h says
> >> * it means "Does not change network namespaces".
> >> */
> >> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> >> index cadfccd7876e..de21df30b0d9 100644
> >> --- a/net/hsr/hsr_forward.c
> >> +++ b/net/hsr/hsr_forward.c
> >> @@ -208,6 +208,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) */
> >> @@ -218,7 +219,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;
> >
> > Instead, I would pass the header from the caller, which knows the
> > offset because it moves the previous headers to make space.
> >
> So if I understood you correctly a diff for the above would like this
> where pass dst + movelen as struct hsr_ethhdr *hsr_ethhdr
> pointer to hsr_fill_tag(), right?
>
> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index de21df30b0d9..4d9192c8bcf8 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -204,11 +204,10 @@ static void hsr_set_path_id(struct hsr_ethhdr
> *hsr_ethhdr,
> }
>
> static struct sk_buff *hsr_fill_tag(struct sk_buff *skb,
> + struct hsr_ethhdr *hsr_ethhdr,
> struct hsr_frame_info *frame,
> 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) */
> @@ -219,19 +218,6 @@ static struct sk_buff *hsr_fill_tag(struct sk_buff
> *skb,
> if (frame->is_vlan)
> lsdu_size -= 4;
>
> - 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);
> hsr_ethhdr->hsr_tag.sequence_nr = htons(frame->sequence_nr);
> @@ -280,10 +266,12 @@ struct sk_buff *hsr_create_tagged_frame(struct
> hsr_frame_info *frame,
> memmove(dst, src, movelen);
> skb_reset_mac_header(skb);
>
> - /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
> - * that case
> + /* dst point to the start of hsr tag. So pass it to fill the
> + * hsr info. Also skb_put_padto free skb on error and hsr_fill_tag
> + * returns NULL in that case.
> */
> - return hsr_fill_tag(skb, frame, port, port->hsr->prot_version);
> + return hsr_fill_tag(skb, (struct hsr_ethhdr *)(dst + movelen),
> + frame, port, port->hsr->prot_version);
It's a bit hard to see, since this is a draft patch on top of the
existing patch. But I think so.
Only, instead of dst + movelen, you can use src.
> }
>
>
> > Also, supporting VLAN probably also requires supporting 802.1ad QinQ,
> > which means code should parse the headers instead of hardcoding
> > VLAN_HLEN.
> >
>
> iec-62439-3 standard only talks about VLAN (TPID 0x8100), not about
> QinQ. So what I could do is to check and bail out if 802.1ad frame is
> received at the interface from upper layer. Something like below and
> frame will get dropped.
>
> @@ -519,6 +507,8 @@ static int fill_frame_info(struct hsr_frame_info *frame,
>
> if (proto == htons(ETH_P_8021Q))
> frame->is_vlan = true;
> + else if (proto == htons(ETH_P_8021AD))
> + return -1; /* Don't support 802.1ad */
>
> if (frame->is_vlan) {
> vlan_hdr = (struct hsr_vlan_ethhdr *)ethhdr;
>
> What do you think?
It's good to err on the safe side. That should probably be a separate patch.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-08 16:55 ` Murali Karicheri
@ 2020-09-08 17:51 ` Willem de Bruijn
2020-09-08 18:50 ` Willem de Bruijn
2020-09-09 16:08 ` Murali Karicheri
0 siblings, 2 replies; 12+ messages in thread
From: Willem de Bruijn @ 2020-09-08 17:51 UTC (permalink / raw)
To: Murali Karicheri
Cc: Willem de Bruijn, David Miller, Jakub Kicinski,
Network Development, linux-kernel, nsekhar, Grygorii Strashko
On Tue, Sep 8, 2020 at 6:55 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>
> Hi Willem,
>
> On 9/4/20 11:52 AM, Willem de Bruijn wrote:
> > On Thu, Sep 3, 2020 at 12:30 AM Murali Karicheri <m-karicheri2@ti.com> wrote:
> >>
> >> All,
> >>
> >> On 9/2/20 12:14 PM, Murali Karicheri wrote:
> >>> All,
> >>>
> >>> On 9/1/20 3:54 PM, Murali Karicheri wrote:
> >>>> This series add support for creating VLAN interface over HSR or
> >>>> PRP interface. Typically industrial networks uses VLAN in
> >>>> deployment and this capability is needed to support these
> >>>> networks.
> >>>>
> >>>> This is tested using two TI AM572x IDK boards connected back
> >>>> to back over CPSW ports (eth0 and eth1).
> >>>>
> >>>> Following is the setup
> >>>>
> >>>> Physical Setup
> >>>> ++++++++++++++
> >>>> _______________ (CPSW) _______________
> >>>> | |----eth0-----| |
> >>>> |TI AM572x IDK1| | TI AM572x IDK2|
> >>>> |______________|----eth1-----|_______________|
> >>>>
> >>>>
> >>>> Network Topolgy
> >>>> +++++++++++++++
> >>>>
> >>>> TI AM571x IDK TI AM572x IDK
> >>>>
> >>>> 192.168.100.10 CPSW ports 192.168.100.20
> >>>> IDK-1 IDK-2
> >>>> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
> >>>> |----hsr0/prp0--| |---hsr0/prp0--|
> >>>> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
> >>>>
> >>>> 192.168.101.10 192.168.101.20
> >>>>
> >>>> Following tests:-
> >>>> - create hsr or prp interface and ping the interface IP address
> >>>> and verify ping is successful.
> >>>> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
> >>>> 101). Ping between the IP address of the VLAN interfaces
> >>>> - Do iperf UDP traffic test with server on one IDK and client on the
> >>>> other. Do this using 100 and 101 subnet IP addresses
> >>>> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
> >>>> and received at these interfaces.
> >>>> - Delete the vlan and hsr/prp interface and verify interfaces are
> >>>> removed cleanly.
> >>>>
> >>>> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
> >>>> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
> >>>>
> >>>> Murali Karicheri (1):
> >>>> net: hsr/prp: add vlan support
> >>>>
> >>>> net/hsr/hsr_device.c | 4 ----
> >>>> net/hsr/hsr_forward.c | 16 +++++++++++++---
> >>>> 2 files changed, 13 insertions(+), 7 deletions(-)
> >>>>
> >>> I am not sure if the packet flow is right for this?
> >>>
> >>> VLAN over HSR frame format is like this.
> >>>
> >>> <Start of Frame><VLAN tag><HSR Tag><IP><CRC>
> >>>
> >>> My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
> >>> frames.
> >>>
> >>> So I did a WARN_ON() in HSR driver before frame is forwarded to upper
> >>> layer.
> >>>
> >>> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
> >>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> >>> index de21df30b0d9..545a3cd8c71b 100644
> >>> --- a/net/hsr/hsr_forward.c
> >>> +++ b/net/hsr/hsr_forward.c
> >>> @@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
> >>> *frame)
> >>> }
> >>>
> >>> skb->dev = port->dev;
> >>> - if (port->type == HSR_PT_MASTER)
> >>> + if (port->type == HSR_PT_MASTER) {
> >>> + if (skb_vlan_tag_present(skb))
> >>> + WARN_ON(1);
> >>> hsr_deliver_master(skb, port->dev,
> >>> frame->node_src);
> >>> - else
> >>> + } else
> >>> hsr_xmit(skb, port, frame);
> >>> }
> >>> }
> >>>
> >>> And I get the trace shown below.
> >>>
> >>> [ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
> >>> hsr_forward_skb+0x460/0x564
> >>> [ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
> >>> snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
> >>> [ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
> >>> 5.9.0-rc1-00658-g473e463812c2-dirty #8
> >>> [ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
> >>> [ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
> >>> (show_stack+0x10/0x14)
> >>> [ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
> >>> (dump_stack+0xc4/0xe4)
> >>> [ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
> >>> (__warn+0xc0/0xf4)
> >>> [ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
> >>> (warn_slowpath_fmt+0x58/0xb8)
> >>> [ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
> >>> (hsr_forward_skb+0x460/0x564)
> >>> [ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
> >>> (hsr_handle_frame+0x15c/0x190)
> >>> [ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
> >>> (__netif_receive_skb_core+0x23c/0xc88)
> >>> [ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
> >>> (__netif_receive_skb_one_core+0x30/0x74)
> >>> [ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
> >>> [<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
> >>> [ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
> >>> (cpsw_rx_handler+0x230/0x308)
> >>> [ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
> >>> (__cpdma_chan_process+0xf4/0x188)
> >>> [ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
> >>> (cpdma_chan_process+0x3c/0x5c)
> >>> [ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
> >>> (cpsw_rx_mq_poll+0x44/0x98)
> >>> [ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
> >>> (net_rx_action+0xf0/0x400)
> >>> [ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
> >>> (__do_softirq+0xf0/0x3ac)
> >>> [ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
> >>> (irq_exit+0xa8/0xe4)
> >>> [ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
> >>> (__handle_domain_irq+0x6c/0xe0)
> >>> [ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
> >>> (gic_handle_irq+0x4c/0xa8)
> >>> [ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
> >>> (__irq_svc+0x6c/0x90)
> >>> [ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
> >>>
> >>> Shouldn't it show vlan_do_receive() ?
> >>>
> >>> if (skb_vlan_tag_present(skb)) {
> >>> if (pt_prev) {
> >>> ret = deliver_skb(skb, pt_prev, orig_dev);
> >>> pt_prev = NULL;
> >>> }
> >>> if (vlan_do_receive(&skb))
> >>> goto another_round;
> >>> else if (unlikely(!skb))
> >>> goto out;
> >>> }
> >>>
> >>> Thanks
> >>>
> >>
> >> I did an ftrace today and I find vlan_do_receive() is called for the
> >> incoming frames before passing SKB to hsr_handle_frame(). If someone
> >> can review this, it will help. Thanks.
> >>
> >> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
> >
> > hsr_handle_frame is an rx_handler called after
> > __netif_receive_skb_core called vlan_do_receive and jumped back to
> > another_round.
>
> Yes. hsr_handle_frame() is a rx_handler() after the above code that
> does vlan_do_receive(). The ftrace shows vlan_do_receive() is called
> followed by call to hsr_handle_frame(). From ifconfig I can see both
> hsr and vlan interface stats increments by same count. So I assume,
> vlan_do_receive() is called initially and it removes the tag, update
> stats and then return true and go for another round. Do you think that
> is the case?
That was my understanding.
> vlan_do_receive() calls vlan_find_dev(skb->dev, vlan_proto, vlan_id)
> to retrieve the real netdevice (real device). However VLAN device is
> attached to hsr device (real device), but SKB will have HSR slave
> Ethernet netdevice (in our case it is cpsw device) and vlan_find_dev()
> would have failed since there is no vlan_info in cpsw netdev struct. So
> below code in vlan_do_receive() should have failed and return false.
>
> vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
> if (!vlan_dev)
> return false;
>
> So how does it goes for another_round ? May be vlan_find_dev is
> finding the hsr netdevice?
It's good to answer this through code inspection and/or
instrumentation. I do not have the answer immediately either.
There certainly is prior art in having vlan with an rx_handler,
judging from the netif_is_macvlan_port(vlan_dev) and
netif_is_bridge_port(vlan_dev) helpers in vlan_do_receive.
> I am not an expert and so the question. Probably I can put a
> traceprintk() to confirm this, but if someone can clarify this
> it will be great. But for that, I will spin v2 with the above comments
> addressed as in my reply and post.
Please don't send a patch before we understand this part.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-08 17:51 ` Willem de Bruijn
@ 2020-09-08 18:50 ` Willem de Bruijn
2020-09-09 16:08 ` Murali Karicheri
1 sibling, 0 replies; 12+ messages in thread
From: Willem de Bruijn @ 2020-09-08 18:50 UTC (permalink / raw)
To: Murali Karicheri
Cc: Willem de Bruijn, David Miller, Jakub Kicinski,
Network Development, linux-kernel, nsekhar, Grygorii Strashko
> > >>>
> > >>> Shouldn't it show vlan_do_receive() ?
> > >>>
> > >>> if (skb_vlan_tag_present(skb)) {
> > >>> if (pt_prev) {
> > >>> ret = deliver_skb(skb, pt_prev, orig_dev);
> > >>> pt_prev = NULL;
> > >>> }
> > >>> if (vlan_do_receive(&skb))
> > >>> goto another_round;
> > >>> else if (unlikely(!skb))
> > >>> goto out;
> > >>> }
> > >>>
> > >>> Thanks
> > >>>
> > >>
> > >> I did an ftrace today and I find vlan_do_receive() is called for the
> > >> incoming frames before passing SKB to hsr_handle_frame(). If someone
> > >> can review this, it will help. Thanks.
> > >>
> > >> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
> > >
> > > hsr_handle_frame is an rx_handler called after
> > > __netif_receive_skb_core called vlan_do_receive and jumped back to
> > > another_round.
> >
> > Yes. hsr_handle_frame() is a rx_handler() after the above code that
> > does vlan_do_receive(). The ftrace shows vlan_do_receive() is called
> > followed by call to hsr_handle_frame(). From ifconfig I can see both
> > hsr and vlan interface stats increments by same count. So I assume,
> > vlan_do_receive() is called initially and it removes the tag, update
> > stats and then return true and go for another round. Do you think that
> > is the case?
>
> That was my understanding.
>
> > vlan_do_receive() calls vlan_find_dev(skb->dev, vlan_proto, vlan_id)
> > to retrieve the real netdevice (real device). However VLAN device is
> > attached to hsr device (real device), but SKB will have HSR slave
> > Ethernet netdevice (in our case it is cpsw device) and vlan_find_dev()
> > would have failed since there is no vlan_info in cpsw netdev struct. So
> > below code in vlan_do_receive() should have failed and return false.
> >
> > vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
> > if (!vlan_dev)
> > return false;
> >
> > So how does it goes for another_round ? May be vlan_find_dev is
> > finding the hsr netdevice?
>
> It's good to answer this through code inspection and/or
> instrumentation. I do not have the answer immediately either.
Agreed that from reading the code I would vlan_do_receive to not find
a vlan dev associated with the physical nic, then passes the packet
unmodified to hsr_handle_frame.
Perhaps this seems to work because skb_vlan_untag has already
pulled the tag out of the packet?
But then you should not see counters increased on the vlan dev.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP
2020-09-08 17:51 ` Willem de Bruijn
2020-09-08 18:50 ` Willem de Bruijn
@ 2020-09-09 16:08 ` Murali Karicheri
1 sibling, 0 replies; 12+ messages in thread
From: Murali Karicheri @ 2020-09-09 16:08 UTC (permalink / raw)
To: Willem de Bruijn
Cc: David Miller, Jakub Kicinski, Network Development, linux-kernel,
nsekhar, Grygorii Strashko
Hi Willem,
On 9/8/20 1:51 PM, Willem de Bruijn wrote:
> On Tue, Sep 8, 2020 at 6:55 PM Murali Karicheri <m-karicheri2@ti.com> wrote:
>>
>> Hi Willem,
>>
>> On 9/4/20 11:52 AM, Willem de Bruijn wrote:
>>> On Thu, Sep 3, 2020 at 12:30 AM Murali Karicheri <m-karicheri2@ti.com> wrote:
>>>>
>>>> All,
>>>>
>>>> On 9/2/20 12:14 PM, Murali Karicheri wrote:
>>>>> All,
>>>>>
>>>>> On 9/1/20 3:54 PM, Murali Karicheri wrote:
>>>>>> This series add support for creating VLAN interface over HSR or
>>>>>> PRP interface. Typically industrial networks uses VLAN in
>>>>>> deployment and this capability is needed to support these
>>>>>> networks.
>>>>>>
>>>>>> This is tested using two TI AM572x IDK boards connected back
>>>>>> to back over CPSW ports (eth0 and eth1).
>>>>>>
>>>>>> Following is the setup
>>>>>>
>>>>>> Physical Setup
>>>>>> ++++++++++++++
>>>>>> _______________ (CPSW) _______________
>>>>>> | |----eth0-----| |
>>>>>> |TI AM572x IDK1| | TI AM572x IDK2|
>>>>>> |______________|----eth1-----|_______________|
>>>>>>
>>>>>>
>>>>>> Network Topolgy
>>>>>> +++++++++++++++
>>>>>>
>>>>>> TI AM571x IDK TI AM572x IDK
>>>>>>
>>>>>> 192.168.100.10 CPSW ports 192.168.100.20
>>>>>> IDK-1 IDK-2
>>>>>> hsr0/prp0.100--| 192.168.2.10 |--eth0--| 192.168.2.20 |--hsr0/prp0.100
>>>>>> |----hsr0/prp0--| |---hsr0/prp0--|
>>>>>> hsr0/prp0.101--| |--eth1--| |--hsr0/prp0/101
>>>>>>
>>>>>> 192.168.101.10 192.168.101.20
>>>>>>
>>>>>> Following tests:-
>>>>>> - create hsr or prp interface and ping the interface IP address
>>>>>> and verify ping is successful.
>>>>>> - Create 2 VLANs over hsr or prp interface on both IDKs (VID 100 and
>>>>>> 101). Ping between the IP address of the VLAN interfaces
>>>>>> - Do iperf UDP traffic test with server on one IDK and client on the
>>>>>> other. Do this using 100 and 101 subnet IP addresses
>>>>>> - Dump /proc/net/vlan/{hsr|prp}0.100 and verify frames are transmitted
>>>>>> and received at these interfaces.
>>>>>> - Delete the vlan and hsr/prp interface and verify interfaces are
>>>>>> removed cleanly.
>>>>>>
>>>>>> Logs for IDK-1 at https://pastebin.ubuntu.com/p/NxF83yZFDX/
>>>>>> Logs for IDK-2 at https://pastebin.ubuntu.com/p/YBXBcsPgVK/
>>>>>>
>>>>>> Murali Karicheri (1):
>>>>>> net: hsr/prp: add vlan support
>>>>>>
>>>>>> net/hsr/hsr_device.c | 4 ----
>>>>>> net/hsr/hsr_forward.c | 16 +++++++++++++---
>>>>>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>>>>>
>>>>> I am not sure if the packet flow is right for this?
>>>>>
>>>>> VLAN over HSR frame format is like this.
>>>>>
>>>>> <Start of Frame><VLAN tag><HSR Tag><IP><CRC>
>>>>>
>>>>> My ifconfig stats shows both hsr and hsr0.100 interfaces receiving
>>>>> frames.
>>>>>
>>>>> So I did a WARN_ON() in HSR driver before frame is forwarded to upper
>>>>> layer.
>>>>>
>>>>> a0868495local@uda0868495:~/Projects/upstream-kernel$ git diff
>>>>> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
>>>>> index de21df30b0d9..545a3cd8c71b 100644
>>>>> --- a/net/hsr/hsr_forward.c
>>>>> +++ b/net/hsr/hsr_forward.c
>>>>> @@ -415,9 +415,11 @@ static void hsr_forward_do(struct hsr_frame_info
>>>>> *frame)
>>>>> }
>>>>>
>>>>> skb->dev = port->dev;
>>>>> - if (port->type == HSR_PT_MASTER)
>>>>> + if (port->type == HSR_PT_MASTER) {
>>>>> + if (skb_vlan_tag_present(skb))
>>>>> + WARN_ON(1);
>>>>> hsr_deliver_master(skb, port->dev,
>>>>> frame->node_src);
>>>>> - else
>>>>> + } else
>>>>> hsr_xmit(skb, port, frame);
>>>>> }
>>>>> }
>>>>>
>>>>> And I get the trace shown below.
>>>>>
>>>>> [ 275.125431] WARNING: CPU: 0 PID: 0 at net/hsr/hsr_forward.c:420
>>>>> hsr_forward_skb+0x460/0x564
>>>>> [ 275.133822] Modules linked in: snd_soc_omap_hdmi snd_soc_ti_sdma
>>>>> snd_soc_core snd_pcm_dmaengine snd_pcm snd_time4
>>>>> [ 275.199705] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W
>>>>> 5.9.0-rc1-00658-g473e463812c2-dirty #8
>>>>> [ 275.209573] Hardware name: Generic DRA74X (Flattened Device Tree)
>>>>> [ 275.215703] [<c011177c>] (unwind_backtrace) from [<c010b6f0>]
>>>>> (show_stack+0x10/0x14)
>>>>> [ 275.223487] [<c010b6f0>] (show_stack) from [<c055690c>]
>>>>> (dump_stack+0xc4/0xe4)
>>>>> [ 275.230747] [<c055690c>] (dump_stack) from [<c01386ac>]
>>>>> (__warn+0xc0/0xf4)
>>>>> [ 275.237656] [<c01386ac>] (__warn) from [<c0138a3c>]
>>>>> (warn_slowpath_fmt+0x58/0xb8)
>>>>> [ 275.245177] [<c0138a3c>] (warn_slowpath_fmt) from [<c09564bc>]
>>>>> (hsr_forward_skb+0x460/0x564)
>>>>> [ 275.253657] [<c09564bc>] (hsr_forward_skb) from [<c0955534>]
>>>>> (hsr_handle_frame+0x15c/0x190)
>>>>> [ 275.262047] [<c0955534>] (hsr_handle_frame) from [<c07c6704>]
>>>>> (__netif_receive_skb_core+0x23c/0xc88)
>>>>> [ 275.271223] [<c07c6704>] (__netif_receive_skb_core) from [<c07c7180>]
>>>>> (__netif_receive_skb_one_core+0x30/0x74)
>>>>> [ 275.281266] [<c07c7180>] (__netif_receive_skb_one_core) from
>>>>> [<c07c72a4>] (netif_receive_skb+0x50/0x1c4)
>>>>> [ 275.290793] [<c07c72a4>] (netif_receive_skb) from [<c071a55c>]
>>>>> (cpsw_rx_handler+0x230/0x308)
>>>>> [ 275.299272] [<c071a55c>] (cpsw_rx_handler) from [<c0715ee8>]
>>>>> (__cpdma_chan_process+0xf4/0x188)
>>>>> [ 275.307925] [<c0715ee8>] (__cpdma_chan_process) from [<c0717294>]
>>>>> (cpdma_chan_process+0x3c/0x5c)
>>>>> [ 275.316754] [<c0717294>] (cpdma_chan_process) from [<c071dd14>]
>>>>> (cpsw_rx_mq_poll+0x44/0x98)
>>>>> [ 275.325145] [<c071dd14>] (cpsw_rx_mq_poll) from [<c07c8ae0>]
>>>>> (net_rx_action+0xf0/0x400)
>>>>> [ 275.333185] [<c07c8ae0>] (net_rx_action) from [<c0101370>]
>>>>> (__do_softirq+0xf0/0x3ac)
>>>>> [ 275.340965] [<c0101370>] (__do_softirq) from [<c013f5ec>]
>>>>> (irq_exit+0xa8/0xe4)
>>>>> [ 275.348224] [<c013f5ec>] (irq_exit) from [<c0199344>]
>>>>> (__handle_domain_irq+0x6c/0xe0)
>>>>> [ 275.356093] [<c0199344>] (__handle_domain_irq) from [<c056f8fc>]
>>>>> (gic_handle_irq+0x4c/0xa8)
>>>>> [ 275.364481] [<c056f8fc>] (gic_handle_irq) from [<c0100b6c>]
>>>>> (__irq_svc+0x6c/0x90)
>>>>> [ 275.371996] Exception stack(0xc0e01f18 to 0xc0e01f60)
>>>>>
>>>>> Shouldn't it show vlan_do_receive() ?
>>>>>
>>>>> if (skb_vlan_tag_present(skb)) {
>>>>> if (pt_prev) {
>>>>> ret = deliver_skb(skb, pt_prev, orig_dev);
>>>>> pt_prev = NULL;
>>>>> }
>>>>> if (vlan_do_receive(&skb))
>>>>> goto another_round;
>>>>> else if (unlikely(!skb))
>>>>> goto out;
>>>>> }
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>> I did an ftrace today and I find vlan_do_receive() is called for the
>>>> incoming frames before passing SKB to hsr_handle_frame(). If someone
>>>> can review this, it will help. Thanks.
>>>>
>>>> https://pastebin.ubuntu.com/p/CbRzXjwjR5/
>>>
>>> hsr_handle_frame is an rx_handler called after
>>> __netif_receive_skb_core called vlan_do_receive and jumped back to
>>> another_round.
>>
>> Yes. hsr_handle_frame() is a rx_handler() after the above code that
>> does vlan_do_receive(). The ftrace shows vlan_do_receive() is called
>> followed by call to hsr_handle_frame(). From ifconfig I can see both
>> hsr and vlan interface stats increments by same count. So I assume,
>> vlan_do_receive() is called initially and it removes the tag, update
>> stats and then return true and go for another round. Do you think that
>> is the case?
>
> That was my understanding.
>
>> vlan_do_receive() calls vlan_find_dev(skb->dev, vlan_proto, vlan_id)
>> to retrieve the real netdevice (real device). However VLAN device is
>> attached to hsr device (real device), but SKB will have HSR slave
>> Ethernet netdevice (in our case it is cpsw device) and vlan_find_dev()
>> would have failed since there is no vlan_info in cpsw netdev struct. So
>> below code in vlan_do_receive() should have failed and return false.
>>
>> vlan_dev = vlan_find_dev(skb->dev, vlan_proto, vlan_id);
>> if (!vlan_dev)
>> return false;
>>
>> So how does it goes for another_round ? May be vlan_find_dev is
>> finding the hsr netdevice?
>
> It's good to answer this through code inspection and/or
> instrumentation. I do not have the answer immediately either.
>
> There certainly is prior art in having vlan with an rx_handler,
> judging from the netif_is_macvlan_port(vlan_dev) and
> netif_is_bridge_port(vlan_dev) helpers in vlan_do_receive.
>> I am not an expert and so the question. Probably I can put a
>> traceprintk() to confirm this, but if someone can clarify this
>> it will be great. But for that, I will spin v2 with the above comments
>> addressed as in my reply and post.
>
> Please don't send a patch before we understand this part.
>
Sure! That is on my TODO. Will confirm this before sending next
revision of the patch.
--
Murali Karicheri
Texas Instruments
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-09-09 16:11 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-01 19:54 [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
2020-09-01 19:54 ` [PATCH net-next 1/1] net: hsr/prp: add vlan support Murali Karicheri
2020-09-04 15:45 ` Willem de Bruijn
2020-09-08 16:38 ` Murali Karicheri
2020-09-08 17:34 ` Willem de Bruijn
2020-09-02 16:14 ` [PATCH net-next 0/1] Support for VLAN interface over HSR/PRP Murali Karicheri
2020-09-02 22:29 ` Murali Karicheri
2020-09-04 15:52 ` Willem de Bruijn
2020-09-08 16:55 ` Murali Karicheri
2020-09-08 17:51 ` Willem de Bruijn
2020-09-08 18:50 ` Willem de Bruijn
2020-09-09 16:08 ` Murali Karicheri
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.