Re: [PATCH RFC net-next v3] hsr: Allow to send a specific port and with HSR header

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Sebastian Andrzej Siewior wrote:
> HSR forwards all packets it received on slave port 1 to slave port 2 and
> one of the two copies to the user (master) interface.
> In terms of PTP this is not good because the latency introduced by
> forwarding makes the timestamp in the PTP packet inaccurate.
> The PTP packets should not forwarded like regular packets.
> 
> In order to work with PTP over HSR the following has been done:
> - PTP packets which are received are dropped within the HSR stack. That
>   means they are not forwarded or injected into the master port. If the
>   user requires them, then they need to be obtained directly from the
>   SLAVE interface.
> 
> - Sending packets. If the ethernet type of the packet is ETH_P_1588 then
>   the stack assumes a header of type struct hsr_inline_header. The size
>   of this header is as ethhdr. As a safeguard, the header contains a
>   magic field which matches the position of h_source and it needs to
>   match HSR_INLINE_HDR.
>   Once this is verified, the header contains the port on which this
>   packet needs to be sent and if system's HSR header should be added.
>   This information is used with the HSR stack and also attached to skb
>   as a skb-extension. The packet is then pulled passed the custom header
>   so the remaining stack will see the actual data.
>   The skb-extension is needed so that an ethernet driver, with
>   HSR-offload capabilities, knows that it must not add HSR-header (unless
>   requested) and send the packet on both ports.
> 
> The originally submitted skb is freed and only the (altered) clone is
> submitted to the slave interface for sending. Therefore on cloning, the
> socket and tx_flags/ tskey are copied so that the PTP timestamp is
> forwarded to the submitting socket.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Great to see a solution that keeps the added logic mostly within HSR
itself, and works for the userspace component too.


> diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
> index f4cf2dd36d193..220f6e5d7b24c 100644
> --- a/include/linux/if_hsr.h
> +++ b/include/linux/if_hsr.h
> @@ -3,6 +3,7 @@
>  #define _LINUX_IF_HSR_H_
>  
>  #include <linux/types.h>
> +#include <linux/skbuff.h>
>  
>  struct net_device;
>  
> @@ -22,6 +23,21 @@ enum hsr_port_type {
>  	HSR_PT_PORTS,	/* This must be the last item in the enum */
>  };
>  
> +struct hsr_ptp_ext {
> +	u8	port;
> +	u8	header;
> +};
> +
> +#define HSR_INLINE_HDR	0xaf485352
> +struct hsr_inline_header {
> +	uint8_t tx_port;
> +	uint8_t hsr_hdr;
> +	uint8_t __pad0[4];
> +	uint32_t magic;
> +	uint8_t __pad1[2];
> +	uint16_t eth_type;
> +} __packed;
> +

No specific need to make this header ethhdr like?

