Re: [PATCH v2 net-next] net: hsr: Send supervisory frames to HSR network with ProxyNodeTable data
From: Paolo Abeni
Date: Thu Jun 06 2024 - 04:53:28 EST
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.
>
> Signed-off-by: Lukasz Majewski <lukma@xxxxxxx>
> ---
>
> 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
> {
> - 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.
> {
> 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'?
> + 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];
Thanks,
Paolo