[PATCH net-next v2 0/3] net: dsa: mv88e6xxx: Add RMU support

[PATCH net-next v2 0/3] net: dsa: mv88e6xxx: Add RMU support

[Mattias Forsblad]

The Marvell SOHO switches have the ability to receive and transmit
Remote Management Frames (Frame2Reg) to the CPU through the
attached network interface.
This is handled by the Remote Management Unit (RMU).
These frames can contain different payloads:
single switch register read and writes, daisy chained switch
register read and writes, RMON/MIB dump/dump clear and ATU dump.
The dump functions are very costly over MDIO but it's
only a couple of network packets via the RMU.

Next step could be to implement ATU dump.
We've found that the gain to use RMU for single register
read and writes is neglible.

RFC -> v1:
  - Track master interface availability.
  - Validate destination MAC for incoming frames.
  - Rate limit outputs.
  - Cleanup setup function validating upstream port on switch.
  - Fix return values when setting up RMU.
  - Prefix defines correctly.
  - Fix aligned accesses.
  - Validate that switch exists for incoming frames.
  - Split RMON stats function.

v1 -> v2:
  - Remove unused variable.

Regards,
Mattias Forsblad

Mattias Forsblad (3):
  dsa: Implement RMU layer in DSA
  dsa: mv88e6xxx: Add support for RMU in select switches
  rmon: Use RMU if available

 drivers/net/dsa/mv88e6xxx/Makefile  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c    |  54 +++++-
 drivers/net/dsa/mv88e6xxx/chip.h    |  20 ++
 drivers/net/dsa/mv88e6xxx/global1.c |  84 +++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |   3 +
 drivers/net/dsa/mv88e6xxx/rmu.c     | 273 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h     |  33 ++++
 include/net/dsa.h                   |   7 +
 include/uapi/linux/if_ether.h       |   1 +
 net/dsa/tag_dsa.c                   | 109 ++++++++++-
 10 files changed, 575 insertions(+), 10 deletions(-)
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.h

--
2.25.1

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

[Mattias Forsblad]

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);
 };
 
 #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 */
 
 /*
  *	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);
+
+	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;
+
+	ds = dev->dsa_ptr->ds;
+	if (!ds)
+		return 0;
+
+	dsa_header = skb->data - 2;
+
+	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)) {
+		dev_dbg_ratelimited(ds->dev, "DSA inband: error decoding packet");
+		ret = -EIO;
+	}
+
+	return ret;
+}
+
 static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 				   u8 extra)
 {
@@ -218,9 +302,7 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 
 		switch (code) {
 		case DSA_CODE_FRAME2REG:
-			/* Remote management is not implemented yet,
-			 * drop.
-			 */
+			dsa_inband_rcv_ll(skb, dev);
 			return NULL;
 		case DSA_CODE_ARP_MIRROR:
 		case DSA_CODE_POLICY_MIRROR:
@@ -325,6 +407,12 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
 
+static int dsa_inband_xmit(struct sk_buff *skb, struct net_device *dev,
+			   int seq_no)
+{
+	return dsa_inband_xmit_ll(skb, dev, NULL, 0, seq_no);
+}
+
 static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	return dsa_xmit_ll(skb, dev, 0);
@@ -343,6 +431,7 @@ static const struct dsa_device_ops dsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_DSA,
 	.xmit	  = dsa_xmit,
 	.rcv	  = dsa_rcv,
+	.inband_xmit = dsa_inband_xmit,
 	.needed_headroom = DSA_HLEN,
 };
 
@@ -354,6 +443,19 @@ MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_DSA);
 
 #define EDSA_HLEN 8
 
+static int edsa_inband_xmit(struct sk_buff *skb, struct net_device *dev,
+			    int seq_no)
+{
+	u8 edsa_header[4];
+
+	edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
+	edsa_header[1] = ETH_P_EDSA & 0xff;
+	edsa_header[2] = 0x00;
+	edsa_header[3] = 0x00;
+
+	return dsa_inband_xmit_ll(skb, dev, edsa_header, 4, seq_no);
+}
+
 static struct sk_buff *edsa_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	u8 *edsa_header;
@@ -385,6 +487,7 @@ static const struct dsa_device_ops edsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_EDSA,
 	.xmit	  = edsa_xmit,
 	.rcv	  = edsa_rcv,
+	.inband_xmit = edsa_inband_xmit,
 	.needed_headroom = EDSA_HLEN,
 };
 
-- 
2.25.1

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

[Andrew Lunn]

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
mv88e6xxx. But also look deeper. What else can be shared? You need a
buffer to put the request in, you need to send it, you need to wait
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?

    Andrew

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

[Mattias Forsblad]

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

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

[Andrew Lunn]

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

Yes, the tagger layer passes the skb, which contains the complete
Ethernet frame, plus additional meta data. But you need to watch out
for the life cycle of that skb:

        /* Ethernet mgmt read/write packet */
        if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
                if (likely(tagger_data->rw_reg_ack_handler))
                        tagger_data->rw_reg_ack_handler(ds, skb);
                return NULL;
        }

returning NULL means the DSA core will free the skb when the call to
qca_tag_rcv() returns. So either you need to take a copy of the data,
clone the skb, or change this code somehow. See what makes most sense
for generic code.


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

You can change it. You can at least compile test your change. The QCA
developers are also quite active, and so will probably help out
testing whatever you change. And ideally, you want lots of simple,
obviously correct changes, which i can review and say look correct.

Changing code which you cannot test is very normal in Linux, do your
best, and ask for testing.

      Andrew

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

[Vladimir Oltean]

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@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);

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;

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

[Mattias Forsblad]

On 2022-08-30 17:47, Vladimir Oltean wrote:

>> 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);
> 
> 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;

[PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Mattias Forsblad]

Implement support for handling RMU layer 3 frames
including receive and transmit.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c    |  13 ++
 drivers/net/dsa/mv88e6xxx/chip.h    |  20 ++
 drivers/net/dsa/mv88e6xxx/global1.c |  84 +++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |   3 +
 drivers/net/dsa/mv88e6xxx/rmu.c     | 273 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h     |  33 ++++
 7 files changed, 427 insertions(+)
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.c
 create mode 100644 drivers/net/dsa/mv88e6xxx/rmu.h

diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile
index c8eca2b6f959..105d7bd832c9 100644
--- a/drivers/net/dsa/mv88e6xxx/Makefile
+++ b/drivers/net/dsa/mv88e6xxx/Makefile
@@ -15,3 +15,4 @@ mv88e6xxx-objs += port_hidden.o
 mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o
 mv88e6xxx-objs += serdes.o
 mv88e6xxx-objs += smi.o
+mv88e6xxx-objs += rmu.o
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 07e9a4da924c..4c0510abd875 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -42,6 +42,7 @@
 #include "ptp.h"
 #include "serdes.h"
 #include "smi.h"
