[PATCH v2 0/3] mfd: syscon: Cleanup, fix race condition and remove platform driver

[PATCH v2 0/3] mfd: syscon: Cleanup, fix race condition and remove platform driver

[Rob Herring (Arm)]

Questions from Krzysztof and a syscon related binding review got me 
looking at the syscon "driver". This series is the result.

This short series drops the stale platform driver part of syscon 
support, fixes a race condition in device_node_get_regmap()
which is used by all the lookup functions, and allows for registering 
nodes without "syscon" compatibles.

Compile tested only. Testing on Tensor, the one user of 
of_syscon_register_regmap(), would be helpful.

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v2:
- Fix patch 3 logic when a syscon is found in list to not return an 
  error
- Add tags
- Link to v1: https://lore.kernel.org/r/20241211-syscon-fixes-v1-0-b5ac8c219e96@kernel.org

---
Rob Herring (Arm) (3):
      mfd: syscon: Fix race in device_node_get_regmap()
      mfd: syscon: Remove the platform driver support
      mfd: syscon: Allow syscon nodes without a "syscon" compatible

 drivers/mfd/syscon.c                 | 95 ++++++------------------------------
 drivers/mfd/vexpress-sysreg.c        |  1 -
 include/linux/platform_data/syscon.h |  9 ----
 3 files changed, 15 insertions(+), 90 deletions(-)
---
base-commit: 40384c840ea1944d7c5a392e8975ed088ecf0b37
change-id: 20241211-syscon-fixes-9f35ecb9bfbe

Best regards,
-- 
Rob Herring (Arm) <robh@kernel.org>

[PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[Rob Herring (Arm)]

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

[PATCH v2 2/3] mfd: syscon: Remove the platform driver support

[Rob Herring (Arm)]

The platform driver is dead code. It is not used by DT platforms since
commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
platform devices") which said:

    For non-DT based platforms, this patch keeps syscon platform driver
    structure so that syscon can be probed and such non-DT based drivers
    can use syscon_regmap_lookup_by_pdev API and access regmap handles.
    Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
    we can completely remove platform driver of syscon, and keep only helper
    functions to get regmap handles.

The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
commit failed to remove the rest of the platform driver.

Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
 drivers/mfd/syscon.c                 | 66 ------------------------------------
 drivers/mfd/vexpress-sysreg.c        |  1 -
 include/linux/platform_data/syscon.h |  9 -----
 3 files changed, 76 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 72f20de9652da2d7bad12e4bc2c43ac0c9a97f76..bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -12,22 +12,15 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/hwspinlock.h>
-#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>
-#include <linux/platform_data/syscon.h>
-#include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/mfd/syscon.h>
 #include <linux/slab.h>
 
-static struct platform_driver syscon_driver;
-
 static DEFINE_MUTEX(syscon_list_lock);
 static LIST_HEAD(syscon_list);
 
@@ -337,62 +330,3 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
 	return regmap;
 }
 EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
-
-static int syscon_probe(struct platform_device *pdev)
-{
-	struct device *dev = &pdev->dev;
-	struct syscon_platform_data *pdata = dev_get_platdata(dev);
-	struct syscon *syscon;
-	struct regmap_config syscon_config = syscon_regmap_config;
-	struct resource *res;
-	void __iomem *base;
-
-	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
-	if (!syscon)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -ENOENT;
-
-	base = devm_ioremap(dev, res->start, resource_size(res));
-	if (!base)
-		return -ENOMEM;
-
-	syscon_config.max_register = resource_size(res) - 4;
-	if (!syscon_config.max_register)
-		syscon_config.max_register_is_0 = true;
-
-	if (pdata)
-		syscon_config.name = pdata->label;
-	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);
-	if (IS_ERR(syscon->regmap)) {
-		dev_err(dev, "regmap init failed\n");
-		return PTR_ERR(syscon->regmap);
-	}
-
-	platform_set_drvdata(pdev, syscon);
-
-	dev_dbg(dev, "regmap %pR registered\n", res);
-
-	return 0;
-}
-
-static const struct platform_device_id syscon_ids[] = {
-	{ "syscon", },
-	{ }
-};
-
-static struct platform_driver syscon_driver = {
-	.driver = {
-		.name = "syscon",
-	},
-	.probe		= syscon_probe,
-	.id_table	= syscon_ids,
-};
-
-static int __init syscon_init(void)
-{
-	return platform_driver_register(&syscon_driver);
-}
-postcore_initcall(syscon_init);
diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
index d34d58ce46db2ad0d53b3daeabc9d3763883b39a..ef03d6cec9ff6927668d051ca459eb1d8ff7269e 100644
--- a/drivers/mfd/vexpress-sysreg.c
+++ b/drivers/mfd/vexpress-sysreg.c
@@ -10,7 +10,6 @@
 #include <linux/mfd/core.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
-#include <linux/platform_data/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
diff --git a/include/linux/platform_data/syscon.h b/include/linux/platform_data/syscon.h
deleted file mode 100644
index 2c089dd3e2bda3baf5cef201ef43bca709e12c0b..0000000000000000000000000000000000000000
--- a/include/linux/platform_data/syscon.h
+++ /dev/null
@@ -1,9 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef PLATFORM_DATA_SYSCON_H
-#define PLATFORM_DATA_SYSCON_H
-
-struct syscon_platform_data {
-	const char *label;
-};
-
-#endif

-- 
2.45.2

[PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Rob Herring (Arm)]

of_syscon_register_regmap() was added for nodes which need a custom
regmap setup. It's not really correct for those nodes to claim they are
compatible with "syscon" as the default handling likely doesn't work in
those cases. If device_node_get_regmap() happens to be called first,
then of_syscon_register() will be called and an incorrect regmap will be
created (barring some other error). That may lead to unknown results in
the worst case. In the best case, of_syscon_register_regmap() will fail
with -EEXIST. This problem remains unless these cases drop "syscon" (an
ABI issue) or we exclude them using their specific compatible. ATM,
there is only one user: "google,gs101-pmu"

There are also cases of adding "syscon" compatible to existing nodes
after the fact in order to register the syscon. That presents a
potential DT ABI problem. Instead, if there's a kernel change needing a
syscon for a node, then it should be possible to allow the kernel to
register a syscon without a DT change. That's only possible by using
of_syscon_register_regmap() currently, but in the future we may want to
support a match list for cases which don't need a custom regmap.

With this change, the lookup functions will succeed for any node
registered by of_syscon_register_regmap() regardless of whether the node
compatible contains "syscon".

Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
---
v2:
 - Fix logic when a syscon is found in list to not return an error
---
 drivers/mfd/syscon.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 			break;
 		}
 
-	if (!syscon)
-		syscon = of_syscon_register(np, check_res);
-
+	if (!syscon) {
+		if (of_device_is_compatible(np, "syscon"))
+			syscon = of_syscon_register(np, check_res);
+		else
+			syscon = ERR_PTR(-EINVAL);
+	}
 	mutex_unlock(&syscon_list_lock);
 
 	if (IS_ERR(syscon))
@@ -238,9 +241,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap);
 
 struct regmap *syscon_node_to_regmap(struct device_node *np)
 {
-	if (!of_device_is_compatible(np, "syscon"))
-		return ERR_PTR(-EINVAL);
-
 	return device_node_get_regmap(np, true);
 }
 EXPORT_SYMBOL_GPL(syscon_node_to_regmap);

-- 
2.45.2

