Re: [PATCH net-next v9 2/8] hsr: Use skb_clone() while adding the HSR header

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

On 2026-10-16 11:09:40 [+0200], Daniel Machon wrote:
> > diff --git a/net/hsr/hsr_forward.c b/net/hsr/hsr_forward.c
> > index 7734a521a96c2..5862c05f52110 100644
> > --- a/net/hsr/hsr_forward.c
> > +++ b/net/hsr/hsr_forward.c
> > @@ -342,33 +341,40 @@ struct sk_buff *hsr_create_tagged_frame(struct hsr_frame_info *frame,
> >  		/* set the lane id properly */
> >  		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);
> >  	}
> >  
> > -	/* Create the new skb with enough headroom to fit the HSR tag */
> > -	skb = __pskb_copy(frame->skb_std,
> > -			  skb_headroom(frame->skb_std) + HSR_HLEN, GFP_ATOMIC);
> > +	skb = skb_clone(frame->skb_std, GFP_ATOMIC);
> >  	if (!skb)
> >  		return NULL;
> > -	skb_reset_mac_header(skb);
> >  
> > -	if (skb->ip_summed == CHECKSUM_PARTIAL)
> > -		skb->csum_start += HSR_HLEN;
> > +	if (port->dev->features & NETIF_F_HW_HSR_TAG_INS)
> > +		return skb;
> > +
> > +	/* Ensure the cloned skb has enough headroom to fit the HSR tag */
> > +	if (skb_cow_head(skb, HSR_HLEN))
> > +		goto err;
> >  
> >  	movelen = ETH_HLEN;
> >  	if (frame->is_vlan)
> >  		movelen += VLAN_HLEN;
> >  
> > -	src = skb_mac_header(skb);
> > -	dst = skb_push(skb, HSR_HLEN);
> > -	memmove(dst, src, movelen);
> > +	/* The entire header area needs to be linear */
> > +	if (!pskb_may_pull(skb, movelen))
> > +		goto err;
> > +
> > +	skb_push(skb, HSR_HLEN);
> > +	skb_postpush_data_move(skb, HSR_HLEN, movelen);
> > +	skb_set_network_header(skb, HSR_HLEN + movelen);
> >  	skb_reset_mac_header(skb);
> > +	skb_reset_mac_len(skb);
> 
> It took me some time to realize why skb_reset_mac_header() and the
> csum_start adjustment were removed, and why skb_cow_head(),
> skb_set_network_header() and skb_reset_mac_len() were added. Could the
> commit message say why? Or maybe a comment.

If this is important, I could add to the patch description. The removed
skb_reset_mac_header() should be obvious.
The csum_start() removal and so on was mostly sashiko complaining about
one thing at at time and memove() got replaced with
skb_postpush_data_move() in the last iteration which I just learned
about myself ;) That looks like a nice clean up.

> /Daniel

Sebastian