+#include "rmu.h"
 
 static void assert_reg_lock(struct mv88e6xxx_chip *chip)
 {
@@ -1529,6 +1530,10 @@ static int mv88e6xxx_trunk_setup(struct mv88e6xxx_chip *chip)
 
 static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
 {
+	if (chip->info->ops->rmu_enable)
+		if (!chip->info->ops->rmu_enable(chip))
+			return mv88e6xxx_rmu_init(chip);
+
 	if (chip->info->ops->rmu_disable)
 		return chip->info->ops->rmu_disable(chip);
 
@@ -4090,6 +4095,7 @@ static const struct mv88e6xxx_ops mv88e6085_ops = {
 	.ppu_disable = mv88e6185_g1_ppu_disable,
 	.reset = mv88e6185_g1_reset,
 	.rmu_disable = mv88e6085_g1_rmu_disable,
+	.rmu_enable = mv88e6085_g1_rmu_enable,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.stu_getnext = mv88e6352_g1_stu_getnext,
@@ -4173,6 +4179,7 @@ static const struct mv88e6xxx_ops mv88e6097_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6085_g1_rmu_disable,
+	.rmu_enable = mv88e6085_g1_rmu_enable,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.phylink_get_caps = mv88e6095_phylink_get_caps,
@@ -5292,6 +5299,7 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6352_g1_rmu_disable,
+	.rmu_enable = mv88e6352_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
@@ -5359,6 +5367,7 @@ static const struct mv88e6xxx_ops mv88e6390_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -5426,6 +5435,7 @@ static const struct mv88e6xxx_ops mv88e6390x_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -5496,6 +5506,7 @@ static const struct mv88e6xxx_ops mv88e6393x_ops = {
 	.pot_clear = mv88e6xxx_g2_pot_clear,
 	.reset = mv88e6352_g1_reset,
 	.rmu_disable = mv88e6390_g1_rmu_disable,
+	.rmu_enable = mv88e6390_g1_rmu_enable,
 	.atu_get_hash = mv88e6165_g1_atu_get_hash,
 	.atu_set_hash = mv88e6165_g1_atu_set_hash,
 	.vtu_getnext = mv88e6390_g1_vtu_getnext,
@@ -6918,6 +6929,8 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.crosschip_lag_change	= mv88e6xxx_crosschip_lag_change,
 	.crosschip_lag_join	= mv88e6xxx_crosschip_lag_join,
 	.crosschip_lag_leave	= mv88e6xxx_crosschip_lag_leave,
+	.inband_receive         = mv88e6xxx_inband_rcv,
+	.master_state_change	= mv88e6xxx_rmu_master_change,
 };
 
 static int mv88e6xxx_register_switch(struct mv88e6xxx_chip *chip)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..024f45cc1476 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -33,6 +33,8 @@
 
 #define MV88E6XXX_MAX_GPIO	16
 
+#define MV88E6XXX_WAIT_POLL_TIME_MS		200
+
 enum mv88e6xxx_egress_mode {
 	MV88E6XXX_EGRESS_MODE_UNMODIFIED,
 	MV88E6XXX_EGRESS_MODE_UNTAGGED,
@@ -266,6 +268,7 @@ struct mv88e6xxx_vlan {
 struct mv88e6xxx_port {
 	struct mv88e6xxx_chip *chip;
 	int port;
+	u64 rmu_raw_stats[64];
 	struct mv88e6xxx_vlan bridge_pvid;
 	u64 serdes_stats[2];
 	u64 atu_member_violation;
@@ -282,6 +285,18 @@ struct mv88e6xxx_port {
 	struct devlink_region *region;
 };
 
+struct mv88e6xxx_rmu {
+	/* RMU resources */
+	struct net_device *netdev;
+	struct mv88e6xxx_bus_ops *ops;
+	struct completion completion;
+	/* Mutex for RMU operations */
+	struct mutex mutex;
+	u16 got_id;
+	u8 request_cmd;
+	u8 seq_no;
+};
+
 enum mv88e6xxx_region_id {
 	MV88E6XXX_REGION_GLOBAL1 = 0,
 	MV88E6XXX_REGION_GLOBAL2,
@@ -410,12 +425,16 @@ struct mv88e6xxx_chip {
 
 	/* Bridge MST to SID mappings */
 	struct list_head msts;
+
+	/* RMU resources */
+	struct mv88e6xxx_rmu rmu;
 };
 
 struct mv88e6xxx_bus_ops {
 	int (*read)(struct mv88e6xxx_chip *chip, int addr, int reg, u16 *val);
 	int (*write)(struct mv88e6xxx_chip *chip, int addr, int reg, u16 val);
 	int (*init)(struct mv88e6xxx_chip *chip);
+	int (*get_rmon)(struct mv88e6xxx_chip *chip, int port, uint64_t *data);
 };
 
 struct mv88e6xxx_mdio_bus {
@@ -637,6 +656,7 @@ struct mv88e6xxx_ops {
 
 	/* Remote Management Unit operations */
 	int (*rmu_disable)(struct mv88e6xxx_chip *chip);
+	int (*rmu_enable)(struct mv88e6xxx_chip *chip);
 
 	/* Precision Time Protocol operations */
 	const struct mv88e6xxx_ptp_ops *ptp_ops;
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index 5848112036b0..5d86dbf39347 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -466,18 +466,102 @@ int mv88e6085_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 				      MV88E6085_G1_CTL2_RM_ENABLE, 0);
 }
 
+int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port = -1;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_dbg(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -EOPNOTSUPP;
+
+	switch (upstream_port) {
+	case 9:
+		val = MV88E6085_G1_CTL2_RM_ENABLE;
+		break;
+	case 10:
+		val = MV88E6085_G1_CTL2_RM_ENABLE | MV88E6085_G1_CTL2_P10RM;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6085_G1_CTL2_P10RM |
+				      MV88E6085_G1_CTL2_RM_ENABLE, val);
+}
+
 int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
 				      MV88E6352_G1_CTL2_RMU_MODE_DISABLED);
 }
 
+int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_dbg(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -EOPNOTSUPP;
+
+	switch (upstream_port) {
+	case 4:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_4;
+		break;
+	case 5:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_5;
+		break;
+	case 6:
+		val = MV88E6352_G1_CTL2_RMU_MODE_PORT_6;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6352_G1_CTL2_RMU_MODE_MASK,
+			val);
+}
+
 int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_RMU_MODE_MASK,
 				      MV88E6390_G1_CTL2_RMU_MODE_DISABLED);
 }
 
+int mv88e6390_g1_rmu_enable(struct mv88e6xxx_chip *chip)
+{
+	int val = MV88E6390_G1_CTL2_RMU_MODE_DISABLED;
+	int upstream_port;
+
+	upstream_port = dsa_switch_upstream_port(chip->ds);
+	dev_dbg(chip->dev, "RMU: Enabling on port %d", upstream_port);
+	if (upstream_port < 0)
+		return -EOPNOTSUPP;
+
+	switch (upstream_port) {
+	case 0:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_0;
+		break;
+	case 1:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_1;
+		break;
+	case 9:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_9;
+		break;
+	case 10:
+		val = MV88E6390_G1_CTL2_RMU_MODE_PORT_10;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_RMU_MODE_MASK,
+			val);
+}
+
 int mv88e6390_g1_stats_set_histogram(struct mv88e6xxx_chip *chip)
 {
 	return mv88e6xxx_g1_ctl2_mask(chip, MV88E6390_G1_CTL2_HIST_MODE_MASK,
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index 65958b2a0d3a..7e786503734a 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -313,8 +313,11 @@ int mv88e6250_g1_ieee_pri_map(struct mv88e6xxx_chip *chip);
 int mv88e6185_g1_set_cascade_port(struct mv88e6xxx_chip *chip, int port);
 
 int mv88e6085_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6085_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6390_g1_rmu_enable(struct mv88e6xxx_chip *chip);
 
 int mv88e6xxx_g1_set_device_number(struct mv88e6xxx_chip *chip, int index);
 
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
new file mode 100644
index 000000000000..b7d850c099c5
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Marvell 88E6xxx Switch Remote Management Unit Support
+ *
+ * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
+ *
+ */
+
+#include <asm/unaligned.h>
+#include "rmu.h"
+#include "global1.h"
+
+static int mv88e6xxx_validate_port_mac(struct dsa_switch *ds, struct sk_buff *skb)
+{
+	unsigned char *ethhdr;
+	struct dsa_port *dp;
+	u8 pkt_port;
+
+	pkt_port = (skb->data[7] >> 3) & 0xf;
+	dp = dsa_to_port(ds, pkt_port);
+	if (!dp) {
+		dev_dbg_ratelimited(ds->dev, "RMU: couldn't find port for %d\n", pkt_port);
+		return -ENXIO;
+	}
+
+	/* Check matching MAC */
+	ethhdr = skb_mac_header(skb);
+	if (memcmp(dp->slave->dev_addr, ethhdr, ETH_ALEN)) {
+		dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
+				    ethhdr, dp->slave->dev_addr);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct mv88e6xxx_port *port;
+	u16 prodnum;
+	u16 format;
+	u8 pkt_dev;
+	u8 pkt_prt;
+	u16 code;
+	int i;
+
+	/* Extract response data */
+	format = get_unaligned_be16(&skb->data[0]);
+	if (format != htons(MV88E6XXX_RMU_RESP_FORMAT_1) &&
+	    format != htons(MV88E6XXX_RMU_RESP_FORMAT_2)) {
+		dev_err_ratelimited(chip->dev, "RMU: received unknown format 0x%04x", format);
+		goto out;
+	}
+
+	code = get_unaligned_be16(&skb->data[4]);
+	if (code == ntohs(MV88E6XXX_RMU_RESP_ERROR)) {
+		dev_err_ratelimited(chip->dev, "RMU: error response code 0x%04x", code);
+		goto out;
+	}
+
+	pkt_dev = skb->data[6] & 0x1f;
+	if (!dsa_switch_find(ds->dst->index, pkt_dev)) {
+		dev_err_ratelimited(chip->dev, "RMU: response from unknown chip with index %d\n",
+				    pkt_dev);
+		goto out;
+	}
+
+	/* Check sequence number */
+	if (seq_no != chip->rmu.seq_no) {
+		dev_err_ratelimited(chip->dev, "RMU: wrong seqno received %d, expected %d",
+				    seq_no, chip->rmu.seq_no);
+		goto out;
+	}
+
+	/* Check response code */
+	switch (chip->rmu.request_cmd) {
+	case MV88E6XXX_RMU_REQ_GET_ID: {
+		if (code == MV88E6XXX_RMU_RESP_CODE_GOT_ID) {
+			prodnum = get_unaligned_be16(&skb->data[2]);
+			chip->rmu.got_id = prodnum;
+			dev_info_ratelimited(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
+					     chip->rmu.got_id);
+		} else {
+			dev_dbg_ratelimited(chip->dev,
+					    "RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
+					    format, code);
+		}
+		break;
+	}
+	case MV88E6XXX_RMU_REQ_DUMP_MIB:
+		if (code == MV88E6XXX_RMU_RESP_CODE_DUMP_MIB &&
+		    !mv88e6xxx_validate_port_mac(ds, skb)) {
+			pkt_prt = (skb->data[7] & 0x78) >> 3;
+			port = &chip->ports[pkt_prt];
+			if (!port) {
+				dev_err_ratelimited(chip->dev, "RMU: illegal port number in response: %d\n",
+						    pkt_prt);
+				goto out;
+			}
+
+			/* Copy whole array for further
+			 * processing according to chip type
+			 */
+			for (i = 0; i < MV88E6XXX_RMU_MAX_RMON; i++)
+				port->rmu_raw_stats[i] = get_unaligned_be32(&skb->data[12 + i * 4]);
+		}
+		break;
+	default:
+		dev_err_ratelimited(chip->dev,
+				    "RMU: unknown response format 0x%04x and code 0x%04x from chip %d\n",
+				    format, code, chip->ds->index);
+		break;
+	}
+
+out:
+	complete(&chip->rmu.completion);
+
+	return 0;
+}
+
+static int mv88e6xxx_rmu_tx(struct mv88e6xxx_chip *chip, int port,
+			    const char *msg, int len)
+{
+	const struct dsa_device_ops *tag_ops;
+	const struct dsa_port *dp;
+	unsigned char *data;
+	struct sk_buff *skb;
+
+	dp = dsa_to_port(chip->ds, port);
+	if (!dp || !dp->cpu_dp)
+		return 0;
+
+	tag_ops = dp->cpu_dp->tag_ops;
+	if (!tag_ops)
+		return -ENODEV;
+
+	skb = netdev_alloc_skb(chip->rmu.netdev, 64);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_reserve(skb, 2 * ETH_HLEN + tag_ops->needed_headroom);
+	skb_reset_network_header(skb);
+	skb->pkt_type = PACKET_OUTGOING;
+	skb->dev = chip->rmu.netdev;
+
+	/* Create RMU L3 message */
+	data = skb_put(skb, len);
+	memcpy(data, msg, len);
+
+	return tag_ops->inband_xmit(skb, dp->slave, ++chip->rmu.seq_no);
+}
+
+static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip, int port,
+				   int request, const char *msg, int len)
+{
+	const struct dsa_port *dp;
+	int ret = 0;
+
+	dp = dsa_to_port(chip->ds, port);
+	if (!dp)
+		return 0;
+
+	mutex_lock(&chip->rmu.mutex);
+
+	chip->rmu.request_cmd = request;
+
+	ret = mv88e6xxx_rmu_tx(chip, port, msg, len);
+	if (ret == -ENODEV) {
+		/* Device not ready yet? Try again later */
+		ret = 0;
+		goto out;
+	}
+
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error transmitting request (%d)", ret);
+		goto out;
+	}
+
+	ret = wait_for_completion_timeout(&chip->rmu.completion,
+					  msecs_to_jiffies(MV88E6XXX_WAIT_POLL_TIME_MS));
+	if (ret == 0) {
+		dev_dbg(chip->dev,
+			"RMU: timeout waiting for request %d (%d) on dev:port %d:%d\n",
+			request, ret, chip->ds->index, port);
+		ret = -ETIMEDOUT;
+	}
+
+out:
+	mutex_unlock(&chip->rmu.mutex);
+
+	return ret > 0 ? 0 : ret;
+}
+
+static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
+{
+	const u8 get_id[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+	int ret = -1;
+
+	if (chip->rmu.got_id)
+		return 0;
+
+	chip->rmu.netdev = dev_get_by_name(&init_net, "chan0");
+	if (!chip->rmu.netdev) {
+		dev_dbg(chip->dev, "RMU: unable to get interface");
+		return -ENODEV;
+	}
+
+	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_GET_ID, get_id, 8);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command GET_ID %d index %d\n", ret,
+			chip->ds->index);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
+{
+	u8 dump_mib[8] = { 0x00, 0x01, 0x00, 0x00, 0x10, 0x20, 0x00, 0x00 };
+	int ret;
+
+	if (!chip)
+		return 0;
+
+	ret = mv88e6xxx_rmu_get_id(chip, port);
+	if (ret)
+		return ret;
+
+	/* Send a GET_MIB command */
+	dump_mib[7] = port;
+	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_DUMP_MIB, dump_mib, 8);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %d dev %d:%d\n", ret,
+			chip->ds->index, port);
+		return ret;
+	}
+
+	/* Update MIB for port */
+	if (chip->info->ops->stats_get_stats)
+		return chip->info->ops->stats_get_stats(chip, port, data);
+
+	return 0;
+}
+
+static struct mv88e6xxx_bus_ops mv88e6xxx_bus_ops = {
+	.get_rmon = mv88e6xxx_rmu_stats_get,
+};
+
+int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip)
+{
+	dev_info(chip->dev, "RMU: setting up for switch@%d", chip->sw_addr);
+
+	init_completion(&chip->rmu.completion);
+
+	mutex_init(&chip->rmu.mutex);
+
+	chip->rmu.ops = &mv88e6xxx_bus_ops;
+
+	return 0;
+}
+
+void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
+				 bool operational)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	if (operational)
+		chip->rmu.ops = &mv88e6xxx_bus_ops;
+	else
+		chip->rmu.ops = NULL;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.h b/drivers/net/dsa/mv88e6xxx/rmu.h
new file mode 100644
index 000000000000..19be624a3ebf
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Marvell 88E6xxx Switch Remote Management Unit Support
+ *
+ * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
+ *
+ */
+
+#ifndef _MV88E6XXX_RMU_H_
+#define _MV88E6XXX_RMU_H_
+
+#include "chip.h"
+
+#define MV88E6XXX_RMU_MAX_RMON			64
+
+#define MV88E6XXX_RMU_REQ_GET_ID		1
+#define MV88E6XXX_RMU_REQ_DUMP_MIB		2
+
+#define MV88E6XXX_RMU_RESP_FORMAT_1		0x0001
+#define MV88E6XXX_RMU_RESP_FORMAT_2		0x0002
+#define MV88E6XXX_RMU_RESP_ERROR		0xffff
+
+#define MV88E6XXX_RMU_RESP_CODE_GOT_ID		0x0000
+#define MV88E6XXX_RMU_RESP_CODE_DUMP_MIB	0x1020
+
+int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip);
+
+int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no);
+
+void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
+				 bool operational);
+
+#endif /* _MV88E6XXX_RMU_H_ */
-- 
2.25.1

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Vladimir Oltean]

