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

;)

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

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

> > +
> > +			skb_pull(skb, ETH_HLEN);
> 
> Prefer sizeof(struct hsr_inline_header)

Okay.

Sebastian