Re: [PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[William McVicker]

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

Re: [PATCH v2 2/3] mfd: syscon: Remove the platform driver support

[William McVicker]

On 12/17/2024, Rob Herring (Arm) wrote:
> The platform driver is dead code. It is not used by DT platforms since
> commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
> platform devices") which said:
> 
>     For non-DT based platforms, this patch keeps syscon platform driver
>     structure so that syscon can be probed and such non-DT based drivers
>     can use syscon_regmap_lookup_by_pdev API and access regmap handles.
>     Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
>     we can completely remove platform driver of syscon, and keep only helper
>     functions to get regmap handles.
> 
> The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
> syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
> commit failed to remove the rest of the platform driver.
> 
> 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

Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[William McVicker]

On 12/17/2024, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes
> after the fact in order to register the syscon. That presents a
> potential DT ABI problem. Instead, if there's a kernel change needing a
> syscon for a node, then it should be possible to allow the kernel to
> register a syscon without a DT change. That's only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node
> registered by of_syscon_register_regmap() regardless of whether the node
> compatible contains "syscon".
> 
> 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

Re: [PATCH v2 2/3] mfd: syscon: Remove the platform driver support

[Liviu Dudau]

On Tue, Dec 17, 2024 at 12:11:41PM -0600, Rob Herring (Arm) wrote:
> The platform driver is dead code. It is not used by DT platforms since
> commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
> platform devices") which said:
> 
>     For non-DT based platforms, this patch keeps syscon platform driver
>     structure so that syscon can be probed and such non-DT based drivers
>     can use syscon_regmap_lookup_by_pdev API and access regmap handles.
>     Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
>     we can completely remove platform driver of syscon, and keep only helper
>     functions to get regmap handles.
> 
> The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
> syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
> commit failed to remove the rest of the platform driver.
> 
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>

(Mostly for the vexpress-sysreg.c part, but hey, it's all code being removed):

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
>  drivers/mfd/syscon.c                 | 66 ------------------------------------
>  drivers/mfd/vexpress-sysreg.c        |  1 -
>  include/linux/platform_data/syscon.h |  9 -----
>  3 files changed, 76 deletions(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 72f20de9652da2d7bad12e4bc2c43ac0c9a97f76..bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -12,22 +12,15 @@
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/hwspinlock.h>
> -#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>
> -#include <linux/platform_data/syscon.h>
> -#include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/slab.h>
>  
> -static struct platform_driver syscon_driver;
> -
>  static DEFINE_MUTEX(syscon_list_lock);
>  static LIST_HEAD(syscon_list);
>  
> @@ -337,62 +330,3 @@ struct regmap *syscon_regmap_lookup_by_phandle_optional(struct device_node *np,
>  	return regmap;
>  }
>  EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle_optional);
> -
> -static int syscon_probe(struct platform_device *pdev)
> -{
> -	struct device *dev = &pdev->dev;
> -	struct syscon_platform_data *pdata = dev_get_platdata(dev);
> -	struct syscon *syscon;
> -	struct regmap_config syscon_config = syscon_regmap_config;
> -	struct resource *res;
> -	void __iomem *base;
> -
> -	syscon = devm_kzalloc(dev, sizeof(*syscon), GFP_KERNEL);
> -	if (!syscon)
> -		return -ENOMEM;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res)
> -		return -ENOENT;
> -
> -	base = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!base)
> -		return -ENOMEM;
> -
> -	syscon_config.max_register = resource_size(res) - 4;
> -	if (!syscon_config.max_register)
> -		syscon_config.max_register_is_0 = true;
> -
> -	if (pdata)
> -		syscon_config.name = pdata->label;
> -	syscon->regmap = devm_regmap_init_mmio(dev, base, &syscon_config);
> -	if (IS_ERR(syscon->regmap)) {
> -		dev_err(dev, "regmap init failed\n");
> -		return PTR_ERR(syscon->regmap);
> -	}
> -
> -	platform_set_drvdata(pdev, syscon);
> -
> -	dev_dbg(dev, "regmap %pR registered\n", res);
> -
> -	return 0;
> -}
> -
> -static const struct platform_device_id syscon_ids[] = {
> -	{ "syscon", },
> -	{ }
> -};
> -
> -static struct platform_driver syscon_driver = {
> -	.driver = {
> -		.name = "syscon",
> -	},
> -	.probe		= syscon_probe,
> -	.id_table	= syscon_ids,
> -};
> -
> -static int __init syscon_init(void)
> -{
> -	return platform_driver_register(&syscon_driver);
> -}
> -postcore_initcall(syscon_init);
> diff --git a/drivers/mfd/vexpress-sysreg.c b/drivers/mfd/vexpress-sysreg.c
> index d34d58ce46db2ad0d53b3daeabc9d3763883b39a..ef03d6cec9ff6927668d051ca459eb1d8ff7269e 100644
> --- a/drivers/mfd/vexpress-sysreg.c
> +++ b/drivers/mfd/vexpress-sysreg.c
> @@ -10,7 +10,6 @@
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
> -#include <linux/platform_data/syscon.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/stat.h>
> diff --git a/include/linux/platform_data/syscon.h b/include/linux/platform_data/syscon.h
> deleted file mode 100644
> index 2c089dd3e2bda3baf5cef201ef43bca709e12c0b..0000000000000000000000000000000000000000
> --- a/include/linux/platform_data/syscon.h
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef PLATFORM_DATA_SYSCON_H
> -#define PLATFORM_DATA_SYSCON_H
> -
> -struct syscon_platform_data {
> -	const char *label;
> -};
> -
> -#endif
> 
> -- 
> 2.45.2
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