On Fri, Aug 26, 2022 at 08:38:15AM +0200, Mattias Forsblad wrote:
>  static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>  {
> +	if (chip->info->ops->rmu_enable)
> +		if (!chip->info->ops->rmu_enable(chip))
> +			return mv88e6xxx_rmu_init(chip);
> +
>  	if (chip->info->ops->rmu_disable)
>  		return chip->info->ops->rmu_disable(chip);

I think it's very important for the RMU to still start as disabled.
You enable it dynamically when the master goes up.

> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
> new file mode 100644
> index 000000000000..b7d850c099c5
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
> @@ -0,0 +1,273 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Marvell 88E6xxx Switch Remote Management Unit Support
> + *
> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
> + *
> + */
> +
> +#include <asm/unaligned.h>
> +#include "rmu.h"
> +#include "global1.h"
> +
> +static int mv88e6xxx_validate_port_mac(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> +	unsigned char *ethhdr;
> +	struct dsa_port *dp;
> +	u8 pkt_port;
> +
> +	pkt_port = (skb->data[7] >> 3) & 0xf;
> +	dp = dsa_to_port(ds, pkt_port);
> +	if (!dp) {
> +		dev_dbg_ratelimited(ds->dev, "RMU: couldn't find port for %d\n", pkt_port);
> +		return -ENXIO;
> +	}
> +
> +	/* Check matching MAC */
> +	ethhdr = skb_mac_header(skb);
> +	if (memcmp(dp->slave->dev_addr, ethhdr, ETH_ALEN)) {

ether_addr_equal()

Also, what happens if you don't validate the MAC DA of the response, and
in general, if you just put your MAC SA as the MAC address of the
operationally active RMU DSA master? I guess the whole idea is to
provide a MAC address which the DSA master won't drop with its RX
filter, and its own MAC address is just fine for that.

> +		dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
> +				    ethhdr, dp->slave->dev_addr);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct mv88e6xxx_port *port;
> +	u16 prodnum;
> +	u16 format;
> +	u8 pkt_dev;
> +	u8 pkt_prt;
> +	u16 code;
> +	int i;
> +
> +	/* Extract response data */
> +	format = get_unaligned_be16(&skb->data[0]);
> +	if (format != htons(MV88E6XXX_RMU_RESP_FORMAT_1) &&
> +	    format != htons(MV88E6XXX_RMU_RESP_FORMAT_2)) {
> +		dev_err_ratelimited(chip->dev, "RMU: received unknown format 0x%04x", format);
> +		goto out;
> +	}
> +
> +	code = get_unaligned_be16(&skb->data[4]);
> +	if (code == ntohs(MV88E6XXX_RMU_RESP_ERROR)) {

Please build with sparse, "make W=1 C=1". These are all wrong. The
variables retrieved from packet headers should be __be16, and "htohs"
(network to host) should be "htons" (host to network).

> +		dev_err_ratelimited(chip->dev, "RMU: error response code 0x%04x", code);
> +		goto out;
> +	}
> +
> +	pkt_dev = skb->data[6] & 0x1f;
> +	if (!dsa_switch_find(ds->dst->index, pkt_dev)) {

What is the relation between the pkt_dev from here, the RMU header, and
the source_device from dsa_inband_rcv_ll()? Are they always the same?
Can they be different? You throw away the result from dsa_switch_find()
in any case.

> +		dev_err_ratelimited(chip->dev, "RMU: response from unknown chip with index %d\n",
> +				    pkt_dev);
> +		goto out;
> +	}
> +
> +	/* Check sequence number */
> +	if (seq_no != chip->rmu.seq_no) {
> +		dev_err_ratelimited(chip->dev, "RMU: wrong seqno received %d, expected %d",
> +				    seq_no, chip->rmu.seq_no);
> +		goto out;
> +	}
> +
> +	/* Check response code */
> +	switch (chip->rmu.request_cmd) {
> +	case MV88E6XXX_RMU_REQ_GET_ID: {
> +		if (code == MV88E6XXX_RMU_RESP_CODE_GOT_ID) {

I'd expect htons to be used even here, with 0, for type consistency.
The compiler should figure things out and not add extra code.

> +			prodnum = get_unaligned_be16(&skb->data[2]);
> +			chip->rmu.got_id = prodnum;
> +			dev_info_ratelimited(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
> +					     chip->rmu.got_id);
> +		} else {
> +			dev_dbg_ratelimited(chip->dev,
> +					    "RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
> +					    format, code);
> +		}
> +		break;
> +	}
> +	case MV88E6XXX_RMU_REQ_DUMP_MIB:
> +		if (code == MV88E6XXX_RMU_RESP_CODE_DUMP_MIB &&
> +		    !mv88e6xxx_validate_port_mac(ds, skb)) {
> +			pkt_prt = (skb->data[7] & 0x78) >> 3;
> +			port = &chip->ports[pkt_prt];

It would be good if you could structure the code in such a way that you
don't parse stuff from the packet twice, once in mv88e6xxx_validate_port_mac()
and once here.

> +			if (!port) {
> +				dev_err_ratelimited(chip->dev, "RMU: illegal port number in response: %d\n",
> +						    pkt_prt);
> +				goto out;
> +			}
> +
> +			/* Copy whole array for further
> +			 * processing according to chip type
> +			 */
> +			for (i = 0; i < MV88E6XXX_RMU_MAX_RMON; i++)
> +				port->rmu_raw_stats[i] = get_unaligned_be32(&skb->data[12 + i * 4]);
> +		}
> +		break;
> +	default:
> +		dev_err_ratelimited(chip->dev,
> +				    "RMU: unknown response format 0x%04x and code 0x%04x from chip %d\n",
> +				    format, code, chip->ds->index);
> +		break;
> +	}
> +
> +out:
> +	complete(&chip->rmu.completion);
> +
> +	return 0;
> +}
> +
> +static int mv88e6xxx_rmu_tx(struct mv88e6xxx_chip *chip, int port,
> +			    const char *msg, int len)
> +{
> +	const struct dsa_device_ops *tag_ops;
> +	const struct dsa_port *dp;
> +	unsigned char *data;
> +	struct sk_buff *skb;
> +
> +	dp = dsa_to_port(chip->ds, port);
> +	if (!dp || !dp->cpu_dp)
> +		return 0;
> +
> +	tag_ops = dp->cpu_dp->tag_ops;
> +	if (!tag_ops)
> +		return -ENODEV;
> +
> +	skb = netdev_alloc_skb(chip->rmu.netdev, 64);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	skb_reserve(skb, 2 * ETH_HLEN + tag_ops->needed_headroom);

If you decide to rework this using the master netdev, you can use
dsa_tag_protocol_overhead(master->dsa_ptr->tag_ops). Or even reserve
enough headroom for the larger header (EDSA) and be done with it.
But then you need to construct a different header depending on whether
DSA or EDSA is used.

> +	skb_reset_network_header(skb);
> +	skb->pkt_type = PACKET_OUTGOING;

Could you please explain for me what will setting skb->pkt_type to
PACKET_OUTGOING achieve?

> +	skb->dev = chip->rmu.netdev;
> +
> +	/* Create RMU L3 message */
> +	data = skb_put(skb, len);
> +	memcpy(data, msg, len);
> +
> +	return tag_ops->inband_xmit(skb, dp->slave, ++chip->rmu.seq_no);
> +}
> +
> +static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip, int port,
> +				   int request, const char *msg, int len)
> +{
> +	const struct dsa_port *dp;
> +	int ret = 0;
> +
> +	dp = dsa_to_port(chip->ds, port);
> +	if (!dp)
> +		return 0;
> +
> +	mutex_lock(&chip->rmu.mutex);
> +
> +	chip->rmu.request_cmd = request;
> +
> +	ret = mv88e6xxx_rmu_tx(chip, port, msg, len);
> +	if (ret == -ENODEV) {
> +		/* Device not ready yet? Try again later */
> +		ret = 0;

All the code paths in mv88e6xxx_rmu_tx() that return -ENODEV are
fabricated reasons to have errors, IMO, and in a correct implementation
we should never even get there. Please drop the special casing.

> +		goto out;
> +	}
> +
> +	if (ret) {
> +		dev_dbg(chip->dev, "RMU: error transmitting request (%d)", ret);

Please have a normal log level for an error, dev_err().
Also you can print the symbolic name for the error:

		dev_err(chip->dev, "RMU: error transmitting request (%pe)",
			ERR_PTR(ret));

> +		goto out;
> +	}
> +
> +	ret = wait_for_completion_timeout(&chip->rmu.completion,
> +					  msecs_to_jiffies(MV88E6XXX_WAIT_POLL_TIME_MS));
> +	if (ret == 0) {
> +		dev_dbg(chip->dev,
> +			"RMU: timeout waiting for request %d (%d) on dev:port %d:%d\n",
> +			request, ret, chip->ds->index, port);

Again, please increase the log level of the error condition.
Also, chip->ds->index is useless information, we have info about the
switch via dev_dbg/dev_err.

> +		ret = -ETIMEDOUT;
> +	}
> +
> +out:
> +	mutex_unlock(&chip->rmu.mutex);
> +
> +	return ret > 0 ? 0 : ret;
> +}
> +
> +static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
> +{
> +	const u8 get_id[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
> +	int ret = -1;
> +
> +	if (chip->rmu.got_id)
> +		return 0;
> +
> +	chip->rmu.netdev = dev_get_by_name(&init_net, "chan0");

What if I don't have a slave device called "chan0"? I can't use the RMU?

> +	if (!chip->rmu.netdev) {
> +		dev_dbg(chip->dev, "RMU: unable to get interface");
> +		return -ENODEV;
> +	}
> +
> +	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_GET_ID, get_id, 8);
> +	if (ret) {
> +		dev_dbg(chip->dev, "RMU: error for command GET_ID %d index %d\n", ret,
> +			chip->ds->index);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
> +{
> +	u8 dump_mib[8] = { 0x00, 0x01, 0x00, 0x00, 0x10, 0x20, 0x00, 0x00 };
> +	int ret;
> +
> +	if (!chip)
> +		return 0;

How can "chip" be NULL?

> +
> +	ret = mv88e6xxx_rmu_get_id(chip, port);
> +	if (ret)
> +		return ret;
> +
> +	/* Send a GET_MIB command */
> +	dump_mib[7] = port;
> +	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_DUMP_MIB, dump_mib, 8);
> +	if (ret) {
> +		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %d dev %d:%d\n", ret,
> +			chip->ds->index, port);
> +		return ret;
> +	}
> +
> +	/* Update MIB for port */
> +	if (chip->info->ops->stats_get_stats)
> +		return chip->info->ops->stats_get_stats(chip, port, data);
> +
> +	return 0;
> +}
> +
> +static struct mv88e6xxx_bus_ops mv88e6xxx_bus_ops = {

static const struct

> +	.get_rmon = mv88e6xxx_rmu_stats_get,
> +};
> +
> +int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip)
> +{
> +	dev_info(chip->dev, "RMU: setting up for switch@%d", chip->sw_addr);

Not a whole lotta useful information brought by this. Also,
chip->sw_addr is already visible via dev_info(chip->dev).
I would just drop this.

> +
> +	init_completion(&chip->rmu.completion);
> +
> +	mutex_init(&chip->rmu.mutex);

I would just move these to mv88e6xxx_rmu_setup(), and move
mv88e6xxx_rmu_setup() to rmu.c.

> +
> +	chip->rmu.ops = &mv88e6xxx_bus_ops;

By doing this (combined with the way in which chip->rmu.ops is actually
tested for), you are saying that RMU is available since driver probe time.
But it isn't. It's only available when the master goes up and is
operational. So you're setting yourself up for lots of I/O failures in
the beginning.

> +
> +	return 0;
> +}
> +
> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
> +				 bool operational)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	if (operational)
> +		chip->rmu.ops = &mv88e6xxx_bus_ops;
> +	else
> +		chip->rmu.ops = NULL;
> +}

