[PATCH net-next v14 0/7] net: dsa: qca8k, mv88e6xxx: rmon: Add RMU support

[PATCH net-next v14 0/7] net: dsa: qca8k, mv88e6xxx: rmon: 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) in the switch
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.

qca8k
=====
There's a newly introduced convenience function for sending
and waiting for frames. Changes have been made for the qca8k
driver to use this. Please test for regressions.

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.

v2 -> v3:
  - Rewrite after feedback. Use tagger_data to handle
    frames more like qca8k.
  - qca8k: Change to use convenience functions introduced.
    Requesting test of this.
    
v3 -> v4:
  - Separated patches more granular.

v4 -> v5:
  - Some small fixes after feedback.

v5 -> v6:
  - Rewrite of send_wait function to more adhere
    to RPC standards
  - Cleanup of ops handling
  - Move get id to when master device is available.

v6 -> v7:
  - Some minor cleanups.

v7 -> v8:
  - Moved defines to header file.
  - Check RMU response length and return actual
    length received.
  - Added disable/enable helpers for RMU.
  - Fixed some error paths.

v8 -> v9:
  - Naming consistency for parameters/functions.
  - Streamlined completion routines.
  - Moved completion init earlier.
  - Spelling corrected.
  - Moved dsa_tagger_data declaration.
  - Minimal frame2reg decoding in tag_dsa.
  - Fixed return codes.
  - Use convenience functions.
  - Streamlined function parameters.
  - Fixed error path when master device changes
    state.
  - Still verify MAC address (per request of Andrew Lunn)
  - Use skb_get instead of skb_copy
  - Prefix defines and structs correctly.
  - Change types to __beXX.

v9 -> v10:
  - Patchworks feedback fixed.

v10 -> v11:
  - Fixed sparse warnings.

v11 -> v12:
  - Split mv88e6xxx_stats_get_stats into separate
    functions, one for RMU and one for legacy
    access.

v12 -> v13:
  - Expose all RMON counters via RMU.

v13 -> v14:
  - Fix feedback on qca8k. 80 char line length.
  - Pass response data to RMON decoding routine.
  - Remove unused function.
  - Split patch.
  
Regards,
Mattias Forsblad

Mattias Forsblad (7):
  net: dsa: mv88e6xxx: Add RMU enable for select switches.
  net: dsa: Add convenience functions for frame handling
  net: dsa: Introduce dsa tagger data operation.
  net: dsa: mv88e6xxxx: Add RMU functionality.
  net: dsa: mv88e6xxx: rmu: Add functionality to get RMON
  net: dsa: mv88e6xxx: rmon: Use RMU for reading RMON data
  net: dsa: qca8k: Use new convenience functions

 drivers/net/dsa/mv88e6xxx/Makefile  |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c    | 120 ++++++++++-
 drivers/net/dsa/mv88e6xxx/chip.h    |  26 +++
 drivers/net/dsa/mv88e6xxx/global1.c |  64 ++++++
 drivers/net/dsa/mv88e6xxx/global1.h |   3 +
 drivers/net/dsa/mv88e6xxx/rmu.c     | 316 ++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h     |  73 +++++++
 drivers/net/dsa/mv88e6xxx/smi.c     |   3 +
 drivers/net/dsa/qca/qca8k-8xxx.c    |  68 +++---
 include/linux/dsa/mv88e6xxx.h       |   6 +
 include/net/dsa.h                   |  11 +
 net/dsa/dsa.c                       |  17 ++
 net/dsa/dsa2.c                      |   2 +
 net/dsa/dsa_priv.h                  |   2 +
 net/dsa/tag_dsa.c                   |  40 +++-
 15 files changed, 693 insertions(+), 59 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 v14 1/7] net: dsa: mv88e6xxx: Add RMU enable for select switches.

[Mattias Forsblad]

Add RMU enable functionality for some Marvell SOHO switches.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c    |  6 +++
 drivers/net/dsa/mv88e6xxx/chip.h    |  1 +
 drivers/net/dsa/mv88e6xxx/global1.c | 64 +++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/global1.h |  3 ++
 4 files changed, 74 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 6f4ea39ab466..46e12b53a9e4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -4098,6 +4098,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,
@@ -4181,6 +4182,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,
@@ -5300,6 +5302,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,
@@ -5367,6 +5370,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,
@@ -5434,6 +5438,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,
@@ -5504,6 +5509,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,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index e693154cf803..7ce3c41f6caf 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -637,6 +637,7 @@ struct mv88e6xxx_ops {
 
 	/* Remote Management Unit operations */
 	int (*rmu_disable)(struct mv88e6xxx_chip *chip);
+	int (*rmu_enable)(struct mv88e6xxx_chip *chip, int port);
 
 	/* 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..1b3a3218c0b5 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -466,18 +466,82 @@ 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 port)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+
+	switch (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 port)
+{
+	int val = MV88E6352_G1_CTL2_RMU_MODE_DISABLED;
+
+	switch (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 port)
+{
+	int val = MV88E6390_G1_CTL2_RMU_MODE_DISABLED;
+
+	switch (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..b9aa66712037 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 port);
 int mv88e6352_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6352_g1_rmu_enable(struct mv88e6xxx_chip *chip, int port);
 int mv88e6390_g1_rmu_disable(struct mv88e6xxx_chip *chip);
+int mv88e6390_g1_rmu_enable(struct mv88e6xxx_chip *chip, int port);
 
 int mv88e6xxx_g1_set_device_number(struct mv88e6xxx_chip *chip, int index);
 
-- 
2.25.1

[PATCH net-next v14 2/7] net: dsa: Add convenience functions for frame handling

[Mattias Forsblad]

Add common control functions for drivers that need
to send and wait for control frames.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 include/net/dsa.h | 11 +++++++++++
 net/dsa/dsa.c     | 17 +++++++++++++++++
 net/dsa/dsa2.c    |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f2ce12860546..08f3fff5f4df 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -495,6 +495,8 @@ struct dsa_switch {
 	unsigned int		max_num_bridges;
 
 	unsigned int		num_ports;
+
+	struct completion	inband_done;
 };
 
 static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
@@ -1390,6 +1392,15 @@ void dsa_tag_drivers_register(struct dsa_tag_driver *dsa_tag_driver_array[],
 void dsa_tag_drivers_unregister(struct dsa_tag_driver *dsa_tag_driver_array[],
 				unsigned int count);
 
+int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
+			 struct completion *completion, unsigned long timeout);
+
+static inline void dsa_switch_inband_complete(struct dsa_switch *ds, struct completion *completion)
+{
+	/* Custom completion? */
+	complete(completion ?: &ds->inband_done);
+}
+
 #define dsa_tag_driver_module_drivers(__dsa_tag_drivers_array, __count)	\
 static int __init dsa_tag_driver_module_init(void)			\
 {									\
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index be7b320cda76..ad870494d68b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -324,6 +324,23 @@ int dsa_switch_resume(struct dsa_switch *ds)
 EXPORT_SYMBOL_GPL(dsa_switch_resume);
 #endif
 
+int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
+			 struct completion *completion, unsigned long timeout)
+{
+	struct completion *com;
+
+	/* Custom completion? */
+	com = completion ? : &ds->inband_done;
+
+	reinit_completion(com);
+
+	if (skb)
+		dev_queue_xmit(skb);
+
+	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
+}
+EXPORT_SYMBOL_GPL(dsa_switch_inband_tx);
+
 static struct packet_type dsa_pack_type __read_mostly = {
 	.type	= cpu_to_be16(ETH_P_XDSA),
 	.func	= dsa_switch_rcv,
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ed56c7a554b8..a048a6200789 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -874,6 +874,8 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 	if (ds->setup)
 		return 0;
 
+	init_completion(&ds->inband_done);
+
 	/* Initialize ds->phys_mii_mask before registering the slave MDIO bus
 	 * driver and before ops->setup() has run, since the switch drivers and
 	 * the slave MDIO bus driver rely on these values for probing PHY
-- 
2.25.1

Re: [PATCH net-next v14 2/7] net: dsa: Add convenience functions for frame handling

[Vladimir Oltean]

On Mon, Sep 19, 2022 at 01:08:42PM +0200, Mattias Forsblad wrote:
> +int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
> +			 struct completion *completion, unsigned long timeout)
> +{
> +	struct completion *com;
> +
> +	/* Custom completion? */
> +	com = completion ? : &ds->inband_done;
> +
> +	reinit_completion(com);
> +
> +	if (skb)
> +		dev_queue_xmit(skb);

Does it make sense from an API perspective to call dsa_switch_inband_tx()
with a NULL skb? If yes, please add a comment explaining why. If not,
please delete this check.

> +
> +	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));

If this is going to be provided as a DSA common layer "helper" function,
at least make an effort to document the expected return code.

Hint, wait_for_completion_timeout() returns an unsigned long time_left,
you return an int. What does it mean?!

At the bare minimum, leave a comment, especially when it's not obvious
(DSA typically uses negative integer values as error codes, and zero on
success. Here, zero is an error - timeout. If the amount of time left
does not matter, do translate this into 0 for success, and -ETIMEDOUT
for timeout). If you're also feeling generous, do please also update
Documentation/networking/dsa/dsa.rst with the info about the flow of
Ethernet-based register access that you wish was available while you
were figuring out how things worked.

> +}
> +EXPORT_SYMBOL_GPL(dsa_switch_inband_tx);

Re: [PATCH net-next v14 2/7] net: dsa: Add convenience functions for frame handling

[Andrew Lunn]

> > +
> > +	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
> 
> If this is going to be provided as a DSA common layer "helper" function,
> at least make an effort to document the expected return code.
> 
> Hint, wait_for_completion_timeout() returns an unsigned long time_left,
> you return an int. What does it mean?!
> 
> At the bare minimum, leave a comment, especially when it's not obvious
> (DSA typically uses negative integer values as error codes, and zero on
> success. Here, zero is an error - timeout. If the amount of time left
> does not matter, do translate this into 0 for success, and -ETIMEDOUT
> for timeout). If you're also feeling generous, do please also update
> Documentation/networking/dsa/dsa.rst with the info about the flow of
> Ethernet-based register access that you wish was available while you
> were figuring out how things worked.

Hi Vladimir

I just posted an alternative to this patch. One of the patches
addresses this point. I convert the return value to the more normal
-ve for error, 0 for success, including -ETIMEOUT if the completion
times out.

My patchset is however missing documentation. Once we get the basic
API agreed on, i can extend Documentation/networking/dsa/dsa.rst.

       Andrew

[PATCH rfc v0 0/9] DSA: Move parts of inband signalling into the DSA

[Andrew Lunn]

This is an RFC patchset.

Mattias Forsblad proposal for adding some core helpers to DSA for
inband signalling is going in a good direction, but there are a couple
of things which i think can be better. This patchset offs an
alternative to

patch 2/7: net: dsa: Add convenience functions for frame handling

and

patch 7/7 net: dsa: qca8k: Use new convenience functions

This patchset takes the abstraction further, putting more into the
core. It also makes the qca8k fully use the abstraction unlike 7/7.

The end result has a slightly different structure, in that there is a
struct dsa_inband of which qca8k has two instances of this. Doing this
avoids the custom completion code. If qca8k can have multiple parallel
request/replies in flight, it seems likely other devices can as well,
so this should be part of the abstraction.

Since i don't have the qck8 hardware, i hope that lots of small
patches make the review work easier, and finding the introduced bugs
is quicker.

The MIB handling of the qc8k is somewhat odd. It would be nice to work
on that further and try to make it better fit the model used
here. That work can be done later, and probably is more invasive than
the step by step approach taken here.

Another aim has been to make it easy to merge Mattias mv88e6xxx
patches with this patchset. The basic API is the same, so i think it
should be possible.

These are compile tested only....

Andrew Lunn (9):
  net: dsa: qca8k: Fix inconsistent use of jiffies vs milliseconds
  net: dsa: qca8k: Move completion into DSA core
  net: dsa: qca8K: Move queuing for request frame into the core
  net: dsa: qca8k: dsa_inband_request: More normal return values
  net: dsa: qca8k: Move request sequence number handling into core
  net: dsa: qca8k: Refactor sequence number mismatch to use error code
  net: dsa: qca8k: Pass error code from reply decoder to requester
  net: dsa: qca8k: Pass response buffer via dsa_rmu_request
  net: dsa: qca8k: Move inband mutex into DSA core

 drivers/net/dsa/qca/qca8k-8xxx.c | 234 ++++++++-----------------------
 drivers/net/dsa/qca/qca8k.h      |   8 +-
 include/net/dsa.h                |  25 ++++
 net/dsa/dsa.c                    |  90 ++++++++++++
 4 files changed, 172 insertions(+), 185 deletions(-)

-- 
2.37.2

[PATCH rfc v0 1/9] net: dsa: qca8k: Fix inconsistent use of jiffies vs milliseconds

[Andrew Lunn]

wait_for_complete_timeout() expects a timeout in jiffies. With the
driver, some call sites converted QCA8K_ETHERNET_TIMEOUT to jiffies,
others did not. Make the code consistent by changes the #define to
include a call to msecs_to_jiffies, and remove all other calls to
msecs_to_jiffies.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 4 ++--
 drivers/net/dsa/qca/qca8k.h      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index c181346388a4..1c9a8764d1d9 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -258,7 +258,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	dev_queue_xmit(skb);
 
 	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+					  QCA8K_ETHERNET_TIMEOUT);
 
 	*val = mgmt_eth_data->data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -310,7 +310,7 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	dev_queue_xmit(skb);
 
 	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+					  QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index e36ecc9777f4..74578b7c3283 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -15,7 +15,7 @@
 
 #define QCA8K_ETHERNET_MDIO_PRIORITY			7
 #define QCA8K_ETHERNET_PHY_PRIORITY			6
-#define QCA8K_ETHERNET_TIMEOUT				5
+#define QCA8K_ETHERNET_TIMEOUT				msecs_to_jiffies(5)
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
-- 
2.37.2

[PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

[Andrew Lunn]

When performing operations on a remote switch using Ethernet frames, a
completion is used between the sender of the request and the code
which receives the reply.

Move this completion into the DSA core, simplifying the driver.  The
initialisation and reinitialisation of the completion is now performed
in the core. Also, the conversion of milliseconds to jiffies is also
in the core.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 49 ++++++++++++--------------------
 drivers/net/dsa/qca/qca8k.h      |  6 ++--
 include/net/dsa.h                | 12 ++++++++
 net/dsa/dsa.c                    | 22 ++++++++++++++
 4 files changed, 55 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 1c9a8764d1d9..f4e92156bd32 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -160,7 +160,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 			       QCA_HDR_MGMT_DATA2_LEN);
 	}
 
-	complete(&mgmt_eth_data->rw_done);
+	dsa_inband_complete(&mgmt_eth_data->inband);
 }
 
 static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
@@ -248,8 +248,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the mdio pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
@@ -257,8 +255,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	dev_queue_xmit(skb);
 
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+					     QCA8K_ETHERNET_TIMEOUT);
 
 	*val = mgmt_eth_data->data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -300,8 +298,6 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the mdio pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
@@ -309,8 +305,8 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	dev_queue_xmit(skb);
 
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+					     QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -448,8 +444,6 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	bool ack;
 	int ret;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the copy pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
@@ -457,8 +451,8 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 
 	dev_queue_xmit(skb);
 
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+					     QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -540,8 +534,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_master;
 	write_skb->dev = mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the write pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(write_skb, mgmt_eth_data->seq);
@@ -549,8 +541,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 	dev_queue_xmit(write_skb);
 
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+					     QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -577,8 +569,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		reinit_completion(&mgmt_eth_data->rw_done);
-
 		/* Increment seq_num and set it in the read pkt */
 		mgmt_eth_data->seq++;
 		qca8k_mdio_header_fill_seq_num(read_skb, mgmt_eth_data->seq);
@@ -586,8 +576,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 		dev_queue_xmit(read_skb);
 
-		ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-						  QCA8K_ETHERNET_TIMEOUT);
+		ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+						     QCA8K_ETHERNET_TIMEOUT);
 
 		ack = mgmt_eth_data->ack;
 
@@ -606,8 +596,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the clear pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
@@ -615,8 +603,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 	dev_queue_xmit(clear_skb);
 
-	wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-				    QCA8K_ETHERNET_TIMEOUT);
+	dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
+				       QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
@@ -1528,7 +1516,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 exit:
 	/* Complete on receiving all the mib packet */
 	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
-		complete(&mib_eth_data->rw_done);
+		dsa_inband_complete(&mib_eth_data->inband);
 }
 
 static int
@@ -1543,8 +1531,6 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 
 	mutex_lock(&mib_eth_data->mutex);
 
-	reinit_completion(&mib_eth_data->rw_done);
-
 	mib_eth_data->req_port = dp->index;
 	mib_eth_data->data = data;
 	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
@@ -1562,7 +1548,8 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	if (ret)
 		goto exit;
 
-	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_wait_for_completion(&mib_eth_data->inband,
+					     QCA8K_ETHERNET_TIMEOUT);
 
 exit:
 	mutex_unlock(&mib_eth_data->mutex);
@@ -1929,10 +1916,10 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return -ENOMEM;
 
 	mutex_init(&priv->mgmt_eth_data.mutex);
-	init_completion(&priv->mgmt_eth_data.rw_done);
+	dsa_inband_init(&priv->mgmt_eth_data.inband);
 
 	mutex_init(&priv->mib_eth_data.mutex);
-	init_completion(&priv->mib_eth_data.rw_done);
+	dsa_inband_init(&priv->mib_eth_data.inband);
 
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 74578b7c3283..685628716ed2 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -15,7 +15,7 @@
 
 #define QCA8K_ETHERNET_MDIO_PRIORITY			7
 #define QCA8K_ETHERNET_PHY_PRIORITY			6
-#define QCA8K_ETHERNET_TIMEOUT				msecs_to_jiffies(5)
+#define QCA8K_ETHERNET_TIMEOUT				5
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -346,7 +346,7 @@ enum {
 };
 
 struct qca8k_mgmt_eth_data {
-	struct completion rw_done;
+	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
 	bool ack;
 	u32 seq;
@@ -354,7 +354,7 @@ struct qca8k_mgmt_eth_data {
 };
 
 struct qca8k_mib_eth_data {
-	struct completion rw_done;
+	struct dsa_inband inband;
 	struct mutex mutex; /* Process one command at time */
 	refcount_t port_parsed; /* Counter to track parsed port */
 	u8 req_port;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index f2ce12860546..ca81541703f4 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -7,6 +7,7 @@
 #ifndef __LINUX_NET_DSA_H
 #define __LINUX_NET_DSA_H
 
+#include <linux/completion.h>
 #include <linux/if.h>
 #include <linux/if_ether.h>
 #include <linux/list.h>
@@ -1276,6 +1277,17 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
 				 const struct switchdev_obj_port_mdb *mdb,
 				 struct dsa_db db);
 
+/* Perform operations on a switch by sending it request in Ethernet
+ * frames and expecting a response in a frame.
+ */
+struct dsa_inband {
+	struct completion completion;
+};
+
+void dsa_inband_init(struct dsa_inband *inband);
+void dsa_inband_complete(struct dsa_inband *inband);
+int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
+
 /* Keep inline for faster access in hot path */
 static inline bool netdev_uses_dsa(const struct net_device *dev)
 {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index be7b320cda76..382dbb9e921a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -518,6 +518,28 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
 
+void dsa_inband_init(struct dsa_inband *inband)
+{
+	init_completion(&inband->completion);
+}
+EXPORT_SYMBOL_GPL(dsa_inband_init);
+
+void dsa_inband_complete(struct dsa_inband *inband)
+{
+	complete(&inband->completion);
+}
+EXPORT_SYMBOL_GPL(dsa_inband_complete);
+
+int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
+{
+	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
+
+	reinit_completion(&inband->completion);
+
+	return wait_for_completion_timeout(&inband->completion, jiffies);
+}
+EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
+
 static int __init dsa_init_module(void)
 {
 	int rc;
-- 
2.37.2

Re: [PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 12:18:46AM +0200, Andrew Lunn wrote:
> @@ -248,8 +248,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	skb->dev = priv->mgmt_master;
>  
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the mdio pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
> @@ -257,8 +255,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	dev_queue_xmit(skb);
>  
> -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -					  QCA8K_ETHERNET_TIMEOUT);
> +	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> +					     QCA8K_ETHERNET_TIMEOUT);
>  
>  	*val = mgmt_eth_data->data[0];
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)

Replacing the pattern above with this pattern:

int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
{
	unsigned long jiffies = msecs_to_jiffies(timeout_ms);

	reinit_completion(&inband->completion);

	return wait_for_completion_timeout(&inband->completion, jiffies);
}

is buggy because we reinitialize the completion later than the original
code used to. We now call reinit_completion() from a code path that
races with the handler that is supposed to call dsa_inband_complete().

Re: [PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

[Andrew Lunn]

On Tue, Sep 20, 2022 at 02:43:04PM +0000, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 12:18:46AM +0200, Andrew Lunn wrote:
> > @@ -248,8 +248,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  
> >  	skb->dev = priv->mgmt_master;
> >  
> > -	reinit_completion(&mgmt_eth_data->rw_done);
> > -
> >  	/* Increment seq_num and set it in the mdio pkt */
> >  	mgmt_eth_data->seq++;
> >  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
> > @@ -257,8 +255,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  
> >  	dev_queue_xmit(skb);
> >  
> > -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> > -					  QCA8K_ETHERNET_TIMEOUT);
> > +	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> > +					     QCA8K_ETHERNET_TIMEOUT);
> >  
> >  	*val = mgmt_eth_data->data[0];
> >  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> 
> Replacing the pattern above with this pattern:
> 
> int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
> {
> 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
> 
> 	reinit_completion(&inband->completion);
> 
> 	return wait_for_completion_timeout(&inband->completion, jiffies);
> }
> 
> is buggy because we reinitialize the completion later than the original
> code used to. We now call reinit_completion() from a code path that
> races with the handler that is supposed to call dsa_inband_complete().

I've been thinking about this a bit. And i think the bug is in the old
code.

As soon as we call reinit_completion(), a late arriving packet can
trigger the completion. Note that the sequence number has not been
incremented yet. So that late arriving packet can pass the sequence
number test, and the results will be copied and complete() called.

