It is possible for multiple, simultaneous callers calling
device_node_get_regmap() with the same node to fail to find an entry in
the syscon_list. There is a period of time while the first caller is
calling of_syscon_register() that subsequent callers also fail to find
an entry in the syscon_list and then call of_syscon_register() a second
time.

Fix this by keeping the lock held until after of_syscon_register()
completes and adds the node to syscon_list. Convert the spinlock to a
mutex as many of the functions called in of_syscon_register() such as
kzalloc() and of_clk_get() may sleep.

Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/mfd/syscon.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3e1d699ba9340f8135dfdeae6feca474980cc48d..72f20de9652da2d7bad12e4bc2c43ac0c9a97f76 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -15,6 +15,7 @@
 #include <linux/io.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -27,7 +28,7 @@
 
 static struct platform_driver syscon_driver;
 
-static DEFINE_SPINLOCK(syscon_list_slock);
+static DEFINE_MUTEX(syscon_list_lock);
 static LIST_HEAD(syscon_list);
 
 struct syscon {
@@ -54,6 +55,8 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	struct resource res;
 	struct reset_control *reset;
 
+	WARN_ON(!mutex_is_locked(&syscon_list_lock));
+
 	struct syscon *syscon __free(kfree) = kzalloc(sizeof(*syscon), GFP_KERNEL);
 	if (!syscon)
 		return ERR_PTR(-ENOMEM);
@@ -146,9 +149,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_res)
 	syscon->regmap = regmap;
 	syscon->np = np;
 
-	spin_lock(&syscon_list_slock);
 	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
 
 	return_ptr(syscon);
 
@@ -169,7 +170,7 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 {
 	struct syscon *entry, *syscon = NULL;
 
-	spin_lock(&syscon_list_slock);
+	mutex_lock(&syscon_list_lock);
 
 	list_for_each_entry(entry, &syscon_list, list)
 		if (entry->np == np) {
@@ -177,11 +178,11 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 			break;
 		}
 
-	spin_unlock(&syscon_list_slock);
-
 	if (!syscon)
 		syscon = of_syscon_register(np, check_res);
 
+	mutex_unlock(&syscon_list_lock);
+
 	if (IS_ERR(syscon))
 		return ERR_CAST(syscon);
 
@@ -212,7 +213,7 @@ int of_syscon_register_regmap(struct device_node *np, struct regmap *regmap)
 		return -ENOMEM;
 
 	/* check if syscon entry already exists */
-	spin_lock(&syscon_list_slock);
+	mutex_lock(&syscon_list_lock);
 
 	list_for_each_entry(entry, &syscon_list, list)
 		if (entry->np == np) {
@@ -225,12 +226,12 @@ int of_syscon_register_regmap(struct device_node *np, struct regmap *regmap)
 
 	/* register the regmap in syscon list */
 	list_add_tail(&syscon->list, &syscon_list);
-	spin_unlock(&syscon_list_slock);
+	mutex_unlock(&syscon_list_lock);
 
 	return 0;
 
 err_unlock:
-	spin_unlock(&syscon_list_slock);
+	mutex_unlock(&syscon_list_lock);
 	kfree(syscon);
 	return ret;
 }

-- 
2.45.2

> -----Original Message-----
> From: Rob Herring (Arm) <robh@kernel.org>
> Sent: Tuesday, December 17, 2024 11:42 PM
> To: Lee Jones <lee@kernel.org>; Arnd Bergmann <arnd@arndb.de>; Pankaj
> Dubey <pankaj.dubey@samsung.com>; Heiko Stuebner <heiko@sntech.de>;
> Liviu Dudau <liviu.dudau@arm.com>; Sudeep Holla <sudeep.holla@arm.com>;
> Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Peter Griffin <peter.griffin@linaro.org>; Will McVicker
> <willmcvicker@google.com>; John Madieu <john.madieu.xa@bp.renesas.com>;
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()
> 
> It is possible for multiple, simultaneous callers calling
> device_node_get_regmap() with the same node to fail to find an entry in the
> syscon_list. There is a period of time while the first caller is calling
> of_syscon_register() that subsequent callers also fail to find an entry in the
> syscon_list and then call of_syscon_register() a second time.
> 
> Fix this by keeping the lock held until after of_syscon_register() completes and
> adds the node to syscon_list. Convert the spinlock to a mutex as many of the
> functions called in of_syscon_register() such as
> kzalloc() and of_clk_get() may sleep.
> 
> Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform
> devices")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

Tested on arm64: Tesla FSD 

Tested-by: Pankaj Dubey <pankaj.dubey@samsung.com>

Thanks.
Pankaj Dubey

> -----Original Message-----
> From: Rob Herring (Arm) <robh@kernel.org>
> Sent: Tuesday, December 17, 2024 11:42 PM
> To: Lee Jones <lee@kernel.org>; Arnd Bergmann <arnd@arndb.de>; Pankaj
> Dubey <pankaj.dubey@samsung.com>; Heiko Stuebner <heiko@sntech.de>;
> Liviu Dudau <liviu.dudau@arm.com>; Sudeep Holla <sudeep.holla@arm.com>;
> Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Peter Griffin <peter.griffin@linaro.org>; Will McVicker
> <willmcvicker@google.com>; John Madieu <john.madieu.xa@bp.renesas.com>;
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Subject: [PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()
> 
> It is possible for multiple, simultaneous callers calling
> device_node_get_regmap() with the same node to fail to find an entry in the
> syscon_list. There is a period of time while the first caller is calling
> of_syscon_register() that subsequent callers also fail to find an entry in the
> syscon_list and then call of_syscon_register() a second time.
> 
> Fix this by keeping the lock held until after of_syscon_register() completes and
> adds the node to syscon_list. Convert the spinlock to a mutex as many of the
> functions called in of_syscon_register() such as
> kzalloc() and of_clk_get() may sleep.
> 
> Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform
> devices")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>

On 12/17/2024, Rob Herring (Arm) wrote:
> It is possible for multiple, simultaneous callers calling
> device_node_get_regmap() with the same node to fail to find an entry in
> the syscon_list. There is a period of time while the first caller is
> calling of_syscon_register() that subsequent callers also fail to find
> an entry in the syscon_list and then call of_syscon_register() a second
> time.
> 
> Fix this by keeping the lock held until after of_syscon_register()
> completes and adds the node to syscon_list. Convert the spinlock to a
> mutex as many of the functions called in of_syscon_register() such as
> kzalloc() and of_clk_get() may sleep.
> 
> Fixes: bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

I verified this works on my Pixel 6. Thanks!

Tested-by: Will McVicker <willmcvicker@google.com>

Thanks,
Will