[PATCH 0/2] pinctrl: pinconf-generic: clean up pinconf_parse_dt_config()

[PATCH 0/2] pinctrl: pinconf-generic: clean up pinconf_parse_dt_config()

[Masahiro Yamada]

Masahiro Yamada (2):
  pinctrl: pinconf-generic: resize the pin config array directly
  pinctrl: pinconf-generic: remove the special handling for no config
    case

 drivers/pinctrl/pinconf-generic.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

-- 
2.40.1

[PATCH 1/2] pinctrl: pinconf-generic: resize the pin config array directly

[Masahiro Yamada]

pinconf_generic_parse_dt_config() allocates memory that is large enough
to contain all the config parameters. Then, kmemdup() copies the found
configs to the memory with the exact size.

There is no need to allocate memory twice; you can directly resize the
initial memory using krealloc_array().

I also changed kcalloc() to kmalloc_array() to keep the consistency with
krealloc_array(). This change has no impact because you do not need to
zero out the 'cfg' array.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/pinctrl/pinconf-generic.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 8313cb5f3b3c..ba4fe2466e78 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -247,7 +247,6 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 {
 	unsigned long *cfg;
 	unsigned int max_cfg, ncfg = 0;
-	int ret;
 
 	if (!np)
 		return -EINVAL;
@@ -256,7 +255,7 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 	max_cfg = ARRAY_SIZE(dt_params);
 	if (pctldev)
 		max_cfg += pctldev->desc->num_custom_params;
-	cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
+	cfg = kmalloc_array(max_cfg, sizeof(*cfg), GFP_KERNEL);
 	if (!cfg)
 		return -ENOMEM;
 
@@ -266,30 +265,22 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		parse_dt_cfg(np, pctldev->desc->custom_params,
 			     pctldev->desc->num_custom_params, cfg, &ncfg);
 
-	ret = 0;
-
 	/* no configs found at all */
 	if (ncfg == 0) {
+		kfree(cfg);
 		*configs = NULL;
 		*nconfigs = 0;
-		goto out;
+		return 0;
 	}
 
-	/*
-	 * Now limit the number of configs to the real number of
-	 * found properties.
-	 */
-	*configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
-	if (!*configs) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	/* Now resize the array to store the real number of found properties. */
+	*configs = krealloc_array(cfg, ncfg, sizeof(unsigned long), GFP_KERNEL);
+	if (!*configs)
+		return -ENOMEM;
 
 	*nconfigs = ncfg;
 
-out:
-	kfree(cfg);
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(pinconf_generic_parse_dt_config);
 
-- 
2.40.1

[PATCH 2/2] pinctrl: pinconf-generic: remove the special handling for no config case

[Masahiro Yamada]

To further simplify pinconf_generic_parse_dt_config(), eliminate the
handling of the case where no configuration is found.

When ncfg is zero, krealloc_array() will set ZERO_SIZE_PTR to *configs,
which is a natural approach for managing a zero-size buffer.

This should have no impact because none of the callers accesses 'configs'
when ncfg is zero. Also, it is safe to pass ZERO_SIZE_PTR to kfree().

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 drivers/pinctrl/pinconf-generic.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index ba4fe2466e78..252d69ee2b68 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -265,14 +265,6 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
 		parse_dt_cfg(np, pctldev->desc->custom_params,
 			     pctldev->desc->num_custom_params, cfg, &ncfg);
 
-	/* no configs found at all */
-	if (ncfg == 0) {
-		kfree(cfg);
-		*configs = NULL;
-		*nconfigs = 0;
-		return 0;
-	}
-
 	/* Now resize the array to store the real number of found properties. */
 	*configs = krealloc_array(cfg, ncfg, sizeof(unsigned long), GFP_KERNEL);
 	if (!*configs)
-- 
2.40.1

Re: [PATCH 1/2] pinctrl: pinconf-generic: resize the pin config array directly

[Masahiro Yamada]

On Tue, Nov 21, 2023 at 7:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> pinconf_generic_parse_dt_config() allocates memory that is large enough
> to contain all the config parameters. Then, kmemdup() copies the found
> configs to the memory with the exact size.
>
> There is no need to allocate memory twice; you can directly resize the
> initial memory using krealloc_array().
>
> I also changed kcalloc() to kmalloc_array() to keep the consistency with
> krealloc_array(). This change has no impact because you do not need to
> zero out the 'cfg' array.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>



Sorry, I retract this patch set.

krealloc() does not save any memory
when the new_size is smaller than the current size.













> ---
>
>  drivers/pinctrl/pinconf-generic.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
> index 8313cb5f3b3c..ba4fe2466e78 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -247,7 +247,6 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
>  {
>         unsigned long *cfg;
>         unsigned int max_cfg, ncfg = 0;
> -       int ret;
>
>         if (!np)
>                 return -EINVAL;
> @@ -256,7 +255,7 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
>         max_cfg = ARRAY_SIZE(dt_params);
>         if (pctldev)
>                 max_cfg += pctldev->desc->num_custom_params;
> -       cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
> +       cfg = kmalloc_array(max_cfg, sizeof(*cfg), GFP_KERNEL);
>         if (!cfg)
>                 return -ENOMEM;
>
> @@ -266,30 +265,22 @@ int pinconf_generic_parse_dt_config(struct device_node *np,
>                 parse_dt_cfg(np, pctldev->desc->custom_params,
>                              pctldev->desc->num_custom_params, cfg, &ncfg);
>
> -       ret = 0;
> -
>         /* no configs found at all */
>         if (ncfg == 0) {
> +               kfree(cfg);
>                 *configs = NULL;
>                 *nconfigs = 0;
> -               goto out;
> +               return 0;
>         }
>
> -       /*
> -        * Now limit the number of configs to the real number of
> -        * found properties.
> -        */
> -       *configs = kmemdup(cfg, ncfg * sizeof(unsigned long), GFP_KERNEL);
> -       if (!*configs) {
> -               ret = -ENOMEM;
> -               goto out;
> -       }
> +       /* Now resize the array to store the real number of found properties. */
> +       *configs = krealloc_array(cfg, ncfg, sizeof(unsigned long), GFP_KERNEL);
> +       if (!*configs)
> +               return -ENOMEM;
>
>         *nconfigs = ncfg;
>
> -out:
> -       kfree(cfg);
> -       return ret;
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(pinconf_generic_parse_dt_config);
>
> --
> 2.40.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] pinctrl: pinconf-generic: resize the pin config array directly

[Linus Walleij]

Hi Masahiro,

On Tue, Nov 21, 2023 at 11:21 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> On Tue, Nov 21, 2023 at 7:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > pinconf_generic_parse_dt_config() allocates memory that is large enough
> > to contain all the config parameters. Then, kmemdup() copies the found
> > configs to the memory with the exact size.
> >
> > There is no need to allocate memory twice; you can directly resize the
> > initial memory using krealloc_array().
> >
> > I also changed kcalloc() to kmalloc_array() to keep the consistency with
> > krealloc_array(). This change has no impact because you do not need to
> > zero out the 'cfg' array.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>
> Sorry, I retract this patch set.
>
> krealloc() does not save any memory
> when the new_size is smaller than the current size.

But the first part where you switch to kmalloc_array() is still a nice change.

The fact that we use kmemdup to be able to also shrink the allocation is a
bit of an oddity I guess, but let's run this patch by Andy Shevchenko, and
ask what he thinks about simply introducing kmemdup_array() or if he
has other ideas for this.

Yours,
Linus Walleij

Re: [PATCH 1/2] pinctrl: pinconf-generic: resize the pin config array directly

[Andy Shevchenko]

On Fri, Nov 24, 2023 at 11:06:50AM +0100, Linus Walleij wrote:
> On Tue, Nov 21, 2023 at 11:21 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > On Tue, Nov 21, 2023 at 7:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > pinconf_generic_parse_dt_config() allocates memory that is large enough
> > > to contain all the config parameters. Then, kmemdup() copies the found
> > > configs to the memory with the exact size.
> > >
> > > There is no need to allocate memory twice; you can directly resize the
> > > initial memory using krealloc_array().
> > >
> > > I also changed kcalloc() to kmalloc_array() to keep the consistency with
> > > krealloc_array(). This change has no impact because you do not need to
> > > zero out the 'cfg' array.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> >
> > Sorry, I retract this patch set.
> >
> > krealloc() does not save any memory
> > when the new_size is smaller than the current size.
> 
> But the first part where you switch to kmalloc_array() is still a nice change.
> 
> The fact that we use kmemdup to be able to also shrink the allocation is a
> bit of an oddity I guess, but let's run this patch by Andy Shevchenko, and
> ask what he thinks about simply introducing kmemdup_array() or if he
> has other ideas for this.

https://lore.kernel.org/all/20231017052322.2636-2-kkartik@nvidia.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] pinctrl: pinconf-generic: resize the pin config array directly

[Masahiro Yamada]

On Fri, Nov 24, 2023 at 8:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Nov 24, 2023 at 11:06:50AM +0100, Linus Walleij wrote:
> > On Tue, Nov 21, 2023 at 11:21 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > On Tue, Nov 21, 2023 at 7:28 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> > > >
> > > > pinconf_generic_parse_dt_config() allocates memory that is large enough
> > > > to contain all the config parameters. Then, kmemdup() copies the found
> > > > configs to the memory with the exact size.
> > > >
> > > > There is no need to allocate memory twice; you can directly resize the
> > > > initial memory using krealloc_array().
> > > >
> > > > I also changed kcalloc() to kmalloc_array() to keep the consistency with
> > > > krealloc_array(). This change has no impact because you do not need to
> > > > zero out the 'cfg' array.
> > > >
> > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > >
> > > Sorry, I retract this patch set.
> > >
> > > krealloc() does not save any memory
> > > when the new_size is smaller than the current size.
> >
> > But the first part where you switch to kmalloc_array() is still a nice change.
> >
> > The fact that we use kmemdup to be able to also shrink the allocation is a
> > bit of an oddity I guess, but let's run this patch by Andy Shevchenko, and
> > ask what he thinks about simply introducing kmemdup_array() or if he
> > has other ideas for this.
>
> https://lore.kernel.org/all/20231017052322.2636-2-kkartik@nvidia.com/


Ok, I will come back when kmemdup_array() is upstreamed.



> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Best Regards
Masahiro Yamada