Message-ID: <20220830154737.no5rd5k4ateu56zs@skbuf>
Date:   Tue, 30 Aug 2022 18:47:37 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v2 1/3] dsa: Implement RMU layer in DSA

On Fri, Aug 26, 2022 at 08:38:14AM +0200, Mattias Forsblad wrote:
> Support handling of layer 2 part for RMU frames which is
> handled in-band with other DSA traffic.

Great explanation, everything is clear!

> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@...il.com>
> ---
>  include/net/dsa.h             |   7 +++
>  include/uapi/linux/if_ether.h |   1 +
>  net/dsa/tag_dsa.c             | 109 +++++++++++++++++++++++++++++++++-
>  3 files changed, 114 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index f2ce12860546..54f7f3494f84 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -92,6 +92,7 @@ struct dsa_switch;
>  struct dsa_device_ops {
>  	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
>  	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
> +	int (*inband_xmit)(struct sk_buff *skb, struct net_device *dev, int seq_no);

There isn't a reason that I can see to expand the DSA tagger ops with an
inband_xmit(). DSA tagging protocol ops are reactive hooks to modify
packets belonging to slave interfaces, rather than initiators of traffic.

Here you are calling tag_ops->inband_xmit() from mv88e6xxx_rmu_tx(),
i.e. this operation is never invoked from the DSA core, but from a code
path fully in control of the hardware driver. We don't usually (ever?)
use DSA ops in this way, but rather just a way for the framework to
invoke driver-specific code.

Is there a reason why dsa_inband_xmit_ll() cannot simply live within the
mv88e6xxx driver (the direct caller) rather than within the dsa/edsa tagger?

Tagging protocols can be changed at driver runtime, but only while the
DSA master is down. So when the master goes up, you can also check which
tagging protocol is in use, and cache/use that to construct the skb.

Furthermore, there is no slave interface associated with RMU traffic,
although in your proposed implementation here, there is (that's what
"struct net_device *dev" passed here is).

Which slave @dev are you passing? That's right, dev_get_by_name(&init_net, "chan0").
I think it's pretty obvious there is a systematic problem with your approach.
Not everyone has a slave net device called chan0 in the main netns.

The qca8k implementation, as opposed to yours, calls dev_queue_xmit()
with skb->dev directly on the DSA master, and forgoes the DSA tagger on
TX. I don't see a problem with that approach, on the contrary, I think
it's better and simpler.

>  	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
>  			     int *offset);
>  	int (*connect)(struct dsa_switch *ds);
> @@ -1193,6 +1194,12 @@ struct dsa_switch_ops {
>  	void	(*master_state_change)(struct dsa_switch *ds,
>  				       const struct net_device *master,
>  				       bool operational);
> +
> +	/*
> +	 * RMU operations
> +	 */
> +	int (*inband_receive)(struct dsa_switch *ds, struct sk_buff *skb,
> +			int seq_no);
>  };
>  
>  #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index d370165bc621..9de1bdc7cccc 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -158,6 +158,7 @@
>  #define ETH_P_MCTP	0x00FA		/* Management component transport
>  					 * protocol packets
>  					 */
> +#define ETH_P_RMU_DSA   0x00FB          /* RMU DSA protocol */

I think it's more normal to set skb->protocol = eth->h_proto = htons(the actual EtherType),
rather than introducing a new skb->protocol which won't be used anywhere.

>  
>  /*
>   *	This is an Ethernet frame header.
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e4b6e3f2a3db..36f02e7dd3c3 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -123,6 +123,90 @@ enum dsa_code {
>  	DSA_CODE_RESERVED_7    = 7
>  };
>  
> +#define DSA_RMU_RESV1   0x3e
> +#define DSA_RMU         1
> +#define DSA_RMU_PRIO    6
> +#define DSA_RMU_RESV2   0xf
> +
> +static int dsa_inband_xmit_ll(struct sk_buff *skb, struct net_device *dev,
> +			      const u8 *header, int header_len, int seq_no)
> +{
> +	static const u8 dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
> +	struct dsa_port *dp;
> +	struct ethhdr *eth;
> +	u8 *data;
> +
> +	if (!dev)
> +		return -ENODEV;
> +
> +	dp = dsa_slave_to_port(dev);
> +	if (!dp)
> +		return -ENODEV;
> +
> +	/* Create RMU L2 header */
> +	data = skb_push(skb, 6);
> +	data[0] = (DSA_CMD_FROM_CPU << 6) | dp->ds->index;
> +	data[1] = DSA_RMU_RESV1 << 2 | DSA_RMU << 1;
> +	data[2] = DSA_RMU_PRIO << 5 | DSA_RMU_RESV2;
> +	data[3] = seq_no;
> +	data[4] = 0;
> +	data[5] = 0;
> +
> +	/* Add header if any */
> +	if (header) {
> +		data = skb_push(skb, header_len);
> +		memcpy(data, header, header_len);
> +	}
> +
> +	/* Create MAC header */
> +	eth = (struct ethhdr *)skb_push(skb, 2 * ETH_ALEN);
> +	memcpy(eth->h_source, dev->dev_addr, ETH_ALEN);
> +	memcpy(eth->h_dest, dest_addr, ETH_ALEN);
> +
> +	skb->protocol = htons(ETH_P_RMU_DSA);
> +
> +	dev_queue_xmit(skb);

