[net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Ansuel Smith]

Add qca8k side support for mdio read/write in Ethernet packet.
qca8k supports some specially crafted Ethernet packet that can be used
for mdio read/write instead of the legacy method uart/internal mdio.
This add support for the qca8k side to craft the packet and enqueue it.
Each port and the qca8k_priv have a special struct to put data in it.
The completion API is used to wait for the packet to be received back
with the requested data.

The various steps are:
1. Craft the special packet with the qca hdr set to mdio read/write
   mode.
2. Set the lock in the dedicated mdio struct.
3. Reinit the completion.
4. Enqueue the packet.
5. Wait the packet to be received.
6. Use the data set by the tagger to complete the mdio operation.

If the completion timeouts or the ack value is not true, the legacy
mdio way is used.

It has to be considered that in the initial setup mdio is still used and
mdio is still used until DSA is ready to accept and tag packet.

tag_proto_connect() is used to fill the required handler for the tagger
to correctly parse and elaborate the special Ethernet mdio packet.

Locking is added to qca8k_master_change() to make sure no mdio Ethernet
are in progress.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  13 +++
 2 files changed, 205 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f317f527dd6d..b35ba26a0696 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -20,6 +20,7 @@
 #include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
+#include <linux/dsa/tag_qca.h>
 
 #include "qca8k.h"
 
@@ -170,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
 	return regmap_update_bits(priv->regmap, reg, mask, write_val);
 }
 
