[net-next RFC PATCH 1/2] net: mdio: implement mdio_mutex_nested guard() variant

[net-next RFC PATCH 1/2] net: mdio: implement mdio_mutex_nested guard() variant

[Christian Marangi]

Implement mdio_mutex_nested guard() variant.

guard() compes from the cleanup.h API that define handy class to
define the lifecycle of a critical section.

Many driver makes use of the mutex_lock_nested()/mutex_unlock() hence it
might be sensible to provide a variant of the generic guard(mutex),
guard(mdio_mutex_nested) to also support drivers that use
mutex_lock_nested with MDIO_MUTEX_NESTED.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 include/linux/mdio.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 68f8d2e970d4..f13a02d05eb2 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -8,6 +8,8 @@
 
 #include <uapi/linux/mdio.h>
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/mutex.h>
 #include <linux/mod_devicetable.h>
 
 struct gpio_desc;
@@ -25,6 +27,9 @@ enum mdio_mutex_lock_class {
 	MDIO_MUTEX_NESTED,
 };
 
+DEFINE_GUARD(mdio_mutex_nested, struct mutex *,
+	     mutex_lock_nested(_T, MDIO_MUTEX_NESTED), mutex_unlock(_T))
+
 struct mdio_device {
 	struct device dev;
 
-- 
2.45.1

[net-next RFC PATCH 2/2] net: dsa: qca: qca8k: convert to guard API

[Christian Marangi]

Convert every entry of mutex_lock/unlock() to guard API.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/qca8k-8xxx.c   |  99 +++++++----------------
 drivers/net/dsa/qca/qca8k-common.c | 122 ++++++++++++-----------------
 2 files changed, 81 insertions(+), 140 deletions(-)

diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index b3c27cf538e8..2d9526b696f2 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2016 John Crispin <john@phrozen.org>
  */
 
+#include <linux/cleanup.h>
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/netdevice.h>
@@ -321,12 +322,11 @@ 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);
+	guard(mutex)(&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;
 	}
 
@@ -350,8 +350,6 @@ static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	ack = mgmt_eth_data->ack;
 
-	mutex_unlock(&mgmt_eth_data->mutex);
-
 	if (ret <= 0)
 		return -ETIMEDOUT;
 
@@ -373,12 +371,11 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 	if (!skb)
 		return -ENOMEM;
 
-	mutex_lock(&mgmt_eth_data->mutex);
+	guard(mutex)(&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;
 	}
 
@@ -398,8 +395,6 @@ static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 *val, int len)
 
 	ack = mgmt_eth_data->ack;
 
-	mutex_unlock(&mgmt_eth_data->mutex);
-
 	if (ret <= 0)
 		return -ETIMEDOUT;
 
@@ -434,17 +429,13 @@ qca8k_read_mii(struct qca8k_priv *priv, uint32_t reg, uint32_t *val)
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	guard(mdio_mutex_nested)(&bus->mdio_lock);
 
 	ret = qca8k_set_page(priv, page);
 	if (ret < 0)
-		goto exit;
-
-	ret = qca8k_mii_read32(bus, 0x10 | r2, r1, val);
+		return ret;
 
-exit:
-	mutex_unlock(&bus->mdio_lock);
-	return ret;
+	return qca8k_mii_read32(bus, 0x10 | r2, r1, val);
 }
 
 static int
@@ -456,17 +447,15 @@ qca8k_write_mii(struct qca8k_priv *priv, uint32_t reg, uint32_t val)
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	guard(mdio_mutex_nested)(&bus->mdio_lock);
 
 	ret = qca8k_set_page(priv, page);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
-exit:
-	mutex_unlock(&bus->mdio_lock);
-	return ret;
+	return 0;
 }
 
 static int
@@ -480,24 +469,21 @@ qca8k_regmap_update_bits_mii(struct qca8k_priv *priv, uint32_t reg,
 
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
-	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	guard(mdio_mutex_nested)(&bus->mdio_lock);
 
 	ret = qca8k_set_page(priv, page);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	ret = qca8k_mii_read32(bus, 0x10 | r2, r1, &val);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	val &= ~mask;
 	val |= write_val;
 	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
-exit:
-	mutex_unlock(&bus->mdio_lock);
-
-	return ret;
+	return 0;
 }
 
 static int
@@ -673,7 +659,7 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	 * We therefore need to lock the MDIO bus onto which the switch is
 	 * connected.
 	 */