qca8k_read_eth() can continue, increment the sequence number, call
wait_for_completion_timeout() and immediately exit, returning the
contents of the late arriving reply.

To make this safe:

1) The sequence number needs to be incremented before
   reinit_completion(). That closes one race

2) If the sequence numbers don't match, silently drop the packet, it
   is either later, or bogus. Hopefully the correct reply packet will
   come along soon and trigger the completion.

I've also got some of this wrong in my code.

     Andrew

Re: [PATCH rfc v0 2/9] net: dsa: qca8k: Move completion into DSA core

[Vladimir Oltean]

On Wed, Sep 21, 2022 at 02:19:54AM +0200, Andrew Lunn wrote:
> I've been thinking about this a bit. And i think the bug is in the old
> code.
> 
> As soon as we call reinit_completion(), a late arriving packet can
> trigger the completion. Note that the sequence number has not been
> incremented yet. So that late arriving packet can pass the sequence
> number test, and the results will be copied and complete() called.
> 
> qca8k_read_eth() can continue, increment the sequence number, call
> wait_for_completion_timeout() and immediately exit, returning the
> contents of the late arriving reply.
> 
> To make this safe:
> 
> 1) The sequence number needs to be incremented before
>    reinit_completion(). That closes one race
> 
> 2) If the sequence numbers don't match, silently drop the packet, it
>    is either later, or bogus. Hopefully the correct reply packet will
>    come along soon and trigger the completion.
> 
> I've also got some of this wrong in my code.

Wouldn't the programming be more obvious if we didn't try to reuse the
same dsa_inband structure for every request/response, but simply
allocate/free on demand and use only once?

[PATCH rfc v0 3/9] net: dsa: qca8K: Move queuing for request frame into the core

[Andrew Lunn]

Combine the queuing of the request and waiting for the completion into
one core helper. Add the function dsa_rmu_request() to perform this.

Access to statistics is not a strict request/reply, so the
dsa_rmu_wait_for_completion needs to be kept.

It is also no possible to combine dsa_rmu_request() and
dsa_rmu_wait_for_completion() since we need to avoid the race of
sending the request, receiving a reply, and the completion has not
been reinitialised because the schedule at decided to do other things.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 32 ++++++++++----------------------
 include/net/dsa.h                |  2 ++
 net/dsa/dsa.c                    | 16 ++++++++++++++++
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index f4e92156bd32..9c44a09590a6 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -253,10 +253,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-					     QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+				 QCA8K_ETHERNET_TIMEOUT);
 
 	*val = mgmt_eth_data->data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -303,10 +301,8 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-					     QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -449,10 +445,8 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-					     QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -539,10 +533,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	qca8k_mdio_header_fill_seq_num(write_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(write_skb);
-
-	ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-					     QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
+				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -574,10 +566,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		qca8k_mdio_header_fill_seq_num(read_skb, mgmt_eth_data->seq);
 		mgmt_eth_data->ack = false;
 
-		dev_queue_xmit(read_skb);
-
-		ret = dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-						     QCA8K_ETHERNET_TIMEOUT);
+		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
+					 QCA8K_ETHERNET_TIMEOUT);
 
 		ack = mgmt_eth_data->ack;
 
@@ -601,8 +591,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(clear_skb);
-
 	dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
 				       QCA8K_ETHERNET_TIMEOUT);
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index ca81541703f4..50c319832939 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1286,6 +1286,8 @@ struct dsa_inband {
 
 void dsa_inband_init(struct dsa_inband *inband);
 void dsa_inband_complete(struct dsa_inband *inband);
+int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
+		       int timeout_ms);
 int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
 
 /* Keep inline for faster access in hot path */
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 382dbb9e921a..8de0c3124abf 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -540,6 +540,22 @@ int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
 }
 EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
 
+/* Cannot use dsa_inband_wait_completion since the completion needs to be
+ * reinitialized before the skb is queue to avoid races.
+ */
+int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
+		       int timeout_ms)
+{
+	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
+
+	reinit_completion(&inband->completion);
+
+	dev_queue_xmit(skb);
+
+	return wait_for_completion_timeout(&inband->completion, jiffies);
+}
+EXPORT_SYMBOL_GPL(dsa_inband_request);
+
 static int __init dsa_init_module(void)
 {
 	int rc;
-- 
2.37.2

[PATCH rfc v0 4/9] net: dsa: qca8k: dsa_inband_request: More normal return values

[Andrew Lunn]

wait_for_completion_timeout() has unusual return values.  It can
return negative error conditions. If it times out, it returns 0, and
on success it returns the number of remaining jiffies for the timeout.

For the use case here, the remaining time is not needed. All that is
really interesting is, it succeeded and returns 0, or there was an
error or a timeout. Massage the return value to fit this, and modify
the callers to the more usual pattern of ret < 0 is an error.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 23 ++++++++++-------------
 net/dsa/dsa.c                    |  8 +++++++-
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 9c44a09590a6..9481a248273a 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -264,8 +264,8 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
-	if (ret <= 0)
-		return -ETIMEDOUT;
+	if (ret)
+		return ret;
 
 	if (!ack)
 		return -EINVAL;
@@ -308,8 +308,8 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
-	if (ret <= 0)
-		return -ETIMEDOUT;
+	if (ret)
+		return ret;
 
 	if (!ack)
 		return -EINVAL;
@@ -450,8 +450,8 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 
 	ack = mgmt_eth_data->ack;
 
-	if (ret <= 0)
-		return -ETIMEDOUT;
+	if (ret)
+		return ret;
 
 	if (!ack)
 		return -EINVAL;
@@ -538,8 +538,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 	ack = mgmt_eth_data->ack;
 
-	if (ret <= 0) {
-		ret = -ETIMEDOUT;
+	if (ret) {
 		kfree_skb(read_skb);
 		goto exit;
 	}
@@ -571,10 +570,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 		ack = mgmt_eth_data->ack;
 
-		if (ret <= 0) {
-			ret = -ETIMEDOUT;
+		if (ret)
 			goto exit;
-		}
 
 		if (!ack) {
 			ret = -EINVAL;
@@ -591,8 +588,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
-				       QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+				 QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 8de0c3124abf..68576f1c5b02 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -547,12 +547,18 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 		       int timeout_ms)
 {
 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
+	int ret;
 
 	reinit_completion(&inband->completion);
 
 	dev_queue_xmit(skb);
 
-	return wait_for_completion_timeout(&inband->completion, jiffies);
+	ret = wait_for_completion_timeout(&inband->completion, jiffies);
+	if (ret < 0)
+		return ret;
+	if (ret == 0)
+		return -ETIMEDOUT;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_request);
 
-- 
2.37.2

Re: [PATCH rfc v0 4/9] net: dsa: qca8k: dsa_inband_request: More normal return values

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote:
> wait_for_completion_timeout() has unusual return values.  It can
> return negative error conditions. If it times out, it returns 0, and
> on success it returns the number of remaining jiffies for the timeout.

The one that also returns negative errors is wait_for_completion_interruptible()
(and its variants).  In my experience the interruptible version is also
a huge foot gun, since user space can kill the process waiting for the
RMU response, and the RMU response can still come afterwards, while no
one is waiting for it.  The noninterruptible wait that we use here
really returns an unsigned long, so no negatives.

Re: [PATCH rfc v0 4/9] net: dsa: qca8k: dsa_inband_request: More normal return values

[Andrew Lunn]

On Mon, Sep 19, 2022 at 11:02:14PM +0000, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote:
> > wait_for_completion_timeout() has unusual return values.  It can
> > return negative error conditions. If it times out, it returns 0, and
> > on success it returns the number of remaining jiffies for the timeout.
> 
> The one that also returns negative errors is wait_for_completion_interruptible()
> (and its variants).  In my experience the interruptible version is also
> a huge foot gun, since user space can kill the process waiting for the
> RMU response, and the RMU response can still come afterwards, while no
> one is waiting for it.  The noninterruptible wait that we use here
> really returns an unsigned long, so no negatives.

The driver needs to handle the reply coming later independent of ^C
handling, etc. The qca8k has a timeout of 5ms. I don't know if that is
actually enough, if 1G of traffic is being passed over the interface,
and the TX queue is full, and the request frame does not get put at
the head of the queue.  And if there is 1G of traffic also being
received from the switch, how long are the queues for the reply? Does
the switch put the reply at the head of the queue?

This is one thing i want to play with sometime soon, heavily load the
CPU link and see how well the RMU interface to mv88e6xxx works, are
the timeouts big enough? Do frames get dropped and are retires needed?
Do we need to play with the QoS bits of the skb to make Linux put the
RMU packets at the head of the queue etc.

I would also like to have another look at the code and make sure it is
sane for exactly this case.

     Andrew

Re: [PATCH rfc v0 4/9] net: dsa: qca8k: dsa_inband_request: More normal return values

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 12:18:48AM +0200, Andrew Lunn wrote:
> wait_for_completion_timeout() has unusual return values.  It can
> @@ -591,8 +588,8 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dsa_inband_wait_for_completion(&mgmt_eth_data->inband,
> -				       QCA8K_ETHERNET_TIMEOUT);
> +	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
> +				 QCA8K_ETHERNET_TIMEOUT);

Ansuel commented in Message-ID 12edaefc-89a2-f231-156e-5dbe198ae6f6@gmail.com
that not checking the error code here was deliberate, and that when Mattias
did check the error code, things broke.

>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  

[PATCH rfc v0 5/9] net: dsa: qca8k: Move request sequence number handling into core

[Andrew Lunn]

Each request/reply frame is likely to have a sequence number so that
request and the reply can be matched together. Move this sequence
number into the inband structure. The driver must provide a helper to
insert the sequence number into the skb, and the core will perform the
increment.

To allow different devices to have different size sequence numbers, a
mask is provided. This can be used for example to reduce the u32
sequence number down to a u8.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 35 +++++++++-----------------------
 drivers/net/dsa/qca/qca8k.h      |  1 -
 include/net/dsa.h                |  6 +++++-
 net/dsa/dsa.c                    | 16 ++++++++++++++-
 4 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 9481a248273a..a354ba070d33 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -146,7 +146,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
 
 	/* Make sure the seq match the requested packet */
-	if (mgmt_ethhdr->seq == mgmt_eth_data->seq)
+	if (mgmt_ethhdr->seq == dsa_inband_seqno(&mgmt_eth_data->inband))
 		mgmt_eth_data->ack = true;
 
 	if (cmd == MDIO_READ) {
@@ -247,14 +247,11 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	}
 
 	skb->dev = priv->mgmt_master;
-
-	/* Increment seq_num and set it in the mdio pkt */
-	mgmt_eth_data->seq++;
-	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
-				 QCA8K_ETHERNET_TIMEOUT);
+			      qca8k_mdio_header_fill_seq_num,
+			      QCA8K_ETHERNET_TIMEOUT);
 
 	*val = mgmt_eth_data->data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -295,13 +292,10 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	}
 
 	skb->dev = priv->mgmt_master;
-
-	/* Increment seq_num and set it in the mdio pkt */
-	mgmt_eth_data->seq++;
-	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
@@ -440,12 +434,10 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	bool ack;
 	int ret;
 
-	/* Increment seq_num and set it in the copy pkt */
-	mgmt_eth_data->seq++;
-	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
@@ -527,13 +519,10 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	read_skb->dev = mgmt_master;
 	clear_skb->dev = mgmt_master;
 	write_skb->dev = mgmt_master;
-
-	/* Increment seq_num and set it in the write pkt */
-	mgmt_eth_data->seq++;
-	qca8k_mdio_header_fill_seq_num(write_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
+				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
@@ -560,12 +549,10 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		/* Increment seq_num and set it in the read pkt */
-		mgmt_eth_data->seq++;
-		qca8k_mdio_header_fill_seq_num(read_skb, mgmt_eth_data->seq);
 		mgmt_eth_data->ack = false;
 
 		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
+					 qca8k_mdio_header_fill_seq_num,
 					 QCA8K_ETHERNET_TIMEOUT);
 
 		ack = mgmt_eth_data->ack;
@@ -583,12 +570,10 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	/* Increment seq_num and set it in the clear pkt */
-	mgmt_eth_data->seq++;
-	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
@@ -1901,10 +1886,10 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return -ENOMEM;
 
 	mutex_init(&priv->mgmt_eth_data.mutex);
-	dsa_inband_init(&priv->mgmt_eth_data.inband);
+	dsa_inband_init(&priv->mgmt_eth_data.inband, U32_MAX);
 
 	mutex_init(&priv->mib_eth_data.mutex);
-	dsa_inband_init(&priv->mib_eth_data.inband);
+	dsa_inband_init(&priv->mib_eth_data.inband, U32_MAX);
 
 	priv->ds->dev = &mdiodev->dev;
 	priv->ds->num_ports = QCA8K_NUM_PORTS;
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 685628716ed2..a5abc340471c 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -349,7 +349,6 @@ struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
 	bool ack;
-	u32 seq;
 	u32 data[4];
 };
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 50c319832939..2d6b7c7f158b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1282,13 +1282,17 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
 */
 struct dsa_inband {
 	struct completion completion;
+	u32 seqno;
+	u32 seqno_mask;
 };
 
-void dsa_inband_init(struct dsa_inband *inband);
+void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask);
 void dsa_inband_complete(struct dsa_inband *inband);
 int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
+		       void (*insert_seqno)(struct sk_buff *skb, u32 seqno),
 		       int timeout_ms);
 int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
+u32 dsa_inband_seqno(struct dsa_inband *inband);
 
 /* Keep inline for faster access in hot path */
 static inline bool netdev_uses_dsa(const struct net_device *dev)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 68576f1c5b02..5a8d95f8acec 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -518,9 +518,11 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
 
-void dsa_inband_init(struct dsa_inband *inband)
+void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 {
 	init_completion(&inband->completion);
+	inband->seqno_mask = seqno_mask;
+	inband->seqno = 0;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_init);
 
@@ -544,6 +546,7 @@ EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
  * reinitialized before the skb is queue to avoid races.
  */
 int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
+		       void (*insert_seqno)(struct sk_buff *skb, u32 seqno),
 		       int timeout_ms)
 {
 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
@@ -551,6 +554,11 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 
 	reinit_completion(&inband->completion);
 
+	if (insert_seqno) {
+		inband->seqno++;
+		insert_seqno(skb, inband->seqno & inband->seqno_mask);
+	}
+
 	dev_queue_xmit(skb);
 
 	ret = wait_for_completion_timeout(&inband->completion, jiffies);
@@ -562,6 +570,12 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 }
 EXPORT_SYMBOL_GPL(dsa_inband_request);
 
+u32 dsa_inband_seqno(struct dsa_inband *inband)
+{
+	return inband->seqno & inband->seqno_mask;
+}
+EXPORT_SYMBOL_GPL(dsa_inband_seqno);
+
 static int __init dsa_init_module(void)
 {
 	int rc;
-- 
2.37.2

[PATCH rfc v0 6/9] net: dsa: qca8k: Refactor sequence number mismatch to use error code

[Andrew Lunn]

Replace the boolean that the sequence numbers matches with an error
code. Set the error code to -EINVAL if the sequence numbers are wrong,
otherwise 0.

The value is only safe to us if the completion happens. Ensure the
return from the completion is always considered, and if it fails, a
timeout error is returned.

This is a preparation step to moving the error tracking into the DSA
core.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 50 ++++++++++++++------------------
 drivers/net/dsa/qca/qca8k.h      |  2 +-
 2 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index a354ba070d33..69b807d87367 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -147,7 +147,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 
 	/* Make sure the seq match the requested packet */
 	if (mgmt_ethhdr->seq == dsa_inband_seqno(&mgmt_eth_data->inband))
-		mgmt_eth_data->ack = true;
+		mgmt_eth_data->err = 0;
+	else
+		mgmt_eth_data->err = -EINVAL;
 
 	if (cmd == MDIO_READ) {
 		mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
@@ -229,7 +231,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
-	bool ack;
+	int err;
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
@@ -247,7 +249,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	}
 
 	skb->dev = priv->mgmt_master;
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 			      qca8k_mdio_header_fill_seq_num,
@@ -257,15 +258,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
 		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
 	if (ret)
 		return ret;
 
-	if (!ack)
-		return -EINVAL;
+	if (err)
+		return -ret;
 
 	return 0;
 }
@@ -274,7 +275,7 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
-	bool ack;
+	int err;
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val,
@@ -292,21 +293,20 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	}
 
 	skb->dev = priv->mgmt_master;
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
 	if (ret)
 		return ret;
 
-	if (!ack)
-		return -EINVAL;
+	if (err)
+		return err;
 
 	return 0;
 }
@@ -431,22 +431,20 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 			struct sk_buff *read_skb, u32 *val)
 {
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
-	bool ack;
+	int err;
 	int ret;
 
-	mgmt_eth_data->ack = false;
-
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	if (ret)
 		return ret;
 
-	if (!ack)
-		return -EINVAL;
+	if (err)
+		return err;
 
 	*val = mgmt_eth_data->data[0];
 
@@ -462,7 +460,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	u32 write_val, clear_val = 0, val;
 	struct net_device *mgmt_master;
 	int ret, ret1;
-	bool ack;
+	int err;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -519,21 +517,20 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	read_skb->dev = mgmt_master;
 	clear_skb->dev = mgmt_master;
 	write_skb->dev = mgmt_master;
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	ack = mgmt_eth_data->ack;
+	err = mgmt_eth_data->err;
 
 	if (ret) {
 		kfree_skb(read_skb);
 		goto exit;
 	}
 
-	if (!ack) {
-		ret = -EINVAL;
+	if (err) {
+		ret = err;
 		kfree_skb(read_skb);
 		goto exit;
 	}
@@ -549,19 +546,17 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		mgmt_eth_data->ack = false;
-
 		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
 					 qca8k_mdio_header_fill_seq_num,
 					 QCA8K_ETHERNET_TIMEOUT);
 
-		ack = mgmt_eth_data->ack;
+		err = mgmt_eth_data->err;
 
 		if (ret)
 			goto exit;
 
-		if (!ack) {
-			ret = -EINVAL;
+		if (err) {
+			ret = err;
 			goto exit;
 		}
 
@@ -570,7 +565,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	mgmt_eth_data->ack = false;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
 				 qca8k_mdio_header_fill_seq_num,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index a5abc340471c..79f7197a1790 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -348,7 +348,7 @@ enum {
 struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
-	bool ack;
+	int err;
 	u32 data[4];
 };
 
-- 
2.37.2

Re: [PATCH rfc v0 6/9] net: dsa: qca8k: Refactor sequence number mismatch to use error code

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 12:18:50AM +0200, Andrew Lunn wrote:
> @@ -229,7 +231,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
>  	struct sk_buff *skb;
> -	bool ack;
> +	int err;
>  	int ret;
>  
>  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> @@ -247,7 +249,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	}
>  
>  	skb->dev = priv->mgmt_master;
> -	mgmt_eth_data->ack = false;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
>  			      qca8k_mdio_header_fill_seq_num,
> @@ -257,15 +258,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)
>  		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
>  
> -	ack = mgmt_eth_data->ack;
> +	err = mgmt_eth_data->err;
>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  
>  	if (ret)
>  		return ret;
>  
> -	if (!ack)
> -		return -EINVAL;
> +	if (err)
> +		return -ret;

Probably "if (err) return -ret" is not what you intend. We know ret is 0,
we just checked for it earlier.

Also, maybe a variable named "match" would be more expressive? This
shows how easy it is to make mistakes, mixing "err" with "ret" in the
same function.

>  
>  	return 0;

Can it actually be expressed as "return err", here and everywhere else?

>  }

Re: [PATCH rfc v0 6/9] net: dsa: qca8k: Refactor sequence number mismatch to use error code

[Andrew Lunn]

> > -	if (!ack)
> > -		return -EINVAL;
> > +	if (err)
> > +		return -ret;
> 
> Probably "if (err) return -ret" is not what you intend. We know ret is 0,
> we just checked for it earlier.

Good catch. Thanks.

> 
> Also, maybe a variable named "match" would be more expressive? This
> shows how easy it is to make mistakes, mixing "err" with "ret" in the
> same function.

A lot of this code gets removed in the next patch. I'm just trying to
keep to lots of small, easy to review patches, which in this case
results in some not so nice intermediary state, but the next patch
cleans it up.

       Andrew

[PATCH rfc v0 7/9] net: dsa: qca8k: Pass error code from reply decoder to requester

[Andrew Lunn]

The code which decodes the frame and signals the complete can
experience error, such as wrong sequence number. Pass an error code
between the completer and the function waiting on the complete. This
simplifies the error handling, since all errors are combined into one
place.

At the same time, return -EPROTO if the sequence numbers don't match.
This is more appropriate than EINVAL.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 60 ++++++--------------------------
 drivers/net/dsa/qca/qca8k.h      |  1 -
 include/net/dsa.h                |  3 +-
 net/dsa/dsa.c                    |  7 ++--
 4 files changed, 18 insertions(+), 53 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 69b807d87367..55a781851e28 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -138,6 +138,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	struct qca8k_priv *priv = ds->priv;
 	struct qca_mgmt_ethhdr *mgmt_ethhdr;
 	u8 len, cmd;
+	int err = 0;
 
 	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
 	mgmt_eth_data = &priv->mgmt_eth_data;
@@ -146,10 +147,8 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	len = FIELD_GET(QCA_HDR_MGMT_LENGTH, mgmt_ethhdr->command);
 
 	/* Make sure the seq match the requested packet */
-	if (mgmt_ethhdr->seq == dsa_inband_seqno(&mgmt_eth_data->inband))
-		mgmt_eth_data->err = 0;
-	else
-		mgmt_eth_data->err = -EINVAL;
+	if (mgmt_ethhdr->seq != dsa_inband_seqno(&mgmt_eth_data->inband))
+		err = -EPROTO;
 
 	if (cmd == MDIO_READ) {
 		mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
@@ -162,7 +161,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 			       QCA_HDR_MGMT_DATA2_LEN);
 	}
 
-	dsa_inband_complete(&mgmt_eth_data->inband);
+	dsa_inband_complete(&mgmt_eth_data->inband, err);
 }
 
 static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
@@ -231,7 +230,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
-	int err;
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
@@ -258,24 +256,15 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
 		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
 
