* Re: [PATCH] pinctrl: pinctrl-generic-mux: Fix provider resource leak on re-parse
2026-09-16 3:04 [PATCH] pinctrl: pinctrl-generic-mux: Fix provider resource leak on re-parse Chancel Liu
@ 2026-09-16 20:23 ` Frank Li
2026-09-17 2:29 ` Chancel Liu
2026-09-17 21:02 ` Frank Li
1 sibling, 1 reply; 4+ messages in thread
From: Frank Li @ 2026-09-16 20:23 UTC (permalink / raw)
To: Chancel Liu; +Cc: linusw, Frank.Li, linux-gpio, linux-kernel
On Wed, Sep 16, 2026 at 12:04:35PM +0900, Chancel Liu wrote:
> From: Chancel Liu <chancel.liu@nxp.com>
>
> The pinctrl core re-parses a consumer's pinctrl on every probe attempt
> (pinctrl_bind_pins() runs before the driver's probe(), and again on each
> deferred-probe retry or re-bind), so .dt_node_to_map() must be side-effect
> free and safe to call repeatedly.
>
> mux_pinmux_dt_node_to_map() instead allocated provider-side resources on
> every call - the mux_pin_function, the group name table and a mux_state
> reference, plus a group name and a pinctrl_generic_add_group()
> registration via pinctrl_generic_to_map(). All of these are allocated
> with devm_*() against the provider device, so they live for the provider's
> lifetime and are only released when the provider itself is unbound. The
> .dt_free_map() path never touches them; it frees the per-consumer
> pinctrl_map only. Every re-parse therefore adds another set of
> provider-side allocations that are never reclaimed, so a consumer that
> repeatedly defers probe or is re-bound leaks memory and mux_state
> references on the provider without bound.
>
> The groups and functions of a board-level mux are static and fully
> described by the device tree, so build them once at probe time in the new
> mux_pinctrl_probe_dt(). mux_pinmux_dt_node_to_map() then only looks up the
> already registered group and builds the per-consumer mux map, which the
> core frees via .dt_free_map. As the mux has no pin electrical
> configuration, no config map is emitted.
>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260824022020.2812810-1-chancel.liu%40oss.nxp.com
> Fixes: 34acc5a8adfb ("pinctrl: add generic board-level pinctrl driver using mux framework")
> Signed-off-by: Chancel Liu <chancel.liu@nxp.com>
> Assisted-by: VeroCoder:claude-opus-4-8
> ---
> drivers/pinctrl/pinctrl-generic-mux.c | 76 ++++++++++++++++++++-------
> 1 file changed, 56 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-generic-mux.c b/drivers/pinctrl/pinctrl-generic-mux.c
> index 202b72351efb..de85b4e75eb8 100644
> --- a/drivers/pinctrl/pinctrl-generic-mux.c
> +++ b/drivers/pinctrl/pinctrl-generic-mux.c
> @@ -10,13 +10,11 @@
> #include <linux/mutex.h>
> #include <linux/mux/consumer.h>
> #include <linux/platform_device.h>
> -#include <linux/pinctrl/pinconf-generic.h>
> #include <linux/pinctrl/pinctrl.h>
> #include <linux/pinctrl/pinmux.h>
> #include <linux/slab.h>
>
> #include "core.h"
> -#include "pinconf.h"
> #include "pinmux.h"
> #include "pinctrl-utils.h"
>
> @@ -38,39 +36,73 @@ mux_pinmux_dt_node_to_map(struct pinctrl_dev *pctldev,
> struct pinctrl_map **maps, unsigned int *num_maps)
> {
> unsigned int num_reserved_maps = 0;
> - struct mux_pin_function *function;
> - const char **group_names;
> int ret;
>
> - 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;
devm alloc is quite popular under drivers/pinctrl
airoha/pinctrl-airoha.c dt_node_to_map devm_kzalloc, devm_pinctrl_register_and_init
meson/pinctrl-amlogic-a4.c dt_node_to_map devm_kcalloc
pinctrl-at91.c at91_dt_node_to_map devm_kcalloc, devm_kfree
pinctrl-axp209.c dt_node_to_map devm_kcalloc
pinctrl-generic-mux.c mux_pinmux_dt_node_to_map devm_kcalloc, devm_kzalloc, devm_mux_state_get_from_np
pinctrl-generic.c pinctrl_generic_dt_node_to_map devm_kcalloc
pinctrl-single.c pcs_dt_node_to_map devm_kcalloc, devm_kfree, devm_kzalloc
pinctrl-st.c st_pctl_dt_node_to_map devm_kcalloc, devm_kfree
pinctrl-th1520.c th1520_pinctrl_dt_node_to_map devm_kasprintf, devm_kcalloc
pinctrl-tps6594.c dt_node_to_map devm_gpio_regmap_register, devm_kzalloc, devm_pinctrl_register
renesas/pinctrl-rza1.c rza1_dt_node_to_map devm_kcalloc, devm_kzalloc
renesas/pinctrl-rza2.c rza2_dt_node_to_map devm_kcalloc, devm_kzalloc
sophgo/pinctrl-sophgo-common.c sophgo_pctrl_dt_node_to_map devm_kasprintf, devm_kcalloc
starfive/pinctrl-starfive-jh7100.c starfive_dt_node_to_map devm_kasprintf, devm_kcalloc
starfive/pinctrl-starfive-jh7110.c jh7110_dt_node_to_map devm_kasprintf, devm_kcalloc
sunplus/sppctl.c dt_node_to_map devm_kcalloc
ti/pinctrl-ti-iodelay.c ti_iodelay_dt_node_to_map devm_kcalloc, devm_kfree, devm_kzalloc
does all have similar problem? I suspect this when do this patch, but I
have not tracked these life cycle.
Frank
> -
> - function->mux_state = devm_mux_state_get_from_np(pctldev->dev, NULL, np_config);
> - if (IS_ERR(function->mux_state))
> - return PTR_ERR(function->mux_state);
> + *maps = NULL;
> + *num_maps = 0;
>
> - ret = pinctrl_generic_to_map(pctldev, np_config, np_config, maps,
> - num_maps, &num_reserved_maps, group_names,
> - 0, &np_config->name, NULL, 0);
> + if (pinctrl_get_group_selector(pctldev, np_config->name) < 0)
> + return -ENODEV;
>
> + ret = pinctrl_utils_reserve_map(pctldev, maps, &num_reserved_maps,
> + num_maps, 1);
> if (ret)
> return ret;
>
> - ret = pinmux_generic_add_function(pctldev, np_config->name, group_names,
> - 1, function);
> + ret = pinctrl_utils_add_map_mux(pctldev, maps, &num_reserved_maps,
> + num_maps, np_config->name,
> + np_config->name);
> if (ret < 0) {
> pinctrl_utils_free_map(pctldev, *maps, *num_maps);
> + *maps = NULL;
> + *num_maps = 0;
> return ret;
> }
>
> return 0;
> }
>
> +static int mux_pinctrl_probe_dt(struct pinctrl_dev *pctldev,
> + struct device_node *np)
> +{
> + struct device *dev = pctldev->dev;
> +
> + for_each_available_child_of_node_scoped(np, grp) {
> + struct mux_pin_function *function;
> + const char **group_names;
> + int ret;
> +
> + function = devm_kzalloc(dev, sizeof(*function), GFP_KERNEL);
> + if (!function)
> + return -ENOMEM;
> +
> + group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
> + if (!group_names)
> + return -ENOMEM;
> +
> + group_names[0] = grp->name;
> +
> + function->mux_state = devm_mux_state_get_from_np(dev, NULL, grp);
> + if (IS_ERR(function->mux_state))
> + return dev_err_probe(dev, PTR_ERR(function->mux_state),
> + "failed to get mux-state for %pOFn\n",
> + grp);
> +
> + ret = pinctrl_generic_add_group(pctldev, grp->name, NULL, 0, NULL);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to add group %pOFn\n", grp);
> +
> + ret = pinmux_generic_add_function(pctldev, grp->name, group_names,
> + 1, function);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to add function %pOFn\n", grp);
> + }
> +
> + return 0;
> +}
> +
> static const struct pinctrl_ops mux_pinctrl_ops = {
> .get_groups_count = pinctrl_generic_get_group_count,
> .get_group_name = pinctrl_generic_get_group_name,
> @@ -157,6 +189,10 @@ static int mux_pinctrl_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "Failed to register pinctrl.\n");
>
> + ret = mux_pinctrl_probe_dt(mpctl->pctl, dev->of_node);
> + if (ret)
> + return ret;
> +
> ret = pinctrl_enable(mpctl->pctl);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to enable pinctrl.\n");
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] pinctrl: pinctrl-generic-mux: Fix provider resource leak on re-parse
2026-09-16 3:04 [PATCH] pinctrl: pinctrl-generic-mux: Fix provider resource leak on re-parse Chancel Liu
2026-09-16 20:23 ` Frank Li
@ 2026-09-17 21:02 ` Frank Li
1 sibling, 0 replies; 4+ messages in thread
From: Frank Li @ 2026-09-17 21:02 UTC (permalink / raw)
To: Chancel Liu; +Cc: linusw, Frank.Li, linux-gpio, linux-kernel
On Wed, Sep 16, 2026 at 12:04:35PM +0900, Chancel Liu wrote:
> From: Chancel Liu <chancel.liu@nxp.com>
>
...
>
> +static int mux_pinctrl_probe_dt(struct pinctrl_dev *pctldev,
> + struct device_node *np)
> +{
> + struct device *dev = pctldev->dev;
> +
> + for_each_available_child_of_node_scoped(np, grp) {
> + struct mux_pin_function *function;
> + const char **group_names;
> + int ret;
> +
> + function = devm_kzalloc(dev, sizeof(*function), GFP_KERNEL);
> + if (!function)
> + return -ENOMEM;
> +
> + group_names = devm_kcalloc(dev, 1, sizeof(*group_names), GFP_KERNEL);
> + if (!group_names)
> + return -ENOMEM;
> +
> + group_names[0] = grp->name;
> +
> + function->mux_state = devm_mux_state_get_from_np(dev, NULL, grp);
> + if (IS_ERR(function->mux_state))
> + return dev_err_probe(dev, PTR_ERR(function->mux_state),
> + "failed to get mux-state for %pOFn\n",
> + grp);
> +
> + ret = pinctrl_generic_add_group(pctldev, grp->name, NULL, 0, NULL);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to add group %pOFn\n", grp);
> +
> + ret = pinmux_generic_add_function(pctldev, grp->name, group_names,
> + 1, function);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to add function %pOFn\n", grp);
> + }
> +
> + return 0;
> +}
This method also works, slice difference, if some pin have not used by
consumer, old method may save some memory.
leave to linus to decide which is better way, this one or add free() at
.dt_free_map()
Frank
> +
> static const struct pinctrl_ops mux_pinctrl_ops = {
> .get_groups_count = pinctrl_generic_get_group_count,
> .get_group_name = pinctrl_generic_get_group_name,
> @@ -157,6 +189,10 @@ static int mux_pinctrl_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "Failed to register pinctrl.\n");
>
> + ret = mux_pinctrl_probe_dt(mpctl->pctl, dev->of_node);
> + if (ret)
> + return ret;
> +
> ret = pinctrl_enable(mpctl->pctl);
> if (ret)
> return dev_err_probe(dev, ret, "Failed to enable pinctrl.\n");
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread