Re: [RFC] net: hsr: Provide RedBox support

["Ziyang Xuan (William)" <william.xuanziyang@huawei.com>]

Give opinions only from the code level.

>  
>  void hsr_debugfs_rename(struct net_device *dev)
>  {
> @@ -95,6 +114,19 @@ void hsr_debugfs_init(struct hsr_priv *priv, struct net_device *hsr_dev)
>  		priv->node_tbl_root = NULL;
>  		return;
>  	}
> +
> +	if (!priv->redbox)
> +		return;
> +
> +	de = debugfs_create_file("proxy_node_table", S_IFREG | 0444,
> +				 priv->node_tbl_root, priv,
> +				 &hsr_proxy_node_table_fops);
> +	if (IS_ERR(de)) {
> +		pr_err("Cannot create hsr proxy node_table file\n");
> +		debugfs_remove(priv->node_tbl_root);
> +		priv->node_tbl_root = NULL;
> +		return;
> +	}
I think we can use "goto label" to reduce duplicate codes for error handling.

>  }
>  
> @@ -296,6 +298,7 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  	struct hsr_priv *hsr = master->hsr;
>  	__u8 type = HSR_TLV_LIFE_CHECK;
>  	struct hsr_sup_payload *hsr_sp;
> +	struct hsr_sup_tlv *hsr_stlv;
>  	struct hsr_sup_tag *hsr_stag;
>  	struct sk_buff *skb;
>  
> @@ -335,6 +338,16 @@ static void send_hsr_supervision_frame(struct hsr_port *master,
>  	hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
>  	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
>  
> +	if (hsr->redbox) {
> +		hsr_stlv = skb_put(skb, sizeof(struct hsr_sup_tlv));
> +		hsr_stlv->HSR_TLV_type = PRP_TLV_REDBOX_MAC;
> +		hsr_stlv->HSR_TLV_length = sizeof(struct hsr_sup_payload);
> +
> +		/* Payload: MacAddressRedBox */
> +		hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));
> +		ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
> +	}
If hsr->redbox is true, hsr_sp->macaddress_A will be covered. Do ether_addr_copy() twice.
Is it better like this:

hsr_sp = skb_put(skb, sizeof(struct hsr_sup_payload));

if (hsr->redbox) {
	...
	ether_addr_copy(hsr_sp->macaddress_A, hsr->macaddress_redbox);
} else {
	ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr);
}

> +
>  	if (skb_put_padto(skb, ETH_ZLEN)) {
>  		spin_unlock_bh(&hsr->seqnr_lock);
>  		return;

>  
> @@ -448,13 +455,14 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  		}
>  
>  		/* Check if frame is to be dropped. Eg. for PRP no forward
> -		 * between ports.
> +		 * between ports, or sending HSR supervision to RedBox.
>  		 */
>  		if (hsr->proto_ops->drop_frame &&
>  		    hsr->proto_ops->drop_frame(frame, port))
>  			continue;
>  
> -		if (port->type != HSR_PT_MASTER)
> +		if (port->type == HSR_PT_SLAVE_A ||
> +		    port->type == HSR_PT_SLAVE_B)

(port->type != HSR_PT_MASTER) is not equivalent to (port->type == HSR_PT_SLAVE_A || port->type == HSR_PT_SLAVE_B).
port->type may be HSR_PT_INTERLINK or others. Or here is a bugfix? Please check.

>  			skb = hsr->proto_ops->create_tagged_frame(frame, port);
>  		else
>  			skb = hsr->proto_ops->get_untagged_frame(frame, port);
> @@ -469,7 +477,9 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
>  			hsr_deliver_master(skb, port->dev, frame->node_src);
>  		} else {
>  			if (!hsr_xmit(skb, port, frame))
> -				sent = true;
> +				if (port->type == HSR_PT_SLAVE_A ||
> +				    port->type == HSR_PT_SLAVE_B)
> +					sent = true;
>  		}
>  	}
>  }

If my opinions be accepted, Can you add "Reviewed-by: Ziyang Xuan <william.xuanziyang@huawei.com>" at next version of patch?