There is a subtle but very important point to be careful about here,
which is compatibility with multiple CPU ports. If there is a second DSA
master whose state flaps from up to down, this should not affect the
fact that you can still use RMU over the first DSA master. But in your
case it does, so this is a case of how not to write code that accounts
for that.

In fact, given this fact, I think your function prototypes for
chip->info->ops->rmu_enable() are all wrong / not sufficiently
reflective of what the hardware can do. If the hardware has a bit mask
of ports on which RMU operations are possible, why hardcode using
dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
are actually up? At least for the top-most switch. For downstream
switches we can use dsa_switch_upstream_port(), I guess (even that can
be refined, but I'm not aware of setups using multiple DSA links, where
each DSA link ultimately goes to a different upstream switch).

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Vladimir Oltean]

On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote:
> > +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
> > +				 bool operational)
> > +{
> > +	struct mv88e6xxx_chip *chip = ds->priv;
> > +
> > +	if (operational)
> > +		chip->rmu.ops = &mv88e6xxx_bus_ops;
> > +	else
> > +		chip->rmu.ops = NULL;
> > +}
> 
> There is a subtle but very important point to be careful about here,
> which is compatibility with multiple CPU ports. If there is a second DSA
> master whose state flaps from up to down, this should not affect the
> fact that you can still use RMU over the first DSA master. But in your
> case it does, so this is a case of how not to write code that accounts
> for that.
> 
> In fact, given this fact, I think your function prototypes for
> chip->info->ops->rmu_enable() are all wrong / not sufficiently
> reflective of what the hardware can do. If the hardware has a bit mask
> of ports on which RMU operations are possible, why hardcode using
> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
> are actually up? At least for the top-most switch. For downstream
> switches we can use dsa_switch_upstream_port(), I guess (even that can
> be refined, but I'm not aware of setups using multiple DSA links, where
> each DSA link ultimately goes to a different upstream switch).