+static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
+{
+	struct qca8k_mdio_hdr_data *mdio_hdr_data;
+	struct qca8k_priv *priv = dp->ds->priv;
+	struct mdio_ethhdr *mdio_ethhdr;
+	u8 len, cmd;
+
+	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
+	mdio_hdr_data = &priv->mdio_hdr_data;
+
+	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
+	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
+
+	/* Make sure the seq match the requested packet */
+	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
+		mdio_hdr_data->ack = true;
+
+	if (cmd == MDIO_READ) {
+		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
+
+		/* Get the rest of the 12 byte of data */
+		if (len > QCA_HDR_MDIO_DATA1_LEN)
+			memcpy(mdio_hdr_data->data + 1, skb->data,
+			       QCA_HDR_MDIO_DATA2_LEN);
+	}
+
+	complete(&mdio_hdr_data->rw_done);
+}
+
+static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
+					       int seq_num, int priority)
+{
+	struct mdio_ethhdr *mdio_ethhdr;
+	struct sk_buff *skb;
+	u16 hdr;
+
+	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
+
+	skb_reset_mac_header(skb);
+	skb_set_network_header(skb, skb->len);
+
+	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
+
+	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
+	hdr |= QCA_HDR_XMIT_FROM_CPU;
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
+
+	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, seq_num);
+
+	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
+
+	if (cmd == MDIO_WRITE)
+		mdio_ethhdr->mdio_data = *val;
+
+	mdio_ethhdr->hdr = htons(hdr);
+
+	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
+	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
+
+	return skb;
+}
+
+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+{
+	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb->dev = (struct net_device *)priv->master;
+
+	mutex_lock(&mdio_hdr_data->mutex);
+
+	reinit_completion(&mdio_hdr_data->rw_done);
+	mdio_hdr_data->seq = 200;
+	mdio_hdr_data->ack = false;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
+					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+	*val = mdio_hdr_data->data[0];
+	ack = mdio_hdr_data->ack;
+
+	mutex_unlock(&mdio_hdr_data->mutex);
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+{
+	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb->dev = (struct net_device *)priv->master;
+
+	mutex_lock(&mdio_hdr_data->mutex);
+
+	reinit_completion(&mdio_hdr_data->rw_done);
+	mdio_hdr_data->ack = false;
+	mdio_hdr_data->seq = 200;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
+					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+	ack = mdio_hdr_data->ack;
+
+	mutex_unlock(&mdio_hdr_data->mutex);
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
+{
+	u32 val = 0;
+	int ret;
+
+	ret = qca8k_read_eth(priv, reg, &val);
+	if (ret)
+		return ret;
+
+	val &= ~mask;
+	val |= write_val;
+
+	return qca8k_write_eth(priv, reg, val);
+}
+
 static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
@@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 	u16 r1, r2, page;
 	int ret;
 
+	if (priv->master && !qca8k_read_eth(priv, reg, val))
+		return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 	u16 r1, r2, page;
 	int ret;
 
+	if (priv->master && !qca8k_write_eth(priv, reg, val))
+		return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	u32 val;
 	int ret;
 
+	if (priv->master &&
+	    !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
+		return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 	if (dp->index != 0)
 		return;
 
+	mutex_lock(&priv->mdio_hdr_data.mutex);
+
 	if (operational)
 		priv->master = master;
 	else
 		priv->master = NULL;
+
+	mutex_unlock(&priv->mdio_hdr_data.mutex);
+}
+
+static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
+				      enum dsa_tag_protocol proto)
+{
+	struct qca8k_priv *qca8k_priv = ds->priv;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_QCA:
+		struct tag_qca_priv *priv;
+
+		priv = ds->tagger_data;
+
+		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
+		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
+
+		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
 }
 
 static const struct dsa_switch_ops qca8k_switch_ops = {
@@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_lag_join		= qca8k_port_lag_join,
 	.port_lag_leave		= qca8k_port_lag_leave,
 	.master_state_change	= qca8k_master_change,
+	.connect_tag_protocol	= qca8k_connect_tag_protocol,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 6edd6adc3063..dbe8c74c9793 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,10 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/dsa/tag_qca.h>
+
+#define QCA8K_ETHERNET_MDIO_PRIORITY			7
+#define QCA8K_ETHERNET_TIMEOUT				100
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -328,6 +332,14 @@ enum {
 	QCA8K_CPU_PORT6,
 };
 
+struct qca8k_mdio_hdr_data {
+	struct completion rw_done;
+	struct mutex mutex; /* Enforce one mdio read/write at time */
+	bool ack;
+	u32 seq;
+	u32 data[4];
+};
+
 struct qca8k_ports_config {
 	bool sgmii_rx_clk_falling_edge;
 	bool sgmii_tx_clk_falling_edge;
@@ -354,6 +366,7 @@ struct qca8k_priv {
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
 	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
+	struct qca8k_mdio_hdr_data mdio_hdr_data;
 };
 
 struct qca8k_mib_desc {
-- 
2.33.1

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4100 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211214224409.5770-13-ansuelsmth@gmail.com>
References: <20211214224409.5770-13-ansuelsmth@gmail.com>
TO: Ansuel Smith <ansuelsmth@gmail.com>

Hi Ansuel,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git fe4c82a7e0f06abdf5a6978aa00457b63bd46680
:::::: branch date: 10 hours ago
:::::: commit date: 10 hours ago
config: microblaze-randconfig-s032-20211214 (https://download.01.org/0day-ci/archive/20211215/202112151621.ebBfoRll-lkp(a)intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/8036c636992760ad100109c1385110b8fda46e25
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
        git checkout 8036c636992760ad100109c1385110b8fda46e25
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/net/dsa/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/net/dsa/qca8k.c:2577:17: sparse: sparse: typename in expression
   drivers/net/dsa/qca8k.c:2577:24: sparse: sparse: Expected ; at end of statement
   drivers/net/dsa/qca8k.c:2577:24: sparse: sparse: got tag_qca_priv
   drivers/net/dsa/qca8k.c:2577:17: sparse: sparse: undefined identifier 'struct'
   drivers/net/dsa/qca8k.c:2579:17: sparse: sparse: undefined identifier 'priv'
   drivers/net/dsa/qca8k.c:2584:17: sparse: sparse: undefined identifier 'priv'

vim +2577 drivers/net/dsa/qca8k.c

8036c636992760a Ansuel Smith 2021-12-14  2569  
8036c636992760a Ansuel Smith 2021-12-14  2570  static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
8036c636992760a Ansuel Smith 2021-12-14  2571  				      enum dsa_tag_protocol proto)
8036c636992760a Ansuel Smith 2021-12-14  2572  {
8036c636992760a Ansuel Smith 2021-12-14  2573  	struct qca8k_priv *qca8k_priv = ds->priv;
8036c636992760a Ansuel Smith 2021-12-14  2574  
8036c636992760a Ansuel Smith 2021-12-14  2575  	switch (proto) {
8036c636992760a Ansuel Smith 2021-12-14  2576  	case DSA_TAG_PROTO_QCA:
8036c636992760a Ansuel Smith 2021-12-14 @2577  		struct tag_qca_priv *priv;
8036c636992760a Ansuel Smith 2021-12-14  2578  
8036c636992760a Ansuel Smith 2021-12-14  2579  		priv = ds->tagger_data;
8036c636992760a Ansuel Smith 2021-12-14  2580  
8036c636992760a Ansuel Smith 2021-12-14  2581  		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
8036c636992760a Ansuel Smith 2021-12-14  2582  		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
8036c636992760a Ansuel Smith 2021-12-14  2583  
8036c636992760a Ansuel Smith 2021-12-14  2584  		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
8036c636992760a Ansuel Smith 2021-12-14  2585  
8036c636992760a Ansuel Smith 2021-12-14  2586  		break;
8036c636992760a Ansuel Smith 2021-12-14  2587  	default:
8036c636992760a Ansuel Smith 2021-12-14  2588  		return -EOPNOTSUPP;
8036c636992760a Ansuel Smith 2021-12-14  2589  	}
8036c636992760a Ansuel Smith 2021-12-14  2590  
8036c636992760a Ansuel Smith 2021-12-14  2591  	return 0;
b5bf3a0669edf1c Ansuel Smith 2021-12-14  2592  }
b5bf3a0669edf1c Ansuel Smith 2021-12-14  2593  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[kernel test robot]

Hi Ansuel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git fe4c82a7e0f06abdf5a6978aa00457b63bd46680
config: arm64-randconfig-r016-20211214 (https://download.01.org/0day-ci/archive/20211215/202112151702.Xhos4slJ-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a2ddb6c8ac29412b1361810972e15221fa021c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/8036c636992760ad100109c1385110b8fda46e25
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
        git checkout 8036c636992760ad100109c1385110b8fda46e25
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/net/dsa/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/dsa/qca8k.c:2577:3: error: expected expression
                   struct tag_qca_priv *priv;
                   ^
>> drivers/net/dsa/qca8k.c:2579:3: error: use of undeclared identifier 'priv'
                   priv = ds->tagger_data;
                   ^
   drivers/net/dsa/qca8k.c:2584:3: error: use of undeclared identifier 'priv'
                   priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
                   ^
   3 errors generated.


vim +2577 drivers/net/dsa/qca8k.c

  2569	
  2570	static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
  2571					      enum dsa_tag_protocol proto)
  2572	{
  2573		struct qca8k_priv *qca8k_priv = ds->priv;
  2574	
  2575		switch (proto) {
  2576		case DSA_TAG_PROTO_QCA:
> 2577			struct tag_qca_priv *priv;
  2578	
> 2579			priv = ds->tagger_data;
  2580	
  2581			mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
  2582			init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
  2583	
  2584			priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
  2585	
  2586			break;
  2587		default:
  2588			return -EOPNOTSUPP;
  2589		}
  2590	
  2591		return 0;
  2592	}
  2593	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]

Hi Ansuel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git fe4c82a7e0f06abdf5a6978aa00457b63bd46680
config: arm64-randconfig-r016-20211214 (https://download.01.org/0day-ci/archive/20211215/202112151702.Xhos4slJ-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a2ddb6c8ac29412b1361810972e15221fa021c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/8036c636992760ad100109c1385110b8fda46e25
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ansuel-Smith/Add-support-for-qca8k-mdio-rw-in-Ethernet-packet/20211215-064645
        git checkout 8036c636992760ad100109c1385110b8fda46e25
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/net/dsa/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/dsa/qca8k.c:2577:3: error: expected expression
                   struct tag_qca_priv *priv;
                   ^
>> drivers/net/dsa/qca8k.c:2579:3: error: use of undeclared identifier 'priv'
                   priv = ds->tagger_data;
                   ^
   drivers/net/dsa/qca8k.c:2584:3: error: use of undeclared identifier 'priv'
                   priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
                   ^
   3 errors generated.


vim +2577 drivers/net/dsa/qca8k.c

  2569	
  2570	static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
  2571					      enum dsa_tag_protocol proto)
  2572	{
  2573		struct qca8k_priv *qca8k_priv = ds->priv;
  2574	
  2575		switch (proto) {
  2576		case DSA_TAG_PROTO_QCA:
> 2577			struct tag_qca_priv *priv;
  2578	
> 2579			priv = ds->tagger_data;
  2580	
  2581			mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
  2582			init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
  2583	
  2584			priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
  2585	
  2586			break;
  2587		default:
  2588			return -EOPNOTSUPP;
  2589		}
  2590	
  2591		return 0;
  2592	}
  2593	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> Add qca8k side support for mdio read/write in Ethernet packet.
> qca8k supports some specially crafted Ethernet packet that can be used
> for mdio read/write instead of the legacy method uart/internal mdio.
> This add support for the qca8k side to craft the packet and enqueue it.
> Each port and the qca8k_priv have a special struct to put data in it.
> The completion API is used to wait for the packet to be received back
> with the requested data.
> 
> The various steps are:
> 1. Craft the special packet with the qca hdr set to mdio read/write
>    mode.
> 2. Set the lock in the dedicated mdio struct.
> 3. Reinit the completion.
> 4. Enqueue the packet.
> 5. Wait the packet to be received.
> 6. Use the data set by the tagger to complete the mdio operation.
> 
> If the completion timeouts or the ack value is not true, the legacy
> mdio way is used.
> 
> It has to be considered that in the initial setup mdio is still used and
> mdio is still used until DSA is ready to accept and tag packet.
> 
> tag_proto_connect() is used to fill the required handler for the tagger
> to correctly parse and elaborate the special Ethernet mdio packet.
> 
> Locking is added to qca8k_master_change() to make sure no mdio Ethernet
> are in progress.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  13 +++
>  2 files changed, 205 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index f317f527dd6d..b35ba26a0696 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -20,6 +20,7 @@
>  #include <linux/phylink.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/etherdevice.h>
> +#include <linux/dsa/tag_qca.h>
>  
>  #include "qca8k.h"
>  
> @@ -170,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
>  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
>  }
>  
> +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data;
> +	struct qca8k_priv *priv = dp->ds->priv;
> +	struct mdio_ethhdr *mdio_ethhdr;
> +	u8 len, cmd;
> +
> +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> +	mdio_hdr_data = &priv->mdio_hdr_data;
> +
> +	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
> +	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
> +
> +	/* Make sure the seq match the requested packet */
> +	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
> +		mdio_hdr_data->ack = true;
> +
> +	if (cmd == MDIO_READ) {
> +		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
> +
> +		/* Get the rest of the 12 byte of data */
> +		if (len > QCA_HDR_MDIO_DATA1_LEN)
> +			memcpy(mdio_hdr_data->data + 1, skb->data,
> +			       QCA_HDR_MDIO_DATA2_LEN);
> +	}
> +
> +	complete(&mdio_hdr_data->rw_done);
> +}
> +
> +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> +					       int seq_num, int priority)
> +{
> +	struct mdio_ethhdr *mdio_ethhdr;
> +	struct sk_buff *skb;
> +	u16 hdr;
> +
> +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);

