[PATCH] hsr: fix don't prune the master node from the node_db

[PATCH] hsr: fix don't prune the master node from the node_db

[Andreas Oetken]

Don't prune master node in the hsr_prune_nodes function.
Neither time_in[HSR_PT_SLAVE_A], nor time_in[HSR_PT_SLAVE_B],
will ever be updated by hsr_register_frame_in for the master port.
Thus the master node will be repeatedly pruned leading to
repeated packet loss.
This bug never appeared because the hsr_prune_nodes function
was only called once. Since commit 5150b45fd355
("net: hsr: Fix node prune function for forget time expiry") this issue
is fixed unvealing the issue described above.

Signed-off-by: Andreas Oetken <andreas.oetken@siemens.com>
---
 net/hsr/hsr_framereg.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 9fa9abd83018..2d7a19750436 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -365,6 +365,14 @@ void hsr_prune_nodes(struct timer_list *t)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(node, &hsr->node_db, mac_list) {
+		/* Don't prune own node. Neither time_in[HSR_PT_SLAVE_A]
+		 * nor time_in[HSR_PT_SLAVE_B], will ever be updated for
+		 * the master port. Thus the master node will be repeatedly
+		 * pruned leading to packet loss.
+		 */
+		if (hsr_addr_is_self(hsr, node->macaddress_A))
+			continue;
+
 		/* Shorthand */
 		time_a = node->time_in[HSR_PT_SLAVE_A];
 		time_b = node->time_in[HSR_PT_SLAVE_B];
-- 
2.20.1

Re: [PATCH] hsr: fix don't prune the master node from the node_db

[Murali Karicheri]

Hi Andreas,

On 05/22/2019 02:07 AM, Andreas Oetken wrote:
> Don't prune master node in the hsr_prune_nodes function.
> Neither time_in[HSR_PT_SLAVE_A], nor time_in[HSR_PT_SLAVE_B],
> will ever be updated by hsr_register_frame_in for the master port.
> Thus the master node will be repeatedly pruned leading to
> repeated packet loss.
> This bug never appeared because the hsr_prune_nodes function
> was only called once. Since commit 5150b45fd355
> ("net: hsr: Fix node prune function for forget time expiry") this issue
> is fixed unvealing the issue described above.
> 
> Signed-off-by: Andreas Oetken <andreas.oetken@siemens.com>
> ---
>   net/hsr/hsr_framereg.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index 9fa9abd83018..2d7a19750436 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -365,6 +365,14 @@ void hsr_prune_nodes(struct timer_list *t)
>   
>   	rcu_read_lock();
>   	list_for_each_entry_rcu(node, &hsr->node_db, mac_list) {
> +		/* Don't prune own node. Neither time_in[HSR_PT_SLAVE_A]
> +		 * nor time_in[HSR_PT_SLAVE_B], will ever be updated for
> +		 * the master port. Thus the master node will be repeatedly
> +		 * pruned leading to packet loss.
> +		 */
> +		if (hsr_addr_is_self(hsr, node->macaddress_A))
> +			continue;
> +
>   		/* Shorthand */
>   		time_a = node->time_in[HSR_PT_SLAVE_A];
>   		time_b = node->time_in[HSR_PT_SLAVE_B];
> 
Thanks for the patch!

I have applied this at the tip of master branch and it has passed my
tests. See Test logs https://pastebin.ubuntu.com/p/hQ6JmfQmkz/

But few things to take care in the patch.

1) Whenever you send an updated patch, I believe you need to increment
    the version number. So this should have been v2
2) As per Submitting patches, if a commit fixes another commit, need to
    add a "Fixes " statement as per Documentation/process/submitting-
    patches.rst

See below statements in the above documentation
======================================================================
    If your patch fixes a bug in a specific commit, e.g. you found an
   issue using ``git bisect``, please use the 'Fixes:' tag with the first
   12 characters of the SHA-1 ID, and the one line summary.  Do not split
   the tag across multiple lines, tags are exempt from the "wrap at 75
   columns" rule in order to simplify parsing scripts.  For example::

	Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return
   the number of pages it actually freed")
=======================================================================

So after fixing the above, you may add below for next revision.

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

