[PATCH net-next 00/12] net: dsa: support for inband management of switches

[PATCH net-next 00/12] net: dsa: support for inband management of switches

[Luke Howard]

This patch set shephards a series of commits made by Andrew Lunn
and others to support inband management of DSA switches. [1]

Specifically, it allows the switch itself to be managed using
DSA tagged frames instead of MDIO. MDIO is typically required
to enable inband management. An implementation is included for
the qca8k driver.

This is patch set one of four: the remaining sets add support
for the Remote Management Unit (RMU) in the mv88e6xxx, and then
general and mv88e6xxx support for "inband-only" mode where MDIO
is unavailable on the hardware.

Note: the patch to validate the source trunk against lags_len
was previously submitted and rejected, but I am including it as
the issue has been triggered when running in inband mode.

[1] https://github.com/lunn/linux/tree/v6.15-rc2-next-next-inband

Signed-off-by: Luke Howard <lukeh@padl.com>
---
Andrew Lunn (10):
      net: dsa: qca8k: Move register access 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: Drop replies with wrong sequence numbers
      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: Update error handling
      net: dsa: qca8k: Move inband mutex into DSA core
      net: dsa: Add helper to find ds_switch by index

Christian Marangi (1):
      net: dsa: qca8k: drop redundant mgmt_eth_data

Luke Howard (1):
      net: dsa: validate source trunk against lags_len

 drivers/net/dsa/qca/qca8k-8xxx.c   | 238 +++++++++----------------------------
 drivers/net/dsa/qca/qca8k-common.c |   2 +-
 drivers/net/dsa/qca/qca8k.h        |  12 +-
 include/net/dsa.h                  |  36 ++++++
 net/dsa/dsa.c                      | 109 +++++++++++++++++
 net/dsa/tag.h                      |  14 +++
 6 files changed, 220 insertions(+), 191 deletions(-)
---
base-commit: 2bb62a85aff6d4c14a62a476dfabaada3c1cc014
change-id: 20260612-net-next-dsa-rmu-5cbb0f10ab8b

Best regards,
--  
Luke Howard <lukeh@padl.com>

[PATCH net-next 01/12] net: dsa: qca8k: Move register access completion into DSA core

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

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.

The MIB access via left untouched because it is not a simple
request/reply protocol.

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

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4c928983b8623..fe6fb69b6f92f 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -212,7 +212,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 		}
 	}
 
-	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,
@@ -332,8 +332,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	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);
@@ -341,8 +339,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)
@@ -384,8 +382,6 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	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);
@@ -393,8 +389,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;
 
@@ -594,8 +590,6 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	if (!skb)
 		return -ENOMEM;
 
-	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);
@@ -603,8 +597,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;
 
@@ -696,8 +690,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_conduit;
 	write_skb->dev = mgmt_conduit;
 
-	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);
@@ -705,8 +697,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;
 
@@ -733,8 +725,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);
@@ -742,8 +732,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;
 
@@ -762,8 +752,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);
@@ -771,8 +759,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);
 	mutex_unlock(&priv->bus->mdio_lock);
@@ -1728,7 +1716,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 = wait_for_completion_timeout(&mib_eth_data->rw_done,
+					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
 
 exit:
 	mutex_unlock(&mib_eth_data->mutex);
@@ -2105,7 +2094,7 @@ 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);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 9563388930325..938f2c9ff0cac 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -16,7 +16,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
@@ -392,7 +392,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;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8c16ef23cc102..9b0c109b3058a 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>
@@ -1347,6 +1348,17 @@ int dsa_port_simple_hsr_join(struct dsa_switch *ds, int port,
 int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
 			      struct net_device *hsr);
 