-	err = mgmt_eth_data->err;
-
 	mutex_unlock(&mgmt_eth_data->mutex);
 
-	if (ret)
-		return ret;
-
-	if (err)
-		return -ret;
-
-	return 0;
+	return ret;
 }
 
 static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
-	int err;
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val,
@@ -298,17 +287,9 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	err = mgmt_eth_data->err;
-
 	mutex_unlock(&mgmt_eth_data->mutex);
 
-	if (ret)
-		return ret;
-
-	if (err)
-		return err;
-
-	return 0;
+	return ret;
 }
 
 static int
@@ -431,21 +412,15 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 			struct sk_buff *read_skb, u32 *val)
 {
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
-	int err;
 	int ret;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	err = mgmt_eth_data->err;
-
 	if (ret)
 		return ret;
 
-	if (err)
-		return err;
-
 	*val = mgmt_eth_data->data[0];
 
 	return 0;
@@ -460,7 +435,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	u32 write_val, clear_val = 0, val;
 	struct net_device *mgmt_master;
 	int ret, ret1;
-	int err;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -522,19 +496,11 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	err = mgmt_eth_data->err;
-
 	if (ret) {
 		kfree_skb(read_skb);
 		goto exit;
 	}
 
-	if (err) {
-		ret = err;
-		kfree_skb(read_skb);
-		goto exit;
-	}
-
 	ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
 				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
 				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
@@ -550,16 +516,9 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 					 qca8k_mdio_header_fill_seq_num,
 					 QCA8K_ETHERNET_TIMEOUT);
 
-		err = mgmt_eth_data->err;
-
 		if (ret)
 			goto exit;
 
-		if (err) {
-			ret = err;
-			goto exit;
-		}
-
 		ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
 	} else {
 		kfree_skb(read_skb);
@@ -1440,6 +1399,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	const struct qca8k_mib_desc *mib;
 	struct mib_ethhdr *mib_ethhdr;
 	int i, mib_len, offset = 0;
+	int err = 0;
 	u64 *data;
 	u8 port;
 
@@ -1450,8 +1410,10 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 	 * parse only the requested one.
 	 */
 	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
-	if (port != mib_eth_data->req_port)
+	if (port != mib_eth_data->req_port) {
+		err = -EPROTO;
 		goto exit;
+	}
 
 	data = mib_eth_data->data;
 
@@ -1480,7 +1442,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 exit:
 	/* Complete on receiving all the mib packet */
 	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
-		dsa_inband_complete(&mib_eth_data->inband);
+		dsa_inband_complete(&mib_eth_data->inband, err);
 }
 
 static int
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 79f7197a1790..682106206282 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -348,7 +348,6 @@ enum {
 struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
-	int err;
 	u32 data[4];
 };
 
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 2d6b7c7f158b..1a920f89b667 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1284,10 +1284,11 @@ struct dsa_inband {
 	struct completion completion;
 	u32 seqno;
 	u32 seqno_mask;
+	int err;
 };
 
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask);
-void dsa_inband_complete(struct dsa_inband *inband);
+void dsa_inband_complete(struct dsa_inband *inband, int err);
 int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 		       void (* insert_seqno)(struct sk_buff *skb, u32 seqno),
 		       int timeout_ms);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5a8d95f8acec..0de283ac0bfc 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -526,8 +526,9 @@ void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 }
 EXPORT_SYMBOL_GPL(dsa_inband_init);
 
-void dsa_inband_complete(struct dsa_inband *inband)
+void dsa_inband_complete(struct dsa_inband *inband, int err)
 {
+	inband->err = err;
 	complete(&inband->completion);
 }
 EXPORT_SYMBOL_GPL(dsa_inband_complete);
@@ -553,6 +554,7 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	int ret;
 
 	reinit_completion(&inband->completion);
+	inband->err = 0;
 
 	if (insert_seqno) {
 		inband->seqno++;
@@ -566,7 +568,8 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 		return ret;
 	if (ret == 0)
 		return -ETIMEDOUT;
-	return 0;
+
+	return inband->err;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_request);
 
-- 
2.37.2

[PATCH rfc v0 8/9] net: dsa: qca8k: Pass response buffer via dsa_rmu_request

[Andrew Lunn]

Make the calling of operations on the switch more like a request
response API by passing the address of the response buffer, rather
than making use of global state.

To avoid race conditions with the completion timeout, and late
arriving responses, protect the resp members via a mutex.

The qca8k response frame has an odd layout, the reply is not
contiguous. Use a small intermediary buffer to convert the reply into
something which can be memcpy'ed.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 31 ++++++++++++++++++++-----------
 drivers/net/dsa/qca/qca8k.h      |  1 -
 include/net/dsa.h                |  7 ++++++-
 net/dsa/dsa.c                    | 24 +++++++++++++++++++++++-
 4 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 55a781851e28..234d79a09e78 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -138,6 +138,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	struct qca8k_priv *priv = ds->priv;
 	struct qca_mgmt_ethhdr *mgmt_ethhdr;
 	u8 len, cmd;
+	u32 data[4];
 	int err = 0;
 
 	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
@@ -151,17 +152,16 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 		err = -EPROTO;
 
 	if (cmd == MDIO_READ) {
-		mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
+		data[0] = mgmt_ethhdr->mdio_data;
 
 		/* Get the rest of the 12 byte of data.
 		 * The read/write function will extract the requested data.
 		 */
 		if (len > QCA_HDR_MGMT_DATA1_LEN)
-			memcpy(mgmt_eth_data->data + 1, skb->data,
-			       QCA_HDR_MGMT_DATA2_LEN);
+			memcpy(&data[1], skb->data, QCA_HDR_MGMT_DATA2_LEN);
 	}
 
-	dsa_inband_complete(&mgmt_eth_data->inband, err);
+	dsa_inband_complete(&mgmt_eth_data->inband, &data, sizeof(data), err);
 }
 
 static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
@@ -230,6 +230,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
+	u32 data[4];
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
@@ -249,12 +250,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	skb->dev = priv->mgmt_master;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
-			      qca8k_mdio_header_fill_seq_num,
-			      QCA8K_ETHERNET_TIMEOUT);
+				 qca8k_mdio_header_fill_seq_num,
+				 &data, sizeof(data),
+				 QCA8K_ETHERNET_TIMEOUT);
 
-	*val = mgmt_eth_data->data[0];
+	*val = data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
-		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
+		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
@@ -285,6 +287,7 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
+				 NULL, 0,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
@@ -412,16 +415,18 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 			struct sk_buff *read_skb, u32 *val)
 {
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
+	u32 data[4];
 	int ret;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
+				 &data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	if (ret)
 		return ret;
 
-	*val = mgmt_eth_data->data[0];
+	*val = data[0];
 
 	return 0;
 }
@@ -434,6 +439,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	struct qca8k_mgmt_eth_data *mgmt_eth_data;
 	u32 write_val, clear_val = 0, val;
 	struct net_device *mgmt_master;
+	u32 resp_data[4];
 	int ret, ret1;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
@@ -494,6 +500,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
 				 qca8k_mdio_header_fill_seq_num,
+				 NULL, 0,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	if (ret) {
@@ -514,12 +521,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	if (read) {
 		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
 					 qca8k_mdio_header_fill_seq_num,
+					 &resp_data, sizeof(resp_data),
 					 QCA8K_ETHERNET_TIMEOUT);
 
 		if (ret)
 			goto exit;
 
-		ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
+		ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
 	} else {
 		kfree_skb(read_skb);
 	}
@@ -527,6 +535,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
 				 qca8k_mdio_header_fill_seq_num,
+				 NULL, 0,
 				 QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
@@ -1442,7 +1451,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 exit:
 	/* Complete on receiving all the mib packet */
 	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
-		dsa_inband_complete(&mib_eth_data->inband, err);
+		dsa_inband_complete(&mib_eth_data->inband, NULL, 0, err);
 }
 
 static int
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 682106206282..70494096e251 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -348,7 +348,6 @@ enum {
 struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
 	struct mutex mutex; /* Enforce one mdio read/write at time */
-	u32 data[4];
 };
 
 struct qca8k_mib_eth_data {
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 1a920f89b667..dad9e31d36ce 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1285,12 +1285,17 @@ struct dsa_inband {
 	u32 seqno;
 	u32 seqno_mask;
 	int err;
+	struct mutex resp_lock; /* Protects resp* members */
+	void *resp;
+	unsigned int resp_len;
 };
 
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask);
-void dsa_inband_complete(struct dsa_inband *inband, int err);
+void dsa_inband_complete(struct dsa_inband *inband,
+		      void *resp, unsigned int resp_len, int err);
 int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 		       void (* insert_seqno)(struct sk_buff *skb, u32 seqno),
+		       void *resp, unsigned int resp_len,
 		       int timeout_ms);
 int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
 u32 dsa_inband_seqno(struct dsa_inband *inband);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 0de283ac0bfc..4fa0ab4ae58e 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -521,14 +521,24 @@ EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 {
 	init_completion(&inband->completion);
+	mutex_init(&inband->resp_lock);
 	inband->seqno_mask = seqno_mask;
 	inband->seqno = 0;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_init);
 
-void dsa_inband_complete(struct dsa_inband *inband, int err)
+void dsa_inband_complete(struct dsa_inband *inband,
+			 void *resp, unsigned int resp_len,
+			 int err)
 {
 	inband->err = err;
+
+	mutex_lock(&inband->resp_lock);
+	resp_len = min(inband->resp_len, resp_len);
+	if (inband->resp && resp)
+		memcpy(inband->resp, resp, resp_len);
+	mutex_unlock(&inband->resp_lock);
+
 	complete(&inband->completion);
 }
 EXPORT_SYMBOL_GPL(dsa_inband_complete);
@@ -548,6 +558,7 @@ EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
  */
 int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 		       void (* insert_seqno)(struct sk_buff *skb, u32 seqno),
+		       void *resp, unsigned int resp_len,
 		       int timeout_ms)
 {
 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
@@ -556,6 +567,11 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	reinit_completion(&inband->completion);
 	inband->err = 0;
 
+	mutex_lock(&inband->resp_lock);
+	inband->resp = resp;
+	inband->resp_len = resp_len;
+	mutex_unlock(&inband->resp_lock);
+
 	if (insert_seqno) {
 		inband->seqno++;
 		insert_seqno(skb, inband->seqno & inband->seqno_mask);
@@ -564,6 +580,12 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	dev_queue_xmit(skb);
 
 	ret = wait_for_completion_timeout(&inband->completion, jiffies);
+
+	mutex_lock(&inband->resp_lock);
+	inband->resp = NULL;
+	inband->resp_len = 0;
+	mutex_unlock(&inband->resp_lock);
+
 	if (ret < 0)
 		return ret;
 	if (ret == 0)
-- 
2.37.2

Re: [PATCH rfc v0 8/9] net: dsa: qca8k: Pass response buffer via dsa_rmu_request

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 12:18:52AM +0200, Andrew Lunn wrote:
> Make the calling of operations on the switch more like a request
> response API by passing the address of the response buffer, rather
> than making use of global state.
> 
> To avoid race conditions with the completion timeout, and late
> arriving responses, protect the resp members via a mutex.

Cannot be a mutex; the context of qca8k_rw_reg_ack_handler(), caller of
dsa_inband_complete(), is NET_RX softirq and that is not sleepable.

> 
> The qca8k response frame has an odd layout, the reply is not
> contiguous. Use a small intermediary buffer to convert the reply into
> something which can be memcpy'ed.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 31 ++++++++++++++++++++-----------
>  drivers/net/dsa/qca/qca8k.h      |  1 -
>  include/net/dsa.h                |  7 ++++++-
>  net/dsa/dsa.c                    | 24 +++++++++++++++++++++++-
>  4 files changed, 49 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 55a781851e28..234d79a09e78 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -138,6 +138,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  	struct qca8k_priv *priv = ds->priv;
>  	struct qca_mgmt_ethhdr *mgmt_ethhdr;
>  	u8 len, cmd;
> +	u32 data[4];
>  	int err = 0;
>  
>  	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> @@ -151,17 +152,16 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  		err = -EPROTO;
>  
>  	if (cmd == MDIO_READ) {
> -		mgmt_eth_data->data[0] = mgmt_ethhdr->mdio_data;
> +		data[0] = mgmt_ethhdr->mdio_data;
>  
>  		/* Get the rest of the 12 byte of data.
>  		 * The read/write function will extract the requested data.
>  		 */
>  		if (len > QCA_HDR_MGMT_DATA1_LEN)
> -			memcpy(mgmt_eth_data->data + 1, skb->data,
> -			       QCA_HDR_MGMT_DATA2_LEN);
> +			memcpy(&data[1], skb->data, QCA_HDR_MGMT_DATA2_LEN);
>  	}
>  
> -	dsa_inband_complete(&mgmt_eth_data->inband, err);
> +	dsa_inband_complete(&mgmt_eth_data->inband, &data, sizeof(data), err);
>  }
>  
>  static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> @@ -230,6 +230,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
>  	struct sk_buff *skb;
> +	u32 data[4];
>  	int ret;
>  
>  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> @@ -249,12 +250,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	skb->dev = priv->mgmt_master;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> -			      qca8k_mdio_header_fill_seq_num,
> -			      QCA8K_ETHERNET_TIMEOUT);

Argument list should have been properly aligned when this patch set introduced it.

> +				 qca8k_mdio_header_fill_seq_num,
> +				 &data, sizeof(data),
> +				 QCA8K_ETHERNET_TIMEOUT);

Kind of feeling the need for an error check right here, instead of
proceeding to look at the buffer.

>  
> -	*val = mgmt_eth_data->data[0];
> +	*val = data[0];
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> -		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> +		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);

This is pretty hard to digest, but it looks like it could work.
So this can run concurrently with qca8k_rw_reg_ack_handler(), but since
the end of dsa_inband_request() sets inband->resp to NULL, then even if
the response will come later, it won't touch the driver-provided on-stack
buffer, since the DSA completion structure lost the reference to it.

How do we deal with the response being processed so late by the handler
that it overlaps with the dsa_inband_request() call of the next seqid?
We open up to another window of opportunity for the handler to have a
valid buffer and length to which it can copy stuff. Does it matter,
since the seqid of the response will be smaller than that of the request?
Is reordering on multi-CPU, multi-queue masters handled in any way? This
will be a problem regardless of QoS - currently we assume that all
management frames are treated the same by the DSA master. But it has no
insight into the DSA header format, so why would it? It could be doing
RSS and even find some entropy in our seqid junk data. It's a bit late
to think through right now.

>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  
> @@ -285,6 +287,7 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
>  				 qca8k_mdio_header_fill_seq_num,
> +				 NULL, 0,
>  				 QCA8K_ETHERNET_TIMEOUT);
>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
> @@ -412,16 +415,18 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
>  			struct sk_buff *read_skb, u32 *val)
>  {
>  	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> +	u32 data[4];
>  	int ret;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
>  				 qca8k_mdio_header_fill_seq_num,
> +				 &data, sizeof(data),
>  				 QCA8K_ETHERNET_TIMEOUT);
>  
>  	if (ret)
>  		return ret;
>  
> -	*val = mgmt_eth_data->data[0];
> +	*val = data[0];
>  
>  	return 0;
>  }
> @@ -434,6 +439,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data;
>  	u32 write_val, clear_val = 0, val;
>  	struct net_device *mgmt_master;
> +	u32 resp_data[4];
>  	int ret, ret1;
>  
>  	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
> @@ -494,6 +500,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
>  				 qca8k_mdio_header_fill_seq_num,
> +				 NULL, 0,
>  				 QCA8K_ETHERNET_TIMEOUT);
>  
>  	if (ret) {
> @@ -514,12 +521,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	if (read) {
>  		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
>  					 qca8k_mdio_header_fill_seq_num,
> +					 &resp_data, sizeof(resp_data),
>  					 QCA8K_ETHERNET_TIMEOUT);
>  
>  		if (ret)
>  			goto exit;
>  
> -		ret = mgmt_eth_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
> +		ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
>  	} else {
>  		kfree_skb(read_skb);
>  	}
> @@ -527,6 +535,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
>  				 qca8k_mdio_header_fill_seq_num,
> +				 NULL, 0,
>  				 QCA8K_ETHERNET_TIMEOUT);
>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
> @@ -1442,7 +1451,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  exit:
>  	/* Complete on receiving all the mib packet */
>  	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
> -		dsa_inband_complete(&mib_eth_data->inband, err);
> +		dsa_inband_complete(&mib_eth_data->inband, NULL, 0, err);
>  }
>  
>  static int
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 682106206282..70494096e251 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -348,7 +348,6 @@ enum {
>  struct qca8k_mgmt_eth_data {
>  	struct dsa_inband inband;
>  	struct mutex mutex; /* Enforce one mdio read/write at time */
> -	u32 data[4];
>  };
>  
>  struct qca8k_mib_eth_data {
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 1a920f89b667..dad9e31d36ce 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1285,12 +1285,17 @@ struct dsa_inband {
>  	u32 seqno;
>  	u32 seqno_mask;
>  	int err;
> +	struct mutex resp_lock; /* Protects resp* members */
> +	void *resp;
> +	unsigned int resp_len;

Would be good to be a bit more verbose about what "protecting" means
here (just offering a consistent view of the buffer pointer and of its
length from DSA's perspective).

>  };
>  
>  void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask);
> -void dsa_inband_complete(struct dsa_inband *inband, int err);
> +void dsa_inband_complete(struct dsa_inband *inband,
> +		      void *resp, unsigned int resp_len, int err);
>  int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  		       void (* insert_seqno)(struct sk_buff *skb, u32 seqno),
> +		       void *resp, unsigned int resp_len,
>  		       int timeout_ms);
>  int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms);
>  u32 dsa_inband_seqno(struct dsa_inband *inband);
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 0de283ac0bfc..4fa0ab4ae58e 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -521,14 +521,24 @@ EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
>  void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
>  {
>  	init_completion(&inband->completion);
> +	mutex_init(&inband->resp_lock);
>  	inband->seqno_mask = seqno_mask;
>  	inband->seqno = 0;
>  }
>  EXPORT_SYMBOL_GPL(dsa_inband_init);
>  
> -void dsa_inband_complete(struct dsa_inband *inband, int err)
> +void dsa_inband_complete(struct dsa_inband *inband,
> +			 void *resp, unsigned int resp_len,
> +			 int err)
>  {
>  	inband->err = err;
> +
> +	mutex_lock(&inband->resp_lock);
> +	resp_len = min(inband->resp_len, resp_len);

No warning for truncation caused by resp_len > inband->resp_len?
It seems like a valid error. At least I tried to test Mattias' patch
set, and this is one of the problems that really happened.

> +	if (inband->resp && resp)
> +		memcpy(inband->resp, resp, resp_len);
> +	mutex_unlock(&inband->resp_lock);
> +
>  	complete(&inband->completion);
>  }
>  EXPORT_SYMBOL_GPL(dsa_inband_complete);
> @@ -548,6 +558,7 @@ EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
>   */
>  int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  		       void (* insert_seqno)(struct sk_buff *skb, u32 seqno),
> +		       void *resp, unsigned int resp_len,
>  		       int timeout_ms)
>  {
>  	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
> @@ -556,6 +567,11 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  	reinit_completion(&inband->completion);
>  	inband->err = 0;
>  
> +	mutex_lock(&inband->resp_lock);
> +	inband->resp = resp;
> +	inband->resp_len = resp_len;
> +	mutex_unlock(&inband->resp_lock);
> +
>  	if (insert_seqno) {
>  		inband->seqno++;
>  		insert_seqno(skb, inband->seqno & inband->seqno_mask);
> @@ -564,6 +580,12 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  	dev_queue_xmit(skb);
>  
>  	ret = wait_for_completion_timeout(&inband->completion, jiffies);
> +
> +	mutex_lock(&inband->resp_lock);
> +	inband->resp = NULL;
> +	inband->resp_len = 0;
> +	mutex_unlock(&inband->resp_lock);
> +
>  	if (ret < 0)
>  		return ret;
>  	if (ret == 0)
> -- 
> 2.37.2
>

Re: [PATCH rfc v0 8/9] net: dsa: qca8k: Pass response buffer via dsa_rmu_request

[Andrew Lunn]

On Tue, Sep 20, 2022 at 12:27:56AM +0000, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 12:18:52AM +0200, Andrew Lunn wrote:
> > Make the calling of operations on the switch more like a request
> > response API by passing the address of the response buffer, rather
> > than making use of global state.
> > 
> > To avoid race conditions with the completion timeout, and late
> > arriving responses, protect the resp members via a mutex.
> 
> Cannot be a mutex; the context of qca8k_rw_reg_ack_handler(), caller of
> dsa_inband_complete(), is NET_RX softirq and that is not sleepable.

Thanks. I will make it a spinlock.

> >  static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > @@ -230,6 +230,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  {
> >  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
> >  	struct sk_buff *skb;
> > +	u32 data[4];
> >  	int ret;
> >  
> >  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> > @@ -249,12 +250,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
> >  	skb->dev = priv->mgmt_master;
> >  
> >  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> > -			      qca8k_mdio_header_fill_seq_num,
> > -			      QCA8K_ETHERNET_TIMEOUT);
> 
> Argument list should have been properly aligned when this patch set introduced it.
> 
> > +				 qca8k_mdio_header_fill_seq_num,
> > +				 &data, sizeof(data),
> > +				 QCA8K_ETHERNET_TIMEOUT);
> 
> Kind of feeling the need for an error check right here, instead of
> proceeding to look at the buffer.

Yes, i can add an error check. data is however safe to access, even if
it is uninitilized. It is on this functions stack and known to be big
enough.

> 
> >  
> > -	*val = mgmt_eth_data->data[0];
> > +	*val = data[0];
> >  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> > -		memcpy(val + 1, mgmt_eth_data->data + 1, len - QCA_HDR_MGMT_DATA1_LEN);
> > +		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
> 
> This is pretty hard to digest, but it looks like it could work.
> So this can run concurrently with qca8k_rw_reg_ack_handler(), but since
> the end of dsa_inband_request() sets inband->resp to NULL, then even if
> the response will come later, it won't touch the driver-provided on-stack
> buffer, since the DSA completion structure lost the reference to it.

Yes, that is what i want the mutex for, soon to become a spinlock.

> 
> How do we deal with the response being processed so late by the handler
> that it overlaps with the dsa_inband_request() call of the next seqid?
> We open up to another window of opportunity for the handler to have a
> valid buffer and length to which it can copy stuff. Does it matter,
> since the seqid of the response will be smaller than that of the request?

That is what i need to look at. So long as the sequence number is
incremented first, then the completion reinitialized, i think we are
safe.

> Is reordering on multi-CPU, multi-queue masters handled in any way? This
> will be a problem regardless of QoS - currently we assume that all
> management frames are treated the same by the DSA master. But it has no
> insight into the DSA header format, so why would it? It could be doing
> RSS and even find some entropy in our seqid junk data. It's a bit late
> to think through right now.

There is a big mutex serializing all inband operations. There should
never be two or more valid operations in flight.

The way the sequence numbers work, i think in theory you could have
multiple operations in flight and the hardware would do the right
thing, and you could get a little bit more performance. But you really
need to worry about packets getting reordered. I've no numbers yet,
but i think the performance gains from MDIO to single in flight inband
is sufficient we don't need to worry about squeezing the last bit of
performance out.

> >  struct qca8k_mib_eth_data {
> > diff --git a/include/net/dsa.h b/include/net/dsa.h
> > index 1a920f89b667..dad9e31d36ce 100644
> > --- a/include/net/dsa.h
> > +++ b/include/net/dsa.h
> > @@ -1285,12 +1285,17 @@ struct dsa_inband {
> >  	u32 seqno;
> >  	u32 seqno_mask;
> >  	int err;
> > +	struct mutex resp_lock; /* Protects resp* members */
> > +	void *resp;
> > +	unsigned int resp_len;
> 
> Would be good to be a bit more verbose about what "protecting" means
> here (just offering a consistent view of the buffer pointer and of its
> length from DSA's perspective).

Yes, i can expand the comment.

> > -void dsa_inband_complete(struct dsa_inband *inband, int err)
> > +void dsa_inband_complete(struct dsa_inband *inband,
> > +			 void *resp, unsigned int resp_len,
> > +			 int err)
> >  {
> >  	inband->err = err;
> > +
> > +	mutex_lock(&inband->resp_lock);
> > +	resp_len = min(inband->resp_len, resp_len);
> 
> No warning for truncation caused by resp_len > inband->resp_len?
> It seems like a valid error. At least I tried to test Mattias' patch
> set, and this is one of the problems that really happened.

I need to go back and look at Mattias patches, but he made the comment
that you don't always know the size of the response. It can be family
dependent. I think the 6390 adds additional statistics, so its
response it going to be bigger than other devices. We don't currently
handle those additional statistics. I made the same comment as you
did, maybe return -ENOBUF or something if it truncates. QCA8k is
simpler in this respect, it can have a maximum of 12 bytes of optional
data, but given that the frame needs to be padded to 64 bytes, you
know you will always have those bytes, even if they are full of junk.

This is one of those areas where i'm not sure there is a correct
answer. Do more checking here, and force some complexity into the
caller? Truncate, but the caller has no idea a truncate has happened?

	Andrew

[PATCH rfc v0 9/9] net: dsa: qca8k: Move inband mutex into DSA core

[Andrew Lunn]

The mutex serves two purposes:

It serialises operations on the switch, so that only one
request/response can be happening at once.

It protects priv->mgmt_master, which itself has two purposes.  If the
hardware is wrongly wires, the wrong switch port is connected to the
cpu, inband cannot be used. In this case it has the value
NULL. Additionally, if the master is down, it is set to
NULL. Otherwise it points to the netdev used to send frames to the
switch.

The protection of priv->mgmt_master is not required. It is a single
pointer, which will be updated atomically. It is not expected that the
interface disappears, it only goes down. Hence mgmt_master will always
be valid, or NULL.

Move the check for the master device being NULL into the core.  Also,
move the mutex for serialisation into the core.

The MIB operations don't follow request/response semantics, so its
mutex is left untouched.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 68 ++++++--------------------------
 drivers/net/dsa/qca/qca8k.h      |  1 -
 include/net/dsa.h                |  1 +
 net/dsa/dsa.c                    |  7 ++++
 4 files changed, 19 insertions(+), 58 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 234d79a09e78..3e60bbe2570d 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -238,15 +238,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	if (!skb)
 		return -ENOMEM;
 
-	mutex_lock(&mgmt_eth_data->mutex);
-
-	/* Check mgmt_master if is operational */
-	if (!priv->mgmt_master) {
-		kfree_skb(skb);
-		mutex_unlock(&mgmt_eth_data->mutex);
-		return -EINVAL;
-	}
-
 	skb->dev = priv->mgmt_master;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
@@ -258,8 +249,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
 		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
 
-	mutex_unlock(&mgmt_eth_data->mutex);
-
 	return ret;
 }
 
@@ -267,32 +256,18 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb;
-	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val,
 				      QCA8K_ETHERNET_MDIO_PRIORITY, len);
 	if (!skb)
 		return -ENOMEM;
 
-	mutex_lock(&mgmt_eth_data->mutex);
-
-	/* Check mgmt_master if is operational */
-	if (!priv->mgmt_master) {
-		kfree_skb(skb);
-		mutex_unlock(&mgmt_eth_data->mutex);
-		return -EINVAL;
-	}
-
 	skb->dev = priv->mgmt_master;
 
-	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
-				 qca8k_mdio_header_fill_seq_num,
-				 NULL, 0,
-				 QCA8K_ETHERNET_TIMEOUT);
-
-	mutex_unlock(&mgmt_eth_data->mutex);
-
-	return ret;
+	return dsa_inband_request(&mgmt_eth_data->inband, skb,
+				  qca8k_mdio_header_fill_seq_num,
+				  NULL, 0,
+				  QCA8K_ETHERNET_TIMEOUT);
 }
 
 static int
@@ -438,7 +413,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	struct sk_buff *write_skb, *clear_skb, *read_skb;
 	struct qca8k_mgmt_eth_data *mgmt_eth_data;
 	u32 write_val, clear_val = 0, val;
-	struct net_device *mgmt_master;
 	u32 resp_data[4];
 	int ret, ret1;
 
@@ -484,19 +458,9 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	 * 3. Get the data if we are reading
 	 * 4. Reset the mdio master (even with error)
 	 */
-	mutex_lock(&mgmt_eth_data->mutex);
-
-	/* Check if mgmt_master is operational */
-	mgmt_master = priv->mgmt_master;
-	if (!mgmt_master) {
-		mutex_unlock(&mgmt_eth_data->mutex);
-		ret = -EINVAL;
-		goto err_mgmt_master;
-	}
-
-	read_skb->dev = mgmt_master;
-	clear_skb->dev = mgmt_master;
-	write_skb->dev = mgmt_master;
+	read_skb->dev = priv->mgmt_master;
+	clear_skb->dev = priv->mgmt_master;
+	write_skb->dev = priv->mgmt_master;
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
 				 qca8k_mdio_header_fill_seq_num,
@@ -533,18 +497,11 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 exit:
 
-	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
-				 qca8k_mdio_header_fill_seq_num,
-				 NULL, 0,
-				 QCA8K_ETHERNET_TIMEOUT);
-
-	mutex_unlock(&mgmt_eth_data->mutex);
-
-	return ret;
+	return dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+				  qca8k_mdio_header_fill_seq_num,
+				  NULL, 0,
+				  QCA8K_ETHERNET_TIMEOUT);
 