Still no if (!skb) checking... Not only here, but also at the call sites
of this.

> +
> +	skb_reset_mac_header(skb);
> +	skb_set_network_header(skb, skb->len);
> +
> +	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
> +
> +	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> +	hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> +	hdr |= QCA_HDR_XMIT_FROM_CPU;
> +	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> +	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> +
> +	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, seq_num);
> +
> +	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
> +
> +	if (cmd == MDIO_WRITE)
> +		mdio_ethhdr->mdio_data = *val;
> +
> +	mdio_ethhdr->hdr = htons(hdr);
> +
> +	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
> +	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);

Maybe a single call to skb_put_zero, and pass the sum as argument?

> +
> +	return skb;
> +}
> +
> +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);

You hardcode "seq" to 200? Aren't you supposed to increment it or
something?

> +	skb->dev = (struct net_device *)priv->master;

You access priv->master outside of priv->mdio_hdr_data.mutex from
qca8k_master_change(), that can't be good.

> +
> +	mutex_lock(&mdio_hdr_data->mutex);
> +
> +	reinit_completion(&mdio_hdr_data->rw_done);
> +	mdio_hdr_data->seq = 200;

Why do you rewrite the seq here?

> +	mdio_hdr_data->ack = false;
> +
> +	dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> +	*val = mdio_hdr_data->data[0];
> +	ack = mdio_hdr_data->ack;
> +
> +	mutex_unlock(&mdio_hdr_data->mutex);
> +
> +	if (ret <= 0)
> +		return -ETIMEDOUT;
> +
> +	if (!ack)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> +	skb->dev = (struct net_device *)priv->master;
> +
> +	mutex_lock(&mdio_hdr_data->mutex);
> +
> +	reinit_completion(&mdio_hdr_data->rw_done);
> +	mdio_hdr_data->ack = false;
> +	mdio_hdr_data->seq = 200;
> +
> +	dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> +	ack = mdio_hdr_data->ack;
> +
> +	mutex_unlock(&mdio_hdr_data->mutex);
> +
> +	if (ret <= 0)
> +		return -ETIMEDOUT;
> +
> +	if (!ack)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> +{
> +	u32 val = 0;
> +	int ret;
> +
> +	ret = qca8k_read_eth(priv, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~mask;
> +	val |= write_val;
> +
> +	return qca8k_write_eth(priv, reg, val);
> +}
> +
>  static int
>  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
>  {
> @@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	if (priv->master && !qca8k_read_eth(priv, reg, val))
> +		return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	if (priv->master && !qca8k_write_eth(priv, reg, val))
> +		return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
>  	u32 val;
>  	int ret;
>  
> +	if (priv->master &&
> +	    !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> +		return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
>  	if (dp->index != 0)
>  		return;
>  
> +	mutex_lock(&priv->mdio_hdr_data.mutex);
> +
>  	if (operational)
>  		priv->master = master;
>  	else
>  		priv->master = NULL;
> +
> +	mutex_unlock(&priv->mdio_hdr_data.mutex);
> +}
> +
> +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> +				      enum dsa_tag_protocol proto)
> +{
> +	struct qca8k_priv *qca8k_priv = ds->priv;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_QCA:
> +		struct tag_qca_priv *priv;
> +
> +		priv = ds->tagger_data;
> +
> +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);

I think having these initializations here dilutes the purpose of this
callback. Could you please move these two lines to qca8k_sw_probe()?

> +
> +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> +
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }
>  
>  static const struct dsa_switch_ops qca8k_switch_ops = {
> @@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_lag_join		= qca8k_port_lag_join,
>  	.port_lag_leave		= qca8k_port_lag_leave,
>  	.master_state_change	= qca8k_master_change,
> +	.connect_tag_protocol	= qca8k_connect_tag_protocol,
>  };
>  
>  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 6edd6adc3063..dbe8c74c9793 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -11,6 +11,10 @@
>  #include <linux/delay.h>
>  #include <linux/regmap.h>
>  #include <linux/gpio.h>
> +#include <linux/dsa/tag_qca.h>
> +
> +#define QCA8K_ETHERNET_MDIO_PRIORITY			7
> +#define QCA8K_ETHERNET_TIMEOUT				100
>  
>  #define QCA8K_NUM_PORTS					7
>  #define QCA8K_NUM_CPU_PORTS				2
> @@ -328,6 +332,14 @@ enum {
>  	QCA8K_CPU_PORT6,
>  };
>  
> +struct qca8k_mdio_hdr_data {

What do you think about the "qca8k_eth_mgmt_data" name rather than
"mdio_hdr_data"? I don't think this has anything to do with MDIO.

> +	struct completion rw_done;
> +	struct mutex mutex; /* Enforce one mdio read/write at time */
> +	bool ack;
> +	u32 seq;
> +	u32 data[4];
> +};
> +
>  struct qca8k_ports_config {
>  	bool sgmii_rx_clk_falling_edge;
>  	bool sgmii_tx_clk_falling_edge;
> @@ -354,6 +366,7 @@ struct qca8k_priv {
>  	struct gpio_desc *reset_gpio;
>  	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
> +	struct qca8k_mdio_hdr_data mdio_hdr_data;
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.33.1
> 

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> +				      enum dsa_tag_protocol proto)
> +{
> +	struct qca8k_priv *qca8k_priv = ds->priv;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_QCA:
> +		struct tag_qca_priv *priv;

Actually this fails to compile:

drivers/net/dsa/qca8k.c: In function ‘qca8k_connect_tag_protocol’:
drivers/net/dsa/qca8k.c:2893:3: error: a label can only be part of a statement and a declaration is not a statement
 2893 |   struct tag_qca_priv *priv;
      |   ^~~~~~
make[3]: *** [scripts/Makefile.build:287: drivers/net/dsa/qca8k.o] Error 1

This is what the {} brackets are for.

Also, while you're at this, please name "priv" "tagger_data".

> +
> +		priv = ds->tagger_data;
> +
> +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> +
> +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> +
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Ansuel Smith]

On Wed, Dec 15, 2021 at 11:49:12AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> > Add qca8k side support for mdio read/write in Ethernet packet.
> > qca8k supports some specially crafted Ethernet packet that can be used
> > for mdio read/write instead of the legacy method uart/internal mdio.
> > This add support for the qca8k side to craft the packet and enqueue it.
> > Each port and the qca8k_priv have a special struct to put data in it.
> > The completion API is used to wait for the packet to be received back
> > with the requested data.
> > 
> > The various steps are:
> > 1. Craft the special packet with the qca hdr set to mdio read/write
> >    mode.
> > 2. Set the lock in the dedicated mdio struct.
> > 3. Reinit the completion.
> > 4. Enqueue the packet.
> > 5. Wait the packet to be received.
> > 6. Use the data set by the tagger to complete the mdio operation.
> > 
> > If the completion timeouts or the ack value is not true, the legacy
> > mdio way is used.
> > 
> > It has to be considered that in the initial setup mdio is still used and
> > mdio is still used until DSA is ready to accept and tag packet.
> > 
> > tag_proto_connect() is used to fill the required handler for the tagger
> > to correctly parse and elaborate the special Ethernet mdio packet.
> > 
> > Locking is added to qca8k_master_change() to make sure no mdio Ethernet
> > are in progress.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |  13 +++
> >  2 files changed, 205 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index f317f527dd6d..b35ba26a0696 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/phylink.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/etherdevice.h>
> > +#include <linux/dsa/tag_qca.h>
> >  
> >  #include "qca8k.h"
> >  
> > @@ -170,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> >  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
> >  }
> >  
> > +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data;
> > +	struct qca8k_priv *priv = dp->ds->priv;
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	u8 len, cmd;
> > +
> > +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> > +	mdio_hdr_data = &priv->mdio_hdr_data;
> > +
> > +	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
> > +	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
> > +
> > +	/* Make sure the seq match the requested packet */
> > +	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
> > +		mdio_hdr_data->ack = true;
> > +
> > +	if (cmd == MDIO_READ) {
> > +		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
> > +
> > +		/* Get the rest of the 12 byte of data */
> > +		if (len > QCA_HDR_MDIO_DATA1_LEN)
> > +			memcpy(mdio_hdr_data->data + 1, skb->data,
> > +			       QCA_HDR_MDIO_DATA2_LEN);
> > +	}
> > +
> > +	complete(&mdio_hdr_data->rw_done);
> > +}
> > +
> > +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > +					       int seq_num, int priority)
> > +{
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	struct sk_buff *skb;
> > +	u16 hdr;
> > +
> > +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
> 
> Still no if (!skb) checking... Not only here, but also at the call sites
> of this.
> 
> > +
> > +	skb_reset_mac_header(skb);
> > +	skb_set_network_header(skb, skb->len);
> > +
> > +	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
> > +
> > +	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> > +	hdr |= QCA_HDR_XMIT_FROM_CPU;
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> > +
> > +	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, seq_num);
> > +
> > +	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
> > +
> > +	if (cmd == MDIO_WRITE)
> > +		mdio_ethhdr->mdio_data = *val;
> > +
> > +	mdio_ethhdr->hdr = htons(hdr);
> > +
> > +	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
> > +	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
> 
> Maybe a single call to skb_put_zero, and pass the sum as argument?
> 
> > +
> > +	return skb;
> > +}
> > +
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> 
> You hardcode "seq" to 200? Aren't you supposed to increment it or
> something?
>