+/* 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 9cb732f6b1e3e..dc4e5cdb2f5b2 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1834,6 +1834,28 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
 
+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 const struct dsa_stubs __dsa_stubs = {
 	.conduit_hwtstamp_validate = __dsa_conduit_hwtstamp_validate,
 };

-- 
2.43.0

Re: [PATCH net-next 01/12] net: dsa: qca8k: Move register access completion into DSA core

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: qca8k: Move register access completion into DSA core

This patch factors the request/reply completion used by the qca8k
mgmt-Ethernet path into a new dsa_inband helper in the DSA core, moves
the reinit_completion() and msecs_to_jiffies() conversion into the
helper, and switches every qca8k mgmt-eth call site to use it.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 4c928983b862..fe6fb69b6f92 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -332,8 +332,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	skb->dev = priv->mgmt_conduit;
>  
> -	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);
> @@ -341,8 +339,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);

[High]
Could this reorder introduce a race between the sender and the ack
handler?

Before the patch every mgmt-eth call site did:

    reinit_completion(&mgmt_eth_data->rw_done);
    ...
    dev_queue_xmit(skb);
    wait_for_completion_timeout(&mgmt_eth_data->rw_done, ...);

so the completion was armed before the frame could produce a reply.

After the patch each call site does:

    dev_queue_xmit(skb);
    dsa_inband_wait_for_completion(&mgmt_eth_data->inband, ...);

and dsa_inband_wait_for_completion() itself does:

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

so reinit_completion() now runs after dev_queue_xmit().

The reply is delivered from the RX softirq path via
qca8k_rw_reg_ack_handler() -> dsa_inband_complete() -> complete(), with
no mutex synchronising it against the sender. If the reply is processed
in the window between dev_queue_xmit() returning and the reinit inside
the helper, done is set to 1 by the ack handler and then cleared back to
0 by reinit_completion(), and wait_for_completion_timeout() then blocks
for the full QCA8K_ETHERNET_TIMEOUT and returns 0.

Callers then report -ETIMEDOUT even though the reply arrived. Every
mgmt-eth caller looks affected: qca8k_read_eth(), qca8k_write_eth(),
qca8k_phy_eth_busy_wait() and both branches plus the exit path of
qca8k_phy_eth_command().

The next patch in the series ("net: dsa: qca8K: Move queuing for request
frame into the core", 98b48914b271) introduces dsa_inband_request() and
its comment/commit message state that reinit_completion() must precede
dev_queue_xmit() to avoid this race. Should the reinit be moved back
before dev_queue_xmit() in this patch, or should this patch be squashed
with the follow-up, to avoid a bisect window where mgmt-eth reads and
writes can spuriously fail?

> @@ -384,8 +382,6 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  
>  	skb->dev = priv->mgmt_conduit;
>  
> -	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);
> @@ -393,8 +389,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);

[ ... ]

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 8c16ef23cc10..9b0c109b3058 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1347,6 +1348,17 @@ int dsa_port_simple_hsr_join(struct dsa_switch *ds, int port,
>  int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
>  			      struct net_device *hsr);
>  
> +/* 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);

[Low]
Should the return type of dsa_inband_wait_for_completion() be
unsigned long to match wait_for_completion_timeout(), and should
timeout_ms be unsigned int?

The current declaration narrows the wait_for_completion_timeout() return
(unsigned long jiffies remaining) to int, and a negative timeout_ms
would be interpreted by msecs_to_jiffies() as MAX_JIFFY_OFFSET. qca8k
callers only check ret <= 0 at a 5 ms timeout so the truncation is
invisible today, but this helper is EXPORT_SYMBOL_GPL and intended as a
pattern for further DSA drivers, so fixing the signature later would be
an uAPI-ish change to a core API. Would a documented bool "timed_out"
return also work, since callers only care about zero vs non-zero?

> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 9cb732f6b1e3..dc4e5cdb2f5b 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -1834,6 +1834,28 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
>  }
>  EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
>  
> +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);

[Low]
The local variable is named jiffies, which shadows the global kernel
symbol jiffies from <linux/jiffies.h>. There is no runtime effect today
since the function does not reference the global, but any later addition
to this function that types "jiffies" would silently bind to the local
timeout value. Would renaming to timeout_j or timeout_jiffies be
preferable, especially given the helper is exported and likely to be
copied by other DSA drivers?

[PATCH net-next 02/12] net: dsa: qca8K: Move queuing for request frame into the core

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

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                    | 31 +++++++++++++++++++++++++++++++
 3 files changed, 43 insertions(+), 22 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index fe6fb69b6f92f..4da6094552f1e 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -337,10 +337,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)
@@ -387,10 +385,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;
 
@@ -595,10 +591,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;
 
@@ -695,10 +689,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;
 
@@ -730,10 +722,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;
 
@@ -757,8 +747,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 9b0c109b3058a..6b5aeb99ec3bb 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1357,6 +1357,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 dc4e5cdb2f5b2..534e391fac7b5 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1856,6 +1856,37 @@ int dsa_inband_wait_for_completion(struct dsa_inband *inband, int timeout_ms)
 }
 EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
 
+/* dsa_inband_request - send an inband request frame and wait for the reply.
+ * @inband: inband state for the switch
+ * @skb: request frame; ownership is transferred to this function
+ * @insert_seqno: optional callback to stamp the sequence number into @skb
+ * @resp: buffer to receive the reply payload, or NULL if none is expected
+ * @resp_len: size of @resp in bytes
+ * @timeout_ms: how long to wait for the reply, in milliseconds
+ *
+ * Serialise against other inband operations, transmit @skb and wait for the
+ * matching reply handed back via dsa_inband_complete().
+ *
+ * Return the number of response bytes copied into @resp (0 when no response
+ * is expected) on success, or a negative errno (-EOPNOTSUPP if the conduit
+ * is down, -ETIMEDOUT if no reply arrived, or an error from the completer).
+ *
+ * Cannot use dsa_inband_wait_for_completion() since the completion needs to
+ * be reinitialised before the skb is queued, 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 const struct dsa_stubs __dsa_stubs = {
 	.conduit_hwtstamp_validate = __dsa_conduit_hwtstamp_validate,
 };

-- 
2.43.0

Re: [PATCH net-next 02/12] net: dsa: qca8K: Move queuing for request frame into the core

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: qca8K: Move queuing for request frame into the core

This patch introduces dsa_inband_request() in the DSA core which
reinitialises the completion, transmits the skb via dev_queue_xmit(),
and waits for a reply. The qca8k driver is converted to use this new
helper in place of the open-coded dev_queue_xmit() plus
dsa_inband_wait_for_completion() sequence.

[Low]
The subject line uses qca8K with a capital K, which is inconsistent
with qca8k as used everywhere else in the driver and in the rest of
the series.

The commit body refers to functions that do not exist:

    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.

The introduced symbol is dsa_inband_request(), and the preserved
symbol is dsa_inband_wait_for_completion(); should the message refer
to those names so that git log --grep for dsa_inband_request finds
this commit?

There are also a couple of small grammar issues in the same paragraph
("It is also no possible" and "the schedule at decided to do other
things").

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index fe6fb69b6f92..4da6094552f1 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c

[ ... ]

> @@ -757,8 +747,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);

[High]
The other five hunks replace dev_queue_xmit() plus
dsa_inband_wait_for_completion() with a single dsa_inband_request()
call. This hunk deletes dev_queue_xmit(clear_skb) but leaves the bare
dsa_inband_wait_for_completion() in place, so no packet is ever sent
for the clear step.

A few consequences follow from that:

Does this leak clear_skb on every path through qca8k_phy_eth_command()
that reaches the exit: label? The skb is allocated by
qca8k_alloc_mdio_header() and, after this change, nothing ever hands
it to dev_queue_xmit() or frees it.

Does every PHY read/write through this driver now stall for
QCA8K_ETHERNET_TIMEOUT (200ms) at the exit: label, since the
completion is reinitialised but no request is ever queued for the
switch to reply to?

Does this also skip resetting the switch's MDIO master, which was the
purpose of the clear_skb step?

The next patch in the series, 68baa3a653c1 ("net: dsa: qca8k:
dsa_inband_request: More normal return values"), replaces the
dsa_inband_wait_for_completion() call with

    dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
                       QCA8K_ETHERNET_TIMEOUT);

Would it be worth squashing or reordering so that bisection landing
on this commit does not see hangs and skb leaks on every management
frame PHY access?

> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 9b0c109b3058..6b5aeb99ec3b 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1357,6 +1357,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);

[ ... ]

[PATCH net-next 03/12] net: dsa: qca8k: dsa_inband_request: More normal return values

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

wait_for_completion_timeout() has unusual return values. 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 a timeout.
Massage the return value to fit this, and modify the callers to the
more usual pattern of ret < 0 is an error.

Sending the clear message is expected to fail, so don't check the
return value, and add a comment about this.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
Remove check on the clear message.
wait_for_completion_timeout() does not return negative values
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 24 +++++++++++-------------
 net/dsa/dsa.c                    |  6 +++++-
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 4da6094552f1e..b0cbe72c15b4c 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -348,8 +348,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;
@@ -392,8 +392,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;
@@ -596,8 +596,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;
@@ -694,8 +694,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;
 	}
@@ -727,10 +726,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;
@@ -747,8 +744,9 @@ 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);
+	/* This is expected to fail sometimes, so don't check return value. */
+	dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+			   QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
 	mutex_unlock(&priv->bus->mdio_lock);
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 534e391fac7b5..f6ae11b06b4ab 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1878,12 +1878,16 @@ 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 -ETIMEDOUT;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_request);
 