Hit "send" too soon. Wanted to give the extra hint that the "master"
pointer is given to you here for a reason. You can look at struct
dsa_port *cpu_dp = master->dsa_ptr, and figure out the index of the CPU
port which can be used for RMU operations. I see that the macros are
constructed in a very strange way:

#define MV88E6352_G1_CTL2_RMU_MODE_DISABLED	0x0000
#define MV88E6352_G1_CTL2_RMU_MODE_PORT_4	0x1000
#define MV88E6352_G1_CTL2_RMU_MODE_PORT_5	0x2000
#define MV88E6352_G1_CTL2_RMU_MODE_PORT_6	0x3000

it's as if this is actually a bit mask of ports, and they all can be
combined together. The bit in G1_CTL2 whose state we can flip can be
made to depend on the number of the CPU port attached to the DSA master
which changed state.

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Mattias Forsblad]

On 2022-08-30 18:42, Vladimir Oltean wrote:
> On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote:
>>> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
>>> +				 bool operational)
>>> +{
>>> +	struct mv88e6xxx_chip *chip = ds->priv;
>>> +
>>> +	if (operational)
>>> +		chip->rmu.ops = &mv88e6xxx_bus_ops;
>>> +	else
>>> +		chip->rmu.ops = NULL;
>>> +}
>>
>> There is a subtle but very important point to be careful about here,
>> which is compatibility with multiple CPU ports. If there is a second DSA
>> master whose state flaps from up to down, this should not affect the
>> fact that you can still use RMU over the first DSA master. But in your
>> case it does, so this is a case of how not to write code that accounts
>> for that.
>>
>> In fact, given this fact, I think your function prototypes for
>> chip->info->ops->rmu_enable() are all wrong / not sufficiently
>> reflective of what the hardware can do. If the hardware has a bit mask
>> of ports on which RMU operations are possible, why hardcode using
>> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
>> are actually up? At least for the top-most switch. For downstream
>> switches we can use dsa_switch_upstream_port(), I guess (even that can
>> be refined, but I'm not aware of setups using multiple DSA links, where
>> each DSA link ultimately goes to a different upstream switch).
> 
> Hit "send" too soon. Wanted to give the extra hint that the "master"
> pointer is given to you here for a reason. You can look at struct
> dsa_port *cpu_dp = master->dsa_ptr, and figure out the index of the CPU
> port which can be used for RMU operations. I see that the macros are
> constructed in a very strange way:
> 

Ah, nice one. Thanks.

> #define MV88E6352_G1_CTL2_RMU_MODE_DISABLED	0x0000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_4	0x1000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_5	0x2000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_6	0x3000
> 
> it's as if this is actually a bit mask of ports, and they all can be
> combined together. The bit in G1_CTL2 whose state we can flip can be
> made to depend on the number of the CPU port attached to the DSA master
> which changed state.

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Mattias Forsblad]

On 2022-08-30 18:42, Vladimir Oltean wrote:
> On Tue, Aug 30, 2022 at 07:35:15PM +0300, Vladimir Oltean wrote:
>>> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
>>> +				 bool operational)
>>> +{
>>> +	struct mv88e6xxx_chip *chip = ds->priv;
>>> +
>>> +	if (operational)
>>> +		chip->rmu.ops = &mv88e6xxx_bus_ops;
>>> +	else
>>> +		chip->rmu.ops = NULL;
>>> +}
>>
>> There is a subtle but very important point to be careful about here,
>> which is compatibility with multiple CPU ports. If there is a second DSA
>> master whose state flaps from up to down, this should not affect the
>> fact that you can still use RMU over the first DSA master. But in your
>> case it does, so this is a case of how not to write code that accounts
>> for that.
>>
>> In fact, given this fact, I think your function prototypes for
>> chip->info->ops->rmu_enable() are all wrong / not sufficiently
>> reflective of what the hardware can do. If the hardware has a bit mask
>> of ports on which RMU operations are possible, why hardcode using
>> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
>> are actually up? At least for the top-most switch. For downstream
>> switches we can use dsa_switch_upstream_port(), I guess (even that can
>> be refined, but I'm not aware of setups using multiple DSA links, where
>> each DSA link ultimately goes to a different upstream switch).
> 
> Hit "send" too soon. Wanted to give the extra hint that the "master"
> pointer is given to you here for a reason. You can look at struct
> dsa_port *cpu_dp = master->dsa_ptr, and figure out the index of the CPU

Would this work on a system where there are multiple switches? I.e.

SOC <->port6 SC#1 <->port10 SC#2

Both have the same master interface (chan0) which gives the same
cpu_dp->dsa_ptr->index but they have different upstream ports that should 
be enabled for RMU.

> port which can be used for RMU operations. I see that the macros are
> constructed in a very strange way:
> 
> #define MV88E6352_G1_CTL2_RMU_MODE_DISABLED	0x0000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_4	0x1000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_5	0x2000
> #define MV88E6352_G1_CTL2_RMU_MODE_PORT_6	0x3000
> 
> it's as if this is actually a bit mask of ports, and they all can be
> combined together. The bit in G1_CTL2 whose state we can flip can be
> made to depend on the number of the CPU port attached to the DSA master
> which changed state.

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Vladimir Oltean]

On Thu, Sep 01, 2022 at 11:05:37AM +0200, Mattias Forsblad wrote:
> Would this work on a system where there are multiple switches? I.e.
> 
> SOC <->port6 SC#1 <->port10 SC#2
> 
> Both have the same master interface (chan0) which gives the same
> cpu_dp->dsa_ptr->index but they have different upstream ports that should 
> be enabled for RMU.

I think the answer to that is dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Mattias Forsblad]

