| Message ID | 1481207730-6332-1-git-send-email-arvind.yadav.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/12/16 14:35, Arvind Yadav wrote: > In functions pxa2xx_build_functions, the memory allocated for > 'functions' is live within the function only. After the > allocation it is immediately freed with devm_kfree. There is > no need to allocate memory for 'functions' with devm function > so replace devm_kcalloc with kcalloc and devm_kfree with kfree. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > --- > drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c > index 866aa3c..47b8e3a 100644 > --- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c > +++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c > @@ -277,7 +277,7 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) > * alternate function, 6 * npins is an absolute high limit of the number > * of functions. > */ > - functions = devm_kcalloc(pctl->dev, pctl->npins * 6, > + functions = kcalloc(pctl->npins * 6, > sizeof(*functions), GFP_KERNEL); AFAICS, this is allocating a mere 72 bytes. Why not just declare struct pxa_pinctrl_function functions[6] = {0}; locally and save *all* the bother? Robin. > if (!functions) > return -ENOMEM; > @@ -289,10 +289,12 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) > pctl->functions = devm_kmemdup(pctl->dev, functions, > pctl->nfuncs * sizeof(*functions), > GFP_KERNEL); > - if (!pctl->functions) > + if (!pctl->functions) { > + kfree(functions); > return -ENOMEM; > + } > > - devm_kfree(pctl->dev, functions); > + kfree(functions); > return 0; > } > >
On 08/12/16 15:20, Robin Murphy wrote: > On 08/12/16 14:35, Arvind Yadav wrote: >> In functions pxa2xx_build_functions, the memory allocated for >> 'functions' is live within the function only. After the >> allocation it is immediately freed with devm_kfree. There is >> no need to allocate memory for 'functions' with devm function >> so replace devm_kcalloc with kcalloc and devm_kfree with kfree. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> --- >> drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c >> index 866aa3c..47b8e3a 100644 >> --- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c >> +++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c >> @@ -277,7 +277,7 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) >> * alternate function, 6 * npins is an absolute high limit of the number >> * of functions. >> */ >> - functions = devm_kcalloc(pctl->dev, pctl->npins * 6, >> + functions = kcalloc(pctl->npins * 6, >> sizeof(*functions), GFP_KERNEL); > > AFAICS, this is allocating a mere 72 bytes. Why not just declare > > struct pxa_pinctrl_function functions[6] = {0}; > > locally and save *all* the bother? Bah, ignore me, that was an incredible comprehension failure. Sorry for the noise. Robin. >> if (!functions) >> return -ENOMEM; >> @@ -289,10 +289,12 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) >> pctl->functions = devm_kmemdup(pctl->dev, functions, >> pctl->nfuncs * sizeof(*functions), >> GFP_KERNEL); >> - if (!pctl->functions) >> + if (!pctl->functions) { >> + kfree(functions); >> return -ENOMEM; >> + } >> >> - devm_kfree(pctl->dev, functions); >> + kfree(functions); >> return 0; >> } >> >> >
Arvind Yadav <arvind.yadav.cs@gmail.com> writes: Hi Arvind, > In functions pxa2xx_build_functions, the memory allocated for > 'functions' is live within the function only. After the > allocation it is immediately freed with devm_kfree. There is > no need to allocate memory for 'functions' with devm function > so replace devm_kcalloc with kcalloc and devm_kfree with kfree. That's not very true : the "need" is to spare the "manual" kfree you're adding in your patch for one, and make it consistent with pxa2xx_build_groups() and pxa2xx_build_state() for two. Therefore I'm not very thrilled by this patch and unless it fixes a defect in the driver I'd rather not have it in. Cheers. -- Robert
Yes, It will not fixes any defect. But we are going to free allocate memory then why we need devm api. In this case Devm will first add this entry to list and immediately it will remove from list. -Arvind On Saturday 10 December 2016 02:49 PM, Robert Jarzmik wrote: > Arvind Yadav <arvind.yadav.cs@gmail.com> writes: > > Hi Arvind, > >> In functions pxa2xx_build_functions, the memory allocated for >> 'functions' is live within the function only. After the >> allocation it is immediately freed with devm_kfree. There is >> no need to allocate memory for 'functions' with devm function >> so replace devm_kcalloc with kcalloc and devm_kfree with kfree. > That's not very true : the "need" is to spare the "manual" kfree you're adding > in your patch for one, and make it consistent with pxa2xx_build_groups() and > pxa2xx_build_state() for two. > > Therefore I'm not very thrilled by this patch and unless it fixes a defect in > the driver I'd rather not have it in. > > Cheers. > > -- > Robert
On Thu, Dec 8, 2016 at 3:35 PM, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > In functions pxa2xx_build_functions, the memory allocated for > 'functions' is live within the function only. After the > allocation it is immediately freed with devm_kfree. There is > no need to allocate memory for 'functions' with devm function > so replace devm_kcalloc with kcalloc and devm_kfree with kfree. > > Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> I want the maintainer Robert Jarzmik to review this before I do anything with it. Yours, Linus Walleij
Linus Walleij <linus.walleij@linaro.org> writes: > On Thu, Dec 8, 2016 at 3:35 PM, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: > >> In functions pxa2xx_build_functions, the memory allocated for >> 'functions' is live within the function only. After the >> allocation it is immediately freed with devm_kfree. There is >> no need to allocate memory for 'functions' with devm function >> so replace devm_kcalloc with kcalloc and devm_kfree with kfree. >> >> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> > > I want the maintainer Robert Jarzmik to review this before I do anything Hi Linus, I did review, on December the 10th. I wasn't very enthusiastic about the patch, if you check back my reply. Cheers.
On Thu, Dec 29, 2016 at 8:20 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote: > Linus Walleij <linus.walleij@linaro.org> writes: > >> On Thu, Dec 8, 2016 at 3:35 PM, Arvind Yadav <arvind.yadav.cs@gmail.com> wrote: >> >>> In functions pxa2xx_build_functions, the memory allocated for >>> 'functions' is live within the function only. After the >>> allocation it is immediately freed with devm_kfree. There is >>> no need to allocate memory for 'functions' with devm function >>> so replace devm_kcalloc with kcalloc and devm_kfree with kfree. >>> >>> Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> >> >> I want the maintainer Robert Jarzmik to review this before I do anything > > Hi Linus, > > I did review, on December the 10th. I wasn't very enthusiastic about the patch, > if you check back my reply. Sorry I missed it (mail overload as usual). OK dropping this. Yours, Linus Walleij
diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c index 866aa3c..47b8e3a 100644 --- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c +++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c @@ -277,7 +277,7 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) * alternate function, 6 * npins is an absolute high limit of the number * of functions. */ - functions = devm_kcalloc(pctl->dev, pctl->npins * 6, + functions = kcalloc(pctl->npins * 6, sizeof(*functions), GFP_KERNEL); if (!functions) return -ENOMEM; @@ -289,10 +289,12 @@ static int pxa2xx_build_functions(struct pxa_pinctrl *pctl) pctl->functions = devm_kmemdup(pctl->dev, functions, pctl->nfuncs * sizeof(*functions), GFP_KERNEL); - if (!pctl->functions) + if (!pctl->functions) { + kfree(functions); return -ENOMEM; + } - devm_kfree(pctl->dev, functions); + kfree(functions); return 0; }
In functions pxa2xx_build_functions, the memory allocated for 'functions' is live within the function only. After the allocation it is immediately freed with devm_kfree. There is no need to allocate memory for 'functions' with devm function so replace devm_kcalloc with kcalloc and devm_kfree with kfree. Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com> --- drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)