-- 
2.43.0

[PATCH net-next 04/12] net: dsa: qca8k: Drop replies with wrong sequence numbers

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

A response with the wrong sequence number is likely to be a late
arriving responses, which the driver has already given up waiting for.
Drop it rather than signalling the complete. If the complete was
signalled, this late response could take the place of the genuine
reply which is soon to follow.

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

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index b0cbe72c15b4c..3b3fe96016176 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -185,9 +185,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	/* We can ignore odd value, we always round up them in the alloc function. */
 	len *= sizeof(u16);
 
-	/* Make sure the seq match the requested packet */
-	if (get_unaligned_le32(&mgmt_ethhdr->seq) == mgmt_eth_data->seq)
-		mgmt_eth_data->ack = true;
+	/* Make sure the seq match the requested packet. If not, drop. */
+	if (get_unaligned_le32(&mgmt_ethhdr->seq) != mgmt_eth_data->seq)
+		return;
 
 	if (cmd == MDIO_READ) {
 		u32 *val = mgmt_eth_data->data;

-- 
2.43.0

Re: [PATCH net-next 04/12] net: dsa: qca8k: Drop replies with wrong sequence numbers

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
Subject: net: dsa: qca8k: Drop replies with wrong sequence numbers

The change makes qca8k_rw_reg_ack_handler() return early when the reply
sequence number does not match, rather than only setting ack on match.
The intent is to avoid signalling completion for a late reply that could
be mistaken for the next genuine reply.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index b0cbe72c15b4..3b3fe9601617 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -185,9 +185,9 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  	/* We can ignore odd value, we always round up them in the alloc function. */
>  	len *= sizeof(u16);
>  
> -	/* Make sure the seq match the requested packet */
> -	if (get_unaligned_le32(&mgmt_ethhdr->seq) == mgmt_eth_data->seq)
> -		mgmt_eth_data->ack = true;
> +	/* Make sure the seq match the requested packet. If not, drop. */
> +	if (get_unaligned_le32(&mgmt_ethhdr->seq) != mgmt_eth_data->seq)
> +		return;

[High]
After this change, is mgmt_eth_data->ack ever assigned true anywhere in
the driver?  A grep across drivers/net/dsa/qca/ shows five sites that
initialise it to false before dsa_inband_request(), five sites that read
it afterwards, and no remaining site that ever writes true.

The callers still do, for example in qca8k_read_eth():

	mgmt_eth_data->ack = false;

	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
				 QCA8K_ETHERNET_TIMEOUT);
	...
	ack = mgmt_eth_data->ack;

	mutex_unlock(&mgmt_eth_data->mutex);

	if (ret)
		return ret;

	if (!ack)
		return -EINVAL;

With the assignment gone, does every successful reply now leave ack ==
false and cause qca8k_read_eth(), qca8k_write_eth() and the read/write/
clear paths in qca8k_phy_eth_command() to return -EINVAL on their fast
path?

The higher-level callers (qca8k_bulk_read(), qca8k_bulk_write(),
qca8k_regmap_update_bits(), qca8k_internal_mdio_read() and
qca8k_internal_mdio_write()) fall back to the slow MII path on error, so
the device keeps functioning, but is the Ethernet management fast path
effectively disabled between this patch and its follow-up cleanup?

Would it be cleaner to fold the removal of the ack field (and the
matching if (!ack) return -EINVAL checks in every caller) into this same
commit, so that each commit in the series is self-contained and bisects
cleanly?

[PATCH net-next 05/12] net: dsa: qca8k: Move request sequence number handling into core

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

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 | 29 +++++++++--------------------
 drivers/net/dsa/qca/qca8k.h      |  1 -
 include/net/dsa.h                |  6 +++++-
 net/dsa/dsa.c                    | 16 +++++++++++++++-
 4 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 3b3fe96016176..fc5070402188e 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -186,7 +186,8 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	len *= sizeof(u16);
 
 	/* Make sure the seq match the requested packet. If not, drop. */
-	if (get_unaligned_le32(&mgmt_ethhdr->seq) != mgmt_eth_data->seq)
+	if (get_unaligned_le32(&mgmt_ethhdr->seq) !=
+	    dsa_inband_seqno(&mgmt_eth_data->inband))
 		return;
 
 	if (cmd == MDIO_READ) {
@@ -332,12 +333,10 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	/* 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);
 
 	*val = mgmt_eth_data->data[0];
@@ -380,12 +379,10 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	/* 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;
@@ -586,12 +583,10 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	if (!skb)
 		return -ENOMEM;
 
-	/* 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;
@@ -684,12 +679,10 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_conduit;
 	write_skb->dev = mgmt_conduit;
 
-	/* 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;
@@ -716,12 +709,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;
@@ -739,13 +730,11 @@ 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;
 
 	/* This is expected to fail sometimes, so don't check return value. */
 	dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+			   qca8k_mdio_header_fill_seq_num,
 			   QCA8K_ETHERNET_TIMEOUT);
 
 	mutex_unlock(&mgmt_eth_data->mutex);
@@ -2080,7 +2069,7 @@ 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);
 	init_completion(&priv->mib_eth_data.rw_done);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 938f2c9ff0cac..1054ba1c7e590 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -395,7 +395,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 6b5aeb99ec3bb..cdf2487397c2b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1353,13 +1353,17 @@ int dsa_port_simple_hsr_leave(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 f6ae11b06b4ab..98a75ee08f011 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1834,9 +1834,11 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
 }
 EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
 
-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);
 