-	/* Error handling before lock */
-err_mgmt_master:
-	kfree_skb(read_skb);
 err_read_skb:
 	kfree_skb(clear_skb);
 err_clear_skb:
@@ -1526,13 +1483,11 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 	if (dp->index != 0)
 		return;
 
-	mutex_lock(&priv->mgmt_eth_data.mutex);
 	mutex_lock(&priv->mib_eth_data.mutex);
 
 	priv->mgmt_master = operational ? (struct net_device *)master : NULL;
 
 	mutex_unlock(&priv->mib_eth_data.mutex);
-	mutex_unlock(&priv->mgmt_eth_data.mutex);
 }
 
 static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
@@ -1850,7 +1805,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	if (!priv->ds)
 		return -ENOMEM;
 
-	mutex_init(&priv->mgmt_eth_data.mutex);
 	dsa_inband_init(&priv->mgmt_eth_data.inband, U32_MAX);
 
 	mutex_init(&priv->mib_eth_data.mutex);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 70494096e251..6da36ed6486b 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -347,7 +347,6 @@ enum {
 
 struct qca8k_mgmt_eth_data {
 	struct dsa_inband inband;
-	struct mutex mutex; /* Enforce one mdio read/write at time */
 };
 
 struct qca8k_mib_eth_data {
diff --git a/include/net/dsa.h b/include/net/dsa.h
index dad9e31d36ce..7a545b781e7d 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1281,6 +1281,7 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
  * frames and expecting a response in a frame.
 */
 struct dsa_inband {
+	struct mutex lock; /* Serialise operations */
 	struct completion completion;
 	u32 seqno;
 	u32 seqno_mask;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 4fa0ab4ae58e..82c729d631eb 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -521,6 +521,7 @@ EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 {
 	init_completion(&inband->completion);
+	mutex_init(&inband->lock);
 	mutex_init(&inband->resp_lock);
 	inband->seqno_mask = seqno_mask;
 	inband->seqno = 0;
@@ -567,6 +568,11 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	reinit_completion(&inband->completion);
 	inband->err = 0;
 
+	if (!skb->dev)
+		return -EOPNOTSUPP;
+
+	mutex_lock(&inband->lock);
+
 	mutex_lock(&inband->resp_lock);
 	inband->resp = resp;
 	inband->resp_len = resp_len;
@@ -585,6 +591,7 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	inband->resp = NULL;
 	inband->resp_len = 0;
 	mutex_unlock(&inband->resp_lock);
+	mutex_unlock(&inband->lock);
 
 	if (ret < 0)
 		return ret;
-- 
2.37.2

Re: [PATCH rfc v0 9/9] net: dsa: qca8k: Move inband mutex into DSA core

[Christian Marangi]

On Tue, Sep 20, 2022 at 12:18:53AM +0200, Andrew Lunn wrote:
> The mutex serves two purposes:
> 
> It serialises operations on the switch, so that only one
> request/response can be happening at once.
> 
> It protects priv->mgmt_master, which itself has two purposes.  If the
> hardware is wrongly wires, the wrong switch port is connected to the
> cpu, inband cannot be used. In this case it has the value
> NULL. Additionally, if the master is down, it is set to
> NULL. Otherwise it points to the netdev used to send frames to the
> switch.
> 
> The protection of priv->mgmt_master is not required. It is a single
> pointer, which will be updated atomically. It is not expected that the
> interface disappears, it only goes down. Hence mgmt_master will always
> be valid, or NULL.
> 
> Move the check for the master device being NULL into the core.  Also,
> move the mutex for serialisation into the core.
> 
> The MIB operations don't follow request/response semantics, so its
> mutex is left untouched.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

BTW this patch makes the router crash with a kernel panic. Rest of the
patchset works correctly and seems to be no regression. (I had to fix
the clear_skb return value)

> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 68 ++++++--------------------------
>  drivers/net/dsa/qca/qca8k.h      |  1 -
>  include/net/dsa.h                |  1 +
>  net/dsa/dsa.c                    |  7 ++++
>  4 files changed, 19 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 234d79a09e78..3e60bbe2570d 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -238,15 +238,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	if (!skb)
>  		return -ENOMEM;
>  
> -	mutex_lock(&mgmt_eth_data->mutex);
> -
> -	/* Check mgmt_master if is operational */
> -	if (!priv->mgmt_master) {
> -		kfree_skb(skb);
> -		mutex_unlock(&mgmt_eth_data->mutex);
> -		return -EINVAL;
> -	}
> -
>  	skb->dev = priv->mgmt_master;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> @@ -258,8 +249,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)
>  		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
>  
> -	mutex_unlock(&mgmt_eth_data->mutex);
> -
>  	return ret;
>  }
>  
> @@ -267,32 +256,18 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
>  	struct sk_buff *skb;
> -	int ret;
>  
>  	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val,
>  				      QCA8K_ETHERNET_MDIO_PRIORITY, len);
>  	if (!skb)
>  		return -ENOMEM;
>  
> -	mutex_lock(&mgmt_eth_data->mutex);
> -
> -	/* Check mgmt_master if is operational */
> -	if (!priv->mgmt_master) {
> -		kfree_skb(skb);
> -		mutex_unlock(&mgmt_eth_data->mutex);
> -		return -EINVAL;
> -	}
> -
>  	skb->dev = priv->mgmt_master;
>  
> -	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
> -				 qca8k_mdio_header_fill_seq_num,
> -				 NULL, 0,
> -				 QCA8K_ETHERNET_TIMEOUT);
> -
> -	mutex_unlock(&mgmt_eth_data->mutex);
> -
> -	return ret;
> +	return dsa_inband_request(&mgmt_eth_data->inband, skb,
> +				  qca8k_mdio_header_fill_seq_num,
> +				  NULL, 0,
> +				  QCA8K_ETHERNET_TIMEOUT);
>  }
>  
>  static int
> @@ -438,7 +413,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	struct sk_buff *write_skb, *clear_skb, *read_skb;
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data;
>  	u32 write_val, clear_val = 0, val;
> -	struct net_device *mgmt_master;
>  	u32 resp_data[4];
>  	int ret, ret1;
>  
> @@ -484,19 +458,9 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	 * 3. Get the data if we are reading
>  	 * 4. Reset the mdio master (even with error)
>  	 */
> -	mutex_lock(&mgmt_eth_data->mutex);
> -
> -	/* Check if mgmt_master is operational */
> -	mgmt_master = priv->mgmt_master;
> -	if (!mgmt_master) {
> -		mutex_unlock(&mgmt_eth_data->mutex);
> -		ret = -EINVAL;
> -		goto err_mgmt_master;
> -	}
> -
> -	read_skb->dev = mgmt_master;
> -	clear_skb->dev = mgmt_master;
> -	write_skb->dev = mgmt_master;
> +	read_skb->dev = priv->mgmt_master;
> +	clear_skb->dev = priv->mgmt_master;
> +	write_skb->dev = priv->mgmt_master;
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
>  				 qca8k_mdio_header_fill_seq_num,
> @@ -533,18 +497,11 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	}
>  exit:
>  
> -	ret = dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
> -				 qca8k_mdio_header_fill_seq_num,
> -				 NULL, 0,
> -				 QCA8K_ETHERNET_TIMEOUT);
> -
> -	mutex_unlock(&mgmt_eth_data->mutex);
> -
> -	return ret;
> +	return dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
> +				  qca8k_mdio_header_fill_seq_num,
> +				  NULL, 0,
> +				  QCA8K_ETHERNET_TIMEOUT);
>  
> -	/* Error handling before lock */
> -err_mgmt_master:
> -	kfree_skb(read_skb);
>  err_read_skb:
>  	kfree_skb(clear_skb);
>  err_clear_skb:
> @@ -1526,13 +1483,11 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
>  	if (dp->index != 0)
>  		return;
>  
> -	mutex_lock(&priv->mgmt_eth_data.mutex);
>  	mutex_lock(&priv->mib_eth_data.mutex);
>  
>  	priv->mgmt_master = operational ? (struct net_device *)master : NULL;
>  
>  	mutex_unlock(&priv->mib_eth_data.mutex);
> -	mutex_unlock(&priv->mgmt_eth_data.mutex);
>  }
>  
>  static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> @@ -1850,7 +1805,6 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  	if (!priv->ds)
>  		return -ENOMEM;
>  
> -	mutex_init(&priv->mgmt_eth_data.mutex);
>  	dsa_inband_init(&priv->mgmt_eth_data.inband, U32_MAX);
>  
>  	mutex_init(&priv->mib_eth_data.mutex);
> diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
> index 70494096e251..6da36ed6486b 100644
> --- a/drivers/net/dsa/qca/qca8k.h
> +++ b/drivers/net/dsa/qca/qca8k.h
> @@ -347,7 +347,6 @@ enum {
>  
>  struct qca8k_mgmt_eth_data {
>  	struct dsa_inband inband;
> -	struct mutex mutex; /* Enforce one mdio read/write at time */
>  };
>  
>  struct qca8k_mib_eth_data {
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index dad9e31d36ce..7a545b781e7d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1281,6 +1281,7 @@ bool dsa_mdb_present_in_other_db(struct dsa_switch *ds, int port,
>   * frames and expecting a response in a frame.
>  */
>  struct dsa_inband {
> +	struct mutex lock; /* Serialise operations */
>  	struct completion completion;
>  	u32 seqno;
>  	u32 seqno_mask;
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 4fa0ab4ae58e..82c729d631eb 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -521,6 +521,7 @@ EXPORT_SYMBOL_GPL(dsa_mdb_present_in_other_db);
>  void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
>  {
>  	init_completion(&inband->completion);
> +	mutex_init(&inband->lock);
>  	mutex_init(&inband->resp_lock);
>  	inband->seqno_mask = seqno_mask;
>  	inband->seqno = 0;
> @@ -567,6 +568,11 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  	reinit_completion(&inband->completion);
>  	inband->err = 0;
>  
> +	if (!skb->dev)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&inband->lock);
> +
>  	mutex_lock(&inband->resp_lock);
>  	inband->resp = resp;
>  	inband->resp_len = resp_len;
> @@ -585,6 +591,7 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
>  	inband->resp = NULL;
>  	inband->resp_len = 0;
>  	mutex_unlock(&inband->resp_lock);
> +	mutex_unlock(&inband->lock);
>  
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.37.2
> 

-- 
	Ansuel

Re: [PATCH rfc v0 9/9] net: dsa: qca8k: Move inband mutex into DSA core

[Andrew Lunn]

On Tue, Sep 20, 2022 at 05:19:07AM +0200, Christian Marangi wrote:
> On Tue, Sep 20, 2022 at 12:18:53AM +0200, Andrew Lunn wrote:
> > The mutex serves two purposes:
> > 
> > It serialises operations on the switch, so that only one
> > request/response can be happening at once.
> > 
> > It protects priv->mgmt_master, which itself has two purposes.  If the
> > hardware is wrongly wires, the wrong switch port is connected to the
> > cpu, inband cannot be used. In this case it has the value
> > NULL. Additionally, if the master is down, it is set to
> > NULL. Otherwise it points to the netdev used to send frames to the
> > switch.
> > 
> > The protection of priv->mgmt_master is not required. It is a single
> > pointer, which will be updated atomically. It is not expected that the
> > interface disappears, it only goes down. Hence mgmt_master will always
> > be valid, or NULL.
> > 
> > Move the check for the master device being NULL into the core.  Also,
> > move the mutex for serialisation into the core.
> > 
> > The MIB operations don't follow request/response semantics, so its
> > mutex is left untouched.
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> BTW this patch makes the router crash with a kernel panic. Rest of the
> patchset works correctly and seems to be no regression. (I had to fix
> the clear_skb return value)

Thanks for testing.

As Vladimir pointed out, there is a mutex used in the wrong context. I
will fix that and the other issues pointed out, and see if i can spot
what i did wrong here. If not, we will have to decode the opps.

     Andrew

[PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Mattias Forsblad]

Support connecting dsa tagger for frame2reg decoding
with its associated hookup functions.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 include/linux/dsa/mv88e6xxx.h |  6 ++++++
 net/dsa/dsa_priv.h            |  2 ++
 net/dsa/tag_dsa.c             | 40 +++++++++++++++++++++++++++++++----
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h
index 8c3d45eca46b..a8b6f3c110e5 100644
--- a/include/linux/dsa/mv88e6xxx.h
+++ b/include/linux/dsa/mv88e6xxx.h
@@ -5,9 +5,15 @@
 #ifndef _NET_DSA_TAG_MV88E6XXX_H
 #define _NET_DSA_TAG_MV88E6XXX_H
 
+#include <net/dsa.h>
 #include <linux/if_vlan.h>
 
 #define MV88E6XXX_VID_STANDALONE	0
 #define MV88E6XXX_VID_BRIDGED		(VLAN_N_VID - 1)
 
+struct dsa_tagger_data {
+	void (*decode_frame2reg)(struct dsa_switch *ds,
+				 struct sk_buff *skb);
+};
+
 #endif
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 614fbba8fe39..3b23b37eb0f4 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -17,6 +17,8 @@
 
 #define DSA_MAX_NUM_OFFLOADING_BRIDGES		BITS_PER_LONG
 
+#define DSA_FRAME2REG_SOURCE_DEV		GENMASK(5, 0)
+
 enum {
 	DSA_NOTIFIER_AGEING_TIME,
 	DSA_NOTIFIER_BRIDGE_JOIN,
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index e4b6e3f2a3db..e7fdf3b5cb4a 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -198,8 +198,11 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
 static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 				  u8 extra)
 {
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_tagger_data *tagger_data;
 	bool trap = false, trunk = false;
 	int source_device, source_port;
+	struct dsa_switch *ds;
 	enum dsa_code code;
 	enum dsa_cmd cmd;
 	u8 *dsa_header;
@@ -218,9 +221,16 @@ 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.
-			 */
+			source_device = FIELD_GET(DSA_FRAME2REG_SOURCE_DEV, dsa_header[0]);
+			ds = dsa_switch_find(cpu_dp->dst->index, source_device);
+			if (ds) {
+				tagger_data = ds->tagger_data;
+				if (likely(tagger_data->decode_frame2reg))
+					tagger_data->decode_frame2reg(ds, skb);
+			} else {
+				net_err_ratelimited("RMU: Didn't find switch with index %d",
+						    source_device);
+			}
 			return NULL;
 		case DSA_CODE_ARP_MIRROR:
 		case DSA_CODE_POLICY_MIRROR:
@@ -254,7 +264,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	source_port = (dsa_header[1] >> 3) & 0x1f;
 
 	if (trunk) {
-		struct dsa_port *cpu_dp = dev->dsa_ptr;
 		struct dsa_lag *lag;
 
 		/* The exact source port is not available in the tag,
@@ -323,6 +332,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
 	return skb;
 }
 
+static int dsa_tag_connect(struct dsa_switch *ds)
+{
+	struct dsa_tagger_data *tagger_data;
+
+	tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
+	if (!tagger_data)
+		return -ENOMEM;
+
+	ds->tagger_data = tagger_data;
+
+	return 0;
+}
+
+static void dsa_tag_disconnect(struct dsa_switch *ds)
+{
+	kfree(ds->tagger_data);
+	ds->tagger_data = NULL;
+}
+
 #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
 
 static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -343,6 +371,8 @@ static const struct dsa_device_ops dsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_DSA,
 	.xmit	  = dsa_xmit,
 	.rcv	  = dsa_rcv,
+	.connect  = dsa_tag_connect,
+	.disconnect = dsa_tag_disconnect,
 	.needed_headroom = DSA_HLEN,
 };
 
@@ -385,6 +415,8 @@ static const struct dsa_device_ops edsa_netdev_ops = {
 	.proto	  = DSA_TAG_PROTO_EDSA,
 	.xmit	  = edsa_xmit,
 	.rcv	  = edsa_rcv,
+	.connect  = dsa_tag_connect,
+	.disconnect = dsa_tag_disconnect,
 	.needed_headroom = EDSA_HLEN,
 };
 
-- 
2.25.1

Re: [PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Vladimir Oltean]

Regarding the commit title. There is a difference between DSA the
framework and DSA the Marvell implementation of a tagging protocol.
This patch implements something pertaining to the latter, hence the
prefix should be "net: dsa: tag_dsa: ".

Why do I have a feeling I've said this before?
https://patchwork.kernel.org/project/netdevbpf/patch/20220909085138.3539952-4-mattias.forsblad@gmail.com/#25006260

Also, the body of the commit title, and the commit message, are
meaningless. What dsa tagger data operations? Connect what to what and why?
What does this patch really achieve? What I'd like to see as a reviewer
is a summary of the change, plus a justification for how the solution
came to be what it is.

On Mon, Sep 19, 2022 at 01:08:43PM +0200, Mattias Forsblad wrote:
> Support connecting dsa tagger for frame2reg decoding
> with its associated hookup functions.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/linux/dsa/mv88e6xxx.h |  6 ++++++
>  net/dsa/dsa_priv.h            |  2 ++
>  net/dsa/tag_dsa.c             | 40 +++++++++++++++++++++++++++++++----
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h
> index 8c3d45eca46b..a8b6f3c110e5 100644
> --- a/include/linux/dsa/mv88e6xxx.h
> +++ b/include/linux/dsa/mv88e6xxx.h
> @@ -5,9 +5,15 @@
>  #ifndef _NET_DSA_TAG_MV88E6XXX_H
>  #define _NET_DSA_TAG_MV88E6XXX_H
>  
> +#include <net/dsa.h>
>  #include <linux/if_vlan.h>
>  
>  #define MV88E6XXX_VID_STANDALONE	0
>  #define MV88E6XXX_VID_BRIDGED		(VLAN_N_VID - 1)
>  
> +struct dsa_tagger_data {
> +	void (*decode_frame2reg)(struct dsa_switch *ds,
> +				 struct sk_buff *skb);
> +};
> +
>  #endif
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 614fbba8fe39..3b23b37eb0f4 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -17,6 +17,8 @@
>  
>  #define DSA_MAX_NUM_OFFLOADING_BRIDGES		BITS_PER_LONG
>  
> +#define DSA_FRAME2REG_SOURCE_DEV		GENMASK(5, 0)
> +

So in v8 I said that struct dsa_tagger_data is a hardware-specific data
structure, and it has no place in the common net/dsa/dsa_priv.h header
for the framework, and that you should move it to include/linux/dsa/mv88e6xxx.h.
https://patchwork.kernel.org/project/netdevbpf/patch/20220909085138.3539952-4-mattias.forsblad@gmail.com/#25006260
Which you did in v9. But whoop, one more hardware-specific definition
appears in v9, this DSA_FRAME2REG_SOURCE_DEV, again in net/dsa/dsa_priv.h.
https://patchwork.kernel.org/project/netdevbpf/patch/20220912112855.339804-4-mattias.forsblad@gmail.com/
Please use your own judgement to infer the fact that multiple attempts
to code up things in the same way will just lead to more of the same
observations during review.

>  enum {
>  	DSA_NOTIFIER_AGEING_TIME,
>  	DSA_NOTIFIER_BRIDGE_JOIN,
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e4b6e3f2a3db..e7fdf3b5cb4a 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -198,8 +198,11 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  				  u8 extra)
>  {
> +	struct dsa_port *cpu_dp = dev->dsa_ptr;
> +	struct dsa_tagger_data *tagger_data;
>  	bool trap = false, trunk = false;
>  	int source_device, source_port;
> +	struct dsa_switch *ds;
>  	enum dsa_code code;
>  	enum dsa_cmd cmd;
>  	u8 *dsa_header;
> @@ -218,9 +221,16 @@ 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.
> -			 */
> +			source_device = FIELD_GET(DSA_FRAME2REG_SOURCE_DEV, dsa_header[0]);
> +			ds = dsa_switch_find(cpu_dp->dst->index, source_device);
> +			if (ds) {
> +				tagger_data = ds->tagger_data;

Can you please also parse the sequence number here, so the
decode_frame2reg() data consumer doesn't have to concern itself with the
dsa_header at all?

> +				if (likely(tagger_data->decode_frame2reg))
> +					tagger_data->decode_frame2reg(ds, skb);
> +			} else {
> +				net_err_ratelimited("RMU: Didn't find switch with index %d",
> +						    source_device);
> +			}
>  			return NULL;
>  		case DSA_CODE_ARP_MIRROR:
>  		case DSA_CODE_POLICY_MIRROR:
> @@ -254,7 +264,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  	source_port = (dsa_header[1] >> 3) & 0x1f;
>  
>  	if (trunk) {
> -		struct dsa_port *cpu_dp = dev->dsa_ptr;
>  		struct dsa_lag *lag;
>  
>  		/* The exact source port is not available in the tag,
> @@ -323,6 +332,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  	return skb;
>  }
>  
> +static int dsa_tag_connect(struct dsa_switch *ds)
> +{
> +	struct dsa_tagger_data *tagger_data;
> +
> +	tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
> +	if (!tagger_data)
> +		return -ENOMEM;
> +
> +	ds->tagger_data = tagger_data;
> +
> +	return 0;
> +}
> +
> +static void dsa_tag_disconnect(struct dsa_switch *ds)
> +{
> +	kfree(ds->tagger_data);
> +	ds->tagger_data = NULL;
> +}
> +
>  #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
>  
>  static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -343,6 +371,8 @@ static const struct dsa_device_ops dsa_netdev_ops = {
>  	.proto	  = DSA_TAG_PROTO_DSA,
>  	.xmit	  = dsa_xmit,
>  	.rcv	  = dsa_rcv,
> +	.connect  = dsa_tag_connect,
> +	.disconnect = dsa_tag_disconnect,
>  	.needed_headroom = DSA_HLEN,
>  };
>  
> @@ -385,6 +415,8 @@ static const struct dsa_device_ops edsa_netdev_ops = {
>  	.proto	  = DSA_TAG_PROTO_EDSA,
>  	.xmit	  = edsa_xmit,
>  	.rcv	  = edsa_rcv,
> +	.connect  = dsa_tag_connect,
> +	.disconnect = dsa_tag_disconnect,
>  	.needed_headroom = EDSA_HLEN,
>  };
>  
> -- 
> 2.25.1
> 

Re: [PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Mattias Forsblad]

On 2022-09-20 00:00, Vladimir Oltean wrote:
>> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
>> index e4b6e3f2a3db..e7fdf3b5cb4a 100644
>> --- a/net/dsa/tag_dsa.c
>> +++ b/net/dsa/tag_dsa.c
>> @@ -198,8 +198,11 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>>  static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>>  				  u8 extra)
>>  {
>> +	struct dsa_port *cpu_dp = dev->dsa_ptr;
>> +	struct dsa_tagger_data *tagger_data;
>>  	bool trap = false, trunk = false;
>>  	int source_device, source_port;
>> +	struct dsa_switch *ds;
>>  	enum dsa_code code;
>>  	enum dsa_cmd cmd;
>>  	u8 *dsa_header;
>> @@ -218,9 +221,16 @@ 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.
>> -			 */
>> +			source_device = FIELD_GET(DSA_FRAME2REG_SOURCE_DEV, dsa_header[0]);
>> +			ds = dsa_switch_find(cpu_dp->dst->index, source_device);
>> +			if (ds) {
>> +				tagger_data = ds->tagger_data;
> 
> Can you please also parse the sequence number here, so the
> decode_frame2reg() data consumer doesn't have to concern itself with the
> dsa_header at all?
> 

The sequence number is in the chip structure which isn't available here.
Should we really access that here in the dsa layer?

/Mattias

>> +				if (likely(tagger_data->decode_frame2reg))
>> +					tagger_data->decode_frame2reg(ds, skb);
>> +			} else {
>> +				net_err_ratelimited("RMU: Didn't find switch with index %d",
>> +						    source_device);
>> +			}
>>  			return NULL;
>>  		case DSA_CODE_ARP_MIRROR:
>>  		case DSA_CODE_POLICY_MIRROR:
>> @@ -254,7 +264,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>>  	source_port = (dsa_header[1] >> 3) & 0x1f;
>>  
>>  	if (trunk) {
>> -		struct dsa_port *cpu_dp = dev->dsa_ptr;
>>  		struct dsa_lag *lag;
>>  
>>  		/* The exact source port is not available in the tag,
>> @@ -323,6 +332,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>>  	return skb;
>>  }
>>  
>> +static int dsa_tag_connect(struct dsa_switch *ds)
>> +{
>> +	struct dsa_tagger_data *tagger_data;
>> +
>> +	tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
>> +	if (!tagger_data)
>> +		return -ENOMEM;
>> +
>> +	ds->tagger_data = tagger_data;
>> +
>> +	return 0;
>> +}
>> +
>> +static void dsa_tag_disconnect(struct dsa_switch *ds)
>> +{
>> +	kfree(ds->tagger_data);
>> +	ds->tagger_data = NULL;
>> +}
>> +
>>  #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
>>  
>>  static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
>> @@ -343,6 +371,8 @@ static const struct dsa_device_ops dsa_netdev_ops = {
>>  	.proto	  = DSA_TAG_PROTO_DSA,
>>  	.xmit	  = dsa_xmit,
>>  	.rcv	  = dsa_rcv,
>> +	.connect  = dsa_tag_connect,
>> +	.disconnect = dsa_tag_disconnect,
>>  	.needed_headroom = DSA_HLEN,
>>  };
>>  
>> @@ -385,6 +415,8 @@ static const struct dsa_device_ops edsa_netdev_ops = {
>>  	.proto	  = DSA_TAG_PROTO_EDSA,
>>  	.xmit	  = edsa_xmit,
>>  	.rcv	  = edsa_rcv,
>> +	.connect  = dsa_tag_connect,
>> +	.disconnect = dsa_tag_disconnect,
>>  	.needed_headroom = EDSA_HLEN,
>>  };
>>  
>> -- 
>> 2.25.1
>>
> 

Re: [PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 08:41:30AM +0200, Mattias Forsblad wrote:
> > Can you please also parse the sequence number here, so the
> > decode_frame2reg() data consumer doesn't have to concern itself with the
> > dsa_header at all?
> 
> The sequence number is in the chip structure which isn't available here.
> Should we really access that here in the dsa layer?

I'm talking about this sequence number:

mv88e6xxx_decode_frame2reg_handler:

	/* Decode Frame2Reg DSA portion */
	dsa_header = skb->data - 2;

	seqno = dsa_header[3];

I'm saying, if you get the seqno in net/dsa/tag_dsa.c and pass it as
argument to mv88e6xxx_decode_frame2reg_handler(), then you should no
longer have a reason to look at the dsa_header from drivers/net/dsa/mv88e6xxx/.

Re: [PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Mattias Forsblad]

On 2022-09-20 12:31, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 08:41:30AM +0200, Mattias Forsblad wrote:
>>> Can you please also parse the sequence number here, so the
>>> decode_frame2reg() data consumer doesn't have to concern itself with the
>>> dsa_header at all?
>>
>> The sequence number is in the chip structure which isn't available here.
>> Should we really access that here in the dsa layer?
> 
> I'm talking about this sequence number:
> 
> mv88e6xxx_decode_frame2reg_handler:
> 
> 	/* Decode Frame2Reg DSA portion */
> 	dsa_header = skb->data - 2;
> 
> 	seqno = dsa_header[3];
> 
> I'm saying, if you get the seqno in net/dsa/tag_dsa.c and pass it as
> argument to mv88e6xxx_decode_frame2reg_handler(), then you should no
> longer have a reason to look at the dsa_header from drivers/net/dsa/mv88e6xxx/.

Ok, I misunderstood your intent. I'll change. Thanks.

/Mattias

[PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU functionality.

[Mattias Forsblad]

The Marvell SOHO switches supports a secondary control
channel for accessing data in the switch. Special crafted
ethernet frames can access functions in the switch.
These frames is handled by the Remote Management Unit (RMU)
in the switch. Accessing data structures is specially
efficient and lessens the access contention on the MDIO
bus.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/Makefile |   1 +
 drivers/net/dsa/mv88e6xxx/chip.c   |  28 ++-
 drivers/net/dsa/mv88e6xxx/chip.h   |  17 ++
 drivers/net/dsa/mv88e6xxx/rmu.c    | 263 +++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/rmu.h    |  73 ++++++++
 5 files changed, 374 insertions(+), 8 deletions(-)
 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 46e12b53a9e4..294bf9bbaf3f 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)
 {
@@ -1535,14 +1536,6 @@ static int mv88e6xxx_trunk_setup(struct mv88e6xxx_chip *chip)
 	return 0;
 }
 
-static int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
-{
-	if (chip->info->ops->rmu_disable)
-		return chip->info->ops->rmu_disable(chip);
-
-	return 0;
-}
-
 static int mv88e6xxx_pot_setup(struct mv88e6xxx_chip *chip)
 {
 	if (chip->info->ops->pot_clear)
@@ -6867,6 +6860,23 @@ static int mv88e6xxx_crosschip_lag_leave(struct dsa_switch *ds, int sw_index,
 	return err_sync ? : err_pvt;
 }
 
+static int mv88e6xxx_connect_tag_protocol(struct dsa_switch *ds,
+					  enum dsa_tag_protocol proto)
+{
+	struct dsa_tagger_data *tagger_data = ds->tagger_data;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_DSA:
+	case DSA_TAG_PROTO_EDSA:
+		tagger_data->decode_frame2reg = mv88e6xxx_decode_frame2reg_handler;
+		break;
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	return 0;
+}
+
 static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.get_tag_protocol	= mv88e6xxx_get_tag_protocol,
 	.change_tag_protocol	= mv88e6xxx_change_tag_protocol,
@@ -6932,6 +6942,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,
+	.master_state_change	= mv88e6xxx_master_state_change,
+	.connect_tag_protocol	= mv88e6xxx_connect_tag_protocol,
 };
 
 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 7ce3c41f6caf..440e9b274df4 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -282,6 +282,20 @@ struct mv88e6xxx_port {
 	struct devlink_region *region;
 };
 
+struct mv88e6xxx_rmu {
+	/* RMU resources */
+	struct net_device *master_netdev;
+	const struct mv88e6xxx_bus_ops *smi_ops;
+	const struct dsa_switch_ops *ds_ops;
+	struct dsa_switch_ops *ds_rmu_ops;
+	struct mv88e6xxx_bus_ops *smi_rmu_ops;
+	/* Mutex for RMU operations */
+	struct mutex mutex;
+	u16 prodnr;
+	struct sk_buff *resp;
+	int seqno;
+};
+
 enum mv88e6xxx_region_id {
 	MV88E6XXX_REGION_GLOBAL1 = 0,
 	MV88E6XXX_REGION_GLOBAL2,
@@ -410,6 +424,9 @@ struct mv88e6xxx_chip {
 
 	/* Bridge MST to SID mappings */
 	struct list_head msts;
+
+	/* RMU resources */
+	struct mv88e6xxx_rmu rmu;
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
new file mode 100644
index 000000000000..c5b3c156de40
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.c
@@ -0,0 +1,263 @@
+// 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 const u8 mv88e6xxx_rmu_dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
+
+static void mv88e6xxx_rmu_create_l2(struct dsa_switch *ds, struct sk_buff *skb)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	struct ethhdr *eth;
+	u8 *edsa_header;
+	u8 *dsa_header;
+	u8 extra = 0;
+
+	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
+		extra = 4;
+
+	/* Create RMU L2 header */
+	dsa_header = skb_push(skb, 6);
+	dsa_header[0] = FIELD_PREP(MV88E6XXX_CPU_CODE_MASK, MV88E6XXX_RMU);
+	dsa_header[0] |= FIELD_PREP(MV88E6XXX_TRG_DEV_MASK, ds->index);
+	dsa_header[1] = FIELD_PREP(MV88E6XXX_RMU_CODE_MASK, 1);
+	dsa_header[1] |= FIELD_PREP(MV88E6XXX_RMU_L2_BYTE1_RESV, MV88E6XXX_RMU_L2_BYTE1_RESV_VAL);
+	dsa_header[2] = FIELD_PREP(MV88E6XXX_RMU_PRIO_MASK, MV88E6XXX_RMU_PRIO);
+	dsa_header[2] |= MV88E6XXX_RMU_L2_BYTE2_RESV;
+	dsa_header[3] = ++chip->rmu.seqno;
+	dsa_header[4] = 0;
+	dsa_header[5] = 0;
+
+	/* Insert RMU MAC destination address /w extra if needed */
+	eth = skb_push(skb, ETH_ALEN * 2 + extra);
+	memcpy(eth->h_dest, mv88e6xxx_rmu_dest_addr, ETH_ALEN);
+	ether_addr_copy(eth->h_source, chip->rmu.master_netdev->dev_addr);
+
+	if (extra) {
+		edsa_header = (u8 *)&eth->h_proto;
+		edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
+		edsa_header[1] = ETH_P_EDSA & 0xff;
+		edsa_header[2] = 0x00;
+		edsa_header[3] = 0x00;
+	}
+}
+
+static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip,
+				   const void *req, int req_len,
+				   void *resp, unsigned int *resp_len)
+{
+	struct sk_buff *skb;
+	unsigned char *data;
+	int ret = 0;
+
+	skb = netdev_alloc_skb(chip->rmu.master_netdev, 64);
+	if (!skb)
+		return -ENOMEM;
+
+	/* Take height for an eventual EDSA header */
+	skb_reserve(skb, 2 * ETH_HLEN + 4);
+	skb_reset_network_header(skb);
+
+	/* Insert RMU request message */
+	data = skb_put(skb, req_len);
+	memcpy(data, req, req_len);
+
+	mv88e6xxx_rmu_create_l2(chip->ds, skb);
+
+	mutex_lock(&chip->rmu.mutex);
+
+	ret = dsa_switch_inband_tx(chip->ds, skb, NULL, MV88E6XXX_RMU_WAIT_TIME_MS);
+	if (!ret) {
+		dev_err(chip->dev, "RMU: error waiting for request (%pe)\n",
+			ERR_PTR(ret));
+		goto out;
+	}
+
+	if (chip->rmu.resp->len > *resp_len) {
+		ret = -EMSGSIZE;
+	} else {
+		*resp_len = chip->rmu.resp->len;
+		memcpy(resp, chip->rmu.resp->data, chip->rmu.resp->len);
+	}
+
+	kfree_skb(chip->rmu.resp);
+	chip->rmu.resp = NULL;
+
+out:
+	mutex_unlock(&chip->rmu.mutex);
+
+	return ret > 0 ? 0 : ret;
+}
+
+static int mv88e6xxx_rmu_validate_response(struct mv88e6xxx_rmu_header *resp, int code)
+{
+	if (be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_1 &&
+	    be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_2 &&
+	    be16_to_cpu(resp->code) != code) {
+		net_dbg_ratelimited("RMU: received unknown format 0x%04x code 0x%04x",
+				    resp->format, resp->code);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
+{
+	const u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_GET_ID,
+			     MV88E6XXX_RMU_REQ_PAD,
+			     MV88E6XXX_RMU_REQ_CODE_GET_ID,
+			     MV88E6XXX_RMU_REQ_DATA};
+	struct mv88e6xxx_rmu_header resp;
+	int resp_len;
+	int ret = -1;
+
+	resp_len = sizeof(resp);
+	ret = mv88e6xxx_rmu_send_wait(chip, req, sizeof(req),
+				      &resp, &resp_len);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
+		return ret;
+	}
+
+	/* Got response */
+	ret = mv88e6xxx_rmu_validate_response(&resp, MV88E6XXX_RMU_RESP_CODE_GOT_ID);
+	if (ret)
+		return ret;
+
+	chip->rmu.prodnr = be16_to_cpu(resp.prodnr);
+
+	return ret;
+}
+
+static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
+{
+	chip->smi_ops = chip->rmu.smi_ops;
+	chip->ds->ops = chip->rmu.ds_rmu_ops;
+	chip->rmu.master_netdev = NULL;
+
+	if (chip->info->ops->rmu_disable)
+		chip->info->ops->rmu_disable(chip);
+}
+
+static int mv88e6xxx_enable_check_rmu(const struct net_device *master,
+				      struct mv88e6xxx_chip *chip, int port)
+{
+	int ret;
+
+	chip->rmu.master_netdev = (struct net_device *)master;
+
+	/* Check if chip is alive */
+	ret = mv88e6xxx_rmu_get_id(chip, port);
+	if (!ret)
+		return ret;
+
+	chip->ds->ops = chip->rmu.ds_rmu_ops;
+	chip->smi_ops = chip->rmu.smi_rmu_ops;
+
+	return 0;
+}
+
+void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
+				   bool operational)
+{
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int port;
+	int ret;
+
+	port = dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);
+
+	mv88e6xxx_reg_lock(chip);
+
+	if (operational && chip->info->ops->rmu_enable) {
+		ret = chip->info->ops->rmu_enable(chip, port);
+
+		if (ret == -EOPNOTSUPP)
+			goto out;
+
+		if (!ret) {
+			dev_dbg(chip->dev, "RMU: Enabled on port %d", port);
+
+			ret = mv88e6xxx_enable_check_rmu(master, chip, port);
+			if (!ret)
+				goto out;
+
+		} else {
+			dev_err(chip->dev, "RMU: Unable to enable on port %d %pe",
+				port, ERR_PTR(ret));
+			goto out;
+		}
+	} else {
+		mv88e6xxx_disable_rmu(chip);
+	}
+
+out:
+	mv88e6xxx_reg_unlock(chip);
+}
+
+static int mv88e6xxx_validate_mac(struct dsa_switch *ds, struct sk_buff *skb)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	unsigned char *ethhdr;
+
+	/* Check matching MAC */
+	ethhdr = skb_mac_header(skb);
+	if (!ether_addr_equal(chip->rmu.master_netdev->dev_addr, ethhdr)) {
+		dev_dbg_ratelimited(ds->dev, "RMU: mismatching MAC address for request. Rx %pM expecting %pM\n",
+				    ethhdr, chip->rmu.master_netdev->dev_addr);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+void mv88e6xxx_decode_frame2reg_handler(struct dsa_switch *ds, struct sk_buff *skb)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	u8 *dsa_header;
+	u8 seqno;
+
+	/* Decode Frame2Reg DSA portion */
+	dsa_header = skb->data - 2;
+
+	if (mv88e6xxx_validate_mac(ds, skb))
+		return;
+
+	seqno = dsa_header[3];
+	if (seqno != chip->rmu.seqno) {
+		net_err_ratelimited("RMU: wrong seqno received. Was %d, expected %d",
+				    seqno, chip->rmu.seqno);
+		return;
+	}
+
+	/* Pull DSA L2 data */
+	skb_pull(skb, MV88E6XXX_DSA_HLEN);
+
+	/* Get an reference for further processing in initiator */
+	chip->rmu.resp = skb_get(skb);
+
+	dsa_switch_inband_complete(ds, NULL);
+}
+
+int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
+{
+	mutex_init(&chip->rmu.mutex);
+
+	/* Remember original ops for restore */
+	chip->rmu.smi_ops = chip->smi_ops;
+	chip->rmu.ds_ops = chip->ds->ops;
+
+	/* Start disabled, we'll enable RMU in master_state_change */
+	if (chip->info->ops->rmu_disable)
+		return chip->info->ops->rmu_disable(chip);
+
+	return 0;
+}
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.h b/drivers/net/dsa/mv88e6xxx/rmu.h
new file mode 100644
index 000000000000..67757a3c2f29
--- /dev/null
+++ b/drivers/net/dsa/mv88e6xxx/rmu.h
@@ -0,0 +1,73 @@
+/* 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_DSA_HLEN	4
+
+#define MV88E6XXX_RMU_MAX_RMON			64
+
+#define MV88E6XXX_RMU_WAIT_TIME_MS		20
+
+#define MV88E6XXX_RMU_L2_BYTE1_RESV_VAL		0x3e
+#define MV88E6XXX_RMU				1
+#define MV88E6XXX_RMU_PRIO			6
+#define MV88E6XXX_RMU_RESV2			0xf
+
+#define MV88E6XXX_SOURCE_PORT			GENMASK(6, 3)
+#define MV88E6XXX_CPU_CODE_MASK			GENMASK(7, 6)
+#define MV88E6XXX_TRG_DEV_MASK			GENMASK(4, 0)
+#define MV88E6XXX_RMU_CODE_MASK			GENMASK(1, 1)
+#define MV88E6XXX_RMU_PRIO_MASK			GENMASK(7, 5)
+#define MV88E6XXX_RMU_L2_BYTE1_RESV		GENMASK(7, 2)
+#define MV88E6XXX_RMU_L2_BYTE2_RESV		GENMASK(3, 0)
+
+#define MV88E6XXX_RMU_REQ_GET_ID		1
+#define MV88E6XXX_RMU_REQ_DUMP_MIB		2
+
+#define MV88E6XXX_RMU_REQ_FORMAT_GET_ID		0x0000
+#define MV88E6XXX_RMU_REQ_FORMAT_SOHO		0x0001
+#define MV88E6XXX_RMU_REQ_PAD			0x0000
+#define MV88E6XXX_RMU_REQ_CODE_GET_ID		0x0000
+#define MV88E6XXX_RMU_REQ_CODE_DUMP_MIB		0x1020
+#define MV88E6XXX_RMU_REQ_DATA			0x0000
+
+#define MV88E6XXX_RMU_REQ_DUMP_MIB_PORT_MASK	GENMASK(4, 0)
+
+#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
+
+struct mv88e6xxx_rmu_header {
+	__be16 format;
+	__be16 prodnr;
+	__be16 code;
+} __packed;
+
+struct mv88e6xxx_dump_mib_resp {
+	struct mv88e6xxx_rmu_header rmu_header;
+	u8 devnum;
+	u8 portnum;
+	__be32 timestamp;
+	__be32 mib[MV88E6XXX_RMU_MAX_RMON];
+} __packed;
+
+int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip);
+
+void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
+				   bool operational);
+
+void mv88e6xxx_decode_frame2reg_handler(struct dsa_switch *ds, struct sk_buff *skb);
+
+#endif /* _MV88E6XXX_RMU_H_ */
-- 
2.25.1

Re: [PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU functionality.

[Vladimir Oltean]

On Mon, Sep 19, 2022 at 01:08:44PM +0200, Mattias Forsblad wrote:
>  struct mv88e6xxx_bus_ops {
> diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
> new file mode 100644
> index 000000000000..c5b3c156de40
> --- /dev/null
> +++ b/drivers/net/dsa/mv88e6xxx/rmu.c
> @@ -0,0 +1,263 @@
> +// 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>

Nothing in this file uses asm/unaligned.h. On the other hand, it uses
struct dsa_switch, struct sk_buff, struct net_device, etc etc, which are
nowhere to be found in included headers. Don't rely on transitive header
inclusion. What rmu.h and global1.h include are just for the data
structures referenced within those headers to make sense when included
from any C file.

> +#include "rmu.h"
> +#include "global1.h"
> +
> +static const u8 mv88e6xxx_rmu_dest_addr[ETH_ALEN] = { 0x01, 0x50, 0x43, 0x00, 0x00, 0x00 };
> +
> +static void mv88e6xxx_rmu_create_l2(struct dsa_switch *ds, struct sk_buff *skb)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	struct ethhdr *eth;
> +	u8 *edsa_header;
> +	u8 *dsa_header;
> +	u8 extra = 0;
> +
> +	if (chip->tag_protocol == DSA_TAG_PROTO_EDSA)
> +		extra = 4;
> +
> +	/* Create RMU L2 header */
> +	dsa_header = skb_push(skb, 6);
> +	dsa_header[0] = FIELD_PREP(MV88E6XXX_CPU_CODE_MASK, MV88E6XXX_RMU);
> +	dsa_header[0] |= FIELD_PREP(MV88E6XXX_TRG_DEV_MASK, ds->index);
> +	dsa_header[1] = FIELD_PREP(MV88E6XXX_RMU_CODE_MASK, 1);
> +	dsa_header[1] |= FIELD_PREP(MV88E6XXX_RMU_L2_BYTE1_RESV, MV88E6XXX_RMU_L2_BYTE1_RESV_VAL);
> +	dsa_header[2] = FIELD_PREP(MV88E6XXX_RMU_PRIO_MASK, MV88E6XXX_RMU_PRIO);
> +	dsa_header[2] |= MV88E6XXX_RMU_L2_BYTE2_RESV;
> +	dsa_header[3] = ++chip->rmu.seqno;
> +	dsa_header[4] = 0;
> +	dsa_header[5] = 0;
> +
> +	/* Insert RMU MAC destination address /w extra if needed */
> +	eth = skb_push(skb, ETH_ALEN * 2 + extra);
> +	memcpy(eth->h_dest, mv88e6xxx_rmu_dest_addr, ETH_ALEN);
> +	ether_addr_copy(eth->h_source, chip->rmu.master_netdev->dev_addr);

Come on.... really? Do I need to spell out "ether_addr_copy()" twice
during review, for consecutive lines?

> +
> +	if (extra) {
> +		edsa_header = (u8 *)&eth->h_proto;
> +		edsa_header[0] = (ETH_P_EDSA >> 8) & 0xff;
> +		edsa_header[1] = ETH_P_EDSA & 0xff;
> +		edsa_header[2] = 0x00;
> +		edsa_header[3] = 0x00;
> +	}
> +}
> +
> +static int mv88e6xxx_rmu_send_wait(struct mv88e6xxx_chip *chip,
> +				   const void *req, int req_len,
> +				   void *resp, unsigned int *resp_len)
> +{
> +	struct sk_buff *skb;
> +	unsigned char *data;
> +	int ret = 0;
> +
> +	skb = netdev_alloc_skb(chip->rmu.master_netdev, 64);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	/* Take height for an eventual EDSA header */
> +	skb_reserve(skb, 2 * ETH_HLEN + 4);
> +	skb_reset_network_header(skb);
> +
> +	/* Insert RMU request message */
> +	data = skb_put(skb, req_len);
> +	memcpy(data, req, req_len);
> +
> +	mv88e6xxx_rmu_create_l2(chip->ds, skb);
> +
> +	mutex_lock(&chip->rmu.mutex);
> +
> +	ret = dsa_switch_inband_tx(chip->ds, skb, NULL, MV88E6XXX_RMU_WAIT_TIME_MS);
> +	if (!ret) {
> +		dev_err(chip->dev, "RMU: error waiting for request (%pe)\n",
> +			ERR_PTR(ret));
> +		goto out;
> +	}
> +
> +	if (chip->rmu.resp->len > *resp_len) {
> +		ret = -EMSGSIZE;
> +	} else {
> +		*resp_len = chip->rmu.resp->len;
> +		memcpy(resp, chip->rmu.resp->data, chip->rmu.resp->len);
> +	}
> +
> +	kfree_skb(chip->rmu.resp);
> +	chip->rmu.resp = NULL;
> +
> +out:
> +	mutex_unlock(&chip->rmu.mutex);
> +
> +	return ret > 0 ? 0 : ret;
> +}

This function is mixing return values from dsa_switch_inband_tx()
(really wait_for_completion_timeout(), where 0 is timeout) with negative
return values. What should the caller check for success? Yes.

> +
> +static int mv88e6xxx_rmu_validate_response(struct mv88e6xxx_rmu_header *resp, int code)
> +{
> +	if (be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_1 &&
> +	    be16_to_cpu(resp->format) != MV88E6XXX_RMU_RESP_FORMAT_2 &&
> +	    be16_to_cpu(resp->code) != code) {
> +		net_dbg_ratelimited("RMU: received unknown format 0x%04x code 0x%04x",
> +				    resp->format, resp->code);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
> +{
> +	const u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_GET_ID,
> +			     MV88E6XXX_RMU_REQ_PAD,
> +			     MV88E6XXX_RMU_REQ_CODE_GET_ID,
> +			     MV88E6XXX_RMU_REQ_DATA};
> +	struct mv88e6xxx_rmu_header resp;
> +	int resp_len;
> +	int ret = -1;

Don't initialize variables you'll overwrite immediately afterwards.

There is at least one other place in this patch which does that and
which should therefore also be changed. I won't say where that is,
I want to force you to read your changes.

> +
> +	resp_len = sizeof(resp);
> +	ret = mv88e6xxx_rmu_send_wait(chip, req, sizeof(req),
> +				      &resp, &resp_len);
> +	if (ret) {
> +		dev_dbg(chip->dev, "RMU: error for command GET_ID %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	/* Got response */
> +	ret = mv88e6xxx_rmu_validate_response(&resp, MV88E6XXX_RMU_RESP_CODE_GOT_ID);
> +	if (ret)
> +		return ret;
> +
> +	chip->rmu.prodnr = be16_to_cpu(resp.prodnr);
> +
> +	return ret;

return 0.

> +}
> +
> +static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
> +{
> +	chip->smi_ops = chip->rmu.smi_ops;
> +	chip->ds->ops = chip->rmu.ds_rmu_ops;

Who modifies chip->ds->ops? This smells trouble. Big, big trouble.

> +	chip->rmu.master_netdev = NULL;
> +
> +	if (chip->info->ops->rmu_disable)
> +		chip->info->ops->rmu_disable(chip);
> +}
> +
> +static int mv88e6xxx_enable_check_rmu(const struct net_device *master,
> +				      struct mv88e6xxx_chip *chip, int port)
> +{
> +	int ret;
> +
> +	chip->rmu.master_netdev = (struct net_device *)master;
> +
> +	/* Check if chip is alive */
> +	ret = mv88e6xxx_rmu_get_id(chip, port);
> +	if (!ret)
> +		return ret;

It's really nice that in the mv88e6xxx_enable_check_rmu() ->
mv88e6xxx_rmu_get_id() -> mv88e6xxx_rmu_send_wait() call path, first you
check for ret != 0 as meaning error, then for ret == 0 as meaning error.
So you catch a bit of everything.

> +
> +	chip->ds->ops = chip->rmu.ds_rmu_ops;
> +	chip->smi_ops = chip->rmu.smi_rmu_ops;
> +
> +	return 0;
> +}
> +
> +void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
> +				   bool operational)
> +{
> +	struct dsa_port *cpu_dp = master->dsa_ptr;
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int port;
> +	int ret;
> +
> +	port = dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);
> +
> +	mv88e6xxx_reg_lock(chip);
> +
> +	if (operational && chip->info->ops->rmu_enable) {

This all needs to be rewritten. Like here, if the master is operational
but the chip->info->ops->rmu_enable method is not populated, you call
mv88e6xxx_disable_rmu(). Why?

> +		ret = chip->info->ops->rmu_enable(chip, port);
> +
> +		if (ret == -EOPNOTSUPP)
> +			goto out;
> +
> +		if (!ret) {
> +			dev_dbg(chip->dev, "RMU: Enabled on port %d", port);
> +
> +			ret = mv88e6xxx_enable_check_rmu(master, chip, port);
> +			if (!ret)
> +				goto out;

Or here, if this fails, you goto out, which does nothing special
compared to simply not checking the return code. But you don't disable
the RMU back.

> +
> +		} else {
> +			dev_err(chip->dev, "RMU: Unable to enable on port %d %pe",
> +				port, ERR_PTR(ret));
> +			goto out;
> +		}
> +	} else {
> +		mv88e6xxx_disable_rmu(chip);
> +	}
> +
> +out:
> +	mv88e6xxx_reg_unlock(chip);
> +}

Re: [PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU functionality.

[Mattias Forsblad]

On 2022-09-20 00:39, Vladimir Oltean wrote:
>> +void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
>> +				   bool operational)
>> +{
>> +	struct dsa_port *cpu_dp = master->dsa_ptr;
>> +	struct mv88e6xxx_chip *chip = ds->priv;
>> +	int port;
>> +	int ret;
>> +
>> +	port = dsa_towards_port(ds, cpu_dp->ds->index, cpu_dp->index);
>> +
>> +	mv88e6xxx_reg_lock(chip);
>> +
>> +	if (operational && chip->info->ops->rmu_enable) {
> 
> This all needs to be rewritten. Like here, if the master is operational
> but the chip->info->ops->rmu_enable method is not populated, you call
> mv88e6xxx_disable_rmu(). Why?
>

So what should we do in this case? If the master is operational but we cannot
enable rmu (bc no funcptr), we cannot use RMU -> disable RMU.

/Mattias

>> +		ret = chip->info->ops->rmu_enable(chip, port);
>> +
>> +		if (ret == -EOPNOTSUPP)
>> +			goto out;
>> +
>> +		if (!ret) {
>> +			dev_dbg(chip->dev, "RMU: Enabled on port %d", port);
>> +
>> +			ret = mv88e6xxx_enable_check_rmu(master, chip, port);
>> +			if (!ret)
>> +				goto out;

Re: [PATCH net-next v14 4/7] net: dsa: mv88e6xxxx: Add RMU functionality.

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 01:53:36PM +0200, Mattias Forsblad wrote:
> On 2022-09-20 00:39, Vladimir Oltean wrote:
> >> +void mv88e6xxx_master_state_change(struct dsa_switch *ds, const struct net_device *master,
> >> +				   bool operational)
> >> +{
> >> +	if (operational && chip->info->ops->rmu_enable) {
> > 
> > This all needs to be rewritten. Like here, if the master is operational
> > but the chip->info->ops->rmu_enable method is not populated, you call
> > mv88e6xxx_disable_rmu(). Why?
> 
> So what should we do in this case?

Nothing, obviously.

> If the master is operational but we cannot enable rmu (bc no funcptr),
> we cannot use RMU -> disable RMU.

Again, the RMU should start as disabled. Then why
would you call mv88e6xxx_disable_rmu() a million times as the master
goes up and down, if the switch doesn't support chip->info->ops->rmu_enable()?

In fact, the RMU _is_ disabled, since mv88e6xxx_rmu_setup() has:

int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
{
	mutex_init(&chip->rmu.mutex);

	/* Remember original ops for restore */
	chip->rmu.smi_ops = chip->smi_ops;
	chip->rmu.ds_ops = chip->ds->ops;

	/* Change rmu ops with our own pointer */
	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;

	/* Also change get stats strings for our own */
	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;

	/* Start disabled, we'll enable RMU in master_state_change */
	if (chip->info->ops->rmu_disable)
		return chip->info->ops->rmu_disable(chip);

	return 0;
}

But mv88e6xxx_disable_rmu() has:

static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
{
	chip->smi_ops = chip->rmu.smi_ops;
	chip->ds->ops = chip->rmu.ds_rmu_ops;
	chip->rmu.master_netdev = NULL;

	if (chip->info->ops->rmu_disable)
		chip->info->ops->rmu_disable(chip);
}

Notice in mv88e6xxx_disable_rmu() how:

- all calls to chip->info->ops->rmu_disable() are redundant when
  chip->info->ops->rmu_enable() isn't available.

- the mumbo jumbo pointer logic with chip->smi_ops and chip->ds->ops is
  buggy but at the same time not in the obvious way. What is obvious is
  that you surely don't mean to assign "chip->ds->ops = chip->rmu.ds_rmu_ops;",
  but rather "chip->ds->ops = chip->rmu.ds_ops;". But this does not
  truly matter.

This is because juggling the chip->ds->ops pointer itself is not how you
make mv88e6xxx_get_ethtool_stats() call MDIO or Ethernet-based ops. This
is because in reality in your implementation, ds_rmu_ops and ds_ops (and
same goes for smi_ops and smi_rmu_ops) point to the same data structure.
And when you do this:

	/* Change rmu ops with our own pointer */
	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;

you change the get_rmon() operation of _both_ smi_rmu_ops and smi_ops,
because you dereference two pointers which have the same value.

Therefore, when you attempt to collect ethtool stats, you dereference
"our own" RMU based pointer, regardless of whether RMU is available or
not:

static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
					uint64_t *data)
{
	struct mv88e6xxx_chip *chip = ds->priv;

	chip->smi_ops->get_rmon(chip, port, data);
}

This will proceed to access stuff that isn't available, such as the
master netdev, and crash the kernel.

[PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Mattias Forsblad]

It's possible to access the RMON counters via RMU.
Add functionality to send DUMP_MIB frames.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 74 ++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/chip.h |  6 +++
 drivers/net/dsa/mv88e6xxx/rmu.c  | 53 +++++++++++++++++++++++
 3 files changed, 133 insertions(+)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 294bf9bbaf3f..5b22fe4b3284 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1229,6 +1229,80 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	return count;
 }
 
+int mv88e6xxx_stats_get_sset_count_rmu(struct dsa_switch *ds, int port, int sset)
+{
+	if (sset != ETH_SS_STATS)
+		return 0;
+
+	return ARRAY_SIZE(mv88e6xxx_hw_stats);
+}
+
+void mv88e6xxx_stats_get_strings_rmu(struct dsa_switch *ds, int port,
+				     u32 stringset, uint8_t *data)
+{
+	struct mv88e6xxx_hw_stat *stat;
+	int i;
+
+	if (stringset != ETH_SS_STATS)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(mv88e6xxx_hw_stats); i++) {
+		stat = &mv88e6xxx_hw_stats[i];
+		memcpy(data + i * ETH_GSTRING_LEN, stat->string, ETH_GSTRING_LEN);
+	}
+}
+
+int mv88e6xxx_state_get_stats_rmu(struct mv88e6xxx_chip *chip, int port,
+				  __be32 *raw_cnt, int num_cnt, uint64_t *data)
+{
+	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];
+		offset = stat->reg;
+
+		if (stat->type & STATS_TYPE_PORT) {
+			/* The offsets for the port counters below
+			 * are different in the RMU packet.
+			 */
+			switch (stat->reg) {
+			case 0x10: /* sw_in_discards */
+				offset = 0x81;
+				break;
+			case 0x12: /* sw_in_filtered */
+				offset = 0x85;
+				break;
+			case 0x13: /* sw_out_filtered */
+				offset = 0x89;
+				break;
+			default:
+				dev_err(chip->dev,
+					"RMU: port %d wrong offset requested: %d\n",
+					port, stat->reg);
+				return j;
+			}
+		} else if (stat->type & STATS_TYPE_BANK1) {
+			offset += 32;
+		}
+
+		if (offset >= num_cnt)
+			continue;
+
+		data[j] = be32_to_cpu(raw_cnt[offset]);
+
+		if (stat->size == 8) {
+			high = be32_to_cpu(raw_cnt[offset + 1]);
+			data[j] += (high << 32);
+		}
+
+		j++;
+	}
+	return j;
+}
+
 static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				     uint64_t *data, int types,
 				     u16 bank1_select, u16 histogram)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 440e9b274df4..aca7cfef196e 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -433,6 +433,7 @@ 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);
+	void (*get_rmon)(struct mv88e6xxx_chip *chip, int port, uint64_t *data);
 };
 
 struct mv88e6xxx_mdio_bus {
@@ -822,4 +823,9 @@ static inline void mv88e6xxx_reg_unlock(struct mv88e6xxx_chip *chip)
 
 int mv88e6xxx_fid_map(struct mv88e6xxx_chip *chip, unsigned long *bitmap);
 
+int mv88e6xxx_state_get_stats_rmu(struct mv88e6xxx_chip *chip, int port,
+				  __be32 *raw_cnt, int num_cnt, uint64_t *data);
+int mv88e6xxx_stats_get_sset_count_rmu(struct dsa_switch *ds, int port, int sset);
+void mv88e6xxx_stats_get_strings_rmu(struct dsa_switch *ds, int port,
+				     u32 stringset, uint8_t *data);
 #endif /* _MV88E6XXX_CHIP_H */
diff --git a/drivers/net/dsa/mv88e6xxx/rmu.c b/drivers/net/dsa/mv88e6xxx/rmu.c
index c5b3c156de40..129535777c4b 100644
--- a/drivers/net/dsa/mv88e6xxx/rmu.c
+++ b/drivers/net/dsa/mv88e6xxx/rmu.c
@@ -137,6 +137,50 @@ static int mv88e6xxx_rmu_get_id(struct mv88e6xxx_chip *chip, int port)
 	return ret;
 }
 
+static void mv88e6xxx_rmu_stats_get(struct mv88e6xxx_chip *chip, int port, uint64_t *data)
+{
+	u16 req[4] = { MV88E6XXX_RMU_REQ_FORMAT_SOHO,
+		       MV88E6XXX_RMU_REQ_PAD,
+		       MV88E6XXX_RMU_REQ_CODE_DUMP_MIB,
+		       MV88E6XXX_RMU_REQ_DATA};
+	struct mv88e6xxx_dump_mib_resp resp;
+	struct mv88e6xxx_port *p;
+	u8 resp_port;
+	int resp_len;
+	int num_mibs;
+	int ret;
+
+	/* Populate port number in request */
+	req[3] = FIELD_PREP(MV88E6XXX_RMU_REQ_DUMP_MIB_PORT_MASK, port);
+
+	resp_len = sizeof(resp);
+	ret = mv88e6xxx_rmu_send_wait(chip, req, sizeof(req),
+				      &resp, &resp_len);
+	if (ret) {
+		dev_dbg(chip->dev, "RMU: error for command DUMP_MIB %pe port %d\n",
+			ERR_PTR(ret), port);
+		return;
+	}
+
+	/* Got response */
+	ret = mv88e6xxx_rmu_validate_response(&resp.rmu_header, MV88E6XXX_RMU_RESP_CODE_DUMP_MIB);
+	if (ret)
+		return;
+
+	resp_port = FIELD_GET(MV88E6XXX_SOURCE_PORT, resp.portnum);
+	p = &chip->ports[resp_port];
+	if (!p) {
+		dev_err_ratelimited(chip->dev, "RMU: illegal port number in response: %d\n",
+				    resp_port);
+		return;
+	}
+
+	/* Update MIB for port */
+	num_mibs = (resp_len - offsetof(struct mv88e6xxx_dump_mib_resp, mib)) / sizeof(__be32);
+
+	mv88e6xxx_state_get_stats_rmu(chip, port, resp.mib, num_mibs, data);
+}
+
 static void mv88e6xxx_disable_rmu(struct mv88e6xxx_chip *chip)
 {
 	chip->smi_ops = chip->rmu.smi_ops;
@@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
 	chip->rmu.smi_ops = chip->smi_ops;
 	chip->rmu.ds_ops = chip->ds->ops;
 
+	/* Change rmu ops with our own pointer */
+	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
+	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;
+
+	/* Also change get stats strings for our own */
+	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
+	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
+	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
+
 	/* Start disabled, we'll enable RMU in master_state_change */
 	if (chip->info->ops->rmu_disable)
 		return chip->info->ops->rmu_disable(chip);
-- 
2.25.1

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Vladimir Oltean]

On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:
> @@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>  	chip->rmu.smi_ops = chip->smi_ops;
>  	chip->rmu.ds_ops = chip->ds->ops;
>  
> +	/* Change rmu ops with our own pointer */
> +	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
> +	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;

The patch splitting is still so poor, that I can't even complain about
this bug properly, since no one uses this pointer for now (and when it
will be used, it will not be obvious how it's assigned).

In short, it's called like this:

static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
					uint64_t *data)
{
	struct mv88e6xxx_chip *chip = ds->priv;

	chip->smi_ops->get_rmon(chip, port, data);
}

When the switch doesn't support RMU operations, or when the RMU setup
simply failed, "ethtool -S" will dereference a very NULL pointer during
that indirect call, because mv88e6xxx_rmu_setup() is unconditionally
called for every switch during setup. Probably not a wise choice to
start off with the RMU ops for ethtool -S.

Also, not clear, if RMU fails, why we don't make an effort to fall back
to MDIO for that user space request.

> +
> +	/* Also change get stats strings for our own */

Who is "our own", and implicitly, who are they? You'd expect that there
aren't tribalist factions within the same driver.

> +	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
> +	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
> +	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
> +

Those who cast a const to a non-const pointer and proceed to modify the
read-only structure behind it should go to patchwork arrest for one week.

>  	/* Start disabled, we'll enable RMU in master_state_change */
>  	if (chip->info->ops->rmu_disable)
>  		return chip->info->ops->rmu_disable(chip);
> -- 
> 2.25.1
> 

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Mattias Forsblad]

On 2022-09-20 00:49, Vladimir Oltean wrote:
> On Mon, Sep 19, 2022 at 01:08:45PM +0200, Mattias Forsblad wrote:
>> @@ -255,6 +299,15 @@ int mv88e6xxx_rmu_setup(struct mv88e6xxx_chip *chip)
>>  	chip->rmu.smi_ops = chip->smi_ops;
>>  	chip->rmu.ds_ops = chip->ds->ops;
>>  
>> +	/* Change rmu ops with our own pointer */
>> +	chip->rmu.smi_rmu_ops = (struct mv88e6xxx_bus_ops *)chip->rmu.smi_ops;
>> +	chip->rmu.smi_rmu_ops->get_rmon = mv88e6xxx_rmu_stats_get;
> 
> The patch splitting is still so poor, that I can't even complain about
> this bug properly, since no one uses this pointer for now (and when it
> will be used, it will not be obvious how it's assigned).
> 
> In short, it's called like this:
> 
> static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
> 					uint64_t *data)
> {
> 	struct mv88e6xxx_chip *chip = ds->priv;
> 
> 	chip->smi_ops->get_rmon(chip, port, data);
> }
> 
> When the switch doesn't support RMU operations, or when the RMU setup
> simply failed, "ethtool -S" will dereference a very NULL pointer during
> that indirect call, because mv88e6xxx_rmu_setup() is unconditionally
> called for every switch during setup. Probably not a wise choice to
> start off with the RMU ops for ethtool -S.
> 
> Also, not clear, if RMU fails, why we don't make an effort to fall back
> to MDIO for that user space request.
> 
>> +
>> +	/* Also change get stats strings for our own */
> 
> Who is "our own", and implicitly, who are they? You'd expect that there
> aren't tribalist factions within the same driver.
> 
>> +	chip->rmu.ds_rmu_ops = (struct dsa_switch_ops *)chip->ds->ops;
>> +	chip->rmu.ds_rmu_ops->get_sset_count = mv88e6xxx_stats_get_sset_count_rmu;
>> +	chip->rmu.ds_rmu_ops->get_strings = mv88e6xxx_stats_get_strings_rmu;
>> +
> 
> Those who cast a const to a non-const pointer and proceed to modify the
> read-only structure behind it should go to patchwork arrest for one week.
> 
>>  	/* Start disabled, we'll enable RMU in master_state_change */
>>  	if (chip->info->ops->rmu_disable)
>>  		return chip->info->ops->rmu_disable(chip);
>> -- 
>> 2.25.1
>>
> 

This whole shebang was a suggestion from Andrew. I had a solution with
mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
member? I'm not really sure on how to solve this in a better way?
Suggestions any? Maybe I've misunderstood his suggestion.

/Mattias

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
> This whole shebang was a suggestion from Andrew. I had a solution with
> mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
> The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
> member? I'm not really sure on how to solve this in a better way?
> Suggestions any? Maybe I've misunderstood his suggestion.

Can you point me to the beginning of that exact suggestion? I've removed
everything older than v10 from my inbox, since the flow of patches was
preventing me from seeing other emails.

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Mattias Forsblad]

On 2022-09-20 15:10, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
>> This whole shebang was a suggestion from Andrew. I had a solution with
>> mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
>> The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
>> member? I'm not really sure on how to solve this in a better way?
>> Suggestions any? Maybe I've misunderstood his suggestion.
> 
> Can you point me to the beginning of that exact suggestion? I've removed
> everything older than v10 from my inbox, since the flow of patches was
> preventing me from seeing other emails.

https://lore.kernel.org/netdev/Yxkc6Zav7XoKRLBt@lunn.ch/

/Mattias

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Andrew Lunn]

On Tue, Sep 20, 2022 at 04:10:53PM +0300, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
> > This whole shebang was a suggestion from Andrew. I had a solution with
> > mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
> > The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
> > member? I'm not really sure on how to solve this in a better way?
> > Suggestions any? Maybe I've misunderstood his suggestion.
> 
> Can you point me to the beginning of that exact suggestion? I've removed
> everything older than v10 from my inbox, since the flow of patches was
> preventing me from seeing other emails.

What i want to do is avoid code like:

     if (have_rmu())
     	foo()
     else
	bar()

There is nothing common in the MDIO MIB code and the RMU MIB code,
just the table of statistics. When we get to dumping the ATU, i also
expect there will be little in common between the MDIO and the RMU
functions.

Doing MIB via RMU is a big gain, but i would also like normal register
read and write to go via RMU, probably with some level of
combining. Multiple writes can be combined into one RMU operation
ending with a read. That should give us an mv88e6xxx_bus_ops which
does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
bus_ops.

But how do we mix RMU MIB and ATU dumps into this? My idea was to make
them additional members of mv88e6xxx_bus_ops. The MDIO bus_ops
structures would end up call the mv88e6xxx_ops method for MIB or
ATU. The rmu bus_ops and directly call an RMU function to do it.

What is messy at the moment is that we don't have register read/write
via RMU, so we have some horrible hybrid. We should probably just
implement simple read and write, without combining, so we can skip
this hybrid.

I am assuming here that RMU is reliable. The QCA8K driver currently
falls back to MDIO if its inband function is attempted but fails.  I
want to stress this part, lots of data packets and see if the RMU
frames get dropped, or delayed too much causing failures. If we do see
failures, is a couple of retires enough? Or do we need to fallback to
MDIO which should always work? If we do need to fallback, this
structure is not going to work too well.

	  Andrew

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Mattias Forsblad]

On 2022-09-20 23:04, Andrew Lunn wrote:
> On Tue, Sep 20, 2022 at 04:10:53PM +0300, Vladimir Oltean wrote:
>> On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
>>> This whole shebang was a suggestion from Andrew. I had a solution with
>>> mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
>>> The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
>>> member? I'm not really sure on how to solve this in a better way?
>>> Suggestions any? Maybe I've misunderstood his suggestion.
>>
>> Can you point me to the beginning of that exact suggestion? I've removed
>> everything older than v10 from my inbox, since the flow of patches was
>> preventing me from seeing other emails.
> 
> What i want to do is avoid code like:
> 
>      if (have_rmu())
>      	foo()
>      else
> 	bar()
> 
> There is nothing common in the MDIO MIB code and the RMU MIB code,
> just the table of statistics. When we get to dumping the ATU, i also
> expect there will be little in common between the MDIO and the RMU
> functions.
> 
> Doing MIB via RMU is a big gain, but i would also like normal register
> read and write to go via RMU, probably with some level of
> combining. Multiple writes can be combined into one RMU operation
> ending with a read. That should give us an mv88e6xxx_bus_ops which
> does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
> bus_ops.
> 
> But how do we mix RMU MIB and ATU dumps into this? My idea was to make
> them additional members of mv88e6xxx_bus_ops. The MDIO bus_ops
> structures would end up call the mv88e6xxx_ops method for MIB or
> ATU. The rmu bus_ops and directly call an RMU function to do it.
> 
> What is messy at the moment is that we don't have register read/write
> via RMU, so we have some horrible hybrid. We should probably just
> implement simple read and write, without combining, so we can skip
> this hybrid.
> 
> I am assuming here that RMU is reliable. The QCA8K driver currently
> falls back to MDIO if its inband function is attempted but fails.  I
> want to stress this part, lots of data packets and see if the RMU
> frames get dropped, or delayed too much causing failures. If we do see
> failures, is a couple of retires enough? Or do we need to fallback to
> MDIO which should always work? If we do need to fallback, this
> structure is not going to work too well.
> 
> 	  Andrew

I understand want you want but I can see a lot of risks and pitfalls with moving
ordinary read and writes to RMU, which I wanted to avoid by first doing
RMON dump and then dump ATU and at a later stage with a better architecture
for write/read combining doing that, instead of forcing through read/writes
with all associated testing it would require. Can we please do this in
steps?

/Mattias

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Andrew Lunn]

