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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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