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

Sebastian Andrzej Siewior
2026-05-04T08:59:14Z

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