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

[Lukasz Majewski]

Hi William,

> 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.

This code is already NAK'ed from network maintainers, as debugfs is not
acceptable to give any "API like" information.

Instead the "netlink" API shall be used to provide this information.

> 
> >  }
> >  
> > @@ -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);
> }

It may be a bit misleading, as the hsr_sp is the payload for TLV fields
in HSR supervisory frames.

It shall be done as in the code as:

1. First you need to send this HSR "node" MAC address (the
ether_addr_copy(hsr_sp->macaddress_A, master->dev->dev_addr))

2. If the box is a RedBox, then this supervisory frame shall have
_appended_ another TLV with RedBox mac address (it can be the same
as the HSR node in the special case).

The hsr_sp->macaddress_A holds the address to the V (value) field of
the TLV.

> 
> > +
> >  	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.

Without Redbox support you don't use HSR_PT_INTERLINK. This port shall
behave in similar way to HSR_PT_MASTER - e.g. it will send/receive HSR
untagged frames. That is why the condition has been altered to only
prepare HSR tagged frames for HSR ring port A/B.

> 
> >  			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@xxxxxxxxxx>" at next version of patch?

I will add your Reviewed-by: tag, no problem.

Thanks for the input :-)

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@xxxxxxx

Attachment: pgpFbrgy9JWO1.pgp
Description: OpenPGP digital signature