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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2026-05-04 13:08:04 [-0400], Willem de Bruijn wrote:
> > > > +#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?
> 
> I mean there is no reason your fake header needs to be 14B or
> or otherwise resemble an Ethernet header.
> 
> That ties into my last comment to use sizeof(struct hsr_inline_header)
> where working with that header, rather than ETH_HLEN. It is an
> independent header.

But I can't expect that the header is always there. A random ping/ arp
packet goes via the same flow. At the same time I don't want to make it
mandatory for all AF_PACKET users by checking the skb's socket.

The skb starts always with the ethernet header. To avoid collisions with
a regular packet that looks similar I use the ether-type of the ethernet
frame and it has to match PTP. It makes no sense to send a PTP packet via
HSR. Since ether-type is at the end, I can't make it smaller than 14b.
So eth-type is safeguard #1 and the second is SRC-MAC with a multicast
sender bit set which is usually not the case.

> This padding and packing is unnecessary.

The padding is needed to match the ethernet-type and restrict it to PTP
only. Then packing is needed so it is not aligned by the compiler.

> > > > 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. 
> 
> This assumes that the slave device is somehow HSR aware. But standard
> net_devices are not?

Yes, standard devices are not hsr-aware. It is just your average
ethernet device that sends everything as-is.
But you can have devices with HSR-offload capabilities such as
NETIF_F_HW_HSR_FWD, NETIF_F_HW_HSR_DUP, NETIF_F_HW_HSR_TAG_INS,
NETIF_F_HW_HSR_TAG_RM.

So if a device provides NETIF_F_HW_HSR_TAG_INS and it is enabled
(via ethtool -K) then the HSR stack will pass regular skb and the device
will prepend the HSR-header (and maintain the HSR-sequence number and so
on) before putting it on the wire.
Also, the HSR stack will send a skb on both slave interfaces which are
regular ethernet devices. But with NETIF_F_HW_HSR_DUP enabled it will
pass it only to the first slave and expect the underlying device to
forward it also via the other slave interface.

So I need to pass skb which is can be sent by a dumb device but at the
same time a device which does offload needs to be able to know when it
should not do it.

> Isn't it HSR's ndo_start_xmit that selects which slave to forward to,
> based on the information in this header?

Yes, this is correct. I needed to verify that the slave device with
offload capabilities does not send the skb on both ports and add a
HSR header while doing so. The non-offloading devices are not an issue,
it sends packets as-is.
I managed to rework it and it works without the skb-ext. I just track
the HSR mode (HSR vs PRP) and then parse the packet for its type and
decide what needs to be done.
This works now. Once we settle on the header I can repost it.

Sebastian