Re: [PATCH RFC net-next v2 2/2] af_packet: Add port specific handling for HSR

Willem de Bruijn
2026-03-19T13:29:47Z

Sebastian Andrzej Siewior wrote:
> On 2026-03-16 16:12:41 [-0400], Willem de Bruijn wrote:
> > > The suggested changes to af_packet, that remains a hard no?
> >
> > Yes. Packet sockets are largely superseded by XDP and AF_XDP when it
> > comes to new functionality. And were never open for such protocol
> > specific logic.
>
> ptp4l uses AF_PACKET and while I am not against AF_XDP I am not sure if
> it helps. I don't have any protocol stack so extending SOL_PACKET was
> the only option left.
>
> > Protocol independent extensions, such as reading skb->mark as part of
> > extended auxdata, could be up for debate. But again, we already have
> > XDP and AF_XDP which allow passing arbitrary metadata to/from
> > userspace. That is preferable over adding new structs to the ABI.
>
> Is this struct xsk_tx_metadata and something similar for RX? If so, I
> would require HSR fields for both.
> My understanding is that you bind XDP to device+queue. I would require
> four sockets (generic+event for both slaves) so this does not work,
> right? Also I would need to extend HSR stack so support XDP in a similar
> fashion as network driver do?
>
> > If you only want to attach to a single (hsr0) device, I still think
> > passing the data inline might work. Either by overriding existing
> > fields such as a MAC addr (a hack for sure) or something in the HSR
> > header (it as the direction? or by inserting a custom header (or
> > trailer) akin to virtio_net_hdr for tun/tap. But custom to your
> > workload.
>
> But here you have PACKET_VNET_HDR_SZ to configure virtio_net_hdr.

I mean an implicit header in the packet. virtio_net_hdr is something
that the packet socket needs to inspect. The metadata between ptp4l
and hsr_dev_xmit is not, so the packet socket can be oblivious to it.

> This
> is kind of what I ask for with PACKET_HSR_INFO + PACKET_HSR_BIND_PORT
> (and I added static branches so nobody has to suffer with CONFIG_HSR
> enabled but no HSR enabled socket)..
>
> The diff below would be what means to bypass AF_PACKET entirely and use
> eth0/ eth1 to send via SLAVE_A/ B and hsr0 with SO_MARK to send with
> system's HSR header but only on one of the two ports.
> With HSR-offloading enabled we need to attach the SO_MARK hints also on
> eth0/ eth1 devices, too. Otherwise the offloading part will send the
> packet on both ports and attach a header (as designed).

eth0/eth1 are not HSR devices, so how would they interpret skb->mark
and how would they loop packets to the other port?

>
> Now I have now 8 sockets in userland instead of 4.
> I added icssg to show the offloading case:
>
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 79df641b4fb97..a2e50cae01686 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
> @@ -909,13 +909,17 @@ enum netdev_tx icssg_ndo_start_xmit(struct sk_buff *skb, struct net_device *ndev
> */
> dst_tag_id = emac->port_id | (q_idx << 8);
>
> - if (prueth->is_hsr_offload_mode &&
> - (ndev->features & NETIF_F_HW_HSR_DUP))
> - dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
> + if (prueth->is_hsr_offload_mode) {
> + bool has_header;
> + bool has_port;
>
> - if (prueth->is_hsr_offload_mode &&
> - (ndev->features & NETIF_F_HW_HSR_TAG_INS))
> - epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
> + hsr_skb_get_header_port_mark(skb, &has_header, &has_port);
> + if (ndev->features & NETIF_F_HW_HSR_DUP && !has_port)
> + dst_tag_id = PRUETH_UNDIRECTED_PKT_DST_TAG;
> +
> + if (ndev->features & NETIF_F_HW_HSR_TAG_INS && !has_header)
> + epib[1] |= PRUETH_UNDIRECTED_PKT_TAG_INS;
> + }
>
> cppi5_desc_set_tags_ids(&first_desc->hdr, 0, dst_tag_id);
> k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
> diff --git a/include/linux/if_hsr.h b/include/linux/if_hsr.h
> index d7941fd880329..814b30f65f102 100644
> --- a/include/linux/if_hsr.h
> +++ b/include/linux/if_hsr.h
> @@ -22,6 +22,11 @@ enum hsr_port_type {
> HSR_PT_PORTS, /* This must be the last item in the enum */
> };
>
> +struct hsr_ptp_cb {
> + u8 ptp;
> + u8 port;
> +};
> +
> /* 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,
> @@ -43,6 +48,57 @@ extern bool is_hsr_master(struct net_device *dev);
> extern int hsr_get_version(struct net_device *dev, enum hsr_version *ver);
> struct net_device *hsr_get_port_ndev(struct net_device *ndev,
> enum hsr_port_type pt);
> +
> +static inline unsigned int hsr_skb_get_TX_port(struct sk_buff *skb)
> +{
> + struct hsr_ptp_cb *cb;
> +
> + if (!skb)
> + return 0;
> +
> + cb = (struct hsr_ptp_cb*)skb->cb;
> + if (!cb->ptp)
> + return 0;
> + return cb->port;
> +}
> +
> +static inline void hsr_skb_set_TX_port(struct sk_buff *skb,
> + enum hsr_port_type port)
> +{
> + struct hsr_ptp_cb *cb;
> +
> + cb = (struct hsr_ptp_cb*)skb->cb;
> + cb->ptp = 1;
> + cb->port = port;
> +}
> +
> +static inline void hsr_skb_non_ptp(struct sk_buff *skb)
> +{
> + struct hsr_ptp_cb *cb;
> +
> + cb = (struct hsr_ptp_cb*)skb->cb;
> + cb->ptp = 0;
> +}
> +
> +static inline void hsr_skb_get_header_port_mark(struct sk_buff *skb, bool *has_header,
> + bool *has_port)
> +{
> + u32 mark = skb->mark;
> + u32 val;
> +
> + val = mark & 0b11;
> + if (val == 1 || val == 2)
> + *has_port = true;
> + else
> + *has_port = false;
> +
> + val = mark & 0b100;
> + if (val)
> + *has_header = true;
> + else
> + *has_header = false;
> +}
> +
> #else
> static inline bool is_hsr_master(struct net_device *dev)
> {
> @@ -59,6 +115,13 @@ static inline struct net_device *hsr_get_port_ndev(struct net_device *ndev,
> {
> return ERR_PTR(-EINVAL);
> }
> +
> +static inline void hsr_skb_get_header_port_mark(struct sk_buff *skb, bool *has_header,
> + bool *has_port)
> +{
> + *has_port = false;
> + *has_header = false;
> +}
> #endif /* CONFIG_HSR */
>
> #endif /*_LINUX_IF_HSR_H_*/
> diff --git a/net/hsr/hsr_device.c b/net/hsr/hsr_device.c
> index 2c43776b7c4fb..e01d3a33ac941 100644
> --- a/net/hsr/hsr_device.c
> +++ b/net/hsr/hsr_device.c
> @@ -229,6 +229,14 @@ static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> master = hsr_port_get_hsr(hsr, HSR_PT_MASTER);
> if (master) {
> skb->dev = master->dev;
> +
> + if (skb->mark == 1)
> + hsr_skb_set_TX_port(skb, HSR_PT_SLAVE_A);
> + else if (skb->mark == 2)
> + hsr_skb_set_TX_port(skb, HSR_PT_SLAVE_B);
> + else
> + hsr_skb_non_ptp(skb);
> +
> skb_reset_mac_header(skb);
> skb_reset_mac_len(skb);
> spin_lock_bh(&hsr->seqnr_lock);
> @@ -355,6 +363,7 @@ static void send_hsr_supervision_frame(struct hsr_port *port,
> return;
> }
>
> + hsr_skb_non_ptp(skb);
> hsr_forward_skb(skb, port);
> spin_unlock_bh(&hsr->seqnr_lock);
> return;
> @@ -396,6 +405,7 @@ static void send_prp_supervision_frame(struct hsr_port *master,
> return;
> }
>
> + hsr_skb_non_ptp(skb);
> hsr_forward_skb(skb, master);
> spin_unlock_bh(&hsr->seqnr_lock);
> }
> diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> index c67c0d35921de..d0d6a8342bb33 100644
> --- a/net/hsr/hsr_forward.c
> +++ b/net/hsr/hsr_forward.c
> @@ -12,6 +12,7 @@
> #include <linux/skbuff.h>
> #include <linux/etherdevice.h>
> #include <linux/if_vlan.h>
> +#include <net/sock.h>
> #include "hsr_main.h"
> #include "hsr_framereg.h"
>
> @@ -333,7 +334,10 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> hsr_set_path_id(hsr_ethhdr, port);
> return skb_clone(frame->skb_hsr, GFP_ATOMIC);
> } else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> - return skb_clone(frame->skb_std, GFP_ATOMIC);
> + skb = skb_clone(frame->skb_std, GFP_ATOMIC);
> + if (hsr_skb_get_TX_port(skb))
> + skb_set_owner_w(skb, frame->skb_std->sk);
> + return skb;
> }
>
> /* Create the new skb with enough headroom to fit the HSR tag */
> @@ -355,6 +359,15 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> memmove(dst, src, movelen);
> skb_reset_mac_header(skb);
>
> + if (hsr_skb_get_TX_port(skb)) {
> + /* Packets are bound to a port and the sender may expect time
> + * information.
> + */
> + skb_shinfo(skb)->tx_flags = skb_shinfo(frame->skb_std)->tx_flags;
> + skb_shinfo(skb)->tskey = skb_shinfo(frame->skb_std)->tskey;
> + skb_set_owner_w(skb, frame->skb_std->sk);
> + }
> +
> /* skb_put_padto free skb on error and hsr_fill_tag returns NULL in
> * that case
> */
> @@ -377,12 +390,23 @@ struct sk_buff *prp_create_tagged_frame(struct hsr_frame_info *frame,
> }
> return skb_clone(frame->skb_prp, GFP_ATOMIC);
> } else if (port->dev->features & NETIF_F_HW_HSR_TAG_INS) {
> - return skb_clone(frame->skb_std, GFP_ATOMIC);
> + skb = skb_clone(frame->skb_std, GFP_ATOMIC);
> + if (hsr_skb_get_TX_port(skb))
> + skb_set_owner_w(skb, frame->skb_std->sk);
> + return skb;
> }
>
> skb = skb_copy_expand(frame->skb_std, skb_headroom(frame->skb_std),
> skb_tailroom(frame->skb_std) + HSR_HLEN,
> GFP_ATOMIC);
> + if (hsr_skb_get_TX_port(skb)) {
> + /* Packets are bound to a port and the sender may expect time
> + * information.
> + */
> + skb_shinfo(skb)->tx_flags = skb_shinfo(frame->skb_std)->tx_flags;
> + skb_shinfo(skb)->tskey = skb_shinfo(frame->skb_std)->tskey;
> + skb_set_owner_w(skb, frame->skb_std->sk);
> + }
> return prp_fill_rct(skb, frame, port);
> }
>
> @@ -508,10 +532,13 @@ bool hsr_drop_frame(struct hsr_frame_info *frame, struct hsr_port *port)
> */
> static void hsr_forward_do(struct hsr_frame_info *frame)
> {
> + unsigned int req_tx_port;
> struct hsr_port *port;
> struct sk_buff *skb;
> bool sent = false;
>
> + req_tx_port = hsr_skb_get_TX_port(frame->skb_std);
> +
> hsr_for_each_port(frame->port_rcv->hsr, port) {
> struct hsr_priv *hsr = port->hsr;
> /* Don't send frame back the way it came */
> @@ -532,6 +559,16 @@ static void hsr_forward_do(struct hsr_frame_info *frame)
> if ((port->dev->features & NETIF_F_HW_HSR_DUP) && sent)
> continue;
>
> + /* RX PTP packets have the received port recorded */
> + if (frame->skb_hsr)
> + skb = frame->skb_hsr;
> + else if (frame->skb_prp)
> + skb = frame->skb_prp;
> +
> + /* PTP TX packets have an outgoing port specified */
> + if (req_tx_port != HSR_PT_NONE && req_tx_port != port->type)
> + continue;
> +
> /* Don't send frame over port where it has been sent before.
> * Also for SAN, this shouldn't be done.
> */
> diff --git a/net/hsr/hsr_slave.c b/net/hsr/hsr_slave.c
> index 464f683e016db..f4879d1ee8ab6 100644
> --- a/net/hsr/hsr_slave.c
> +++ b/net/hsr/hsr_slave.c
> @@ -44,8 +44,7 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
>
> if (hsr_addr_is_self(port->hsr, eth_hdr(skb)->h_source)) {
> /* Directly kill frames sent by ourselves */
> - kfree_skb(skb);
> - goto finish_consume;
> + goto finish_free_consume;
> }
>
> /* For HSR, only tagged frames are expected (unless the device offloads
> @@ -70,15 +69,36 @@ static rx_handler_result_t hsr_handle_frame(struct sk_buff **pskb)
> /* Only the frames received over the interlink port will assign a
> * sequence number and require synchronisation vs other sender.
> */
> + hsr_skb_non_ptp(skb);
> if (port->type == HSR_PT_INTERLINK) {
> spin_lock_bh(&hsr->seqnr_lock);
> hsr_forward_skb(skb, port);
> spin_unlock_bh(&hsr->seqnr_lock);
> } else {
> + struct hsr_ethhdr *hsr_ethhdr;
> +
> + /* PTP packets are not supposed to be forwarded via HSR/ PRP
> + * as-is. The latency introduced by forwarding renders
> + * the time information useless.
> + */
> + if ((!hsr->prot_version && protocol == htons(ETH_P_PRP)) ||
> + protocol == htons(ETH_P_HSR)) {
> + /* HSR */
> + hsr_ethhdr = (struct hsr_ethhdr *)skb_mac_header(skb);
> + if (hsr_ethhdr->hsr_tag.encap_proto == htons(ETH_P_1588))
> + goto finish_free_consume;
> + } else {
> + if (protocol == htons(ETH_P_1588))
> + goto finish_free_consume;
> + }
> +
> hsr_forward_skb(skb, port);
> }
>
> -finish_consume:
> + return RX_HANDLER_CONSUMED;
> +
> +finish_free_consume:
> + kfree_skb(skb);
> return RX_HANDLER_CONSUMED;
>
> finish_pass:
>
> Sebastian