> I understand want you want but I can see a lot of risks and pitfalls with moving
> ordinary read and writes to RMU, which I wanted to avoid by first doing
> RMON dump and then dump ATU and at a later stage with a better architecture
> for write/read combining doing that, instead of forcing through read/writes
> with all associated testing it would require. Can we please do this in
> steps?

If we are going to fall back to MDIO when RMU fails, we need a
different code structure for these operations. That different code
structure should also help solve the messy _ops structure stuff.

RMU affects us in two different locations:

ATU and MIB dump: Controlled by struct mv88e6xxx_ops

register read/write: Controlled by struct mv88e6xxx_bus_ops

We could add to struct mv88e6xxx_ops:

        int (*stats_rmu_get_sset_count)(struct mv88e6xxx_chip *chip);
        int (*stats_rmu_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
        int (*stats_rmu_get_stats)(struct mv88e6xxx_chip *chip,  int port,
                                   uint64_t *data);

and then mv88e6xxx_get_stats() would become something like:

static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
                                uint64_t *data)
{
        int count = 0;
	int err;

	if (chip->info->ops->stats_rmu_get_stats && mv88e6xxx_rmu_enabled(chip)) {
		err = chip->info->ops->stats_rmu_get_stats(chip, port, data)
		if (!err)
			return;
	}
			
        if (chip->info->ops->stats_get_stats)
                count = chip->info->ops->stats_get_stats(chip, port, data);

We then get fall back to MDIO, and clean, separate implementations of
RMU and MDIO stats operations.

I hope the ATU/fdb dump can be done in a similar way.

register read/writes, we probably need to extend mv88e6xxx_smi_read()
and mv88e6xxx_smi_write(). Try RMU first, and then fall back to MDIO.

Please try something in this direction. But please, lots of small,
simple patches with good commit messages.

       Andrew

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Vladimir Oltean]

