On 2026-04-29 13:46:13 [-0400], Willem de Bruijn wrote:

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

;)

quoted

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?

What do you mean? eth_type is at the same spot or do you mean it should
be named h_proto?

quoted

 /* 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?

I receive it from af_packet in HSR's ndo_start_xmit, yes. Then
hsr_xmit() is forwarding it to the slave device via dev_queue_xmit().
Here the skb->cb information gets overwritten.

I need this hint in the slave eth driver in case there is hsr-offloading
available. Now that I look at it again, there a netdev-event once the
HSR-master is set/ changed. So maybe I can use this and look at the
skb frame to decide what is required. Le me see.

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.

Sure.

quoted

+
+			skb_pull(skb, ETH_HLEN);

Prefer sizeof(struct hsr_inline_header)

Okay.

Sebastian