>  /* HSR Tag.
>   * As defined in IEC-62439-3:2010, the HSR tag is really { ethertype = 0x88FB,
>   * path, LSDU_size, sequence Nr }. But we let eth_header() create { h_dest,
> @@ -45,6 +61,60 @@ struct net_device *hsr_get_port_ndev(struct net_device *ndev,
>  				     enum hsr_port_type pt);
>  int hsr_get_port_type(struct net_device *hsr_dev, struct net_device *dev,
>  		      enum hsr_port_type *type);
> +
> +static inline bool hsr_skb_has_header(struct sk_buff *skb)
> +{
> +	struct hsr_ptp_ext *ptp_ext;
> +
> +	ptp_ext = skb_ext_find(skb, SKB_EXT_HSR);
> +	if (!ptp_ext)
> +		return false;
> +	return ptp_ext->header;
> +}
> +
> +static inline unsigned int hsr_skb_has_port(struct sk_buff *skb)
> +{
> +	struct hsr_ptp_ext *ptp_ext;
> +
> +	if (!skb)
> +		return 0;
> +
> +	ptp_ext = skb_ext_find(skb, SKB_EXT_HSR);
> +	if (!ptp_ext)
> +		return 0;
> +	return ptp_ext->port;
> +}
> +
> +static inline bool hsr_skb_get_header_port(struct sk_buff *skb, bool *header,
> +					   enum hsr_port_type *port_type)
> +{
> +	struct hsr_ptp_ext *ptp_ext;
> +
> +	*port_type = HSR_PT_NONE;
> +	*header = false;
> +
> +	ptp_ext = skb_ext_find(skb, SKB_EXT_HSR);
> +	if (!ptp_ext)
> +		return false;
> +
> +	*port_type = ptp_ext->port;
> +	*header = ptp_ext->header;
> +	return true;
> +}
> +
> +static inline bool hsr_skb_add_header_port(struct sk_buff *skb, bool header,
> +					   enum hsr_port_type port)
> +{
> +	struct hsr_ptp_ext *ptp_ext;
> +
> +	ptp_ext = skb_ext_add(skb, SKB_EXT_HSR);
> +	if (!ptp_ext)
> +		return false;
> +	ptp_ext->port = port;
> +	ptp_ext->header = header;
> +	return true;
> +}
> +
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 5555b71ab19b5..ac39b2347aa0f 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -228,20 +228,51 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	rcu_read_lock();
>  	master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> -	if (master) {
> -		skb->dev = master->dev;
> -		skb_reset_mac_header(skb);
> -		skb_reset_mac_len(skb);
> -		spin_lock_bh(&hsr->seqnr_lock);
> -		hsr_forward_skb(skb, master);
> -		spin_unlock_bh(&hsr->seqnr_lock);
> -	} else {
> -		dev_core_stats_tx_dropped_inc(dev);
> -		dev_kfree_skb_any(skb);
> +	if (!master)
> +		goto drop;
> +
> +	skb->dev = master->dev;
> +	if (skb->len > ETH_HLEN * 2) {
> +		struct hsr_inline_header *hsr_opt;
> +
> +		BUILD_BUG_ON(sizeof(struct hsr_inline_header) != sizeof(struct ethhdr));
> +		hsr_opt = (struct hsr_inline_header *)skb_mac_header(skb);
> +		if (hsr_opt->eth_type == htons(ETH_P_1588) &&
> +		    hsr_opt->magic == htonl(HSR_INLINE_HDR)) {
> +			enum hsr_port_type tx_port;
> +			bool has_header;
> +
> +			has_header = hsr_opt->hsr_hdr;
> +			tx_port = hsr_opt->tx_port;
> +			if (tx_port != HSR_PT_SLAVE_A && tx_port != HSR_PT_SLAVE_B)
> +				goto drop;
> +
> +			if (!hsr_skb_add_header_port(skb, has_header, tx_port))
> +				goto drop;

All use of this information happens in the context of this ndo_start_xmit?

If so, no skb_ext needed. Can store the data in skb->cb[], which in
that case is assured to not be overwritten by another layer.

Among various options. skb_ext works, but is a bit heavyhanded for
passing around state within the same layer and call stack.

> +
> +			skb_pull(skb, ETH_HLEN);

Prefer sizeof(struct hsr_inline_header)

> +			if (has_header)
> +				skb_set_network_header(skb, ETH_HLEN + HSR_HLEN);
> +			else
> +				skb_set_network_header(skb, ETH_HLEN);
> +		}
>  	}
> +
> +	skb_reset_mac_header(skb);
> +	skb_reset_mac_len(skb);
> +	spin_lock_bh(&hsr->seqnr_lock);
> +	hsr_forward_skb(skb, master);
> +	spin_unlock_bh(&hsr->seqnr_lock);
> +
>  	rcu_read_unlock();
>  
>  	return NETDEV_TX_OK;
> +
> +drop:
> +	dev_core_stats_tx_dropped_inc(dev);
> +	dev_kfree_skb_any(skb);
> +	rcu_read_unlock();
> +	return NETDEV_TX_OK;
>  }
>