On Tue, Sep 20, 2022 at 11:04:07PM +0200, Andrew Lunn wrote:
> On Tue, Sep 20, 2022 at 04:10:53PM +0300, Vladimir Oltean wrote:
> > On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
> > > This whole shebang was a suggestion from Andrew. I had a solution with
> > > mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
> > > The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
> > > member? I'm not really sure on how to solve this in a better way?
> > > Suggestions any? Maybe I've misunderstood his suggestion.
> > 
> > Can you point me to the beginning of that exact suggestion? I've removed
> > everything older than v10 from my inbox, since the flow of patches was
> > preventing me from seeing other emails.
> 
> What i want to do is avoid code like:
> 
>      if (have_rmu())
>      	foo()
>      else
> 	bar()
> 
> There is nothing common in the MDIO MIB code and the RMU MIB code,
> just the table of statistics. When we get to dumping the ATU, i also
> expect there will be little in common between the MDIO and the RMU
> functions.

Sorry, I don't understand what it is about "if (have_rmu()) foo() else bar()"
that you don't like. Isn't the indirection via bus_ops the exact same
thing, but expressed as an indirect function call rather than an if/else?

Or is it that the if/else structure precludes the calling of bar() when
foo() fails? The bus_ops will suffer from the same problem.

> Doing MIB via RMU is a big gain, but i would also like normal register
> read and write to go via RMU, probably with some level of
> combining. Multiple writes can be combined into one RMU operation
> ending with a read. That should give us an mv88e6xxx_bus_ops which
> does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
> bus_ops.

