Re: [PATCH net-next v2 1/3] dsa: Implement RMU layer in DSA

[Mattias Forsblad <mattias.forsblad@gmail.com>]

On 2022-08-26 21:27, Andrew Lunn wrote:
> 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.
>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.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);
>>  	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);
>>  };
> 
> Hi Mattias
> 
> Vladimer pointed you towards the qca driver, in a comment for your
> RFC. qca already has support for switch commands via Ethernet frames.
> 
> The point he was trying to make is that you should look at that
> code. The concept of executing a command via an Ethernet frame, and
> expecting a reply via an Ethernet frame is generic. The format of
> those frames is specific to the switch. We want the generic parts to
> look the same for all switches. If possible, we want to implement it
> once in the dsa core, so all switch drivers share it. Less code,
> better tested code, less bugs, easier maintenance.
> 
> Take a look at qca_tagger_data. Please use the same mechanism with

This I can do which makes sense.

> mv88e6xxx. But also look deeper. What else can be shared? You need a

I can also make a generic dsa_eth_tx_timeout routine which
handles the sending and receiving of cmd frames.

> buffer to put the request in, you need to send it, you need to wait

The skb is the buffer and it's up to the driver to decode it properly?
I've looked into the qca driver and that uses a small static buffer
for replies, no buffer lifetime cycle.

> for the reply, you need to pass the results to the driver, you need to
> free that buffer afterwards. That should all be common. Look at these
> parts in the qca driver. Can you make them generic, move them into the
> DSA core? Are there other parts which could be shared?

I cannot change the qca code as I have no way of verifying that the
resulting code works.

> 
>     Andrew
> 
> 

/Mattias