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

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

> > > +#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?

I mean there is no reason your fake header needs to be 14B or
or otherwise resemble an Ethernet header.

That ties into my last comment to use sizeof(struct hsr_inline_header)
where working with that header, rather than ETH_HLEN. It is an
independent header.

This padding and packing is unnecessary.

> > > 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. 

This assumes that the slave device is somehow HSR aware. But standard
net_devices are not?

Isn't it HSR's ndo_start_xmit that selects which slave to forward to,
based on the information in this header?