At what level would the combining be done? I think the mv88e6xxx doesn't
really make use of bulk operations, C45 MDIO reads with post-increment,
that sort of thing. I could be wrong. And at some higher level, the
register read/write code should not diverge (too much), even if the
operation may be done over Ethernet or MDIO. So we need to find places
which actually make useful sense of bulk reads.

> But how do we mix RMU MIB and ATU dumps into this? My idea was to make
> them additional members of mv88e6xxx_bus_ops. The MDIO bus_ops
> structures would end up call the mv88e6xxx_ops method for MIB or
> ATU. The rmu bus_ops and directly call an RMU function to do it.
> 
> What is messy at the moment is that we don't have register read/write
> via RMU, so we have some horrible hybrid. We should probably just
> implement simple read and write, without combining, so we can skip
> this hybrid.

I see in the manual that there are some registers which aren't
available or not recommended over RMU, for example the PHY registers if
the PPU is disabled, or phylink methods for the upstream facing ports.
There are also less obvious things, like accessing the PTP clock
gettime(). This will surely change the delay characteristic that phc2sys
sees, I'm not sure if for the better or for the worse, but the SMI code
at least had some tuning made to it, so people might care. So bottom
line, I'm not sure whether we do enough to prevent these pitfalls by
simply creating blanket replacements for all register reads/writes and
not inspecting whether the code is fine after we do that.

On the other hand, having RMU operations might also bring subtle
benefits to phc2sys. I think there were some issues with the PTP code
having trouble getting MDIO bus access (because of bridge fdb dumps or
some other things happening in the background). This may also be an
opportunity to have parallel access to independent IP blocks within the
switch, one goes through MDIO and the other through Ethernet.

But then, Mattias' code structure becomes inadequate. Currently we
serialize mv88e6xxx_master_state_change() with respect to bus accesses
via mv88e6xxx_reg_lock(). But if we permit RMU to run in parallel with
MDIO, we need a rwlock, such that multiple 'readers' of the conceptual
have_rmu() function can run in parallel with each other, and just
serialize with the RMU state changes (the 'writers').

> I am assuming here that RMU is reliable. The QCA8K driver currently
> falls back to MDIO if its inband function is attempted but fails.  I
> want to stress this part, lots of data packets and see if the RMU
> frames get dropped, or delayed too much causing failures.

I don't think you even have to stress it too much. Nothing prevents the
user from putting a policer on the DSA master which will randomly drop
responses. Or a shaper that will delay requests beyond the timeout.

> If we do see
> failures, is a couple of retires enough? Or do we need to fallback to
> MDIO which should always work? If we do need to fallback, this
> structure is not going to work too well.

Consider our previous discussion about the switchdev prepare/commit
transactional structure, where the commit stage is not supposed to fail
even if it writes to hardware. You said that the writes are supposed to
be reliable, or else. Either they all work or none do. Not sure how that
is going to hold up with a transport such as Ethernet which has such a
wide arsenal of foot guns. I think that leaving the MDIO fallback in is
inevitable.

In terms of retries, I'm not sure. With the qca8k code structure:

	if (have_rmu()) {
		ret = foo();
		if (ret == 0)
			return 0;
	}

	return bar();

we won't have retries for the _current_ operation, but all further
operations will still try to use Ethernet first. So here, it seems to me
that the timeout needs to be tuned such that everything does not grind
down to a halt even if we have a lossy Ethernet channel.

Otherwise, we need to build some more (perhaps too advanced) logic. Have
"if (have_rmu() && rmu_works())", periodically re-check if RMU works
after a failure, etc.

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Andrew Lunn]

On Thu, Sep 22, 2022 at 02:48:20PM +0300, Vladimir Oltean wrote:
> On Tue, Sep 20, 2022 at 11:04:07PM +0200, Andrew Lunn wrote:
> > On Tue, Sep 20, 2022 at 04:10:53PM +0300, Vladimir Oltean wrote:
> > > On Tue, Sep 20, 2022 at 02:26:22PM +0200, Mattias Forsblad wrote:
> > > > This whole shebang was a suggestion from Andrew. I had a solution with
> > > > mv88e6xxx_rmu_available in mv88e6xxx_get_ethtool_stats which he wasn't fond of.
> > > > The mv88e6xxx_bus_ops is declared const and how am I to change the get_rmon
> > > > member? I'm not really sure on how to solve this in a better way?
> > > > Suggestions any? Maybe I've misunderstood his suggestion.
> > > 
> > > Can you point me to the beginning of that exact suggestion? I've removed
> > > everything older than v10 from my inbox, since the flow of patches was
> > > preventing me from seeing other emails.
> > 
> > What i want to do is avoid code like:
> > 
> >      if (have_rmu())
> >      	foo()
> >      else
> > 	bar()
> > 
> > There is nothing common in the MDIO MIB code and the RMU MIB code,
> > just the table of statistics. When we get to dumping the ATU, i also
> > expect there will be little in common between the MDIO and the RMU
> > functions.
> 
> Sorry, I don't understand what it is about "if (have_rmu()) foo() else bar()"
> that you don't like. Isn't the indirection via bus_ops the exact same
> thing, but expressed as an indirect function call rather than an if/else?

There was code like this deep inside the MIB code, which just looked
ugly. That is now gone.

> > Doing MIB via RMU is a big gain, but i would also like normal register
> > read and write to go via RMU, probably with some level of
> > combining. Multiple writes can be combined into one RMU operation
> > ending with a read. That should give us an mv88e6xxx_bus_ops which
> > does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
> > bus_ops.
> 
> At what level would the combining be done? I think the mv88e6xxx doesn't
> really make use of bulk operations, C45 MDIO reads with post-increment,
> that sort of thing. I could be wrong. And at some higher level, the
> register read/write code should not diverge (too much), even if the
> operation may be done over Ethernet or MDIO. So we need to find places
> which actually make useful sense of bulk reads.

I was thinking within mv88e6xxx_read() and mv88e6xxx_write(). Keep a
buffer for building requests. Each write call appends the write to the
buffer and returns 0. A read call gets appended to the buffer and then
executes the RMU. We probably also need to wrap the reg mutex, so that
when it is released, any buffered writes get executed. If the RMU
fails, we have all the information needed to do the same via MDIO.

What i was not aware of is that some registers are not supposed to be
accessed over RMU. I suppose we can make a list of them, and if there
is a read/write to such a register, execute the RMU and then perform
an MDIO operation for the restricted register.

