[PATCH v4 0/3] Migrate the multiplexer subsystem to fwnode

[PATCH v4 0/3] Migrate the multiplexer subsystem to fwnode

[Fabio Forni via B4 Relay]

This patch migrates the multiplexer subsystem from using the of_*          
family of functions and structs, to the more generic fwnode framework.     
                                                                           
It is a rebase of a single commit[1] contained in a old patch series[2]
submitted by Xu Yang. The original commit plus follow-up comments were
tested on kernel v6.12 on an arm64-based board, but this current rebase
isn't tested yet.                                                  

While at it, a use-after-free bug was fixed in mux_get(), as suggested by
Alvin Šipraga.
                                                                           
Link: https://lore.kernel.org/all/20220823195429.1243516-3-xu.yang_2@nxp.com [1]
Link: https://lore.kernel.org/all/20220823195429.1243516-1-xu.yang_2@nxp.com [2]

Signed-off-by: Fabio Forni <development@redaril.me>
---
Changes in v4:
- The patch that fixes the use-after-free bug is applied before converting
  mux to fwnode, so that it can potentially be backported.
- Link to v3: https://lore.kernel.org/r/20260929-mux_fwnode-v3-0-9b3b9ae1334e@redaril.me

Changes in v3:
- Expand documentation of mux_chip_find_by_fwnode().
- Fix use-after-free bug in mux_get().
- Link to v2: https://lore.kernel.org/r/20260916-mux_fwnode-v2-1-58f1d85b9dde@redaril.me

Changes in v2:
- Rename devm_mux_state_get_from_swnode into devm_mux_state_get_from_fwnode
- Link to v1: https://lore.kernel.org/r/20260915-mux_fwnode-v1-1-ed5a6d8202d4@redaril.me

---
Fabio Forni (3):
      mux: Avoid use-after-free of args.np in mux_get()
      mux: convert to use fwnode interface
      mux: Document mux_chip_find_by_fwnode()

 drivers/mux/core.c                    | 135 ++++++++++++++++++++--------------
 drivers/pinctrl/pinctrl-generic-mux.c |   4 +-
 include/linux/mux/consumer.h          |   6 +-
 3 files changed, 87 insertions(+), 58 deletions(-)
---
base-commit: 6c377d19d4a5116d9bec5203aa3c6c11523e7898
change-id: 20260915-mux_fwnode-a16593c39ffd

Best regards,
-- 
Fabio Forni <development@redaril.me>

[PATCH v4 1/3] mux: Avoid use-after-free of args.np in mux_get()

[Fabio Forni via B4 Relay]

From: Fabio Forni <development@redaril.me>

of_node_put(args.np) was called right after
of_find_mux_chip_by_node(), but it was too early because the error
handling code below would pass args.np to dev_err().
Let's move all freeing functions to the bottom of mux_get() to avoid
use-after-free bugs.

Signed-off-by: Fabio Forni <development@redaril.me>
Suggested-by: Alvin Šipraga <alvin.sipraga@analog.com>
Fixes: a3b02a9c6591 ("mux: minimal mux subsystem")
---
 drivers/mux/core.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 5083e3d19606..6ca40d73ea0e 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -535,6 +535,9 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
  * @optional: Whether to return NULL and silence errors when mux doesn't exist.
  * @node: the device nodes, use dev->of_node if it is NULL.
  *
+ * When a mux-control is found, it is the caller's responsibility to call
+ * mux_control_put() on it when it is no longer needed.
+ *
  * Return: Pointer to the mux-control on success, an ERR_PTR with a negative
  * errno on error, or NULL if optional is true and mux doesn't exist.
  */
@@ -584,9 +587,10 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 	}
 
 	mux_chip = of_find_mux_chip_by_node(args.np);
-	of_node_put(args.np);
-	if (!mux_chip)
-		return ERR_PTR(-EPROBE_DEFER);
+	if (!mux_chip) {
+		ret = -EPROBE_DEFER;
+		goto end;
+	}
 
 	controller = 0;
 	if (state) {
@@ -594,8 +598,8 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 		    (args.args_count < 2 && mux_chip->controllers > 1)) {
 			dev_err(dev, "%pOF: wrong #mux-state-cells for %pOF\n",
 				np, args.np);
-			put_device(&mux_chip->dev);
-			return ERR_PTR(-EINVAL);
+			ret = -EINVAL;
+			goto end;
 		}
 
 		if (args.args_count == 2) {
@@ -610,8 +614,8 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 		    (!args.args_count && mux_chip->controllers > 1)) {
 			dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
 				np, args.np);
