* [PATCH net-next v2 2/2] net: hsr: Add KUnit test for PRP
2025-02-21 10:10 [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection Jaakko Karrenpalo
@ 2025-02-21 10:10 ` Jaakko Karrenpalo
2025-02-25 11:04 ` Paolo Abeni
2025-03-11 17:00 ` Jeff Johnson
2025-02-25 10:58 ` [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection Paolo Abeni
1 sibling, 2 replies; 5+ messages in thread
From: Jaakko Karrenpalo @ 2025-02-21 10:10 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Lukasz Majewski, MD Danish Anwar
Cc: linux-kernel, netdev, Jaakko Karrenpalo, Jaakko Karrenpalo
From: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
Add unit tests for the PRP duplicate detection
Signed-off-by: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
Signed-off-by: Jaakko Karrenpalo <jkarrenpalo@gmail.com>
---
Changes in v2:
- Changed KUnit tests to compile as built-in only
net/hsr/Kconfig | 14 +++
net/hsr/Makefile | 2 +
net/hsr/prp_dup_discard_test.c | 210 +++++++++++++++++++++++++++++++++
3 files changed, 226 insertions(+)
create mode 100644 net/hsr/prp_dup_discard_test.c
diff --git a/net/hsr/Kconfig b/net/hsr/Kconfig
index 1b048c17b6c8..07fc0a768b7e 100644
--- a/net/hsr/Kconfig
+++ b/net/hsr/Kconfig
@@ -38,3 +38,17 @@ config HSR
relying on this code in a safety critical system!
If unsure, say N.
+
+config PRP_DUP_DISCARD_KUNIT_TEST
+ bool "PRP duplicate discard KUnit tests" if !KUNIT_ALL_TESTS
+ depends on HSR = y && KUNIT = y
+ default KUNIT_ALL_TESTS
+ help
+ Covers the PRP duplicate discard algorithm.
+ Only useful for kernel devs running KUnit test harness and are not
+ for inclusion into a production build.
+
+ For more information on KUnit and unit tests in general please refer
+ to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+ If unsure, say N.
diff --git a/net/hsr/Makefile b/net/hsr/Makefile
index 75df90d3b416..34e581db5c41 100644
--- a/net/hsr/Makefile
+++ b/net/hsr/Makefile
@@ -8,3 +8,5 @@ obj-$(CONFIG_HSR) += hsr.o
hsr-y := hsr_main.o hsr_framereg.o hsr_device.o \
hsr_netlink.o hsr_slave.o hsr_forward.o
hsr-$(CONFIG_DEBUG_FS) += hsr_debugfs.o
+
+obj-$(CONFIG_PRP_DUP_DISCARD_KUNIT_TEST) += prp_dup_discard_test.o
diff --git a/net/hsr/prp_dup_discard_test.c b/net/hsr/prp_dup_discard_test.c
new file mode 100644
index 000000000000..e212bdf24720
--- /dev/null
+++ b/net/hsr/prp_dup_discard_test.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <kunit/test.h>
+
+#include "hsr_main.h"
+#include "hsr_framereg.h"
+
+struct prp_test_data {
+ struct hsr_port port;
+ struct hsr_port port_rcv;
+ struct hsr_frame_info frame;
+ struct hsr_node node;
+};
+
+static struct prp_test_data *build_prp_test_data(struct kunit *test)
+{
+ struct prp_test_data *data = kunit_kzalloc(test,
+ sizeof(struct prp_test_data), GFP_USER);
+ KUNIT_EXPECT_NOT_ERR_OR_NULL(test, data);
+
+ data->frame.node_src = &data->node;
+ data->frame.port_rcv = &data->port_rcv;
+ data->port_rcv.type = HSR_PT_SLAVE_A;
+ data->node.seq_start[HSR_PT_SLAVE_A] = 1;
+ data->node.seq_expected[HSR_PT_SLAVE_A] = 1;
+ data->node.seq_start[HSR_PT_SLAVE_B] = 1;
+ data->node.seq_expected[HSR_PT_SLAVE_B] = 1;
+ data->node.seq_out[HSR_PT_MASTER] = 0;
+ data->node.time_out[HSR_PT_MASTER] = jiffies;
+ data->port.type = HSR_PT_MASTER;
+
+ return data;
+}
+
+static void check_prp_counters(struct kunit *test,
+ struct prp_test_data *data,
+ u16 seq_start_a, u16 seq_expected_a,
+ u16 seq_start_b, u16 seq_expected_b)
+{
+ KUNIT_EXPECT_EQ(test, data->node.seq_start[HSR_PT_SLAVE_A],
+ seq_start_a);
+ KUNIT_EXPECT_EQ(test, data->node.seq_start[HSR_PT_SLAVE_B],
+ seq_start_b);
+ KUNIT_EXPECT_EQ(test, data->node.seq_expected[HSR_PT_SLAVE_A],
+ seq_expected_a);
+ KUNIT_EXPECT_EQ(test, data->node.seq_expected[HSR_PT_SLAVE_B],
+ seq_expected_b);
+}
+
+static void prp_dup_discard_forward(struct kunit *test)
+{
+ /* Normal situation, both LANs in sync. Next frame is forwarded */
+ struct prp_test_data *data = build_prp_test_data(test);
+
+ data->frame.sequence_nr = 2;
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ KUNIT_EXPECT_EQ(test, jiffies, data->node.time_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, data->frame.sequence_nr,
+ data->frame.sequence_nr + 1, 1, 1);
+}
+
+static void prp_dup_discard_inside_dropwindow(struct kunit *test)
+{
+ /* Normal situation, other LAN ahead by one. Frame is dropped */
+ struct prp_test_data *data = build_prp_test_data(test);
+ unsigned long time = jiffies - 10;
+
+ data->frame.sequence_nr = 1;
+ data->node.seq_expected[HSR_PT_SLAVE_B] = 3;
+ data->node.seq_out[HSR_PT_MASTER] = 2;
+ data->node.time_out[HSR_PT_MASTER] = time;
+
+ KUNIT_EXPECT_EQ(test, 1,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, 2, data->node.seq_out[HSR_PT_MASTER]);
+ KUNIT_EXPECT_EQ(test, time, data->node.time_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, 2, 2, 2, 3);
+}
+
+static void prp_dup_discard_node_timeout(struct kunit *test)
+{
+ /* Timeout situation, node hasn't sent anything for a while */
+ struct prp_test_data *data = build_prp_test_data(test);
+
+ data->frame.sequence_nr = 7;
+ data->node.seq_start[HSR_PT_SLAVE_A] = 1234;
+ data->node.seq_expected[HSR_PT_SLAVE_A] = 1235;
+ data->node.seq_start[HSR_PT_SLAVE_B] = 1234;
+ data->node.seq_expected[HSR_PT_SLAVE_B] = 1234;
+ data->node.seq_out[HSR_PT_MASTER] = 1234;
+ data->node.time_out[HSR_PT_MASTER] =
+ jiffies - msecs_to_jiffies(HSR_ENTRY_FORGET_TIME) - 1;
+
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ KUNIT_EXPECT_EQ(test, jiffies, data->node.time_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, data->frame.sequence_nr,
+ data->frame.sequence_nr + 1, 1234, 1234);
+}
+
+static void prp_dup_discard_out_of_sequence(struct kunit *test)
+{
+ /* One frame is received out of sequence on both LANs */
+ struct prp_test_data *data = build_prp_test_data(test);
+
+ data->node.seq_start[HSR_PT_SLAVE_A] = 10;
+ data->node.seq_expected[HSR_PT_SLAVE_A] = 10;
+ data->node.seq_start[HSR_PT_SLAVE_B] = 10;
+ data->node.seq_expected[HSR_PT_SLAVE_B] = 10;
+ data->node.seq_out[HSR_PT_MASTER] = 9;
+
+ /* 1st old frame, should be accepted */
+ data->frame.sequence_nr = 8;
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, data->frame.sequence_nr,
+ data->frame.sequence_nr + 1, 10, 10);
+
+ /* 2nd frame should be dropped */
+ data->frame.sequence_nr = 8;
+ data->port_rcv.type = HSR_PT_SLAVE_B;
+ KUNIT_EXPECT_EQ(test, 1,
+ prp_register_frame_out(&data->port, &data->frame));
+ check_prp_counters(test, data, data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1);
+
+ /* Next frame, this is forwarded */
+ data->frame.sequence_nr = 10;
+ data->port_rcv.type = HSR_PT_SLAVE_A;
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, data->frame.sequence_nr,
+ data->frame.sequence_nr + 1, 9, 9);
+
+ /* and next one is dropped */
+ data->frame.sequence_nr = 10;
+ data->port_rcv.type = HSR_PT_SLAVE_B;
+ KUNIT_EXPECT_EQ(test, 1,
+ prp_register_frame_out(&data->port, &data->frame));
+ check_prp_counters(test, data, data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1,
+ data->frame.sequence_nr + 1);
+}
+
+static void prp_dup_discard_lan_b_late(struct kunit *test)
+{
+ /* LAN B is behind */
+ struct prp_test_data *data = build_prp_test_data(test);
+
+ data->node.seq_start[HSR_PT_SLAVE_A] = 9;
+ data->node.seq_expected[HSR_PT_SLAVE_A] = 9;
+ data->node.seq_start[HSR_PT_SLAVE_B] = 9;
+ data->node.seq_expected[HSR_PT_SLAVE_B] = 9;
+ data->node.seq_out[HSR_PT_MASTER] = 8;
+
+ data->frame.sequence_nr = 9;
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, 9, 10, 9, 9);
+
+ data->frame.sequence_nr = 10;
+ KUNIT_EXPECT_EQ(test, 0,
+ prp_register_frame_out(&data->port, &data->frame));
+ KUNIT_EXPECT_EQ(test, data->frame.sequence_nr,
+ data->node.seq_out[HSR_PT_MASTER]);
+ check_prp_counters(test, data, 9, 11, 9, 9);
+
+ data->frame.sequence_nr = 9;
+ data->port_rcv.type = HSR_PT_SLAVE_B;
+ KUNIT_EXPECT_EQ(test, 1,
+ prp_register_frame_out(&data->port, &data->frame));
+ check_prp_counters(test, data, 10, 11, 10, 10);
+
+ data->frame.sequence_nr = 10;
+ data->port_rcv.type = HSR_PT_SLAVE_B;
+ KUNIT_EXPECT_EQ(test, 1,
+ prp_register_frame_out(&data->port, &data->frame));
+ check_prp_counters(test, data, 11, 11, 11, 11);
+}
+
+static struct kunit_case prp_dup_discard_test_cases[] = {
+ KUNIT_CASE(prp_dup_discard_forward),
+ KUNIT_CASE(prp_dup_discard_inside_dropwindow),
+ KUNIT_CASE(prp_dup_discard_node_timeout),
+ KUNIT_CASE(prp_dup_discard_out_of_sequence),
+ KUNIT_CASE(prp_dup_discard_lan_b_late),
+ {}
+};
+
+static struct kunit_suite prp_dup_discard_suite = {
+ .name = "prp_duplicate_discard",
+ .test_cases = prp_dup_discard_test_cases,
+};
+
+kunit_test_suite(prp_dup_discard_suite);
+
+MODULE_LICENSE("GPL");
--
2.43.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection
2025-02-21 10:10 [PATCH net-next v2 1/2] net: hsr: Fix PRP duplicate detection Jaakko Karrenpalo
2025-02-21 10:10 ` [PATCH net-next v2 2/2] net: hsr: Add KUnit test for PRP Jaakko Karrenpalo
@ 2025-02-25 10:58 ` Paolo Abeni
1 sibling, 0 replies; 5+ messages in thread
From: Paolo Abeni @ 2025-02-25 10:58 UTC (permalink / raw)
To: Jaakko Karrenpalo, David S. Miller, Eric Dumazet, Jakub Kicinski,
Simon Horman, Lukasz Majewski, MD Danish Anwar
Cc: linux-kernel, netdev, Jaakko Karrenpalo
On 2/21/25 11:10 AM, Jaakko Karrenpalo wrote:
> From: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
>
> Add PRP specific function for handling duplicate
> packets. This is needed because of potential
> L2 802.1p prioritization done by network switches.
This is IMHO a too terse description of the whys. I don't see how
prioritization should create duplicates. Please expand the commit
message. A cover letter could help
>
> Signed-off-by: Jaakko Karrenpalo <jaakko.karrenpalo@fi.abb.com>
> Signed-off-by: Jaakko Karrenpalo <jkarrenpalo@gmail.com>
Just one of the 2 ;)
> ---
> net/hsr/hsr_device.c | 2 +
> net/hsr/hsr_forward.c | 4 +-
> net/hsr/hsr_framereg.c | 93 ++++++++++++++++++++++++++++++++++++++++--
> net/hsr/hsr_framereg.h | 8 +++-
> net/hsr/hsr_main.h | 2 +
> 5 files changed, 102 insertions(+), 7 deletions(-)
>
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index b6fb18469439..2c43776b7c4f 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -616,6 +616,7 @@ static struct hsr_proto_ops hsr_ops = {
> .drop_frame = hsr_drop_frame,
> .fill_frame_info = hsr_fill_frame_info,
> .invalid_dan_ingress_frame = hsr_invalid_dan_ingress_frame,
> + .register_frame_out = hsr_register_frame_out,
> };
>
> static struct hsr_proto_ops prp_ops = {
> @@ -626,6 +627,7 @@ static struct hsr_proto_ops prp_ops = {
> .fill_frame_info = prp_fill_frame_info,
> .handle_san_frame = prp_handle_san_frame,
> .update_san_info = prp_update_san_info,
> + .register_frame_out = prp_register_frame_out,
> };
>
> void hsr_dev_setup(struct net_device *dev)
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index a4bacf198555..aebeced10ad8 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -536,8 +536,8 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> * Also for SAN, this shouldn't be done.
> */
> if (!frame->is_from_san &&
> - hsr_register_frame_out(port, frame->node_src,
> - frame->sequence_nr))
> + hsr->proto_ops->register_frame_out &&
> + hsr->proto_ops->register_frame_out(port, frame))
The formatting is quite inconsistent and hard to follow in several
places. I'm surprised checkpatch does not complain. Please align the
condition to the relevant bracket.
[...]
> @@ -482,9 +486,11 @@ void hsr_register_frame_in(struct hsr_node *node, struct hsr_port *port,
> * 0 otherwise, or
> * negative error code on error
> */
> -int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
> - u16 sequence_nr)
> +int hsr_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
> {
> + struct hsr_node *node = frame->node_src;
> + u16 sequence_nr = frame->sequence_nr;
> +
> spin_lock_bh(&node->seq_out_lock);
> if (seq_nr_before_or_eq(sequence_nr, node->seq_out[port->type]) &&
> time_is_after_jiffies(node->time_out[port->type] +
> @@ -499,6 +505,87 @@ int hsr_register_frame_out(struct hsr_port *port, struct hsr_node *node,
> return 0;
> }
>
> +/* Adaptation of the PRP duplicate discard algorithm described in wireshark
> + * wiki (https://wiki.wireshark.org/PRP)
> + *
> + * A drop window is maintained for both LANs with start sequence set to the
> + * first sequence accepted on the LAN that has not been seen on the other LAN,
> + * and expected sequence set to the latest received sequence number plus one.
> + *
> + * When a frame is received on either LAN it is compared against the received
> + * frames on the other LAN. If it is outside the drop window of the other LAN
> + * the frame is accepted and the drop window is updated.
> + * The drop window for the other LAN is reset.
> + *
> + * 'port' is the outgoing interface
> + * 'frame' is the frame to be sent
> + *
> + * Return:
> + * 1 if frame can be shown to have been sent recently on this interface,
> + * 0 otherwise
> + */
> +int prp_register_frame_out(struct hsr_port *port, struct hsr_frame_info *frame)
> +{
> + enum hsr_port_type other_port;
> + enum hsr_port_type rcv_port;
> + struct hsr_node *node;
> + u16 sequence_nr;
> +
> + /* out-going frames are always in order
> + *and can be checked the same way as for HSR
> + */
> + if (frame->port_rcv->type == HSR_PT_MASTER)
> + return hsr_register_frame_out(port, frame);
> +
> + /* for PRP we should only forward frames from the slave ports
> + * to the master port
> + */
> + if (port->type != HSR_PT_MASTER)
> + return 1;
> +
> + node = frame->node_src;
> + sequence_nr = frame->sequence_nr;
> + rcv_port = frame->port_rcv->type;
> + other_port =
> + rcv_port == HSR_PT_SLAVE_A ? HSR_PT_SLAVE_B : HSR_PT_SLAVE_A;
> +
> + spin_lock_bh(&node->seq_out_lock);
> + if (time_is_before_jiffies(node->time_out[port->type] +
> + msecs_to_jiffies(HSR_ENTRY_FORGET_TIME)) ||
> + (node->seq_start[rcv_port] == node->seq_expected[rcv_port] &&
> + node->seq_start[other_port] == node->seq_expected[other_port])) {
> + /* the node hasn't been sending for a while
> + * or both drop windows are empty, forward the frame
> + */
> + node->seq_start[rcv_port] = sequence_nr;
> + } else if (seq_nr_before(sequence_nr, node->seq_expected[other_port]) &&
> + seq_nr_before_or_eq(node->seq_start[other_port], sequence_nr)) {
> + /* drop the frame, update the drop window for the other port
> + * and reset our drop window
> + */
> + node->seq_start[other_port] = sequence_nr + 1;
> + node->seq_expected[rcv_port] = sequence_nr + 1;
> + node->seq_start[rcv_port] = node->seq_expected[rcv_port];
> + spin_unlock_bh(&node->seq_out_lock);
> + return 1;
> + }
> +
> + /* update the drop window for the port where this frame was received
> + * and clear the drop window for the other port
> + */
> + node->seq_start[other_port] = node->seq_expected[other_port];
> + node->seq_expected[rcv_port] = sequence_nr + 1;
> + if ((u16)(node->seq_expected[rcv_port] - node->seq_start[rcv_port])
> + > PRP_DROP_WINDOW_LEN)
> + node->seq_start[rcv_port] =
> + node->seq_expected[rcv_port] - PRP_DROP_WINDOW_LEN;
I could be too sensible, but the above really hurts my eyes;) something
alike:
u16 seq_exp, seq_diff;
// [...]
seq_exp = node->seq_expected[rcv_port]
seq_diff = seq_exp - node->seq_start[rcv_port];
if (seq_diff > PRP_DROP_WINDOW_LEN)
node->seq_start[rcv_port] = seq_exp - PRP_DROP_WINDOW_LEN;
would be better.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 5+ messages in thread