> But then, Mattias' code structure becomes inadequate. Currently we
> serialize mv88e6xxx_master_state_change() with respect to bus accesses
> via mv88e6xxx_reg_lock(). But if we permit RMU to run in parallel with
> MDIO, we need a rwlock, such that multiple 'readers' of the conceptual
> have_rmu() function can run in parallel with each other, and just
> serialize with the RMU state changes (the 'writers').

I don't think we can allow RMU to run in parallel to MDIO. The reg
lock will probably prevent that anyway.

> 
> > I am assuming here that RMU is reliable. The QCA8K driver currently
> > falls back to MDIO if its inband function is attempted but fails.  I
> > want to stress this part, lots of data packets and see if the RMU
> > frames get dropped, or delayed too much causing failures.
> 
> I don't think you even have to stress it too much. Nothing prevents the
> user from putting a policer on the DSA master which will randomly drop
> responses. Or a shaper that will delay requests beyond the timeout.

That would be a self inflicted problem. But you are correct, we need
to fall back to MDIO.

This is one area we can experiment with. Maybe we can retry the
operation via RMU a few times? Two retries for MIBs is still going to
be a lot faster, if successful, compared to all the MDIO transactions
for all the statistics. We can also add some fall back tracking
logic. If RMU has failed for N times in a row, stop using it for 60
seconds, etc. That might be something we can put into the DSA core,
since it seems like a generic problem.

There is a lot of experimentation needed here, and it could be we need
to throw it all away and try again a few times until we have explored
the problem sufficiently to get it right...

    Andrew

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Vladimir Oltean]

On Thu, Sep 22, 2022 at 02:45:34PM +0200, Andrew Lunn wrote:
> > > Doing MIB via RMU is a big gain, but i would also like normal register
> > > read and write to go via RMU, probably with some level of
> > > combining. Multiple writes can be combined into one RMU operation
> > > ending with a read. That should give us an mv88e6xxx_bus_ops which
> > > does RMU, and we can swap the bootstrap MDIO bus_ops for the RMU
> > > bus_ops.
> > 
> > At what level would the combining be done? I think the mv88e6xxx doesn't
> > really make use of bulk operations, C45 MDIO reads with post-increment,
> > that sort of thing. I could be wrong. And at some higher level, the
> > register read/write code should not diverge (too much), even if the
> > operation may be done over Ethernet or MDIO. So we need to find places
> > which actually make useful sense of bulk reads.
> 
> I was thinking within mv88e6xxx_read() and mv88e6xxx_write(). Keep a
> buffer for building requests. Each write call appends the write to the
> buffer and returns 0. A read call gets appended to the buffer and then
> executes the RMU. We probably also need to wrap the reg mutex, so that
> when it is released, any buffered writes get executed. If the RMU
> fails, we have all the information needed to do the same via MDIO.

Ah, so you want to make the mv88e6xxx_reg_unlock() become an implicit
write barrier.

That could work, but the trouble seems to be error propagation.
mv88e6xxx_write() will always return 0, the operation will be delayed
until the unlock, and mv88e6xxx_reg_unlock() does not return an error
code (why would it?).

> > But then, Mattias' code structure becomes inadequate. Currently we
> > serialize mv88e6xxx_master_state_change() with respect to bus accesses
> > via mv88e6xxx_reg_lock(). But if we permit RMU to run in parallel with
> > MDIO, we need a rwlock, such that multiple 'readers' of the conceptual
> > have_rmu() function can run in parallel with each other, and just
> > serialize with the RMU state changes (the 'writers').
> 
> I don't think we can allow RMU to run in parallel to MDIO. The reg
> lock will probably prevent that anyway.

Well, I was thinking the locking could get rearchitected, but it seems
you have bigger plans for it, so it becomes even more engrained in the
driver :)

> > > I am assuming here that RMU is reliable. The QCA8K driver currently
> > > falls back to MDIO if its inband function is attempted but fails.  I
> > > want to stress this part, lots of data packets and see if the RMU
> > > frames get dropped, or delayed too much causing failures.
> > 
> > I don't think you even have to stress it too much. Nothing prevents the
> > user from putting a policer on the DSA master which will randomly drop
> > responses. Or a shaper that will delay requests beyond the timeout.
> 
> That would be a self inflicted problem. But you are correct, we need
> to fall back to MDIO.

Here's one variation which is really not self inflicted. You have a 10G
CPU port, and 1G user ports. You use flow control on the DSA master to
avoid packet loss due to the 10G->1G rate adaptation. So the DSA master
goes periodically through states of TX congestion and holds back frames
until it goes away. This creates latency for packets in the TX queues,
including RMU requests, even if the RMU messages don't go to the
external ports. And even with a high skb->priority, you'd still need PFC
to avoid this problem. This can trip up the timeout timers we have for
RMU responses.

> This is one area we can experiment with. Maybe we can retry the
> operation via RMU a few times? Two retries for MIBs is still going to
> be a lot faster, if successful, compared to all the MDIO transactions
> for all the statistics. We can also add some fall back tracking
> logic. If RMU has failed for N times in a row, stop using it for 60
> seconds, etc. That might be something we can put into the DSA core,
> since it seems like a generic problem.

Or the driver might have a worker which periodically sends the GetID
message and tracks whether the switch responded. Maybe the rescheduling
intervals of that are dynamically adjusted based on feedback from
timeouts or successes of register reads/writes. In any case, now we're
starting to talk about really complex logic. And it's not clear how
effective any of these mechanisms would be against random and sporadic
timeouts rather than persistent issues.

Re: [PATCH net-next v14 5/7] net: dsa: mv88e6xxx: rmu: Add functionality to get RMON

[Andrew Lunn]

> > I was thinking within mv88e6xxx_read() and mv88e6xxx_write(). Keep a
> > buffer for building requests. Each write call appends the write to the
> > buffer and returns 0. A read call gets appended to the buffer and then
> > executes the RMU. We probably also need to wrap the reg mutex, so that
> > when it is released, any buffered writes get executed. If the RMU
> > fails, we have all the information needed to do the same via MDIO.
> 
> Ah, so you want to make the mv88e6xxx_reg_unlock() become an implicit
> write barrier.

I'm still thinking this through. It probably needs real code to get
all the details sorted out. The locking could be interesting...  We
need something to flush the queue before we exit from the driver, and
that seems like the obvious synchronisation point. If not that, we
need to add another function call at all the exit points.

> That could work, but the trouble seems to be error propagation.
> mv88e6xxx_write() will always return 0, the operation will be delayed
> until the unlock, and mv88e6xxx_reg_unlock() does not return an error
> code (why would it?).

If the RMU fails and the fallback MDIO also fails, we are in big
trouble, and the switch is probably dead. At which point, do we really
care. netdev_ratelimited_err('Switch has died...') and keep going,
everything afterwards is probably going to go wrong as well.

I don't think there are any instances in the driver where we try to
recover from an MDIO write failure, other than return -ETIMEDOUT or
-EIO or whatever.

> Or the driver might have a worker which periodically sends the GetID
> message and tracks whether the switch responded. Maybe the rescheduling
> intervals of that are dynamically adjusted based on feedback from
> timeouts or successes of register reads/writes. In any case, now we're
> starting to talk about really complex logic. And it's not clear how
> effective any of these mechanisms would be against random and sporadic
> timeouts rather than persistent issues.

I don't think we can assume every switch has an equivalent of
GetID. So a solution like this would have to be per driver. Given the
potential complexity, it would probably be better to have it in the
core, everybody shares it, debugs it, and makes sure it works well.

      Andrew

[PATCH net-next v14 6/7] net: dsa: mv88e6xxx: rmon: Use RMU for reading RMON data

[Mattias Forsblad]

Redirect calls for collecting the RMON counters to RMU
instead of over legacy MDIO.

Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 12 +++++++++---
 drivers/net/dsa/mv88e6xxx/chip.h |  2 ++
 drivers/net/dsa/mv88e6xxx/smi.c  |  3 +++
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5b22fe4b3284..860896849785 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1386,10 +1386,9 @@ 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)
+void mv88e6xxx_get_ethtool_stats_mdio(struct mv88e6xxx_chip *chip, int port,
+				      uint64_t *data)
 {
-	struct mv88e6xxx_chip *chip = ds->priv;
 	int ret;
 
 	mv88e6xxx_reg_lock(chip);
@@ -1401,7 +1400,14 @@ 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;
 
+	chip->smi_ops->get_rmon(chip, port, data);
 }
 
 static int mv88e6xxx_get_regs_len(struct dsa_switch *ds, int port)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index aca7cfef196e..7b692ba688f7 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -811,6 +811,8 @@ int mv88e6xxx_wait_bit(struct mv88e6xxx_chip *chip, int addr, int reg,
 		       int bit, int val);
 struct mii_bus *mv88e6xxx_default_mdio_bus(struct mv88e6xxx_chip *chip);
 
+void mv88e6xxx_get_ethtool_stats_mdio(struct mv88e6xxx_chip *chip, int port,
+				      uint64_t *data);
 static inline void mv88e6xxx_reg_lock(struct mv88e6xxx_chip *chip)
 {
 	mutex_lock(&chip->reg_lock);
diff --git a/drivers/net/dsa/mv88e6xxx/smi.c b/drivers/net/dsa/mv88e6xxx/smi.c
index a990271b7482..ae805c449b85 100644
--- a/drivers/net/dsa/mv88e6xxx/smi.c
+++ b/drivers/net/dsa/mv88e6xxx/smi.c
@@ -83,6 +83,7 @@ static int mv88e6xxx_smi_direct_wait(struct mv88e6xxx_chip *chip,
 static const struct mv88e6xxx_bus_ops mv88e6xxx_smi_direct_ops = {
 	.read = mv88e6xxx_smi_direct_read,
 	.write = mv88e6xxx_smi_direct_write,
+	.get_rmon = mv88e6xxx_get_ethtool_stats_mdio,
 };
 
 static int mv88e6xxx_smi_dual_direct_read(struct mv88e6xxx_chip *chip,
@@ -100,6 +101,7 @@ static int mv88e6xxx_smi_dual_direct_write(struct mv88e6xxx_chip *chip,
 static const struct mv88e6xxx_bus_ops mv88e6xxx_smi_dual_direct_ops = {
 	.read = mv88e6xxx_smi_dual_direct_read,
 	.write = mv88e6xxx_smi_dual_direct_write,
+	.get_rmon = mv88e6xxx_get_ethtool_stats_mdio,
 };
 
 /* Offset 0x00: SMI Command Register
@@ -166,6 +168,7 @@ static const struct mv88e6xxx_bus_ops mv88e6xxx_smi_indirect_ops = {
 	.read = mv88e6xxx_smi_indirect_read,
 	.write = mv88e6xxx_smi_indirect_write,
 	.init = mv88e6xxx_smi_indirect_init,
+	.get_rmon = mv88e6xxx_get_ethtool_stats_mdio,
 };
 
 int mv88e6xxx_smi_init(struct mv88e6xxx_chip *chip,
-- 
2.25.1

[PATCH net-next v14 7/7] net: dsa: qca8k: Use new convenience functions

[Mattias Forsblad]

Use the new common convenience functions for sending and
waiting for frames.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 68 +++++++++++---------------------
 1 file changed, 24 insertions(+), 44 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index c181346388a4..a4ec0d0e608d 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -160,7 +160,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 			       QCA_HDR_MGMT_DATA2_LEN);
 	}
 
-	complete(&mgmt_eth_data->rw_done);
+	dsa_switch_inband_complete(ds, &mgmt_eth_data->rw_done);
 }
 
 static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
@@ -228,6 +228,7 @@ static void qca8k_mdio_header_fill_seq_num(struct sk_buff *skb, u32 seq_num)
 static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
+	struct dsa_switch *ds = priv->ds;
 	struct sk_buff *skb;
 	bool ack;
 	int ret;
@@ -248,17 +249,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the mdio pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
+				   QCA8K_ETHERNET_TIMEOUT);
 
 	*val = mgmt_eth_data->data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -280,6 +277,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 {
 	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
+	struct dsa_switch *ds = priv->ds;
 	struct sk_buff *skb;
 	bool ack;
 	int ret;
@@ -300,17 +298,13 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the mdio pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
+				   QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -441,24 +435,22 @@ static struct regmap_config qca8k_regmap_config = {
 };
 
 static int
-qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
+qca8k_phy_eth_busy_wait(struct qca8k_priv *priv,
 			struct sk_buff *read_skb, u32 *val)
 {
+	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
+	struct dsa_switch *ds = priv->ds;
 	bool ack;
 	int ret;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the copy pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(skb);
-
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
+				   QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -480,6 +472,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	struct sk_buff *write_skb, *clear_skb, *read_skb;
 	struct qca8k_mgmt_eth_data *mgmt_eth_data;
 	u32 write_val, clear_val = 0, val;
+	struct dsa_switch *ds = priv->ds;
 	struct net_device *mgmt_master;
 	int ret, ret1;
 	bool ack;
@@ -540,17 +533,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_master;
 	write_skb->dev = mgmt_master;
 
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the write pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(write_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(write_skb);
-
-	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-					  QCA8K_ETHERNET_TIMEOUT);
+	ret = dsa_switch_inband_tx(ds, write_skb, &mgmt_eth_data->rw_done,
+				   QCA8K_ETHERNET_TIMEOUT);
 
 	ack = mgmt_eth_data->ack;
 
@@ -569,7 +558,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
 				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
 				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
-				mgmt_eth_data, read_skb, &val);
+				priv, read_skb, &val);
 
 	if (ret < 0 && ret1 < 0) {
 		ret = ret1;
@@ -577,17 +566,14 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		reinit_completion(&mgmt_eth_data->rw_done);
-
 		/* Increment seq_num and set it in the read pkt */
 		mgmt_eth_data->seq++;
 		qca8k_mdio_header_fill_seq_num(read_skb, mgmt_eth_data->seq);
 		mgmt_eth_data->ack = false;
 
-		dev_queue_xmit(read_skb);
-
-		ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-						  QCA8K_ETHERNET_TIMEOUT);
+		ret = dsa_switch_inband_tx(ds, read_skb,
+					   &mgmt_eth_data->rw_done,
+					   QCA8K_ETHERNET_TIMEOUT);
 
 		ack = mgmt_eth_data->ack;
 
@@ -606,17 +592,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	reinit_completion(&mgmt_eth_data->rw_done);
-
 	/* Increment seq_num and set it in the clear pkt */
 	mgmt_eth_data->seq++;
 	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
 	mgmt_eth_data->ack = false;
 
-	dev_queue_xmit(clear_skb);
-
-	wait_for_completion_timeout(&mgmt_eth_data->rw_done,
-				    QCA8K_ETHERNET_TIMEOUT);
+	dsa_switch_inband_tx(ds, clear_skb, &mgmt_eth_data->rw_done,
+			     QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 
@@ -1528,7 +1510,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
 exit:
 	/* Complete on receiving all the mib packet */
 	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
-		complete(&mib_eth_data->rw_done);
+		dsa_switch_inband_complete(ds, &mib_eth_data->rw_done);
 }
 
 static int
@@ -1543,8 +1525,6 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 
 	mutex_lock(&mib_eth_data->mutex);
 
-	reinit_completion(&mib_eth_data->rw_done);
-
 	mib_eth_data->req_port = dp->index;
 	mib_eth_data->data = data;
 	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
@@ -1562,8 +1542,8 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	if (ret)
 		goto exit;
 
-	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
-
+	ret = dsa_switch_inband_tx(ds, NULL, &mib_eth_data->rw_done,
+				   QCA8K_ETHERNET_TIMEOUT);
 exit:
 	mutex_unlock(&mib_eth_data->mutex);
 
-- 
2.25.1

Re: [PATCH net-next v14 7/7] net: dsa: qca8k: Use new convenience functions

[Christian Marangi]

On Mon, Sep 19, 2022 at 01:08:47PM +0200, Mattias Forsblad wrote:
> Use the new common convenience functions for sending and
> waiting for frames.
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>

Tested-by: Christian Marangi <ansuelsmth@gmail.com>

> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c | 68 +++++++++++---------------------
>  1 file changed, 24 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index c181346388a4..a4ec0d0e608d 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -160,7 +160,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  			       QCA_HDR_MGMT_DATA2_LEN);
>  	}
>  
> -	complete(&mgmt_eth_data->rw_done);
> +	dsa_switch_inband_complete(ds, &mgmt_eth_data->rw_done);
>  }
>  
>  static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> @@ -228,6 +228,7 @@ static void qca8k_mdio_header_fill_seq_num(struct sk_buff *skb, u32 seq_num)
>  static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
> +	struct dsa_switch *ds = priv->ds;
>  	struct sk_buff *skb;
>  	bool ack;
>  	int ret;
> @@ -248,17 +249,13 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	skb->dev = priv->mgmt_master;
>  
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the mdio pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(skb);
> -
> -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
> +				   QCA8K_ETHERNET_TIMEOUT);
>  
>  	*val = mgmt_eth_data->data[0];
>  	if (len > QCA_HDR_MGMT_DATA1_LEN)
> @@ -280,6 +277,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
> +	struct dsa_switch *ds = priv->ds;
>  	struct sk_buff *skb;
>  	bool ack;
>  	int ret;
> @@ -300,17 +298,13 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	skb->dev = priv->mgmt_master;
>  
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the mdio pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(skb);
> -
> -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
> +				   QCA8K_ETHERNET_TIMEOUT);
>  
>  	ack = mgmt_eth_data->ack;
>  
> @@ -441,24 +435,22 @@ static struct regmap_config qca8k_regmap_config = {
>  };
>  
>  static int
> -qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
> +qca8k_phy_eth_busy_wait(struct qca8k_priv *priv,
>  			struct sk_buff *read_skb, u32 *val)
>  {
> +	struct qca8k_mgmt_eth_data *mgmt_eth_data = &priv->mgmt_eth_data;
>  	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
> +	struct dsa_switch *ds = priv->ds;
>  	bool ack;
>  	int ret;
>  
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the copy pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(skb);
> -
> -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -					  QCA8K_ETHERNET_TIMEOUT);
> +	ret = dsa_switch_inband_tx(ds, skb, &mgmt_eth_data->rw_done,
> +				   QCA8K_ETHERNET_TIMEOUT);
>  
>  	ack = mgmt_eth_data->ack;
>  
> @@ -480,6 +472,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	struct sk_buff *write_skb, *clear_skb, *read_skb;
>  	struct qca8k_mgmt_eth_data *mgmt_eth_data;
>  	u32 write_val, clear_val = 0, val;
> +	struct dsa_switch *ds = priv->ds;
>  	struct net_device *mgmt_master;
>  	int ret, ret1;
>  	bool ack;
> @@ -540,17 +533,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	clear_skb->dev = mgmt_master;
>  	write_skb->dev = mgmt_master;
>  
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the write pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(write_skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(write_skb);
> -
> -	ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -					  QCA8K_ETHERNET_TIMEOUT);
> +	ret = dsa_switch_inband_tx(ds, write_skb, &mgmt_eth_data->rw_done,
> +				   QCA8K_ETHERNET_TIMEOUT);
>  
>  	ack = mgmt_eth_data->ack;
>  
> @@ -569,7 +558,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	ret = read_poll_timeout(qca8k_phy_eth_busy_wait, ret1,
>  				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
>  				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
> -				mgmt_eth_data, read_skb, &val);
> +				priv, read_skb, &val);
>  
>  	if (ret < 0 && ret1 < 0) {
>  		ret = ret1;
> @@ -577,17 +566,14 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  	}
>  
>  	if (read) {
> -		reinit_completion(&mgmt_eth_data->rw_done);
> -
>  		/* Increment seq_num and set it in the read pkt */
>  		mgmt_eth_data->seq++;
>  		qca8k_mdio_header_fill_seq_num(read_skb, mgmt_eth_data->seq);
>  		mgmt_eth_data->ack = false;
>  
> -		dev_queue_xmit(read_skb);
> -
> -		ret = wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -						  QCA8K_ETHERNET_TIMEOUT);
> +		ret = dsa_switch_inband_tx(ds, read_skb,
> +					   &mgmt_eth_data->rw_done,
> +					   QCA8K_ETHERNET_TIMEOUT);
>  
>  		ack = mgmt_eth_data->ack;
>  
> @@ -606,17 +592,13 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
>  		kfree_skb(read_skb);
>  	}
>  exit:
> -	reinit_completion(&mgmt_eth_data->rw_done);
> -
>  	/* Increment seq_num and set it in the clear pkt */
>  	mgmt_eth_data->seq++;
>  	qca8k_mdio_header_fill_seq_num(clear_skb, mgmt_eth_data->seq);
>  	mgmt_eth_data->ack = false;
>  
> -	dev_queue_xmit(clear_skb);
> -
> -	wait_for_completion_timeout(&mgmt_eth_data->rw_done,
> -				    QCA8K_ETHERNET_TIMEOUT);
> +	dsa_switch_inband_tx(ds, clear_skb, &mgmt_eth_data->rw_done,
> +			     QCA8K_ETHERNET_TIMEOUT);
>  
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  
> @@ -1528,7 +1510,7 @@ static void qca8k_mib_autocast_handler(struct dsa_switch *ds, struct sk_buff *sk
>  exit:
>  	/* Complete on receiving all the mib packet */
>  	if (refcount_dec_and_test(&mib_eth_data->port_parsed))
> -		complete(&mib_eth_data->rw_done);
> +		dsa_switch_inband_complete(ds, &mib_eth_data->rw_done);
>  }
>  
>  static int
> @@ -1543,8 +1525,6 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
>  
>  	mutex_lock(&mib_eth_data->mutex);
>  
> -	reinit_completion(&mib_eth_data->rw_done);
> -
>  	mib_eth_data->req_port = dp->index;
>  	mib_eth_data->data = data;
>  	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
> @@ -1562,8 +1542,8 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
>  	if (ret)
>  		goto exit;
>  
> -	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
> -
> +	ret = dsa_switch_inband_tx(ds, NULL, &mib_eth_data->rw_done,
> +				   QCA8K_ETHERNET_TIMEOUT);
>  exit:
>  	mutex_unlock(&mib_eth_data->mutex);
>  
> -- 
> 2.25.1
> 

-- 
	Ansuel