linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
@ 2016-12-08 14:35 Arvind Yadav
  2016-12-08 15:20 ` Robin Murphy
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Arvind Yadav @ 2016-12-08 14:35 UTC (permalink / raw)
  To: daniel, haojian.zhuang, robert.jarzmik, linus.walleij
  Cc: linux-arm-kernel, linux-gpio, linux-kernel

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);
 	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;
 }
 
-- 
2.7.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-08 14:35 [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions Arvind Yadav
@ 2016-12-08 15:20 ` Robin Murphy
  2016-12-08 15:25   ` Robin Murphy
  2016-12-10  9:19 ` Robert Jarzmik
  2016-12-27 12:49 ` Linus Walleij
  2 siblings, 1 reply; 8+ messages in thread
From: Robin Murphy @ 2016-12-08 15:20 UTC (permalink / raw)
  To: Arvind Yadav, daniel, haojian.zhuang, robert.jarzmik, linus.walleij
  Cc: linux-gpio, linux-kernel, linux-arm-kernel

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;
>  }
>  
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-08 15:20 ` Robin Murphy
@ 2016-12-08 15:25   ` Robin Murphy
  0 siblings, 0 replies; 8+ messages in thread
From: Robin Murphy @ 2016-12-08 15:25 UTC (permalink / raw)
  To: Arvind Yadav, daniel, haojian.zhuang, robert.jarzmik, linus.walleij
  Cc: linux-gpio, linux-kernel, linux-arm-kernel

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;
>>  }
>>  
>>
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-08 14:35 [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions Arvind Yadav
  2016-12-08 15:20 ` Robin Murphy
@ 2016-12-10  9:19 ` Robert Jarzmik
  2016-12-11  8:04   ` arvind Yadav
  2016-12-27 12:49 ` Linus Walleij
  2 siblings, 1 reply; 8+ messages in thread
From: Robert Jarzmik @ 2016-12-10  9:19 UTC (permalink / raw)
  To: Arvind Yadav
  Cc: daniel, haojian.zhuang, linus.walleij, linux-arm-kernel,
	linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-10  9:19 ` Robert Jarzmik
@ 2016-12-11  8:04   ` arvind Yadav
  0 siblings, 0 replies; 8+ messages in thread
From: arvind Yadav @ 2016-12-11  8:04 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: daniel, haojian.zhuang, linus.walleij, linux-arm-kernel,
	linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-08 14:35 [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions Arvind Yadav
  2016-12-08 15:20 ` Robin Murphy
  2016-12-10  9:19 ` Robert Jarzmik
@ 2016-12-27 12:49 ` Linus Walleij
  2016-12-29  7:20   ` Robert Jarzmik
  2 siblings, 1 reply; 8+ messages in thread
From: Linus Walleij @ 2016-12-27 12:49 UTC (permalink / raw)
  To: Arvind Yadav, Robert Jarzmik
  Cc: Daniel Mack, Haojian Zhuang, linux-arm-kernel, linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-27 12:49 ` Linus Walleij
@ 2016-12-29  7:20   ` Robert Jarzmik
  2016-12-30 13:02     ` Linus Walleij
  0 siblings, 1 reply; 8+ messages in thread
From: Robert Jarzmik @ 2016-12-29  7:20 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Arvind Yadav, Daniel Mack, Haojian Zhuang, linux-arm-kernel,
	linux-gpio, linux-kernel

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.

-- 
Robert

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions
  2016-12-29  7:20   ` Robert Jarzmik
@ 2016-12-30 13:02     ` Linus Walleij
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2016-12-30 13:02 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: Arvind Yadav, Daniel Mack, Haojian Zhuang, linux-arm-kernel,
	linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-12-30 13:02 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-08 14:35 [PATCH V1] pinctrl:pxa:pinctrl-pxa2xx:- No need of devm functions Arvind Yadav
2016-12-08 15:20 ` Robin Murphy
2016-12-08 15:25   ` Robin Murphy
2016-12-10  9:19 ` Robert Jarzmik
2016-12-11  8:04   ` arvind Yadav
2016-12-27 12:49 ` Linus Walleij
2016-12-29  7:20   ` Robert Jarzmik
2016-12-30 13:02     ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).