On 2022-08-30 18:35, Vladimir Oltean wrote:
> On Fri, Aug 26, 2022 at 08:38:15AM +0200, Mattias Forsblad wrote:
>>  static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>>  {
>> +	if (chip->info->ops->rmu_enable)
>> +		if (!chip->info->ops->rmu_enable(chip))
>> +			return mv88e6xxx_rmu_init(chip);
>> +
>>  	if (chip->info->ops->rmu_disable)
>>  		return chip->info->ops->rmu_disable(chip);
> 
> I think it's very important for the RMU to still start as disabled.
> You enable it dynamically when the master goes up.
> 

Please elaborate why this may pose a problem, I might have missed
some information.

>> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
>> new file mode 100644
>> index 000000000000..b7d850c099c5
>> --- /dev/null
>> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
>> @@ -0,0 +1,273 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Marvell 88E6xxx Switch Remote Management Unit Support
>> + *
>> + * Copyright (c) 2022 Mattias Forsblad <mattias.forsblad@gmail.com>
>> + *
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +#include "rmu.h"
>> +#include "global1.h"
>> +
>> +static int mv88e6xxx_validate_port_mac(struct dsa_switch *ds, struct sk_buff *skb)
>> +{
>> +	unsigned char *ethhdr;
>> +	struct dsa_port *dp;
>> +	u8 pkt_port;
>> +
>> +	pkt_port = (skb->data[7] >> 3) & 0xf;
>> +	dp = dsa_to_port(ds, pkt_port);
>> +	if (!dp) {
>> +		dev_dbg_ratelimited(ds->dev, "RMU: couldn't find port for %d\n", pkt_port);
>> +		return -ENXIO;
>> +	}
>> +
>> +	/* Check matching MAC */
>> +	ethhdr = skb_mac_header(skb);
>> +	if (memcmp(dp->slave->dev_addr, ethhdr, ETH_ALEN)) {
> 
> ether_addr_equal()
> 

Ofc.

> Also, what happens if you don't validate the MAC DA of the response, and
> in general, if you just put your MAC SA as the MAC address of the
> operationally active RMU DSA master? I guess the whole idea is to
> provide a MAC address which the DSA master won't drop with its RX
> filter, and its own MAC address is just fine for that.
>>> +		dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
>> +				    ethhdr, dp->slave->dev_addr);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +int mv88e6xxx_inband_rcv(struct dsa_switch *ds, struct sk_buff *skb, int seq_no)
>> +{
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +	struct mv88e6xxx_port *port;
>> +	u16 prodnum;
>> +	u16 format;
>> +	u8 pkt_dev;
>> +	u8 pkt_prt;
>> +	u16 code;
>> +	int i;
>> +
>> +	/* Extract response data */
>> +	format = get_unaligned_be16(&skb->data[0]);
>> +	if (format != htons(MV88E6XXX_RMU_RESP_FORMAT_1) &&
>> +	    format != htons(MV88E6XXX_RMU_RESP_FORMAT_2)) {
>> +		dev_err_ratelimited(chip->dev, "RMU: received unknown format 0x%04x", format);
>> +		goto out;
>> +	}
>> +
>> +	code = get_unaligned_be16(&skb->data[4]);
>> +	if (code == ntohs(MV88E6XXX_RMU_RESP_ERROR)) {
> 
> Please build with sparse, "make W=1 C=1". These are all wrong. The
> variables retrieved from packet headers should be __be16, and "htohs"
> (network to host) should be "htons" (host to network).
> 

Will check.

>> +		dev_err_ratelimited(chip->dev, "RMU: error response code 0x%04x", code);
>> +		goto out;
>> +	}
>> +
>> +	pkt_dev = skb->data[6] & 0x1f;
>> +	if (!dsa_switch_find(ds->dst->index, pkt_dev)) {
> 
> What is the relation between the pkt_dev from here, the RMU header, and
> the source_device from dsa_inband_rcv_ll()? Are they always the same?
> Can they be different? You throw away the result from dsa_switch_find()
> in any case.
> 

Will check.

>> +		dev_err_ratelimited(chip->dev, "RMU: response from unknown chip with index %d\n",
>> +				    pkt_dev);
>> +		goto out;
>> +	}
>> +
>> +	/* Check sequence number */
>> +	if (seq_no != chip->rmu.seq_no) {
>> +		dev_err_ratelimited(chip->dev, "RMU: wrong seqno received %d, expected %d",
>> +				    seq_no, chip->rmu.seq_no);
>> +		goto out;
>> +	}
>> +
>> +	/* Check response code */
>> +	switch (chip->rmu.request_cmd) {
>> +	case MV88E6XXX_RMU_REQ_GET_ID: {
>> +		if (code == MV88E6XXX_RMU_RESP_CODE_GOT_ID) {
> 
> I'd expect htons to be used even here, with 0, for type consistency.
> The compiler should figure things out and not add extra code.
> 

Thanks.

>> +			prodnum = get_unaligned_be16(&skb->data[2]);
>> +			chip->rmu.got_id = prodnum;
>> +			dev_info_ratelimited(chip->dev, "RMU: received id OK with product number: 0x%04x\n",
>> +					     chip->rmu.got_id);
>> +		} else {
>> +			dev_dbg_ratelimited(chip->dev,
>> +					    "RMU: unknown response for GET_ID format 0x%04x code 0x%04x",
>> +					    format, code);
>> +		}
>> +		break;
>> +	}
>> +	case MV88E6XXX_RMU_REQ_DUMP_MIB:
>> +		if (code == MV88E6XXX_RMU_RESP_CODE_DUMP_MIB &&
>> +		    !mv88e6xxx_validate_port_mac(ds, skb)) {
>> +			pkt_prt = (skb->data[7] & 0x78) >> 3;
>> +			port = &chip->ports[pkt_prt];
> 
> It would be good if you could structure the code in such a way that you
> don't parse stuff from the packet twice, once in mv88e6xxx_validate_port_mac()
> and once here.
> 

Agreed.

>> +			if (!port) {
>> +				dev_err_ratelimited(chip->dev, "RMU: illegal port number in response: %d\n",
>> +						    pkt_prt);
>> +				goto out;
>> +			}
>> +
>> +			/* Copy whole array for further
>> +			 * processing according to chip type
>> +			 */
>> +			for (i = 0; i < MV88E6XXX_RMU_MAX_RMON; i++)
>> +				port->rmu_raw_stats[i] = get_unaligned_be32(&skb->data[12 + i * 4]);
>> +		}
>> +		break;
>> +	default:
>> +		dev_err_ratelimited(chip->dev,
>> +				    "RMU: unknown response format 0x%04x and code 0x%04x from chip %d\n",
>> +				    format, code, chip->ds->index);
>> +		break;
>> +	}
>> +
>> +out:
>> +	complete(&chip->rmu.completion);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mv88e6xxx_rmu_tx(struct mv88e6xxx_chip *chip, int port,
>> +			    const char *msg, int len)
>> +{
>> +	const struct dsa_device_ops *tag_ops;
>> +	const struct dsa_port *dp;
>> +	unsigned char *data;
>> +	struct sk_buff *skb;
>> +
>> +	dp = dsa_to_port(chip->ds, port);
>> +	if (!dp || !dp->cpu_dp)
>> +		return 0;
>> +
>> +	tag_ops = dp->cpu_dp->tag_ops;
>> +	if (!tag_ops)
>> +		return -ENODEV;
>> +
>> +	skb = netdev_alloc_skb(chip->rmu.netdev, 64);
>> +	if (!skb)
>> +		return -ENOMEM;
>> +
>> +	skb_reserve(skb, 2 * ETH_HLEN + tag_ops->needed_headroom);
> 
> If you decide to rework this using the master netdev, you can use
> dsa_tag_protocol_overhead(master->dsa_ptr->tag_ops). Or even reserve
> enough headroom for the larger header (EDSA) and be done with it.
> But then you need to construct a different header depending on whether
> DSA or EDSA is used.
> 

So in the new version a la qca8k we need the 'extra' parameter to
see if we need space for EDSA header, thus we need run through the tagger.
We can discuss that in the next version.

>> +	skb_reset_network_header(skb);
>> +	skb->pkt_type = PACKET_OUTGOING;
> 
> Could you please explain for me what will setting skb->pkt_type to
> PACKET_OUTGOING achieve?
>

I though it was prudent, will remove if it's not needed.
 
>> +	skb->dev = chip->rmu.netdev;
>> +
>> +	/* Create RMU L3 message */
>> +	data = skb_put(skb, len);
>> +	memcpy(data, msg, len);
>> +
>> +	return tag_ops->inband_xmit(skb, dp->slave, ++chip->rmu.seq_no);
>> +}
>> +
>> +static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip, int port,
>> +				   int request, const char *msg, int len)
>> +{
>> +	const struct dsa_port *dp;
>> +	int ret = 0;
>> +
>> +	dp = dsa_to_port(chip->ds, port);
>> +	if (!dp)
>> +		return 0;
>> +
>> +	mutex_lock(&chip->rmu.mutex);
>> +
>> +	chip->rmu.request_cmd = request;
>> +
>> +	ret = mv88e6xxx_rmu_tx(chip, port, msg, len);
>> +	if (ret == -ENODEV) {
>> +		/* Device not ready yet? Try again later */
>> +		ret = 0;
> 
> All the code paths in mv88e6xxx_rmu_tx() that return -ENODEV are
> fabricated reasons to have errors, IMO, and in a correct implementation
> we should never even get there. Please drop the special casing.
> 

Re-worked in the next version.

>> +		goto out;
>> +	}
>> +
>> +	if (ret) {
>> +		dev_dbg(chip->dev, "RMU: error transmitting request (%d)", ret);
> 
> Please have a normal log level for an error, dev_err().
> Also you can print the symbolic name for the error:
> 
> 		dev_err(chip->dev, "RMU: error transmitting request (%pe)",
> 			ERR_PTR(ret));
> 

Thanks.

>> +		goto out;
>> +	}
>> +
>> +	ret = wait_for_completion_timeout(&chip->rmu.completion,
>> +					  msecs_to_jiffies(MV88E6XXX_WAIT_POLL_TIME_MS));
>> +	if (ret == 0) {
>> +		dev_dbg(chip->dev,
>> +			"RMU: timeout waiting for request %d (%d) on dev:port %d:%d\n",
>> +			request, ret, chip->ds->index, port);
> 
> Again, please increase the log level of the error condition.
> Also, chip->ds->index is useless information, we have info about the
> switch via dev_dbg/dev_err.
> 

Ok, thanks.

>> +		ret = -ETIMEDOUT;
>> +	}
>> +
>> +out:
>> +	mutex_unlock(&chip->rmu.mutex);
>> +
>> +	return ret > 0 ? 0 : ret;
>> +}
>> +
>> +static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
>> +{
>> +	const u8 get_id[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
>> +	int ret = -1;
>> +
>> +	if (chip->rmu.got_id)
>> +		return 0;
>> +
>> +	chip->rmu.netdev = dev_get_by_name(&init_net, "chan0");
> 
> What if I don't have a slave device called "chan0"? I can't use the RMU?
> 

Reworked in the next version.

>> +	if (!chip->rmu.netdev) {
>> +		dev_dbg(chip->dev, "RMU: unable to get interface");
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_GET_ID, get_id, 8);
>> +	if (ret) {
>> +		dev_dbg(chip->dev, "RMU: error for command GET_ID %d index %d\n", ret,
>> +			chip->ds->index);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
>> +{
>> +	u8 dump_mib[8] = { 0x00, 0x01, 0x00, 0x00, 0x10, 0x20, 0x00, 0x00 };
>> +	int ret;
>> +
>> +	if (!chip)
>> +		return 0;
> 
> How can "chip" be NULL?
> 

Yes, Andrew pointed that out. Thanks.

>> +
>> +	ret = mv88e6xxx_rmu_get_id(chip, port);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Send a GET_MIB command */
>> +	dump_mib[7] = port;
>> +	ret = mv88e6xxx_rmu_send_wait(chip, port, MV88E6XXX_RMU_REQ_DUMP_MIB, dump_mib, 8);
>> +	if (ret) {
>> +		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %d dev %d:%d\n", ret,
>> +			chip->ds->index, port);
>> +		return ret;
>> +	}
>> +
>> +	/* Update MIB for port */
>> +	if (chip->info->ops->stats_get_stats)
>> +		return chip->info->ops->stats_get_stats(chip, port, data);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct mv88e6xxx_bus_ops mv88e6xxx_bus_ops = {
> 
> static const struct
> 
>> +	.get_rmon = mv88e6xxx_rmu_stats_get,
>> +};
>> +
>> +int mv88e6xxx_rmu_init(struct mv88e6xxx_chip *chip)
>> +{
>> +	dev_info(chip->dev, "RMU: setting up for switch@%d", chip->sw_addr);
> 
> Not a whole lotta useful information brought by this. Also,
> chip->sw_addr is already visible via dev_info(chip->dev).
> I would just drop this.
> 

Ok.

>> +
>> +	init_completion(&chip->rmu.completion);
>> +
>> +	mutex_init(&chip->rmu.mutex);
> 
> I would just move these to mv88e6xxx_rmu_setup(), and move
> mv88e6xxx_rmu_setup() to rmu.c.
> 
>> +
>> +	chip->rmu.ops = &mv88e6xxx_bus_ops;
> 
> By doing this (combined with the way in which chip->rmu.ops is actually
> tested for), you are saying that RMU is available since driver probe time.
> But it isn't. It's only available when the master goes up and is
> operational. So you're setting yourself up for lots of I/O failures in
> the beginning.
> 

Reworked in the next version.

>> +
>> +	return 0;
>> +}
>> +
>> +void mv88e6xxx_rmu_master_change(struct dsa_switch *ds, const struct net_device *master,
>> +				 bool operational)
>> +{
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +
>> +	if (operational)
>> +		chip->rmu.ops = &mv88e6xxx_bus_ops;
>> +	else
>> +		chip->rmu.ops = NULL;
>> +}
> 
> There is a subtle but very important point to be careful about here,
> which is compatibility with multiple CPU ports. If there is a second DSA
> master whose state flaps from up to down, this should not affect the
> fact that you can still use RMU over the first DSA master. But in your
> case it does, so this is a case of how not to write code that accounts
> for that.
> 
> In fact, given this fact, I think your function prototypes for
> chip->info->ops->rmu_enable() are all wrong / not sufficiently
> reflective of what the hardware can do. If the hardware has a bit mask
> of ports on which RMU operations are possible, why hardcode using
> dsa_switch_upstream_port() and not look at which DSA masters/CPU ports
> are actually up? At least for the top-most switch. For downstream
> switches we can use dsa_switch_upstream_port(), I guess (even that can
> be refined, but I'm not aware of setups using multiple DSA links, where
> each DSA link ultimately goes to a different upstream switch).

This is also changed a la qca8k.

Re: [PATCH net-next v2 2/3] dsa: mv88e6xxx: Add support for RMU in select switches

[Vladimir Oltean]

On Wed, Aug 31, 2022 at 08:12:09AM +0200, Mattias Forsblad wrote:
> Please elaborate why this may pose a problem, I might have missed
> some information.

Because as I said, you need to enable it over the CPU port connected to
the master which goes up and is operational, and you don't have this
information at probe time.

> > If you decide to rework this using the master netdev, you can use
> > dsa_tag_protocol_overhead(master->dsa_ptr->tag_ops). Or even reserve
> > enough headroom for the larger header (EDSA) and be done with it.
> > But then you need to construct a different header depending on whether
> > DSA or EDSA is used.
> > 
> 
> So in the new version a la qca8k we need the 'extra' parameter to
> see if we need space for EDSA header, thus we need run through the tagger.
> We can discuss that in the next version.

"thus we need run through the tagger" -> is this a justification that
you're going to keep the tag_ops->inband_xmit?

You don't _have_ to, you already have access to the tagging protocol in
use via chip->tag_protocol, you can derive from that if you need the E
in EDSA or not, and still keep everything within the switch driver.

> > Could you please explain for me what will setting skb->pkt_type to
> > PACKET_OUTGOING achieve?
> >
> 
> I though it was prudent, will remove if it's not needed.

I honestly don't know what it does.

[PATCH net-next v2 3/3] rmon: Use RMU if available

[Mattias Forsblad]

If RMU is supported, use that interface to
collect rmon data.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 4c0510abd875..0d0241ace708 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1226,16 +1226,30 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     u16 bank1_select, u16 histogram)
 {
 	struct mv88e6xxx_hw_stat *stat;
+	int offset = 0;
+	u64 high;
 	int i, j;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
 		stat = &mv88e6xxx_hw_stats[i];
 		if (stat->type & types) {
-			mv88e6xxx_reg_lock(chip);
-			data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
-							      bank1_select,
-							      histogram);
-			mv88e6xxx_reg_unlock(chip);
+			if (chip->rmu.ops && chip->rmu.ops->get_rmon &&
+			    !(stat->type & STATS_TYPE_PORT)) {
+				if (stat->type & STATS_TYPE_BANK1)
+					offset = 32;
+
+				data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
+				if (stat->size == 8) {
+					high = chip->ports[port].rmu_raw_stats[stat->reg + offset
+							+ 1];
+					data[j] += (high << 32);
+				}
+			} else {
+				mv88e6xxx_reg_lock(chip);
+				data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
+								      bank1_select, histogram);
+				mv88e6xxx_reg_unlock(chip);
+			}
 
 			j++;
 		}
@@ -1304,8 +1318,8 @@ static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
 	mv88e6xxx_reg_unlock(chip);
 }
 
-static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
-					uint64_t *data)
+static void mv88e6xxx_get_ethtool_stats_mdio(struct dsa_switch *ds, int port,
+					     uint64_t *data)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 	int ret;
@@ -1319,7 +1333,20 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
 		return;
 
 	mv88e6xxx_get_stats(chip, port, data);
+}
 
