Date:   Wed, 31 Aug 2022 07:55:18 +0200
From:   Mattias Forsblad <mattias.forsblad@...il.com>
To:     Vladimir Oltean <olteanv@...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 2022-08-30 17:47, Vladimir Oltean wrote:

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

Yes, I agree and will rework it more in line with qca8k.

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

See comment above.

>> +
>> +	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().
> 

I did see it, thanks.

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

I'll come back with a new version to discuss.

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

Powered by blists - more mailing lists