@@ -1875,11 +1877,17 @@ EXPORT_SYMBOL_GPL(dsa_inband_wait_for_completion);
  * be reinitialised before the skb is queued, 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);
 	int ret;
 
+	if (insert_seqno) {
+		WRITE_ONCE(inband->seqno, inband->seqno + 1);
+		insert_seqno(skb, inband->seqno & inband->seqno_mask);
+	}
+
 	reinit_completion(&inband->completion);
 
 	dev_queue_xmit(skb);
@@ -1891,6 +1899,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 READ_ONCE(inband->seqno) & inband->seqno_mask;
+}
+EXPORT_SYMBOL_GPL(dsa_inband_seqno);
+
 static const struct dsa_stubs __dsa_stubs = {
 	.conduit_hwtstamp_validate = __dsa_conduit_hwtstamp_validate,
 };

-- 
2.43.0

[PATCH net-next 06/12] net: dsa: qca8k: Refactor sequence number mismatch to use error code

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

Replace the boolean that the sequence numbers matches with an error
code. Now that responses with the wrong sequence number are dropped,
this is actually unused in this driver. However, other devices can
perform additional validation of a response with the correct sequence
number and potentially return -EPROTO to indicate some other sort of
error.

The value is only safe to use 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. This intermediate step is a bit ugly, but that all gets cleaned
up in the next patch.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2
-ret -> err
Extended commit message warning the code is ugly
Point out it is not actually used by this driver
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 49 ++++++++++++++++------------------------
 drivers/net/dsa/qca/qca8k.h      |  2 +-
 2 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index fc5070402188e..499cc8ef5ddea 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -314,7 +314,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,
@@ -333,8 +333,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	mgmt_eth_data->ack = false;
-
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 QCA8K_ETHERNET_TIMEOUT);
@@ -343,15 +341,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 err;
 
 	return 0;
 }
@@ -360,7 +358,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,
@@ -379,21 +377,19 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = priv->mgmt_conduit;
 
-	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;
 }
@@ -577,25 +573,23 @@ 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;
 
 	if (!skb)
 		return -ENOMEM;
 
-	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];
 
@@ -611,7 +605,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	u32 write_val, clear_val = 0, val;
 	struct net_device *mgmt_conduit;
 	int ret, ret1;
-	bool ack;
+	int err;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -679,21 +673,19 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_conduit;
 	write_skb->dev = mgmt_conduit;
 
-	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;
 	}
@@ -709,19 +701,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;
 		}
 
@@ -730,7 +720,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		kfree_skb(read_skb);
 	}
 exit:
-	mgmt_eth_data->ack = false;
 
 	/* This is expected to fail sometimes, so don't check return value. */
 	dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 1054ba1c7e590..4c055aef674c2 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -394,7 +394,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.43.0

[PATCH net-next 07/12] net: dsa: qca8k: Pass error code from reply decoder to requester

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

The code which decodes the frame and signals the complete can detect
error within the reply, such as fields have unexpected values. 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.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
v2:

Remove EPROTO if the sequence numbers don't match, drop the reply
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 70 +++++++++++++---------------------------
 drivers/net/dsa/qca/qca8k.h      |  2 --
 include/net/dsa.h                | 13 +++++++-
 net/dsa/dsa.c                    | 33 +++++++++++++++++--
 4 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 499cc8ef5ddea..ffa3b1ba23bed 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -167,6 +167,8 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	struct qca_mgmt_ethhdr *mgmt_ethhdr;
 	u32 command;
 	u8 len, cmd;
+	u32 data[4];
+	int err = 0;
 	int i;
 
 	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
@@ -191,7 +193,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 		return;
 
 	if (cmd == MDIO_READ) {
-		u32 *val = mgmt_eth_data->data;
+		u32 *val = &data[0];
 
 		*val = get_unaligned_le32(&mgmt_ethhdr->mdio_data);
 
@@ -213,7 +215,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 		}
 	}
 
-	dsa_inband_complete(&mgmt_eth_data->inband);
+	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,
@@ -314,7 +316,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;
-	int err;
+	u32 data[4];
 	int ret;
 
 	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
@@ -335,30 +337,25 @@ static int qca8k_read_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,
+				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
+	if (ret < 0)
+		goto out;
 
-	*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);
-
-	err = mgmt_eth_data->err;
+		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
 
+out:
 	mutex_unlock(&mgmt_eth_data->mutex);
 
-	if (ret)
-		return ret;
-
-	if (err)
-		return err;
-
-	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,
@@ -379,19 +376,12 @@ 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);
 
-	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
@@ -573,7 +563,7 @@ 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;
+	u32 data[4];
 	int ret;
 
 	if (!skb)
@@ -581,17 +571,13 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
+				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	err = mgmt_eth_data->err;
-
 	if (ret)
 		return ret;
 
-	if (err)
-		return err;
-
-	*val = mgmt_eth_data->data[0];
+	*val = data[0];
 
 	return 0;
 }
@@ -604,8 +590,8 @@ 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_conduit;
+	u32 resp_data[4];
 	int ret, ret1;
-	int err;
 
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
@@ -675,21 +661,14 @@ 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);
 
-	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,
@@ -703,19 +682,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);
 
-		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;
+		ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
 	} else {
 		kfree_skb(read_skb);
 	}
@@ -724,6 +697,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	/* This is expected to fail sometimes, so don't check return value. */
 	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);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 4c055aef674c2..db89025b4243a 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -394,8 +394,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];
 };
 
 struct qca8k_mib_eth_data {
diff --git a/include/net/dsa.h b/include/net/dsa.h
index cdf2487397c2b..50ac6f0aa2e67 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1350,17 +1350,28 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
 
 /* Perform operations on a switch by sending it request in Ethernet
  * frames and expecting a response in a frame.
+ *
+ * resp_lock protects resp and resp_len to ensure they are consistent.
+ * If there is a thread waiting for the response, resp will point to a
+ * buffer to copy the response to. If the thread has given up waiting,
+ * resp will be a NULL pointer.
  */
 struct dsa_inband {
 	struct completion completion;
 	u32 seqno;
 	u32 seqno_mask;
+	int err;
+	spinlock_t resp_lock;
+	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);
+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 98a75ee08f011..3e480770854ca 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1837,13 +1837,27 @@ EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 {
 	init_completion(&inband->completion);
+	spin_lock_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)
+void dsa_inband_complete(struct dsa_inband *inband,
+			 void *resp, unsigned int resp_len,
+			 int err)
 {
+	inband->err = err;
+
+	if (!err) {
+		spin_lock_bh(&inband->resp_lock);
+		resp_len = min(inband->resp_len, resp_len);
+		if (inband->resp && resp)
+			memcpy(inband->resp, resp, resp_len);
+		spin_unlock_bh(&inband->resp_lock);
+		inband->err = resp_len;
+	}
+
 	complete(&inband->completion);
 }
 EXPORT_SYMBOL_GPL(dsa_inband_complete);
@@ -1878,11 +1892,19 @@ 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);
 	int ret;
 
