Re: [PATCH net-next v3 02/12] net: dsa: add Renesas RZ/N1 switch tag driver

["Clément Léger" <clement.leger@bootlin.com>]

Le Wed, 4 May 2022 19:00:39 +0300,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> > +/* To define the outgoing port and to discover the incoming port a TAG is
> > + * inserted after Src MAC :
> > + *
> > + *       Dest MAC       Src MAC           TAG         Type
> > + * ...| 1 2 3 4 5 6 | 1 2 3 4 5 6 | 1 2 3 4 5 6 7 8 | 1 2 |...
> > + *                                |<--------------->|
> > + *
> > + * See struct a5psw_tag for layout
> > + */
> > +
> > +#define A5PSW_TAG_VALUE			0xE001  
> 
> Maybe an ETH_P_DSA_A5PSW definition in include/uapi/linux/if_ether.h
> would be appropriate? I'm not sure.

That's a good question. Actually, this value is the default 
but is configurable in the hardware so I'm not sure this should be a
reserved value. Maybe it would make sense to add it anyway to have the
define shared between the switch driver and the tag driver.

> 
> > +#define A5PSW_TAG_LEN			8
> > +#define A5PSW_CTRL_DATA_FORCE_FORWARD	BIT(0)
> > +/* This is both used for xmit tag and rcv tagging */
> > +#define A5PSW_CTRL_DATA_PORT		GENMASK(3, 0)
> > +
> > +struct a5psw_tag {
> > +	__be16 ctrl_tag;
> > +	__be16 ctrl_data;
> > +	__be16 ctrl_data2_hi;
> > +	__be16 ctrl_data2_lo;
> > +};
> > +
> > +static struct sk_buff *a5psw_tag_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +	struct dsa_port *dp = dsa_slave_to_port(dev);
> > +	struct a5psw_tag *ptag;
> > +	u32 data2_val;
> > +
> > +	BUILD_BUG_ON(sizeof(*ptag) != A5PSW_TAG_LEN);
> > +
> > +	/* The Ethernet switch we are interfaced with needs packets to be at
> > +	 * least 64 bytes (including FCS) otherwise they will be discarded when
> > +	 * they enter the switch port logic. When tagging is enabled, we need
> > +	 * to make sure that packets are at least 70 bytes (including FCS and
> > +	 * tag).
> > +	 */
> > +	if (__skb_put_padto(skb, ETH_ZLEN + ETH_FCS_LEN + A5PSW_TAG_LEN, false))
> > +		return NULL;  
> 
> I'm confused by the inclusion of the FCS length in this calculation,
> since the FCS space isn't present in the skb buffer as far as I know?

I'm not sure either, the documentation is not really clear on what is
the requirement for the minimal size of a packet. This was the closest
thing I could find about that requirement:

"A frame has a valid length if it contains at least 64 octets and does
not exceed the programmed maximum length"

And the figure associated to the frame show that the frame length
includes the FCS which lead to a 64bytes frame.
> 
> "64 bytes including FCS" means "60 bytes excluding FCS".
> And ETH_ZLEN is 60...
> 
> And I'm also not sure how we got to the number 70? A5PSW_TAG_LEN is 8.
> If we add it to 60 we get 68. If we add it to 64 we get 72?
>

I'll check all these numbers and find the correct size that is expected
by the switch.

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com