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

Willem de Bruijn
2026-04-29T17:46:16Z

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