+	inband->err = 0;
+
+	spin_lock_bh(&inband->resp_lock);
+	inband->resp = resp;
+	inband->resp_len = resp_len;
+	spin_unlock_bh(&inband->resp_lock);
+
 	if (insert_seqno) {
 		WRITE_ONCE(inband->seqno, inband->seqno + 1);
 		insert_seqno(skb, inband->seqno & inband->seqno_mask);
@@ -1893,9 +1915,16 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	dev_queue_xmit(skb);
 
 	ret = wait_for_completion_timeout(&inband->completion, jiffies);
+
+	spin_lock_bh(&inband->resp_lock);
+	inband->resp = NULL;
+	inband->resp_len = 0;
+	spin_unlock_bh(&inband->resp_lock);
+
 	if (ret == 0)
 		return -ETIMEDOUT;
-	return 0;
+
+	return inband->err;
 }
 EXPORT_SYMBOL_GPL(dsa_inband_request);
 

-- 
2.43.0

Re: [PATCH net-next 07/12] net: dsa: qca8k: Pass error code from reply decoder to requester

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: qca8k: Pass error code from reply decoder to requester

This patch changes dsa_inband_complete() to accept a response buffer and
error code, and updates dsa_inband_request() to return that error code to
the caller. The qca8k driver call sites are updated to pass response
buffers through the new API and to use the returned status directly.

> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index 499cc8ef5dde..ffa3b1ba23be 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -167,6 +167,8 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  	struct qca_mgmt_ethhdr *mgmt_ethhdr;
>  	u32 command;
>  	u8 len, cmd;
> +	u32 data[4];
> +	int err = 0;
>  	int i;
>  
>  	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
> @@ -191,7 +193,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  		return;
>  
>  	if (cmd == MDIO_READ) {
> -		u32 *val = mgmt_eth_data->data;
> +		u32 *val = &data[0];
>  
>  		*val = get_unaligned_le32(&mgmt_ethhdr->mdio_data);

[High]
Can the decode loop overflow the on-stack data[4] buffer? The wire-supplied
length in QCA_HDR_MGMT_LENGTH is a 4-bit field with a maximum value of 15;
after this block in qca8k_rw_reg_ack_handler():

    if (len == 15)
        len++;
    len *= sizeof(u16);

len can reach 32. Later the copy loop uses:

    data_len = min_t(int, QCA_HDR_MGMT_DATA2_LEN /* 28 */,
                     len - QCA_HDR_MGMT_DATA1_LEN /* 4 */);
    ...
    for (i = sizeof(u32); i <= data_len; i += sizeof(u32)) {
        *val = get_unaligned_le32(data2);
        val++;
        data2++;
    }

which writes val[1]..val[7] (28 bytes past *val), overrunning the 16-byte
u32 data[4] destination. This overflow existed in the previous storage
(mgmt_eth_data->data[4] on the heap) and this patch relocates the sink onto
the softirq call stack, where the overrun can clobber saved registers or
the return address. The driver advertises .max_raw_read = 32 through its
regmap, so a legitimate 32-byte bulk read is sufficient to reach this
path; a malformed reply is not required. Should the ack handler bound the
decode to sizeof(data)?

> @@ -213,7 +215,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
>  		}
>  	}
>  
> -	dsa_inband_complete(&mgmt_eth_data->inband);
> +	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,
> @@ -314,7 +316,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;
> -	int err;
> +	u32 data[4];
>  	int ret;
>  
>  	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL,
> @@ -335,30 +337,25 @@ static int qca8k_read_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,
> +				 data, sizeof(data),
>  				 QCA8K_ETHERNET_TIMEOUT);
> +	if (ret < 0)
> +		goto out;
>  
> -	*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);
> -
> -	err = mgmt_eth_data->err;
> +		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
>  
> +out:
>  	mutex_unlock(&mgmt_eth_data->mutex);
>  
> -	if (ret)
> -		return ret;
> -
> -	if (err)
> -		return err;
> -
> -	return 0;
> +	return ret;
>  }

[Critical]
Does qca8k_read_eth() now return a positive value on success? Looking at
dsa_inband_complete() and dsa_inband_request() in net/dsa/dsa.c after this
patch:

net/dsa/dsa.c:dsa_inband_complete() {
    ...
    if (!err) {
        spin_lock_bh(&inband->resp_lock);
        resp_len = min(inband->resp_len, resp_len);
        if (inband->resp && resp)
            memcpy(inband->resp, resp, resp_len);
        spin_unlock_bh(&inband->resp_lock);
        inband->err = resp_len;
    }
    ...
}

net/dsa/dsa.c:dsa_inband_request() {
    ...
    return inband->err;
}

so a successful read returns sizeof(data) = 16, not 0. qca8k_read_eth()
propagates that positive value to its callers, but qca8k_regmap_update_bits_eth()
still uses:

    ret = qca8k_read_eth(priv, reg, &val, sizeof(val));
    if (ret)
        return ret;

and qca8k_bulk_read() uses:

    if (priv->mgmt_conduit && !qca8k_read_eth(...))
        return 0;

Would qca8k_regmap_update_bits_eth() then return 16 as an error to regmap,
and would qca8k_bulk_read() unconditionally fall through to the MII path
even after a successful ethernet read?

>  static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
>  {

[ ... ]

> @@ -573,7 +563,7 @@ 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;
> +	u32 data[4];
>  	int ret;
>  
>  	if (!skb)
> @@ -581,17 +571,13 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
>  
>  	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
>  				 qca8k_mdio_header_fill_seq_num,
> +				 data, sizeof(data),
>  				 QCA8K_ETHERNET_TIMEOUT);
>  
> -	err = mgmt_eth_data->err;
> -
>  	if (ret)
>  		return ret;
>  
> -	if (err)
> -		return err;
> -
> -	*val = mgmt_eth_data->data[0];
> +	*val = data[0];
>  
>  	return 0;
>  }