-	mutex_lock(&priv->bus->mdio_lock);
+	guard(mutex)(&priv->bus->mdio_lock);
 
 	/* Actually start the request:
 	 * 1. Send mdio master packet
@@ -681,13 +667,11 @@ 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);
+	guard(mutex)(&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;
 	}
@@ -774,9 +758,6 @@ qca8k_phy_eth_command(struct qca8k_priv *priv, bool read, int phy,
 	wait_for_completion_timeout(&mgmt_eth_data->rw_done,
 				    QCA8K_ETHERNET_TIMEOUT);
 
-	mutex_unlock(&mgmt_eth_data->mutex);
-	mutex_unlock(&priv->bus->mdio_lock);
-
 	return ret;
 
 	/* Error handling before lock */
@@ -830,24 +811,21 @@ qca8k_mdio_write(struct qca8k_priv *priv, int phy, int regnum, u16 data)
 
 	qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL, &r1, &r2, &page);
 
-	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	guard(mdio_mutex_nested)(&bus->mdio_lock);
 
 	ret = qca8k_set_page(priv, page);
 	if (ret)
-		goto exit;
+		return ret;
 
 	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
 
 	ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
 				   QCA8K_MDIO_MASTER_BUSY);
 
-exit:
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
 	qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
 
-	mutex_unlock(&bus->mdio_lock);
-
-	return ret;
+	return 0;
 }
 
 static int
@@ -867,7 +845,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 
 	qca8k_split_addr(QCA8K_MDIO_MASTER_CTRL, &r1, &r2, &page);
 
-	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
+	guard(mdio_mutex_nested)(&bus->mdio_lock);
 
 	ret = qca8k_set_page(priv, page);
 	if (ret)
@@ -886,12 +864,7 @@ qca8k_mdio_read(struct qca8k_priv *priv, int phy, int regnum)
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
 	qca8k_mii_write_hi(bus, 0x10 | r2, r1 + 1, 0);
 
-	mutex_unlock(&bus->mdio_lock);
-
-	if (ret >= 0)
-		ret = val & QCA8K_MDIO_MASTER_DATA_MASK;
-
-	return ret;
+	return ret >= 0 ? val & QCA8K_MDIO_MASTER_DATA_MASK : ret;
 }
 
 static int
@@ -1698,7 +1671,7 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 
 	mib_eth_data = &priv->mib_eth_data;
 
-	mutex_lock(&mib_eth_data->mutex);
+	guard(mutex)(&mib_eth_data->mutex);
 
 	reinit_completion(&mib_eth_data->rw_done);
 
@@ -1706,25 +1679,16 @@ qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
 	mib_eth_data->data = data;
 	refcount_set(&mib_eth_data->port_parsed, QCA8K_NUM_PORTS);
 
-	mutex_lock(&priv->reg_mutex);
-
 	/* Send mib autocast request */
-	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
-				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
-				 FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
-				 QCA8K_MIB_BUSY);
-
-	mutex_unlock(&priv->reg_mutex);
-
+	scoped_guard(mutex)(&priv->reg_mutex)
+		ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+					 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+					 FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
+					 QCA8K_MIB_BUSY);
 	if (ret)
-		goto exit;
-
-	ret = wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
-
-exit:
-	mutex_unlock(&mib_eth_data->mutex);
+		return ret;
 
-	return ret;
+	return wait_for_completion_timeout(&mib_eth_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
 }
 
 static u32 qca8k_get_phy_flags(struct dsa_switch *ds, int port)
@@ -1761,13 +1725,10 @@ 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);
+	guard(mutex)(&priv->mgmt_eth_data.mutex);
+	guard(mutex)(&priv->mib_eth_data.mutex);
 
 	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,
diff --git a/drivers/net/dsa/qca/qca8k-common.c b/drivers/net/dsa/qca/qca8k-common.c
index 7f80035c5441..e020474de514 100644
--- a/drivers/net/dsa/qca/qca8k-common.c
+++ b/drivers/net/dsa/qca/qca8k-common.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2016 John Crispin <john@phrozen.org>
  */
 
+#include <linux/cleanup.h>
 #include <linux/netdevice.h>
 #include <net/dsa.h>
 #include <linux/if_bridge.h>
