| Message ID | 20231120222832.4063882-1-masahiroy@kernel.org |
|---|---|
| Headers | show |
| Series | pinctrl: pinconf-generic: clean up pinconf_parse_dt_config() | expand |
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 >
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
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/
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 > >
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(-)