[Critical]
With the new positive-on-success return from dsa_inband_request(), does
qca8k_phy_eth_busy_wait() now return before writing *val on every
successful transfer?

    ret = dsa_inband_request(..., data, sizeof(data), ...);
    if (ret)
        return ret;
    *val = data[0];

ret is 16 on success, so the function returns without touching *val. The
caller in qca8k_phy_eth_command() then feeds an uninitialised local into
read_poll_timeout():

drivers/net/dsa/qca/qca8k-8xxx.c:qca8k_phy_eth_command() {
    ...
    u32 write_val, clear_val = 0, val;
    ...
    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);
    ...
}

Does the busy-wait loop then read val uninitialised on the first
iteration?

> @@ -604,8 +590,8 @@ 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_conduit;
> +	u32 resp_data[4];
>  	int ret, ret1;
> -	int err;
>  
>  	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
>  		return -EINVAL;

[ ... ]

> @@ -703,19 +682,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);
>  
> -		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;
> +		ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
>  	} else {
>  		kfree_skb(read_skb);
>  	}

[Critical]
In this read branch dsa_inband_request() returns 16 on a successful
response, so if (ret) goto exit; fires and the code never reaches:

    ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;

qca8k_phy_eth_command() therefore returns 16 (0x10) for every successful
internal PHY read via the ethernet fast path. Is this the intended
behaviour, given that the value is propagated as the register contents up
through qca8k_internal_mdio_read() and qca8k_mdio_read()?

The follow-up commit dacfa5deb518 ("net: dsa: qca8k: Update error
handling") converts each of these if (ret) checks to if (ret < 0) and
adds an explicit ret = 0; in qca8k_read_eth(). Would folding those changes
into this commit make the series bisectable, since between these two
commits every ethernet-fast-path MDIO read returns 16 and
qca8k_phy_eth_busy_wait() leaves its output argument uninitialised?

[PATCH net-next 08/12] net: dsa: qca8k: Update error handling

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

Now that dsa_inband_request() can return positive values for the
length of the received reply, the return value checking needs to
change. This driver only uses short messages, and it is unlikely the
hardware will actually receive messages of less then 64 bytes. So
don't check the message length. However, ensure the return value is
either a negative error code, or 0. Ensure all users of
dsa_inband_request() return either a negative error, or 0 on success,
so as not to change the API.

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

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index ffa3b1ba23bed..94332497902c2 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -341,6 +341,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 				 QCA8K_ETHERNET_TIMEOUT);
 	if (ret < 0)
 		goto out;
+	ret = 0;
 
 	*val = data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
@@ -574,7 +575,7 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	if (ret)
+	if (ret < 0)
 		return ret;
 
 	*val = data[0];
@@ -664,7 +665,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 				 NULL, 0,
 				 QCA8K_ETHERNET_TIMEOUT);
 
-	if (ret) {
+	if (ret < 0) {
 		kfree_skb(read_skb);
 		goto exit;
 	}
@@ -685,7 +686,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 					 resp_data, sizeof(resp_data),
 					 QCA8K_ETHERNET_TIMEOUT);
 
-		if (ret)
+		if (ret < 0)
 			goto exit;
 
 		ret = resp_data[0] & QCA8K_MDIO_MASTER_DATA_MASK;

-- 
2.43.0

[PATCH net-next 09/12] net: dsa: qca8k: Move inband mutex into DSA core

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

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 wired, 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-common.c |  2 +-
 drivers/net/dsa/qca/qca8k.h        |  1 -
 include/net/dsa.h                  |  4 +++
 net/dsa/dsa.c                      |  9 +++++
 5 files changed, 27 insertions(+), 57 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 94332497902c2..14371c3c9a459 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -324,65 +324,39 @@ 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 if the mgmt_conduit if is operational */
-	if (!priv->mgmt_conduit) {
-		kfree_skb(skb);
-		mutex_unlock(&mgmt_eth_data->mutex);
-		return -EINVAL;
-	}
-
-	skb->dev = priv->mgmt_conduit;
+	skb->dev = READ_ONCE(priv->mgmt_conduit);
 
 	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
 	if (ret < 0)
-		goto out;
+		return ret;
 	ret = 0;
 
 	*val = data[0];
 	if (len > QCA_HDR_MGMT_DATA1_LEN)
 		memcpy(val + 1, &data[1], len - QCA_HDR_MGMT_DATA1_LEN);
 
-out:
-	mutex_unlock(&mgmt_eth_data->mutex);
-
-	return ret;
+	return 0;
 }
 
 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);
+	skb->dev = READ_ONCE(priv->mgmt_conduit);
 
