Hi Paolo, > On Tue, 2024-06-04 at 10:43 +0200, Lukasz Majewski wrote: > > This patch provides support for sending supervision HSR frames with > > MAC addresses stored in ProxyNodeTable when RedBox (i.e. HSR-SAN) is > > enabled. > > > > Supervision frames with RedBox MAC address (appended as second TLV) > > are only send for ProxyNodeTable nodes. > > > > This patch series shall be tested with hsr_redbox.sh script. > > I don't see any specific check for mac addresses in hsr_redbox.sh, am > I missing something? Otherwise please update the self-tests, too. Could you be more specific here? This patch adds support for sending supervisory frames on behalf of RedBox device. The result of it could be visible in proxy_node table. However, no special check is required. > > > > > Signed-off-by: Lukasz Majewski > > --- > > > > Changes for v2: > > - Fix the Reverse Christmas Tree formatting > > - Return directly values from hsr_is_node_in_db() and > > ether_addr_equal() > > - Change the internal variable check > > --- > > net/hsr/hsr_device.c | 63 > > ++++++++++++++++++++++++++++++++++-------- net/hsr/hsr_forward.c | > > 37 +++++++++++++++++++++++-- net/hsr/hsr_framereg.c | 12 ++++++++ > > net/hsr/hsr_framereg.h | 2 ++ > > net/hsr/hsr_main.h | 4 ++- > > net/hsr/hsr_netlink.c | 1 + > > 6 files changed, 105 insertions(+), 14 deletions(-) > > > > diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c > > index e6904288d40d..ad7cb9b29273 100644 > > --- a/net/hsr/hsr_device.c > > +++ b/net/hsr/hsr_device.c > > @@ -73,9 +73,15 @@ static void hsr_check_announce(struct net_device > > *hsr_dev) mod_timer(&hsr->announce_timer, jiffies + > > msecs_to_jiffies(HSR_ANNOUNCE_INTERVAL)); > > } > > + > > + if (hsr->redbox && > > !timer_pending(&hsr->announce_proxy_timer)) > > + mod_timer(&hsr->announce_proxy_timer, > > jiffies + > > + > > msecs_to_jiffies(HSR_ANNOUNCE_INTERVAL) / 2); } else { > > /* Deactivate the announce timer */ > > timer_delete(&hsr->announce_timer); > > + if (hsr->redbox) > > + timer_delete(&hsr->announce_proxy_timer); > > } > > } > > > > @@ -279,10 +285,11 @@ static struct sk_buff *hsr_init_skb(struct > > hsr_port *master) return NULL; > > } > > > > -static void send_hsr_supervision_frame(struct hsr_port *master, > > - unsigned long *interval) > > +static void send_hsr_supervision_frame(struct hsr_port *port, > > + unsigned long *interval, > > + const unsigned char > > addr[ETH_ALEN]) > > please use 'const unsigned char *addr' instead. The above syntax is > misleading > Ok. > > { > > - struct hsr_priv *hsr = master->hsr; > > + struct hsr_priv *hsr = port->hsr; > > __u8 type = HSR_TLV_LIFE_CHECK; > > struct hsr_sup_payload *hsr_sp; > > struct hsr_sup_tlv *hsr_stlv; > > [...] > > > @@ -340,13 +348,14 @@ static void send_hsr_supervision_frame(struct > > hsr_port *master, return; > > } > > > > - hsr_forward_skb(skb, master); > > + hsr_forward_skb(skb, port); > > spin_unlock_bh(&hsr->seqnr_lock); > > return; > > } > > > > static void send_prp_supervision_frame(struct hsr_port *master, > > - unsigned long *interval) > > + unsigned long *interval, > > + const unsigned char > > addr[ETH_ALEN]) > > Same here. Ok. > > > { > > struct hsr_priv *hsr = master->hsr; > > struct hsr_sup_payload *hsr_sp; > > @@ -396,7 +405,7 @@ static void hsr_announce(struct timer_list *t) > > > > rcu_read_lock(); > > master = hsr_port_get_hsr(hsr, HSR_PT_MASTER); > > - hsr->proto_ops->send_sv_frame(master, &interval); > > + hsr->proto_ops->send_sv_frame(master, &interval, > > master->dev->dev_addr); > > if (is_admin_up(master->dev)) > > mod_timer(&hsr->announce_timer, jiffies + > > interval); @@ -404,6 +413,37 @@ static void hsr_announce(struct > > timer_list *t) rcu_read_unlock(); > > } > > > > +/* Announce (supervision frame) timer function for RedBox > > + */ > > +static void hsr_proxy_announce(struct timer_list *t) > > +{ > > + struct hsr_priv *hsr = from_timer(hsr, t, > > announce_proxy_timer); > > + struct hsr_port *interlink; > > + unsigned long interval = 0; > > + struct hsr_node *node; > > + > > + rcu_read_lock(); > > + /* RedBOX sends supervisory frames to HSR network with MAC > > addresses > > + * of SAN nodes stored in ProxyNodeTable. > > + */ > > + interlink = hsr_port_get_hsr(hsr, HSR_PT_INTERLINK); > > + list_for_each_entry_rcu(node, &hsr->proxy_node_db, > > mac_list) { > > + if (hsr_addr_is_redbox(hsr, node->macaddress_A)) > > + continue; > > + hsr->proto_ops->send_sv_frame(interlink, &interval, > > + node->macaddress_A); > > + } > > + > > + if (is_admin_up(interlink->dev)) { > > + if (!interval) > > + interval = > > msecs_to_jiffies(HSR_ANNOUNCE_INTERVAL); + > > + mod_timer(&hsr->announce_proxy_timer, jiffies + > > interval); > > + } > > + > > + rcu_read_unlock(); > > +} > > + > > void hsr_del_ports(struct hsr_priv *hsr) > > { > > struct hsr_port *port; > > @@ -590,6 +630,7 @@ int hsr_dev_finalize(struct net_device > > *hsr_dev, struct net_device *slave[2], > > timer_setup(&hsr->announce_timer, hsr_announce, 0); > > timer_setup(&hsr->prune_timer, hsr_prune_nodes, 0); > > timer_setup(&hsr->prune_proxy_timer, hsr_prune_proxy_nodes, 0); > > + timer_setup(&hsr->announce_proxy_timer, > > hsr_proxy_announce, 0); > > ether_addr_copy(hsr->sup_multicast_addr, > > def_multicast_addr); hsr->sup_multicast_addr[ETH_ALEN - 1] = > > multicast_spec; diff --git a/net/hsr/hsr_forward.c > > b/net/hsr/hsr_forward.c index 05a61b8286ec..003070dbfcb4 100644 > > --- a/net/hsr/hsr_forward.c > > +++ b/net/hsr/hsr_forward.c > > @@ -117,6 +117,35 @@ static bool is_supervision_frame(struct > > hsr_priv *hsr, struct sk_buff *skb) return true; > > } > > > > +static bool is_proxy_supervision_frame(struct hsr_priv *hsr, > > + struct sk_buff *skb) > > +{ > > + struct hsr_sup_payload *payload; > > + struct ethhdr *eth_hdr; > > + u16 total_length = 0; > > + > > + eth_hdr = (struct ethhdr *)skb_mac_header(skb); > > + > > + /* Get the HSR protocol revision. */ > > + if (eth_hdr->h_proto == htons(ETH_P_HSR)) > > + total_length = sizeof(struct hsrv1_ethhdr_sp); > > + else > > + total_length = sizeof(struct hsrv0_ethhdr_sp); > > + > > + if (!pskb_may_pull(skb, total_length)) > > It looks like 'total_length' does not include 'sizeof > hsr_sup_payload'? I think that it is expected - I do check if it is possible to pull data from packet up till the point where I want to further extract the hsr payload. > > > + return false; > > + > > + skb_pull(skb, total_length); > > + payload = (struct hsr_sup_payload *)skb->data; > > + skb_push(skb, total_length); > > No need to actually pull the data, you could do directly: > > payload = (struct hsr_sup_payload *)skb->data[total_length]; > Yes, this is better approach - thanks for the idea. > > > Thanks, > > Paolo > Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de