-			put_device(&mux_chip->dev);
-			return ERR_PTR(-EINVAL);
+			ret = -EINVAL;
+			goto end;
 		}
 
 		if (args.args_count)
@@ -621,8 +625,16 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 	if (controller >= mux_chip->controllers) {
 		dev_err(dev, "%pOF: bad mux controller %u specified in %pOF\n",
 			np, controller, args.np);
-		put_device(&mux_chip->dev);
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto end;
+	}
+
+end:
+	of_node_put(args.np);
+	if (ret < 0) {
+		if (mux_chip)
+			put_device(&mux_chip->dev);
+		return ERR_PTR(ret);
 	}
 
 	return &mux_chip->mux[controller];

-- 
2.56.0

[PATCH v4 2/3] mux: convert to use fwnode interface

[Fabio Forni via B4 Relay]

From: Fabio Forni <development@redaril.me>

As firmware node is a more common abstract, this will convert the whole
thing to fwnode interface.

Co-developed-by: Xu Yang <xu.yang_2@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
Signed-off-by: Fabio Forni <development@redaril.me>
---
 drivers/mux/core.c                    | 96 ++++++++++++++++++-----------------
 drivers/pinctrl/pinctrl-generic-mux.c |  4 +-
 include/linux/mux/consumer.h          |  6 ++-
 3 files changed, 57 insertions(+), 49 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 6ca40d73ea0e..d5095905b02e 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -18,7 +18,7 @@
 #include <linux/module.h>
 #include <linux/mux/consumer.h>
 #include <linux/mux/driver.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 
 /*
@@ -118,6 +118,7 @@ struct mux_chip *mux_chip_alloc(struct device *dev,
 	mux_chip->dev.type = &mux_type;
 	mux_chip->dev.parent = dev;
 	mux_chip->dev.of_node = dev->of_node;
+	mux_chip->dev.fwnode = dev->fwnode;
 	dev_set_drvdata(&mux_chip->dev, mux_chip);
 
 	mux_chip->id = ida_alloc(&mux_ida, GFP_KERNEL);
@@ -517,11 +518,11 @@ int mux_state_deselect(struct mux_state *mstate)
 EXPORT_SYMBOL_GPL(mux_state_deselect);
 
 /* Note this function returns a reference to the mux_chip dev. */
-static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
+static struct mux_chip *mux_chip_find_by_fwnode(struct fwnode_handle *fwnode)
 {
 	struct device *dev;
 
-	dev = class_find_device_by_of_node(&mux_class, np);
+	dev = class_find_device_by_fwnode(&mux_class, fwnode);
 
 	return dev ? to_mux_chip(dev) : NULL;
 }
@@ -533,7 +534,7 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
  * @state: Pointer to where the requested state is returned, or NULL when
  *         the required multiplexer states are handled by other means.
  * @optional: Whether to return NULL and silence errors when mux doesn't exist.
- * @node: the device nodes, use dev->of_node if it is NULL.
+ * @node: the device nodes, use dev's fwnode if it is NULL.
  *
  * When a mux-control is found, it is the caller's responsibility to call
  * mux_control_put() on it when it is no longer needed.
@@ -543,10 +544,10 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
  */
 static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 				   unsigned int *state, bool optional,