-	/* Check if the mgmt_conduit if is operational */
-	if (!priv->mgmt_conduit) {
-		kfree_skb(skb);
-		mutex_unlock(&mgmt_eth_data->mutex);
-		return -EINVAL;
-	}
-
-	skb->dev = priv->mgmt_conduit;
-
-	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
@@ -484,7 +458,7 @@ qca8k_bulk_read(void *ctx, const void *reg_buf, size_t reg_len,
 	struct qca8k_priv *priv = ctx;
 	u32 reg = *(u16 *)reg_buf;
 
-	if (priv->mgmt_conduit &&
+	if (READ_ONCE(priv->mgmt_conduit) &&
 	    !qca8k_read_eth(priv, reg, val_buf, val_len))
 		return 0;
 
@@ -507,7 +481,7 @@ qca8k_bulk_gather_write(void *ctx, const void *reg_buf, size_t reg_len,
 	u32 reg = *(u16 *)reg_buf;
 	u32 *val = (u32 *)val_buf;
 
-	if (priv->mgmt_conduit &&
+	if (READ_ONCE(priv->mgmt_conduit) &&
 	    !qca8k_write_eth(priv, reg, val, val_len))
 		return 0;
 
@@ -645,17 +619,7 @@ 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_conduit is operational */
-	mgmt_conduit = priv->mgmt_conduit;
-	if (!mgmt_conduit) {
-		mutex_unlock(&mgmt_eth_data->mutex);
-		mutex_unlock(&priv->bus->mdio_lock);
-		ret = -EINVAL;
-		goto err_mgmt_conduit;
-	}
-
+	mgmt_conduit = READ_ONCE(priv->mgmt_conduit);
 	read_skb->dev = mgmt_conduit;
 	clear_skb->dev = mgmt_conduit;
 	write_skb->dev = mgmt_conduit;
@@ -701,14 +665,10 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 			   NULL, 0,
 			   QCA8K_ETHERNET_TIMEOUT);
 
-	mutex_unlock(&mgmt_eth_data->mutex);
 	mutex_unlock(&priv->bus->mdio_lock);
 
 	return ret;
 
-	/* Error handling before lock */
-err_mgmt_conduit:
-	kfree_skb(read_skb);
 err_read_skb:
 	kfree_skb(clear_skb);
 err_clear_skb:
@@ -1698,13 +1658,12 @@ qca8k_conduit_change(struct dsa_switch *ds, const struct net_device *conduit,
 	if (dp->index != 0)
 		return;
 
-	mutex_lock(&priv->mgmt_eth_data.mutex);
 	mutex_lock(&priv->mib_eth_data.mutex);
 
-	priv->mgmt_conduit = operational ? (struct net_device *)conduit : NULL;
+	WRITE_ONCE(priv->mgmt_conduit,
+		   operational ? (struct net_device *)conduit : 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,
@@ -2032,7 +1991,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-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 13005f10edb7d..def2c44042d7b 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -499,7 +499,7 @@ void qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 	u32 hi = 0;
 	int ret;
 
-	if (priv->mgmt_conduit && priv->info->ops->autocast_mib &&
+	if (READ_ONCE(priv->mgmt_conduit) && priv->info->ops->autocast_mib &&
 	    priv->info->ops->autocast_mib(ds, port, data) > 0)
 		return;
 
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index db89025b4243a..ccf92c85ccb14 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -393,7 +393,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 50ac6f0aa2e67..af16347a3d331 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -1355,8 +1355,12 @@ int dsa_port_simple_hsr_leave(struct dsa_switch *ds, int port,
  * If there is a thread waiting for the response, resp will point to a
  * buffer to copy the response to. If the thread has given up waiting,
  * resp will be a NULL pointer.
+ *
+ * The lock is used to serialise all inband operations. It also protects
+ * the seqno, which is incremented while holding the lock.
  */
 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 3e480770854ca..2a8d47eb58e13 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -1837,6 +1837,7 @@ EXPORT_SYMBOL_GPL(dsa_port_simple_hsr_leave);
 void dsa_inband_init(struct dsa_inband *inband, u32 seqno_mask)
 {
 	init_completion(&inband->completion);
+	mutex_init(&inband->lock);
 	spin_lock_init(&inband->resp_lock);
 	inband->seqno_mask = seqno_mask;
 	inband->seqno = 0;
@@ -1898,6 +1899,13 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	unsigned long jiffies = msecs_to_jiffies(timeout_ms);
 	int ret;
 
+	if (!skb->dev) {
+		kfree_skb(skb);
+		return -EOPNOTSUPP;
+	}
+
+	mutex_lock(&inband->lock);
+
 	inband->err = 0;
 
 	spin_lock_bh(&inband->resp_lock);
@@ -1920,6 +1928,7 @@ int dsa_inband_request(struct dsa_inband *inband, struct sk_buff *skb,
 	inband->resp = NULL;
 	inband->resp_len = 0;
 	spin_unlock_bh(&inband->resp_lock);
+	mutex_unlock(&inband->lock);
 
 	if (ret == 0)
 		return -ETIMEDOUT;

-- 
2.43.0

[PATCH net-next 10/12] net: dsa: qca8k: drop redundant mgmt_eth_data

[Luke Howard]

From: Christian Marangi <ansuelsmth@gmail.com>

Now that we generalized inband logic and functions to dsa core, we can
drop the custom qca8k struct to handle inband mgmt and use directly the
dsa core dsa_inband struct in the priv struct.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/qca/qca8k-8xxx.c | 30 +++++++++++-------------------
 drivers/net/dsa/qca/qca8k.h      |  6 +-----
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index 14371c3c9a459..3314dff580316 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -162,7 +162,6 @@ qca8k_set_page(struct qca8k_priv *priv, u16 page)
 
 static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 {
-	struct qca8k_mgmt_eth_data *mgmt_eth_data;
 	struct qca8k_priv *priv = ds->priv;
 	struct qca_mgmt_ethhdr *mgmt_ethhdr;
 	u32 command;
@@ -172,7 +171,6 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 	int i;
 
 	mgmt_ethhdr = (struct qca_mgmt_ethhdr *)skb_mac_header(skb);
-	mgmt_eth_data = &priv->mgmt_eth_data;
 
 	command = get_unaligned_le32(&mgmt_ethhdr->command);
 	cmd = FIELD_GET(QCA_HDR_MGMT_CMD, command);
@@ -189,7 +187,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 
 	/* Make sure the seq match the requested packet. If not, drop. */
 	if (get_unaligned_le32(&mgmt_ethhdr->seq) !=
-	    dsa_inband_seqno(&mgmt_eth_data->inband))
+	    dsa_inband_seqno(&priv->inband))
 		return;
 
 	if (cmd == MDIO_READ) {
@@ -215,7 +213,7 @@ static void qca8k_rw_reg_ack_handler(struct dsa_switch *ds, struct sk_buff *skb)
 		}
 	}
 
-	dsa_inband_complete(&mgmt_eth_data->inband, &data, sizeof(data), err);
+	dsa_inband_complete(&priv->inband, data, sizeof(data), err);
 }
 
 static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
@@ -314,7 +312,6 @@ 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 sk_buff *skb;
 	u32 data[4];
 	int ret;
@@ -326,7 +323,7 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = READ_ONCE(priv->mgmt_conduit);
 
-	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
+	ret = dsa_inband_request(&priv->inband, skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
@@ -343,7 +340,6 @@ 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 sk_buff *skb;
 
 	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, val,
@@ -353,7 +349,7 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	skb->dev = READ_ONCE(priv->mgmt_conduit);
 
-	return dsa_inband_request(&mgmt_eth_data->inband, skb,
+	return dsa_inband_request(&priv->inband, skb,
 				  qca8k_mdio_header_fill_seq_num,
 				  NULL, 0,
 				  QCA8K_ETHERNET_TIMEOUT);
@@ -534,7 +530,7 @@ static const 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 dsa_inband *inband,
 			struct sk_buff *read_skb, u32 *val)
 {
 	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
@@ -544,8 +540,7 @@ qca8k_phy_eth_busy_wait(struct qca8k_mgmt_eth_data *mgmt_eth_data,
 	if (!skb)
 		return -ENOMEM;
 
-	ret = dsa_inband_request(&mgmt_eth_data->inband, skb,
-				 qca8k_mdio_header_fill_seq_num,
+	ret = dsa_inband_request(inband, skb, qca8k_mdio_header_fill_seq_num,
 				 data, sizeof(data),
 				 QCA8K_ETHERNET_TIMEOUT);
 
@@ -562,7 +557,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 		      int regnum, u16 data)
 {
 	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_conduit;
 	u32 resp_data[4];
@@ -571,8 +565,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
 		return -EINVAL;
 
-	mgmt_eth_data = &priv->mgmt_eth_data;
-
 	write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
 		    QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
 		    QCA8K_MDIO_MASTER_REG_ADDR(regnum);
@@ -624,7 +616,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	clear_skb->dev = mgmt_conduit;
 	write_skb->dev = mgmt_conduit;
 
-	ret = dsa_inband_request(&mgmt_eth_data->inband, write_skb,
+	ret = dsa_inband_request(&priv->inband, write_skb,
 				 qca8k_mdio_header_fill_seq_num,
 				 NULL, 0,
 				 QCA8K_ETHERNET_TIMEOUT);
@@ -637,7 +629,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->inband, read_skb, &val);
 
 	if (ret < 0 && ret1 < 0) {
 		ret = ret1;
@@ -645,7 +637,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	}
 
 	if (read) {
-		ret = dsa_inband_request(&mgmt_eth_data->inband, read_skb,
+		ret = dsa_inband_request(&priv->inband, read_skb,
 					 qca8k_mdio_header_fill_seq_num,
 					 resp_data, sizeof(resp_data),
 					 QCA8K_ETHERNET_TIMEOUT);
@@ -660,7 +652,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 exit:
 
 	/* This is expected to fail sometimes, so don't check return value. */
-	dsa_inband_request(&mgmt_eth_data->inband, clear_skb,
+	dsa_inband_request(&priv->inband, clear_skb,
 			   qca8k_mdio_header_fill_seq_num,
 			   NULL, 0,
 			   QCA8K_ETHERNET_TIMEOUT);
@@ -1991,7 +1983,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	if (!priv->ds)
 		return -ENOMEM;
 
-	dsa_inband_init(&priv->mgmt_eth_data.inband, U32_MAX);
+	dsa_inband_init(&priv->inband, U32_MAX);
 
 	mutex_init(&priv->mib_eth_data.mutex);
 	init_completion(&priv->mib_eth_data.rw_done);
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index ccf92c85ccb14..48bd5531d13e2 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -391,10 +391,6 @@ enum {
 	QCA8K_CPU_PORT6,
 };
 
-struct qca8k_mgmt_eth_data {
-	struct dsa_inband inband;
-};
-
 struct qca8k_mib_eth_data {
 	struct completion rw_done;
 	struct mutex mutex; /* Process one command at time */
@@ -458,7 +454,7 @@ struct qca8k_priv {
 	struct device *dev;
 	struct gpio_desc *reset_gpio;
 	struct net_device *mgmt_conduit; /* Track if mdio/mib Ethernet is available */
-	struct qca8k_mgmt_eth_data mgmt_eth_data;
+	struct dsa_inband inband;
 	struct qca8k_mib_eth_data mib_eth_data;
 	struct qca8k_mdio_cache mdio_cache;
 	struct qca8k_pcs pcs_port_0;

-- 
2.43.0

[PATCH net-next 11/12] net: dsa: Add helper to find ds_switch by index

[Luke Howard]

From: Andrew Lunn <andrew@lunn.ch>

The DSA header for an RMU frame includes the switch index to indicate
which switch sent the RMU frame. Add a helper which walks the list of
ports and finds the corresponding switch. Since this is not the hot
path for data frames, the overhead of multiple ports per switch is not
considered a problem.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/tag.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/dsa/tag.h b/net/dsa/tag.h
index cf52283fe9df4..c21d746652601 100644
--- a/net/dsa/tag.h
+++ b/net/dsa/tag.h
@@ -44,6 +44,20 @@ static inline struct net_device *dsa_conduit_find_user(struct net_device *dev,
 	return NULL;
 }
 
+static inline struct dsa_switch *dsa_conduit_find_switch(struct net_device *dev,
+							 int device)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch_tree *dst = cpu_dp->dst;
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list)
+		if (dp->ds->index == device)
+			return dp->ds;
+
+	return NULL;
+}
+
 /**
  * dsa_software_untag_vlan_aware_bridge: Software untagging for VLAN-aware bridge
  * @skb: Pointer to received socket buffer (packet)

-- 
2.43.0

[PATCH net-next 12/12] net: dsa: validate source trunk against lags_len

[Luke Howard]

A DSA frame with an invalid source trunk ID could cause an out-of-bounds
read access of dst->lags.

Add a check to dsa_lag_by_id() to validate the LAG ID is not zero, and is
less than or equal to dst->lags_len. (The LAG ID is derived by adding one
to the source trunk ID.)

Note: this is in the fast path for any frames within a trunk.

Fixes: 5b60dadb71db ("net: dsa: tag_dsa: Support reception of packets from LAG devices")
Signed-off-by: Luke Howard <lukeh@padl.com>
---
 include/net/dsa.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index af16347a3d331..9f01ea5bbd877 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -190,6 +190,9 @@ struct dsa_switch_tree {
 static inline struct dsa_lag *dsa_lag_by_id(struct dsa_switch_tree *dst,
 					    unsigned int id)
 {
+	if (unlikely(id == 0 || id > dst->lags_len))
+		return NULL;
+
 	/* DSA LAG IDs are one-based, dst->lags is zero-based */
 	return dst->lags[id - 1];
 }

-- 
2.43.0