RE: [PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[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>
> ---

Tested on arm64: Tesla FSD 

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

Thanks.
Pankaj Dubey

RE: [PATCH v2 2/3] mfd: syscon: Remove the platform driver support

[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 2/3] mfd: syscon: Remove the platform driver support
> 
> The platform driver is dead code. It is not used by DT platforms since commit
> bdb0066df96e ("mfd: syscon: Decouple syscon interface from platform devices")
> which said:
> 
>     For non-DT based platforms, this patch keeps syscon platform driver
>     structure so that syscon can be probed and such non-DT based drivers
>     can use syscon_regmap_lookup_by_pdev API and access regmap handles.
>     Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
>     we can completely remove platform driver of syscon, and keep only helper
>     functions to get regmap handles.
> 
> The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
> syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
> commit failed to remove the rest of the platform driver.
> 
> 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>

Tested on arm64: Tesla FSD

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

Thanks,
Pankaj Dubey

RE: [PATCH v2 1/3] mfd: syscon: Fix race in device_node_get_regmap()

[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>

RE: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[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 3/3] mfd: syscon: Allow syscon nodes without a "syscon"
> compatible
> 
> of_syscon_register_regmap() was added for nodes which need a custom regmap
> setup. It's not really correct for those nodes to claim they are compatible with
> "syscon" as the default handling likely doesn't work in those cases. If
> device_node_get_regmap() happens to be called first, then of_syscon_register()
> will be called and an incorrect regmap will be created (barring some other error).
> That may lead to unknown results in the worst case. In the best case,
> of_syscon_register_regmap() will fail with -EEXIST. This problem remains unless
> these cases drop "syscon" (an ABI issue) or we exclude them using their specific
> compatible. ATM, there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes after the
> fact in order to register the syscon. That presents a potential DT ABI problem.
> Instead, if there's a kernel change needing a syscon for a node, then it should be
> possible to allow the kernel to register a syscon without a DT change. That's
> only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node registered by
> of_syscon_register_regmap() regardless of whether the node compatible
> contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---

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

Thanks,
Pankaj Dubey

Re: [PATCH v2 2/3] mfd: syscon: Remove the platform driver support

[Krzysztof Kozlowski]

On 17/12/2024 19:11, Rob Herring (Arm) wrote:
> The platform driver is dead code. It is not used by DT platforms since
> commit bdb0066df96e ("mfd: syscon: Decouple syscon interface from
> platform devices") which said:
> 
>     For non-DT based platforms, this patch keeps syscon platform driver
>     structure so that syscon can be probed and such non-DT based drivers
>     can use syscon_regmap_lookup_by_pdev API and access regmap handles.
>     Once all users of "syscon_regmap_lookup_by_pdev" migrated to DT based,
>     we can completely remove platform driver of syscon, and keep only helper
>     functions to get regmap handles.
> 
> The last user of syscon_regmap_lookup_by_pdevname() was removed in 2018.
> syscon_regmap_lookup_by_pdevname() was then removed in 2019, but that
> commit failed to remove the rest of the platform driver.
> 
> Tested-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Krzysztof Kozlowski]

On 17/12/2024 19:11, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 0/3] mfd: syscon: Cleanup, fix race condition and remove platform driver

[Lee Jones]

On Tue, 17 Dec 2024 12:11:39 -0600, Rob Herring (Arm) wrote:
> Questions from Krzysztof and a syscon related binding review got me
> looking at the syscon "driver". This series is the result.
> 
> This short series drops the stale platform driver part of syscon
> support, fixes a race condition in device_node_get_regmap()
> which is used by all the lookup functions, and allows for registering
> nodes without "syscon" compatibles.
> 
> [...]

Applied, thanks!

[1/3] mfd: syscon: Fix race in device_node_get_regmap()
      commit: 805f7aaf7fee14a57b56af01d270edf6c10765e8
[2/3] mfd: syscon: Remove the platform driver support
      commit: 26769582bf353ae613f5113a1414ff3a80e08264
[3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible
      commit: ba5095ebbc7a83965ac049a50fa493d7c751f19b

--
Lee Jones [李琼斯]

Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Vaishnav Achath]

Hi Rob,

On 17/12/24 23:41, Rob Herring (Arm) wrote:
> of_syscon_register_regmap() was added for nodes which need a custom
> regmap setup. It's not really correct for those nodes to claim they are
> compatible with "syscon" as the default handling likely doesn't work in
> those cases. If device_node_get_regmap() happens to be called first,
> then of_syscon_register() will be called and an incorrect regmap will be
> created (barring some other error). That may lead to unknown results in
> the worst case. In the best case, of_syscon_register_regmap() will fail
> with -EEXIST. This problem remains unless these cases drop "syscon" (an
> ABI issue) or we exclude them using their specific compatible. ATM,
> there is only one user: "google,gs101-pmu"
> 
> There are also cases of adding "syscon" compatible to existing nodes
> after the fact in order to register the syscon. That presents a
> potential DT ABI problem. Instead, if there's a kernel change needing a
> syscon for a node, then it should be possible to allow the kernel to
> register a syscon without a DT change. That's only possible by using
> of_syscon_register_regmap() currently, but in the future we may want to
> support a match list for cases which don't need a custom regmap.
> 
> With this change, the lookup functions will succeed for any node
> registered by of_syscon_register_regmap() regardless of whether the node
> compatible contains "syscon".
> 
> Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> ---
> v2:
>   - Fix logic when a syscon is found in list to not return an error
> ---
>   drivers/mfd/syscon.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
>   			break;
>   		}
>   
> -	if (!syscon)
> -		syscon = of_syscon_register(np, check_res);
> -
> +	if (!syscon) {
> +		if (of_device_is_compatible(np, "syscon"))
> +			syscon = of_syscon_register(np, check_res);
> +		else
> +			syscon = ERR_PTR(-EINVAL);
> +	}

Earlier the above check was only in syscon_node_to_regmap() , but now 
the users of device_node_to_regmap() also undergoes this check and there 
are few drivers in tree which uses device_node_to_regmap() without 
having "syscon" in compatible, likely those drivers need to be fixed, 
but this patch breaks all those drivers now, atleast the below drivers 
are affected and all TI platforms fail to boot due to this:

drivers/soc/ti/k3-socinfo.c
drivers/mux/mmio.c ("reg-mux" users)
drivers/clk/keystone/syscon-clk.c

All the above drivers fail due to the above check for syscon compatible.

What is the recommended solution to fix this?

Thanks and Regards,
Vaishnav

>   	mutex_unlock(&syscon_list_lock);
>   
>   	if (IS_ERR(syscon))
> @@ -238,9 +241,6 @@ EXPORT_SYMBOL_GPL(device_node_to_regmap);
>   
>   struct regmap *syscon_node_to_regmap(struct device_node *np)
>   {
> -	if (!of_device_is_compatible(np, "syscon"))
> -		return ERR_PTR(-EINVAL);
> -
>   	return device_node_get_regmap(np, true);
>   }
>   EXPORT_SYMBOL_GPL(syscon_node_to_regmap);
> 

Re: [PATCH v2 3/3] mfd: syscon: Allow syscon nodes without a "syscon" compatible

[Rob Herring]

On Wed, Jan 22, 2025 at 3:44 AM Vaishnav Achath <vaishnav.a@ti.com> wrote:
>
> Hi Rob,
>
> On 17/12/24 23:41, Rob Herring (Arm) wrote:
> > of_syscon_register_regmap() was added for nodes which need a custom
> > regmap setup. It's not really correct for those nodes to claim they are
> > compatible with "syscon" as the default handling likely doesn't work in
> > those cases. If device_node_get_regmap() happens to be called first,
> > then of_syscon_register() will be called and an incorrect regmap will be
> > created (barring some other error). That may lead to unknown results in
> > the worst case. In the best case, of_syscon_register_regmap() will fail
> > with -EEXIST. This problem remains unless these cases drop "syscon" (an
> > ABI issue) or we exclude them using their specific compatible. ATM,
> > there is only one user: "google,gs101-pmu"
> >
> > There are also cases of adding "syscon" compatible to existing nodes
> > after the fact in order to register the syscon. That presents a
> > potential DT ABI problem. Instead, if there's a kernel change needing a
> > syscon for a node, then it should be possible to allow the kernel to
> > register a syscon without a DT change. That's only possible by using
> > of_syscon_register_regmap() currently, but in the future we may want to
> > support a match list for cases which don't need a custom regmap.
> >
> > With this change, the lookup functions will succeed for any node
> > registered by of_syscon_register_regmap() regardless of whether the node
> > compatible contains "syscon".
> >
> > Signed-off-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> > v2:
> >   - Fix logic when a syscon is found in list to not return an error
> > ---
> >   drivers/mfd/syscon.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index bfb1f69fcff1d3cd35cf04ccd4c449e7d0395c79..226915ca3c93dcaf47bdd46b58e00e10e155f952 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -171,9 +171,12 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
> >                       break;
> >               }
> >
> > -     if (!syscon)
> > -             syscon = of_syscon_register(np, check_res);
> > -
> > +     if (!syscon) {
> > +             if (of_device_is_compatible(np, "syscon"))
> > +                     syscon = of_syscon_register(np, check_res);
> > +             else
> > +                     syscon = ERR_PTR(-EINVAL);
> > +     }
>
> Earlier the above check was only in syscon_node_to_regmap() , but now
> the users of device_node_to_regmap() also undergoes this check and there
> are few drivers in tree which uses device_node_to_regmap() without
> having "syscon" in compatible, likely those drivers need to be fixed,
> but this patch breaks all those drivers now, atleast the below drivers
> are affected and all TI platforms fail to boot due to this:
>
> drivers/soc/ti/k3-socinfo.c
> drivers/mux/mmio.c ("reg-mux" users)
> drivers/clk/keystone/syscon-clk.c
>
> All the above drivers fail due to the above check for syscon compatible.
>
> What is the recommended solution to fix this?

I've sent a fix[1]. That's not to say I don't agree with fixes to
drivers removing device_node_to_regmap() calls. If you just need a
regmap for within a single driver, then just use regmap APIs directly.
What's in syscon.c is for (surprise!) syscon's, and those have the
characteristic of being shared.

Rob

[1] https://lore.kernel.org/all/20250124191644.2309790-1-robh@kernel.org/