+static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
+					uint64_t *data)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+
+	/* If initialization of RMU isn't available
+	 * fall back to MDIO access.
+	 */
+	if (chip->rmu.ops && chip->rmu.ops->get_rmon)
+		chip->rmu.ops->get_rmon(chip, port, data);
+	else
+		mv88e6xxx_get_ethtool_stats_mdio(ds, port, data);
 }
 
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
-- 
2.25.1

Re: [PATCH net-next v2 3/3] rmon: Use RMU if available

[Vladimir Oltean]

Hi Forsblad,

On Fri, Aug 26, 2022 at 08:38:16AM +0200, Mattias Forsblad wrote:
> If RMU is supported, use that interface to
> collect rmon data.

A more adequate commit message would be:

net: dsa: mv88e6xxx: use RMU to collect RMON stats if available

But then, I don't think the splitting of patches is good. I think
mv88e6xxx_inband_rcv(), the producer of rmu_raw_stats[], should be
introduced along with its consumer. Otherwise I have to jump between one
patch and another to be able to comment and see things.

Also, it would be good if you could consider actually reporting the RMON
stats through the standardized interface (ds->ops->get_rmon_stats) and
ethtool -S lan0 --groups rmon, rather than just unstructured ethtool -S.

> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 4c0510abd875..0d0241ace708 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -1226,16 +1226,30 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>  				     u16 bank1_select, u16 histogram)
>  {
>  	struct mv88e6xxx_hw_stat *stat;
> +	int offset = 0;
> +	u64 high;
>  	int i, j;
>  
>  	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
>  		stat = &mv88e6xxx_hw_stats[i];
>  		if (stat->type & types) {
> -			mv88e6xxx_reg_lock(chip);
> -			data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
> -							      bank1_select,
> -							      histogram);
> -			mv88e6xxx_reg_unlock(chip);
> +			if (chip->rmu.ops && chip->rmu.ops->get_rmon &&
> +			    !(stat->type & STATS_TYPE_PORT)) {
> +				if (stat->type & STATS_TYPE_BANK1)
> +					offset = 32;
> +
> +				data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
> +				if (stat->size == 8) {
> +					high = chip->ports[port].rmu_raw_stats[stat->reg + offset
> +							+ 1];
> +					data[j] += (high << 32);

What exactly protects ethtool -S, a reader of rmu_raw_stats[], from
reading an array that is concurrently overwritten by mv88e6xxx_inband_rcv?

> +				}
> +			} else {
> +				mv88e6xxx_reg_lock(chip);
> +				data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
> +								      bank1_select, histogram);
> +				mv88e6xxx_reg_unlock(chip);
> +			}
>  
>  			j++;
>  		}
> @@ -1304,8 +1318,8 @@ static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
>  	mv88e6xxx_reg_unlock(chip);
>  }
>  
> -static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
> -					uint64_t *data)
> +static void mv88e6xxx_get_ethtool_stats_mdio(struct dsa_switch *ds, int port,
> +					     uint64_t *data)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  	int ret;
> @@ -1319,7 +1333,20 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>  		return;
>  
>  	mv88e6xxx_get_stats(chip, port, data);
> +}
>  
> +static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
> +					uint64_t *data)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +
> +	/* If initialization of RMU isn't available
> +	 * fall back to MDIO access.
> +	 */
> +	if (chip->rmu.ops && chip->rmu.ops->get_rmon)