@@ -215,10 +216,10 @@ static int qca8k_fdb_add(struct qca8k_priv *priv, const u8 *mac,
 {
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	qca8k_fdb_write(priv, vid, port_mask, mac, aging);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-	mutex_unlock(&priv->reg_mutex);
 
 	return ret;
 }
@@ -228,19 +229,19 @@ static int qca8k_fdb_del(struct qca8k_priv *priv, const u8 *mac,
 {
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	qca8k_fdb_write(priv, vid, port_mask, mac, 0);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
-	mutex_unlock(&priv->reg_mutex);
 
 	return ret;
 }
 
 void qca8k_fdb_flush(struct qca8k_priv *priv)
 {
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH, -1);
-	mutex_unlock(&priv->reg_mutex);
 }
 
 static int qca8k_fdb_search_and_insert(struct qca8k_priv *priv, u8 port_mask,
@@ -249,22 +250,22 @@ static int qca8k_fdb_search_and_insert(struct qca8k_priv *priv, u8 port_mask,
 	struct qca8k_fdb fdb = { 0 };
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
 
 	qca8k_fdb_write(priv, vid, 0, mac, 0);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	ret = qca8k_fdb_read(priv, &fdb);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	/* Rule exist. Delete first */
 	if (fdb.aging) {
 		ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
 		if (ret)
-			goto exit;
+			return ret;
 	} else {
 		fdb.aging = aging;
 	}
@@ -273,11 +274,7 @@ static int qca8k_fdb_search_and_insert(struct qca8k_priv *priv, u8 port_mask,
 	fdb.port_mask |= port_mask;
 
 	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
+	return qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
 }
 
 static int qca8k_fdb_search_and_del(struct qca8k_priv *priv, u8 port_mask,
@@ -286,40 +283,34 @@ static int qca8k_fdb_search_and_del(struct qca8k_priv *priv, u8 port_mask,
 	struct qca8k_fdb fdb = { 0 };
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
 
 	qca8k_fdb_write(priv, vid, 0, mac, 0);
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_SEARCH, -1);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	ret = qca8k_fdb_read(priv, &fdb);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	/* Rule doesn't exist. Why delete? */
-	if (!fdb.aging) {
-		ret = -EINVAL;
-		goto exit;
-	}
+	if (!fdb.aging)
+		return -EINVAL;
 
 	ret = qca8k_fdb_access(priv, QCA8K_FDB_PURGE, -1);
 	if (ret)
-		goto exit;
+		return ret;
 
 	/* Only port in the rule is this port. Don't re insert */
 	if (fdb.port_mask == port_mask)
-		goto exit;
+		return ret;
 
 	/* Remove port from port mask */
 	fdb.port_mask &= ~port_mask;
 
 	qca8k_fdb_write(priv, vid, fdb.port_mask, mac, fdb.aging);
-	ret = qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
-
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
+	return qca8k_fdb_access(priv, QCA8K_FDB_LOAD, -1);
 }
 
 static int qca8k_vlan_access(struct qca8k_priv *priv,
@@ -367,14 +358,15 @@ static int qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid,
 	if (vid == 0)
 		return 0;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = qca8k_read(priv, QCA8K_REG_VTU_FUNC0, &reg);
 	if (ret < 0)
-		goto out;
+		return ret;
 	reg |= QCA8K_VTU_FUNC0_VALID | QCA8K_VTU_FUNC0_IVL_EN;
 	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
 	if (untagged)
@@ -384,13 +376,9 @@ static int qca8k_vlan_add(struct qca8k_priv *priv, u8 port, u16 vid,
 
 	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
 	if (ret)
-		goto out;
-	ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
-
-out:
-	mutex_unlock(&priv->reg_mutex);
+		return ret;
 
-	return ret;
+	return qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
 }
 
 static int qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
@@ -399,14 +387,15 @@ static int qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 	int ret, i;
 	bool del;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	ret = qca8k_vlan_access(priv, QCA8K_VLAN_READ, vid);
 	if (ret < 0)
-		goto out;
+		return ret;
 
 	ret = qca8k_read(priv, QCA8K_REG_VTU_FUNC0, &reg);
 	if (ret < 0)
-		goto out;
+		return ret;
 	reg &= ~QCA8K_VTU_FUNC0_EG_MODE_PORT_MASK(port);
 	reg |= QCA8K_VTU_FUNC0_EG_MODE_PORT_NOT(port);
 
@@ -421,46 +410,39 @@ static int qca8k_vlan_del(struct qca8k_priv *priv, u8 port, u16 vid)
 		}
 	}
 
-	if (del) {
-		ret = qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
-	} else {
-		ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
-		if (ret)
-			goto out;
-		ret = qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
-	}
+	if (del)
+		return qca8k_vlan_access(priv, QCA8K_VLAN_PURGE, vid);
 
-out:
-	mutex_unlock(&priv->reg_mutex);
 
-	return ret;
+	ret = qca8k_write(priv, QCA8K_REG_VTU_FUNC0, reg);
+	if (ret)
+		return ret;
+
+	return qca8k_vlan_access(priv, QCA8K_VLAN_LOAD, vid);
 }
 
 int qca8k_mib_init(struct qca8k_priv *priv)
 {
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
 				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
 				 FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_FLUSH) |
 				 QCA8K_MIB_BUSY);
 	if (ret)
-		goto exit;
+		return ret;
 
 	ret = qca8k_busy_wait(priv, QCA8K_REG_MIB, QCA8K_MIB_BUSY);
 	if (ret)
-		goto exit;
+		return ret;
 
 	ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
 	if (ret)
-		goto exit;
-
-	ret = qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
+		return ret;
 
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
+	return qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
 }
 
 void qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