Re: [PATCH] hsr: fix don't prune the master node from the node_db

[Murali Karicheri]

Hi Andreas,

On 05/21/2019 06:52 AM, Andreas Oetken wrote:
> Don't prune master node in the hsr_prune_nodes function.
> Neither time_in[HSR_PT_SLAVE_A], nor time_in[HSR_PT_SLAVE_B],
> will ever be updated by hsr_register_frame_in for the master port.
> Thus the master node will be repeatedly pruned leading to
> repeated packet loss.
> This bug never appeared because the hsr_prune_nodes function
> was only called once. Since commit 5150b45fd355
> ("net: hsr: Fix node prune function for forget time expiry") this issue
> is fixed unvealing the issue described above.
> 
> Signed-off-by: Andreas Oetken <andreas.oetken@siemens.com>
> ---
>   net/hsr/hsr_framereg.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
> index 9af16cb68f76..317cddda494e 100644
> --- a/net/hsr/hsr_framereg.c
> +++ b/net/hsr/hsr_framereg.c
> @@ -387,6 +387,14 @@ void hsr_prune_nodes(struct timer_list *t)
>   
>   	rcu_read_lock();
>   	list_for_each_entry_rcu(node, &hsr->node_db, mac_list) {
> +		/* Don't prune own node. Neither time_in[HSR_PT_SLAVE_A]
> +		 * nor time_in[HSR_PT_SLAVE_B], will ever be updated for
> +		 * the master port. Thus the master node will be repeatedly
> +		 * pruned leading to packet loss.
> +		 */
> +		if (hsr_addr_is_self(hsr, node->MacAddressA))
This gives me a compilation issue in the latest master branch

   AR      drivers/base/regmap/built-in.a
   AR      drivers/base/built-in.a
net/hsr/hsr_framereg.c: In function ‘hsr_prune_nodes’:
net/hsr/hsr_framereg.c:373:35: error: ‘struct hsr_node’ has no member 
named ‘MacAddressA’; did you mean ‘macaddress_A’?
    if (hsr_addr_is_self(hsr, node->MacAddressA))
                                    ^~~~~~~~~~~
                                    macaddress_A
   CC      net/core/gen_stats.o
scripts/Makefile.build:278: recipe for target 'net/hsr/hsr_framereg.o' 
failed

Could you address it and re-send?

Thanks

Murali
> +			continue;
> +
>   		/* Shorthand */
>   		time_a = node->time_in[HSR_PT_SLAVE_A];
>   		time_b = node->time_in[HSR_PT_SLAVE_B];
> 

[PATCH] hsr: fix don't prune the master node from the node_db

[Andreas Oetken]

Don't prune master node in the hsr_prune_nodes function.
Neither time_in[HSR_PT_SLAVE_A], nor time_in[HSR_PT_SLAVE_B],
will ever be updated by hsr_register_frame_in for the master port.
Thus the master node will be repeatedly pruned leading to
repeated packet loss.
This bug never appeared because the hsr_prune_nodes function
was only called once. Since commit 5150b45fd355
("net: hsr: Fix node prune function for forget time expiry") this issue
is fixed unvealing the issue described above.

Signed-off-by: Andreas Oetken <andreas.oetken@siemens.com>
---
 net/hsr/hsr_framereg.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/hsr/hsr_framereg.c b/net/hsr/hsr_framereg.c
index 9af16cb68f76..317cddda494e 100644
--- a/net/hsr/hsr_framereg.c
+++ b/net/hsr/hsr_framereg.c
@@ -387,6 +387,14 @@ void hsr_prune_nodes(struct timer_list *t)
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(node, &hsr->node_db, mac_list) {
+		/* Don't prune own node. Neither time_in[HSR_PT_SLAVE_A]
+		 * nor time_in[HSR_PT_SLAVE_B], will ever be updated for
+		 * the master port. Thus the master node will be repeatedly
+		 * pruned leading to packet loss.
+		 */
+		if (hsr_addr_is_self(hsr, node->MacAddressA))
+			continue;
+
 		/* Shorthand */
 		time_a = node->time_in[HSR_PT_SLAVE_A];
 		time_b = node->time_in[HSR_PT_SLAVE_B];
-- 
2.20.1