Just for things to be 100% clear for everyone. Per your design, we have
dsa_inband_xmit() which gets called by the driver with a slave @dev, and
this constructs an skb without the DSA/EDSA header. Then dev_queue_xmit()
will invoke the ndo_start_xmit of DSA, dsa_slave_xmit(). In turn this
will enter the tagging protocol driver a second time, through p->xmit() ->
dsa_xmit_ll(). The second time is when the DSA/EDSA header is actually
introduced.

This is way more complicated than it needs to be.

> +
> +	return 0;
> +}
> +
> +static int dsa_inband_rcv_ll(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct dsa_switch *ds;
> +	int source_device;
> +	u8 *dsa_header;
> +	int rcv_seqno;
> +	int ret = 0;
> +
> +	if (!dev || !dev->dsa_ptr)
> +		return 0;

Way too defensive programming which wastes CPU cycles for nothing. The
DSA rcv hook runs as an ETH_P_XDSA packet_type handler on the DSA master,
based on eth_type_trans() which had set skb->protocol to ETH_P_XDSA
based on the presence of dev->dsa_ptr. So yes, the DSA master is not NULL,
and yes, the DSA master's dev->dsa_ptr is not NULL either.

> +
> +	ds = dev->dsa_ptr->ds;
> +	if (!ds)
> +		return 0;

We don't have NULL pointers in cpu_dp->ds lying around. dp->ds is set by
dsa_port_touch(), which runs earlier than dsa_master_setup() sets
dev->dsa_ptr in such a way that we start processing RX packets.

> +
> +	dsa_header = skb->data - 2;

Please use dsa_etype_header_pos_rx(). In fact, this pointer was already
available in dsa_rcv_ll().

> +
> +	source_device = dsa_header[0] & 0x1f;
> +	ds = dsa_switch_find(ds->dst->index, source_device);
> +	if (!ds) {
> +		net_dbg_ratelimited("DSA inband: Didn't find switch with index %d", source_device);
> +		return -EINVAL;
> +	}
> +
> +	/* Get rcv seqno */
> +	rcv_seqno = dsa_header[3];
> +
> +	skb_pull(skb, DSA_HLEN);
> +
> +	if (ds->ops && ds->ops->inband_receive(ds, skb, rcv_seqno)) {

I think the reason why Andrew is asking you to find common aspects with
qca8k which can be further generalized is because your proposed code is
very ambitious, introducing a generic ds->ops->inband_receive() like this.

I personally wouldn't be so ambitious for myself. The way the qca8k
driver and tagging protocol work together is that they set up a group of
driver-specific function pointers, rw_reg_ack_handler and mib_autocast_handler,
through which the switch driver connected to the tagger may subscribe as
a consumer to the received Ethernet management packets. Whereas you are
proposing a one-size-fits-all ds->ops->inband_receive with no effort to
see if it fits even the single other user of this concept, qca8k.

What I would do is introduce one more case of driver-specific consumer
of RMU packets, specific to the dsa/edsa tagger <-> mv88e6xxx driver pair.
I'd let things sit for a while, maybe wait for a third user, then see
how/if the prototype for consuming Ethernet management packets can be
made generic.

But in general, we need drivers to handle non-data RX packets coming
from the switch for all sorts of reasons. For example SJA1110 delivers
back TX timestamps as Ethernet packets, and I wouldn't consider
expanding ds->ops for that. This driver-specific hook mechanism
("tagger owned storage" as I named it) is flexible enough to allow each
driver to respond to the needs of its hardware, without needing
everybody else to follow suit or suffer of ops bloat because of it.
I wouldn't rush.

> +		dev_dbg_ratelimited(ds->dev, "DSA inband: error decoding packet");
> +		ret = -EIO;
> +	}
> +
> +	return ret;

Powered by blists - more mailing lists