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

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

On 2026-05-08 13:34:04 [-0400], Willem de Bruijn wrote:
> It probably makes sense to integrate this with the main commit message
> that is preserved in the log.
> 
> Or as a cover letter if this goes to a multi-patch series.

b4 did this since it was a single patch. If I split it into multiple
patches (as suggested below) then the cover letter should be created.

> > index 5555b71ab19b5..cab4f7c601257 100644
> > --- a/net/hsr/hsr_device.c
> > +++ b/net/hsr/hsr_device.c
> > @@ -223,24 +223,49 @@ static netdev_features_t hsr_fix_features(struct net_device *dev,
> >  
> >  static netdev_tx_t hsr_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> >  {
> > +	enum hsr_port_type tx_port = HSR_PT_NONE;
> >  	struct hsr_priv *hsr = netdev_priv(dev);
> >  	struct hsr_port *master;
> > +	bool has_header = false;
> >  
> >  	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) {
> 
> Try to avoid magic constants
> 
> sizeof(struct hsr_inline_header + ETH_HLEN is better self-documenting
> that these are two consecutive protocol headers.

okay.

> PF_PACKET sockets also choose an skb->protocol. Even if you don't use
> that as the sole signal that such a header is present, you probably
> want to check it and ignore (or drop) packets with a different than
> the one expected value (ETH_P_1588?).

I have been looking at skb->protocol. This originates from po->num
(packet_snd(), saddr == NULL) because userland does bind() with
sll_protocol = htons(ETH_P_ALL). This gets then overwritten in
packet_parse_headers() and is set to 0 because hsr_header_ops provides only
->create and ->parse, no ->parse_protocol. 

So skb->protocol is 0 in xmit. Having eth_header_parse_protocol() would
return ETH_P_1588 in my inline-header case.

So you want something like

	if (skb->protocol == ETH_P_1588 && skb->len large_enough)

? Then could drop the ETH_P_1588 header check and just stick the magic
check. Did I understood you correctly?

Regarding skb->protocol assignment.
This was introduced in
e78b2915517e8 ("net: Introduce parse_protocol header_ops callback")
which is from 2019 and hsr_header_ops is from 2013.
Should this be added, and if so should it reflect the "normal" header or
the after the hsr header inner type? I lean towards the ethernet header
but I am not sure about the implications here. So I would assign
eth_header_parse_protocol here.

> > +		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);
> 
> Need a pskb_may_pull before accessing fields?

I did check the size. Should I use pskb_may_pull() instead of checking
skb->len directly?

…
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index 0aca859c88cbb..7f92314934c98 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"
> >  
> > @@ -343,7 +344,10 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> >  		hsr_set_path_id(frame, 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 (frame->req_tx_port != HSR_PT_NONE)
> > +			skb_set_owner_w(skb, frame->skb_std->sk);
> 
> How is this related to the rest of the patch?

The user sends the data in the skb. Here the skb gets the hsr header
assigned. In the offload case it is simply cloned, in the SW-case
(below) the header is added. The original (submitted) skb is thrown away
while the cloned one is submitted to the networking driver. I needs a
socket assigned or otherwise the requested timestamp is never submitted
to the user.

> > +		return skb;
> >  	}
> >  
> >  	/* Create the new skb with enough headroom to fit the HSR tag */
…
> > @@ -697,10 +733,15 @@ static int fill_frame_info(struct hsr_frame_info *frame,
> >  	if (port->type == HSR_PT_INTERLINK)
> >  		n_db = &hsr->proxy_node_db;
> >  
> > -	frame->node_src = hsr_get_node(port, n_db, skb,
> > -				       frame->is_supervision, port->type);
> > -	if (!frame->node_src)
> > -		return -1; /* Unknown node and !is_supervision, or no mem */
> > +	frame->req_tx_port = tx_port;
> > +	frame->has_foreign_header = has_hsr_header;
> > +
> > +	if (!frame->has_foreign_header) {
> > +		frame->node_src = hsr_get_node(port, n_db, skb,
> > +					       frame->is_supervision, port->type);
> > +		if (!frame->node_src)
> > +			return -1; /* Unknown node and !is_supervision, or no mem */
> > +	}
> 
> This is quite a large patch. Does it make sense to split this
> has_foreign_header into a separate first patch, before the second
> patch that handles the ETH_P_1588 packets?

If it is easier for you, I could try split it in more individual
patches. The skb_set_owner_w() part from above could be its own logical
change. Let me think of something.

Sebastian