@@ -541,20 +523,18 @@ int qca8k_set_mac_eee(struct dsa_switch *ds, int port,
 	u32 reg;
 	int ret;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	ret = qca8k_read(priv, QCA8K_REG_EEE_CTRL, &reg);
 	if (ret < 0)
-		goto exit;
+		return ret;
 
 	if (eee->eee_enabled)
 		reg |= lpi_en;
 	else
 		reg &= ~lpi_en;
-	ret = qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
 
-exit:
-	mutex_unlock(&priv->reg_mutex);
-	return ret;
+	return qca8k_write(priv, QCA8K_REG_EEE_CTRL, reg);
 }
 
 int qca8k_get_mac_eee(struct dsa_switch *ds, int port,
@@ -708,9 +688,9 @@ void qca8k_port_fast_age(struct dsa_switch *ds, int port)
 {
 	struct qca8k_priv *priv = ds->priv;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	qca8k_fdb_access(priv, QCA8K_FDB_FLUSH_PORT, port);
-	mutex_unlock(&priv->reg_mutex);
 }
 
 int qca8k_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
@@ -841,7 +821,8 @@ int qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 	bool is_static;
 	int ret = 0;
 
-	mutex_lock(&priv->reg_mutex);
+	guard(mutex)(&priv->reg_mutex);
+
 	while (cnt-- && !qca8k_fdb_next(priv, &_fdb, port)) {
 		if (!_fdb.aging)
 			break;
@@ -850,7 +831,6 @@ int qca8k_port_fdb_dump(struct dsa_switch *ds, int port,
 		if (ret)
 			break;
 	}
-	mutex_unlock(&priv->reg_mutex);
 
 	return 0;
 }
-- 
2.45.1

Re: [net-next RFC PATCH 2/2] net: dsa: qca: qca8k: convert to guard API

[Andrew Lunn]

On Thu, Jun 27, 2024 at 01:02:32AM +0200, Christian Marangi wrote:
> Convert every entry of mutex_lock/unlock() to guard API.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca/qca8k-8xxx.c   |  99 +++++++----------------
>  drivers/net/dsa/qca/qca8k-common.c | 122 ++++++++++++-----------------
>  2 files changed, 81 insertions(+), 140 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
> index b3c27cf538e8..2d9526b696f2 100644
> --- a/drivers/net/dsa/qca/qca8k-8xxx.c
> +++ b/drivers/net/dsa/qca/qca8k-8xxx.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2016 John Crispin <john@phrozen.org>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/module.h>
>  #include <linux/phy.h>
>  #include <linux/netdevice.h>
> @@ -321,12 +322,11 @@ 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);
> +	guard(mutex)(&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;

Sorry, but NACK.

There are two issues here.

1) guard() is very magical, the opposite of C which is explicit.  We
   discussed that, and think scoped_guard is O.K, since it is more C
   like.

2) We don't want scope_guard or guard introduced in existing code, at
   least not for the moment, because it seems like it is going to make
   back porting patches for stable harder/more error prone.

We do however see that these mechanisms are useful, could solve
problems, so its O.K. to use scoped_guard in new code. In a few years
we will see how things have actually worked out, and reevaluate our
position and maybe allow scoped_guard to be added to existing code.

	Andrew

Re: [net-next RFC PATCH 1/2] net: mdio: implement mdio_mutex_nested guard() variant

[Markus Elfring]

> Implement mdio_mutex_nested guard() variant.

I find the idea generally helpful.
The concrete implementation needs further clarifications.


> guard() compes from the cleanup.h API that define handy class to

          comes?                             defines?


> define the lifecycle of a critical section.

  handle?


> Many driver makes use of the mutex_lock_nested()/mutex_unlock() hence it

  Several drivers use?                                            function call pair.

Would you like to clarify any application statistics another bit?
https://elixir.bootlin.com/linux/v6.10-rc5/A/ident/mutex_lock_nested


> might be sensible to provide a variant of the generic guard(mutex),

  Hence it is?                                                      :


> guard(mdio_mutex_nested) to also support drivers that use
> mutex_lock_nested with MDIO_MUTEX_NESTED.

Another wording suggestion:
  guard(mdio_mutex_nested) so that drivers can be better supported
  with the call variant “mutex_lock_nested(…, MDIO_MUTEX_NESTED)”.


…
> +++ b/include/linux/mdio.h
> @@ -8,6 +8,8 @@
>
>  #include <uapi/linux/mdio.h>
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>

I suggest to omit this preprocessing directive here.


> +#include <linux/mutex.h>
>  #include <linux/mod_devicetable.h>
…

Further information is included as possibly needed.
https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L22

How reasonable is the added header file dependency so far?


Under which circumstances can remaining change resistance be adjusted
for further benefits from applications of scope-based resource management?

Regards,
Markus