We enforce one operation at time using lock. Seq is currently used to
check if the packet is correct.

> > +	skb->dev = (struct net_device *)priv->master;
> 
> You access priv->master outside of priv->mdio_hdr_data.mutex from
> qca8k_master_change(), that can't be good.
> 

Tell me if the logic is correct.
qca8k_master_change() removes or sets the priv->master under lock (so no
operation in progress)
A read/write checks if priv->master is not NULL and try to use this
alternative way. (not under lock)

Think I should remove the priv->master check from the read/write and
move it here under lock (and release the lock if it's not defined)
(The main idea is try to keep the skb alloc and packet setup outside of
locking to save some locking time)

Can I check priv->master 2 times? One outside the lock and one under
lock when is actually used by the skb? To skip locking and releasing for
every read/write if the alternative way is not available. 

> > +
> > +	mutex_lock(&mdio_hdr_data->mutex);
> > +
> > +	reinit_completion(&mdio_hdr_data->rw_done);
> > +	mdio_hdr_data->seq = 200;
> 
> Why do you rewrite the seq here?
> 

Seq is checked by the handler. Should I set it one time in probe?

> > +	mdio_hdr_data->ack = false;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> > +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > +	*val = mdio_hdr_data->data[0];
> > +	ack = mdio_hdr_data->ack;
> > +
> > +	mutex_unlock(&mdio_hdr_data->mutex);
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > +	skb->dev = (struct net_device *)priv->master;
> > +
> > +	mutex_lock(&mdio_hdr_data->mutex);
> > +
> > +	reinit_completion(&mdio_hdr_data->rw_done);
> > +	mdio_hdr_data->ack = false;
> > +	mdio_hdr_data->seq = 200;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> > +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > +	ack = mdio_hdr_data->ack;
> > +
> > +	mutex_unlock(&mdio_hdr_data->mutex);
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> > +{
> > +	u32 val = 0;
> > +	int ret;
> > +
> > +	ret = qca8k_read_eth(priv, reg, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= ~mask;
> > +	val |= write_val;
> > +
> > +	return qca8k_write_eth(priv, reg, val);
> > +}
> > +
> >  static int
> >  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> >  {
> > @@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	if (priv->master && !qca8k_read_eth(priv, reg, val))
> > +		return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	if (priv->master && !qca8k_write_eth(priv, reg, val))
> > +		return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
> >  	u32 val;
> >  	int ret;
> >  
> > +	if (priv->master &&
> > +	    !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> > +		return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> >  	if (dp->index != 0)
> >  		return;
> >  
> > +	mutex_lock(&priv->mdio_hdr_data.mutex);
> > +
> >  	if (operational)
> >  		priv->master = master;
> >  	else
> >  		priv->master = NULL;
> > +
> > +	mutex_unlock(&priv->mdio_hdr_data.mutex);
> > +}
> > +
> > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> > +				      enum dsa_tag_protocol proto)
> > +{
> > +	struct qca8k_priv *qca8k_priv = ds->priv;
> > +
> > +	switch (proto) {
> > +	case DSA_TAG_PROTO_QCA:
> > +		struct tag_qca_priv *priv;
> > +
> > +		priv = ds->tagger_data;
> > +
> > +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> > +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> 
> I think having these initializations here dilutes the purpose of this
> callback. Could you please move these two lines to qca8k_sw_probe()?
> 
> > +
> > +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> > +
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> >  }
> >  
> >  static const struct dsa_switch_ops qca8k_switch_ops = {
> > @@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.port_lag_join		= qca8k_port_lag_join,
> >  	.port_lag_leave		= qca8k_port_lag_leave,
> >  	.master_state_change	= qca8k_master_change,
> > +	.connect_tag_protocol	= qca8k_connect_tag_protocol,
> >  };
> >  
> >  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 6edd6adc3063..dbe8c74c9793 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -11,6 +11,10 @@
> >  #include <linux/delay.h>
> >  #include <linux/regmap.h>
> >  #include <linux/gpio.h>
> > +#include <linux/dsa/tag_qca.h>
> > +
> > +#define QCA8K_ETHERNET_MDIO_PRIORITY			7
> > +#define QCA8K_ETHERNET_TIMEOUT				100
> >  
> >  #define QCA8K_NUM_PORTS					7
> >  #define QCA8K_NUM_CPU_PORTS				2
> > @@ -328,6 +332,14 @@ enum {
> >  	QCA8K_CPU_PORT6,
> >  };
> >  
> > +struct qca8k_mdio_hdr_data {
> 
> What do you think about the "qca8k_eth_mgmt_data" name rather than
> "mdio_hdr_data"? I don't think this has anything to do with MDIO.
> 
> > +	struct completion rw_done;
> > +	struct mutex mutex; /* Enforce one mdio read/write at time */
> > +	bool ack;
> > +	u32 seq;
> > +	u32 data[4];
> > +};
> > +
> >  struct qca8k_ports_config {
> >  	bool sgmii_rx_clk_falling_edge;
> >  	bool sgmii_tx_clk_falling_edge;
> > @@ -354,6 +366,7 @@ struct qca8k_priv {
> >  	struct gpio_desc *reset_gpio;
> >  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> >  	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
> > +	struct qca8k_mdio_hdr_data mdio_hdr_data;
> >  };
> >  
> >  struct qca8k_mib_desc {
> > -- 
> > 2.33.1
> > 

-- 
	Ansuel

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Ansuel Smith]

On Wed, Dec 15, 2021 at 02:47:58PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> > +				      enum dsa_tag_protocol proto)
> > +{
> > +	struct qca8k_priv *qca8k_priv = ds->priv;
> > +
> > +	switch (proto) {
> > +	case DSA_TAG_PROTO_QCA:
> > +		struct tag_qca_priv *priv;
> 
> Actually this fails to compile:
> 
> drivers/net/dsa/qca8k.c: In function ‘qca8k_connect_tag_protocol’:
> drivers/net/dsa/qca8k.c:2893:3: error: a label can only be part of a statement and a declaration is not a statement
>  2893 |   struct tag_qca_priv *priv;
>       |   ^~~~~~
> make[3]: *** [scripts/Makefile.build:287: drivers/net/dsa/qca8k.o] Error 1
> 
> This is what the {} brackets are for.
> 
> Also, while you're at this, please name "priv" "tagger_data".
>

I didn't have this error, sorry. Just to make sure I didn't make these kind of
error anymore what compile did you use and with what flags? 

> > +
> > +		priv = ds->tagger_data;
> > +
> > +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> > +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> > +
> > +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> > +
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> >  }

-- 
	Ansuel