I would create a helper

static bool mv88e6xxx_rmu_available(struct mv88e6xxx_chip *chip)

and use it here and everywhere, for clarity. Testing the presence of
chip->rmu.ops is not wrong, but confusing.

Also, testing chip->rmu.ops->get_rmon gains exactly nothing, since it is
never NULL when chip->rmu.ops isn't NULL.

> +		chip->rmu.ops->get_rmon(chip, port, data);
> +	else
> +		mv88e6xxx_get_ethtool_stats_mdio(ds, port, data);
>  }
>  
>  static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
> -- 
> 2.25.1
> 

Re: [PATCH net-next v2 3/3] rmon: Use RMU if available

[Mattias Forsblad]

On 2022-08-30 16:20, Vladimir Oltean wrote:
> Hi Forsblad,
> 
> On Fri, Aug 26, 2022 at 08:38:16AM +0200, Mattias Forsblad wrote:
>> If RMU is supported, use that interface to
>> collect rmon data.
> 
> A more adequate commit message would be:
> 
> net: dsa: mv88e6xxx: use RMU to collect RMON stats if available
> 
> But then, I don't think the splitting of patches is good. I think
> mv88e6xxx_inband_rcv(), the producer of rmu_raw_stats[], should be
> introduced along with its consumer. Otherwise I have to jump between one
> patch and another to be able to comment and see things.
> 

I'll have that in mind for the next round. The next version will
look way different after Andrews suggestion.

> Also, it would be good if you could consider actually reporting the RMON
> stats through the standardized interface (ds->ops->get_rmon_stats) and
> ethtool -S lan0 --groups rmon, rather than just unstructured ethtool -S.
>

Cool, I didn't know it existed. I'll look into that.
 
>>
>> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/chip.c | 41 ++++++++++++++++++++++++++------
>>  1 file changed, 34 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 4c0510abd875..0d0241ace708 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -1226,16 +1226,30 @@ static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>>  				     u16 bank1_select, u16 histogram)
>>  {
>>  	struct mv88e6xxx_hw_stat *stat;
>> +	int offset = 0;
>> +	u64 high;
>>  	int i, j;
>>  
>>  	for (i = 0, j = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
>>  		stat = &mv88e6xxx_hw_stats[i];
>>  		if (stat->type & types) {
>> -			mv88e6xxx_reg_lock(chip);
>> -			data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
>> -							      bank1_select,
>> -							      histogram);
>> -			mv88e6xxx_reg_unlock(chip);
>> +			if (chip->rmu.ops && chip->rmu.ops->get_rmon &&
>> +			    !(stat->type & STATS_TYPE_PORT)) {
>> +				if (stat->type & STATS_TYPE_BANK1)
>> +					offset = 32;
>> +
>> +				data[j] = chip->ports[port].rmu_raw_stats[stat->reg + offset];
>> +				if (stat->size == 8) {
>> +					high = chip->ports[port].rmu_raw_stats[stat->reg + offset
>> +							+ 1];
>> +					data[j] += (high << 32);
> 
> What exactly protects ethtool -S, a reader of rmu_raw_stats[], from
> reading an array that is concurrently overwritten by mv88e6xxx_inband_rcv?
> 

So for the Marvell SOHO the RMU is purely a request/response protocol. The switchcore
will not send a frame unless requested, thus no barrier is needed. For other switchcores
which may have send frames spontaneous additional care may be needed.

>> +				}
>> +			} else {
>> +				mv88e6xxx_reg_lock(chip);
>> +				data[j] = _mv88e6xxx_get_ethtool_stat(chip, stat, port,
>> +								      bank1_select, histogram);
>> +				mv88e6xxx_reg_unlock(chip);
>> +			}
>>  
>>  			j++;
>>  		}
>> @@ -1304,8 +1318,8 @@ static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
>>  	mv88e6xxx_reg_unlock(chip);
>>  }
>>  
>> -static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>> -					uint64_t *data)
>> +static void mv88e6xxx_get_ethtool_stats_mdio(struct dsa_switch *ds, int port,
>> +					     uint64_t *data)
>>  {
>>  	struct mv88e6xxx_chip *chip = ds->priv;
>>  	int ret;
>> @@ -1319,7 +1333,20 @@ static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>>  		return;
>>  
>>  	mv88e6xxx_get_stats(chip, port, data);
>> +}
>>  
>> +static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
>> +					uint64_t *data)
>> +{
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +
>> +	/* If initialization of RMU isn't available
>> +	 * fall back to MDIO access.
>> +	 */
>> +	if (chip->rmu.ops && chip->rmu.ops->get_rmon)
> 
> I would create a helper
> 
> static bool mv88e6xxx_rmu_available(struct mv88e6xxx_chip *chip)
> 
> and use it here and everywhere, for clarity. Testing the presence of
> chip->rmu.ops is not wrong, but confusing.
> 
> Also, testing chip->rmu.ops->get_rmon gains exactly nothing, since it is
> never NULL when chip->rmu.ops isn't NULL.
> 

Agreed. The next version will draw inspiration from qca8k.
>> +		chip->rmu.ops->get_rmon(chip, port, data);
>> +	else
>> +		mv88e6xxx_get_ethtool_stats_mdio(ds, port, data);
>>  }
>>  
>>  static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
>> -- 
>> 2.25.1
>>