-				   struct device_node *node)
+				   struct fwnode_handle *node)
 {
-	struct device_node *np = node ? node : dev->of_node;
-	struct of_phandle_args args;
+	struct fwnode_handle *fwnode = node ? node : dev_fwnode(dev);
+	struct fwnode_reference_args args;
 	struct mux_chip *mux_chip;
 	unsigned int controller;
 	int index = 0;
@@ -554,11 +555,13 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 
 	if (mux_name) {
 		if (state)
-			index = of_property_match_string(np, "mux-state-names",
-							 mux_name);
+			index = fwnode_property_match_string(fwnode,
+							     "mux-state-names",
+							     mux_name);
 		else
-			index = of_property_match_string(np, "mux-control-names",
-							 mux_name);
+			index = fwnode_property_match_string(fwnode,
+							     "mux-control-names",
+							     mux_name);
 		if (index < 0 && optional) {
 			return NULL;
 		} else if (index < 0) {
@@ -569,24 +572,25 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 	}
 
 	if (state)
-		ret = of_parse_phandle_with_args(np,
-						 "mux-states", "#mux-state-cells",
-						 index, &args);
+		ret = fwnode_property_get_reference_args(fwnode, "mux-states",
+							 "#mux-state-cells", 0,
+							 index, &args);
 	else
-		ret = of_parse_phandle_with_args(np,
-						 "mux-controls", "#mux-control-cells",
-						 index, &args);
+		ret = fwnode_property_get_reference_args(fwnode,
+							 "mux-controls", "#mux-control-cells",
+							 0, index, &args);
+
 	if (ret) {
 		if (optional && ret == -ENOENT)
 			return NULL;
 
-		dev_err(dev, "%pOF: failed to get mux-%s %s(%i)\n",
-			np, state ? "state" : "control",
-			mux_name ?: "", index);
+		dev_err(dev, "%pfw: failed to get mux-%s %s(%i)\n",
+			fwnode, state ? "state" : "control", mux_name ?: "",
+			index);
 		return ERR_PTR(ret);
 	}
 
-	mux_chip = of_find_mux_chip_by_node(args.np);
+	mux_chip = mux_chip_find_by_fwnode(args.fwnode);
 	if (!mux_chip) {
 		ret = -EPROBE_DEFER;
 		goto end;
@@ -594,15 +598,15 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 
 	controller = 0;
 	if (state) {
-		if (args.args_count > 2 || args.args_count == 0 ||
-		    (args.args_count < 2 && mux_chip->controllers > 1)) {
-			dev_err(dev, "%pOF: wrong #mux-state-cells for %pOF\n",
-				np, args.np);
+		if (args.nargs > 2 || args.nargs == 0 ||
+		    (args.nargs < 2 && mux_chip->controllers > 1)) {
+			dev_err(dev, "%pfw: wrong #mux-state-cells for %pfw\n",
+				fwnode, args.fwnode);
 			ret = -EINVAL;
 			goto end;
 		}
 
-		if (args.args_count == 2) {
+		if (args.nargs == 2) {
 			controller = args.args[0];
 			*state = args.args[1];
 		} else {
@@ -610,27 +614,27 @@ static struct mux_control *mux_get(struct device *dev, const char *mux_name,
 		}
 
 	} else {
-		if (args.args_count > 1 ||
-		    (!args.args_count && mux_chip->controllers > 1)) {
-			dev_err(dev, "%pOF: wrong #mux-control-cells for %pOF\n",
-				np, args.np);
+		if (args.nargs > 1 ||
+		    (!args.nargs && mux_chip->controllers > 1)) {
+			dev_err(dev, "%pfw: wrong #mux-control-cells for %pfw\n",
+				fwnode, args.fwnode);
 			ret = -EINVAL;
 			goto end;
 		}
 
-		if (args.args_count)
+		if (args.nargs)
 			controller = args.args[0];
 	}
 
 	if (controller >= mux_chip->controllers) {
-		dev_err(dev, "%pOF: bad mux controller %u specified in %pOF\n",
-			np, controller, args.np);
+		dev_err(dev, "%pfw: bad mux controller %u specified in %pfw\n",
+			fwnode, controller, args.fwnode);
 		ret = -EINVAL;
 		goto end;
 	}
 
 end:
-	of_node_put(args.np);
+	fwnode_handle_put(args.fwnode);
 	if (ret < 0) {
 		if (mux_chip)
 			put_device(&mux_chip->dev);
@@ -726,14 +730,14 @@ EXPORT_SYMBOL_GPL(devm_mux_control_get);
  * @dev: The device that needs a mux-state.
  * @mux_name: The name identifying the mux-state.
  * @optional: Whether to return NULL and silence errors when mux doesn't exist.
- * @np: the device nodes, use dev->of_node if it is NULL.
+ * @node: the device nodes, use dev's fwnode if it is NULL.
  *
  * Return: Pointer to the mux-state on success, an ERR_PTR with a negative
  * errno on error, or NULL if optional is true and mux doesn't exist.
  */
 static struct mux_state *
 mux_state_get(struct device *dev, const char *mux_name, bool optional,
-	      struct device_node *np)
+	      struct fwnode_handle *node)
 {
 	struct mux_state *mstate;
 
@@ -741,7 +745,7 @@ mux_state_get(struct device *dev, const char *mux_name, bool optional,
 	if (!mstate)
 		return ERR_PTR(-ENOMEM);
 
-	mstate->mux = mux_get(dev, mux_name, &mstate->state, optional, np);
+	mstate->mux = mux_get(dev, mux_name, &mstate->state, optional, node);
 	if (IS_ERR(mstate->mux)) {
 		int err = PTR_ERR(mstate->mux);
 
@@ -783,7 +787,7 @@ static void devm_mux_state_release(struct device *dev, void *res)
  * @dev: The device that needs a mux-state.
  * @mux_name: The name identifying the mux-state.
  * @optional: Whether to return NULL and silence errors when mux doesn't exist.
- * @np: The device nodes, use dev->of_node if it is NULL.
+ * @node: The device nodes, use dev's fwnode if it is NULL.
  * @init: Optional function pointer for mux-state object initialisation.
  * @exit: Optional function pointer for mux-state object cleanup on release.
  *
@@ -791,7 +795,7 @@ static void devm_mux_state_release(struct device *dev, void *res)
  * errno on error, or NULL if optional is true and mux doesn't exist.
  */
 static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mux_name,
-					      bool optional, struct device_node *np,
+					      bool optional, struct fwnode_handle *node,
 					      int (*init)(struct mux_state *mstate),
 					      int (*exit)(struct mux_state *mstate))
 {
@@ -799,7 +803,7 @@ static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mu
 	struct mux_state *mstate;
 	int ret;
 
-	mstate = mux_state_get(dev, mux_name, optional, np);
+	mstate = mux_state_get(dev, mux_name, optional, node);
 	if (IS_ERR(mstate))
 		return ERR_CAST(mstate);
 	else if (optional && !mstate)
@@ -833,23 +837,23 @@ static struct mux_state *__devm_mux_state_get(struct device *dev, const char *mu
 }
 
 /**
- * devm_mux_state_get_from_np() - Get the mux-state for a device, with resource
+ * devm_mux_state_get_from_fwnode() - Get the mux-state for a device, with resource
  *				  management.
  * @dev: The device that needs a mux-control.
  * @mux_name: The name identifying the mux-control.
- * @np: the device nodes, use dev->of_node if it is NULL.
+ * @node: the device nodes, use dev's fwnode if it is NULL.
  *
  * Return: Pointer to the mux-state, or an ERR_PTR with a negative errno.
  *
  * The mux-state will automatically be freed on release.
  */
 struct mux_state *
-devm_mux_state_get_from_np(struct device *dev, const char *mux_name,
-			   struct device_node *np)
+devm_mux_state_get_from_fwnode(struct device *dev, const char *mux_name,
+			       struct fwnode_handle *node)
 {
-	return __devm_mux_state_get(dev, mux_name, false, np, NULL, NULL);
+	return __devm_mux_state_get(dev, mux_name, false, node, NULL, NULL);
 }
-EXPORT_SYMBOL_GPL(devm_mux_state_get_from_np);
+EXPORT_SYMBOL_GPL(devm_mux_state_get_from_fwnode);
 
 /**
  * devm_mux_state_get_optional() - Get the optional mux-state for a device,
diff --git a/drivers/pinctrl/pinctrl-generic-mux.c b/drivers/pinctrl/pinctrl-generic-mux.c
index 202b72351efb..6d5b6100c5ca 100644
--- a/drivers/pinctrl/pinctrl-generic-mux.c
+++ b/drivers/pinctrl/pinctrl-generic-mux.c
@@ -50,7 +50,9 @@ mux_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
 	if (!group_names)
 		return -ENOMEM;
 
-	function->mux_state = devm_mux_state_get_from_np(pctldev->dev, NULL, np_config);
+	function->mux_state = devm_mux_state_get_from_fwnode(pctldev->dev,
+							     NULL,
+							     of_fwnode_handle(np_config));
 	if (IS_ERR(function->mux_state))
 		return PTR_ERR(function->mux_state);
 
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index 449e38e6e2c5..7d121217ca96 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -11,6 +11,7 @@
 #define _LINUX_MUX_CONSUMER_H
 
 #include <linux/compiler.h>
+#include <linux/device.h>
 
 struct device;
 struct mux_control;
@@ -62,7 +63,8 @@ void mux_control_put(struct mux_control *mux);
 struct mux_control *devm_mux_control_get(struct device *dev, const char *mux_name);
 
 struct mux_state *
-devm_mux_state_get_from_np(struct device *dev, const char *mux_name, struct device_node *np);
+devm_mux_state_get_from_fwnode(struct device *dev, const char *mux_name,
+			       struct fwnode_handle *node);
 
 struct mux_state *devm_mux_state_get_optional(struct device *dev, const char *mux_name);
 struct mux_state *devm_mux_state_get_selected(struct device *dev, const char *mux_name);
@@ -165,6 +167,6 @@ static inline struct mux_state *devm_mux_state_get_optional_selected(struct devi
 #endif /* CONFIG_MULTIPLEXER */
 
 #define devm_mux_state_get(dev, mux_name)		\
-	devm_mux_state_get_from_np(dev, mux_name, NULL)
+	devm_mux_state_get_from_fwnode(dev, mux_name, NULL)
 
 #endif /* _LINUX_MUX_CONSUMER_H */

-- 
2.56.0

Re: [PATCH v4 2/3] mux: convert to use fwnode interface

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pinctrl: memory leak in dt_node_to_map on probe deferral
--

commit a66771846f5e6a77740838d742398ffebadf6b60
Author: Fabio Forni <development@redaril.me>

mux: convert to use fwnode interface

This commit converts the mux subsystem to use the fwnode interface
instead of device nodes, as firmware node is a more common abstraction.

> diff --git a/drivers/pinctrl/pinctrl-generic-mux.c b/drivers/pinctrl/pinctrl-generic-mux.c
> index 202b72351efb7..6d5b6100c5ca2 100644
> --- a/drivers/pinctrl/pinctrl-generic-mux.c
> +++ b/drivers/pinctrl/pinctrl-generic-mux.c
[ ... ]
> @@ -50,7 +50,9 @@ mux_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
>  	if (!group_names)
>  		return -ENOMEM;
>  
> -	function->mux_state = devm_mux_state_get_from_np(pctldev->dev, NULL, np_config);
> +	function->mux_state = devm_mux_state_get_from_fwnode(pctldev->dev,
> +							     NULL,
> +							     of_fwnode_handle(np_config));

[Severity: Medium]
Will this leak memory if the consumer device defers probing?

The mux_pinmux_dt_node_to_map() function is called by the pinctrl core each
time a consumer parses its DT mappings. It allocates devm resources explicitly
bound to pctldev->dev (the pinctrl provider device), including the mux_state
allocated here, as well as the function and group_names allocated earlier in
the same function:

	function = devm_kzalloc(pctldev->dev, sizeof(*function), GFP_KERNEL);
	if (!function)
		return -ENOMEM;

	group_names = devm_kcalloc(pctldev->dev, 1, sizeof(*group_names), GFP_KERNEL);
	if (!group_names)
		return -ENOMEM;

If the consumer device's probe later returns -EPROBE_DEFER, the consumer's
pinctrl mappings are freed, but these devm allocations remain permanently bound
to the provider device. 

When the consumer reprobes, it triggers this function again, returning the
existing selector from pinmux_generic_add_function() and completely leaking
the newly allocated devm resources on the provider's devres list. Repeated
probe deferrals can cause an unbounded memory leak.

Should these allocations be tied to the consumer device instead, or explicitly
freed when the map is released?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-mux_fwnode-v4-0-ba3330e44b2a@redaril.me?part=2

[PATCH v4 3/3] mux: Document mux_chip_find_by_fwnode()

[Fabio Forni via B4 Relay]

From: Fabio Forni <development@redaril.me>

What mux_chip_find_by_fwnode() does it pretty obvious. What's
not-so-obvious is that the reference count of mux_chip->dev must be
decreased when done working with it. This commit clarifies the
requirement.

Signed-off-by: Fabio Forni <development@redaril.me>
---
 drivers/mux/core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index d5095905b02e..6289cc98f49e 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -517,7 +517,16 @@ int mux_state_deselect(struct mux_state *mstate)
 }
 EXPORT_SYMBOL_GPL(mux_state_deselect);
 
-/* Note this function returns a reference to the mux_chip dev. */
+/**
+ * mux_chip_find_by_fwnode() - Find a mux-chip by its fwnode.
+ * @fwnode: The fwnode representing the mux-chip.
+ *
+ * When a mux-chip is found, the mux-chip increases the reference count of
+ * the underlying &struct device. The caller is responsible for calling
+ * put_device() on it.
+ *
+ * Return: Pointer to the mux-chip, or NULL if not found.
+ */
 static struct mux_chip *mux_chip_find_by_fwnode(struct fwnode_handle *fwnode)
 {
 	struct device *dev;

-- 
2.56.0