[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
by kstrdup_const() must be always deallocated with kfree_const(),
otherwise there is a risk of kfree'ing ro memory.

Also remove unneeded of_node_put(), if for_each_compatible_node() body
execution is not terminated, this prevents from double kfree() in
OF_DYNAMIC build.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/mach-exynos/pm_domains.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 6001f1c..4a87e86 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
 		pd->base = of_iomap(np, 0);
 		if (!pd->base) {
 			pr_warn("%s: failed to map memory\n", __func__);
-			kfree(pd->pd.name);
+			kfree_const(pd->pd.name);
 			kfree(pd);
-			of_node_put(np);
 			continue;
 		}
 
-- 
2.1.4

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
by kstrdup_const() must be always deallocated with kfree_const(),
otherwise there is a risk of kfree'ing ro memory.

Also remove unneeded of_node_put(), if for_each_compatible_node() body
execution is not terminated, this prevents from double kfree() in
OF_DYNAMIC build.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
 arch/arm/mach-exynos/pm_domains.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 6001f1c..4a87e86 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
 		pd->base = of_iomap(np, 0);
 		if (!pd->base) {
 			pr_warn("%s: failed to map memory\n", __func__);
-			kfree(pd->pd.name);
+			kfree_const(pd->pd.name);
 			kfree(pd);
-			of_node_put(np);
 			continue;
 		}
 
-- 
2.1.4

Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
> by kstrdup_const() must be always deallocated with kfree_const(),
> otherwise there is a risk of kfree'ing ro memory.

This looks good. Can you provide also Cc-stable and fixes tags?

>
> Also remove unneeded of_node_put(), if for_each_compatible_node() body
> execution is not terminated, this prevents from double kfree() in
> OF_DYNAMIC build.

Each iteration of for_each_compatible_node() has a check:
(dn = of_find_compatible_node(dn, type, compatible))
this increases the references to 'np'. If loop continues then previous
'np' is not of_node_put().

Best regards,
Krzysztof

>
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
> index 6001f1c..4a87e86 100644
> --- a/arch/arm/mach-exynos/pm_domains.c
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>                 pd->base = of_iomap(np, 0);
>                 if (!pd->base) {
>                         pr_warn("%s: failed to map memory\n", __func__);
> -                       kfree(pd->pd.name);
> +                       kfree_const(pd->pd.name);
>                         kfree(pd);
> -                       of_node_put(np);
>                         continue;
>                 }
>
> --
> 2.1.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
> by kstrdup_const() must be always deallocated with kfree_const(),
> otherwise there is a risk of kfree'ing ro memory.

This looks good. Can you provide also Cc-stable and fixes tags?

>
> Also remove unneeded of_node_put(), if for_each_compatible_node() body
> execution is not terminated, this prevents from double kfree() in
> OF_DYNAMIC build.

Each iteration of for_each_compatible_node() has a check:
(dn = of_find_compatible_node(dn, type, compatible))
this increases the references to 'np'. If loop continues then previous
'np' is not of_node_put().

Best regards,
Krzysztof

>
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
> index 6001f1c..4a87e86 100644
> --- a/arch/arm/mach-exynos/pm_domains.c
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>                 pd->base = of_iomap(np, 0);
>                 if (!pd->base) {
>                         pr_warn("%s: failed to map memory\n", __func__);
> -                       kfree(pd->pd.name);
> +                       kfree_const(pd->pd.name);
>                         kfree(pd);
> -                       of_node_put(np);
>                         continue;
>                 }
>
> --
> 2.1.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>> by kstrdup_const() must be always deallocated with kfree_const(),
>> otherwise there is a risk of kfree'ing ro memory.
> 
> This looks good. Can you provide also Cc-stable and fixes tags?

Since the change fixes two independent issues I decided not to add a
particular commit to Fixes tag. I can split the commit of course, but I
feel reluctant to send a series in this particular case.

Let me know your decision with respect to my comments.

>>
>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>> execution is not terminated, this prevents from double kfree() in
>> OF_DYNAMIC build.
> 
> Each iteration of for_each_compatible_node() has a check:
> (dn = of_find_compatible_node(dn, type, compatible))
> this increases the references to 'np'. 

Correct.

> If loop continues then previous 'np' is not of_node_put().

This I don't understand. The previous 'np' is of_node_put() on next
iteration of the loop, i.e. if and only if loop continues. Please elaborate.

--
With best wishes,
Vladimir

> 
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>> index 6001f1c..4a87e86 100644
>> --- a/arch/arm/mach-exynos/pm_domains.c
>> +++ b/arch/arm/mach-exynos/pm_domains.c
>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>                 pd->base = of_iomap(np, 0);
>>                 if (!pd->base) {
>>                         pr_warn("%s: failed to map memory\n", __func__);
>> -                       kfree(pd->pd.name);
>> +                       kfree_const(pd->pd.name);
>>                         kfree(pd);
>> -                       of_node_put(np);
>>                         continue;
>>                 }
>>
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>> by kstrdup_const() must be always deallocated with kfree_const(),
>> otherwise there is a risk of kfree'ing ro memory.
> 
> This looks good. Can you provide also Cc-stable and fixes tags?

Since the change fixes two independent issues I decided not to add a
particular commit to Fixes tag. I can split the commit of course, but I
feel reluctant to send a series in this particular case.

Let me know your decision with respect to my comments.

>>
>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>> execution is not terminated, this prevents from double kfree() in
>> OF_DYNAMIC build.
> 
> Each iteration of for_each_compatible_node() has a check:
> (dn = of_find_compatible_node(dn, type, compatible))
> this increases the references to 'np'. 

Correct.

> If loop continues then previous 'np' is not of_node_put().

This I don't understand. The previous 'np' is of_node_put() on next
iteration of the loop, i.e. if and only if loop continues. Please elaborate.

--
With best wishes,
Vladimir

> 
>>
>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>> ---
>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>> index 6001f1c..4a87e86 100644
>> --- a/arch/arm/mach-exynos/pm_domains.c
>> +++ b/arch/arm/mach-exynos/pm_domains.c
>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>                 pd->base = of_iomap(np, 0);
>>                 if (!pd->base) {
>>                         pr_warn("%s: failed to map memory\n", __func__);
>> -                       kfree(pd->pd.name);
>> +                       kfree_const(pd->pd.name);
>>                         kfree(pd);
>> -                       of_node_put(np);
>>                         continue;
>>                 }
>>
>> --
>> 2.1.4
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>> otherwise there is a risk of kfree'ing ro memory.
>>
>> This looks good. Can you provide also Cc-stable and fixes tags?
> 
> Since the change fixes two independent issues I decided not to add a
> particular commit to Fixes tag. I can split the commit of course, but I
> feel reluctant to send a series in this particular case.
> 
> Let me know your decision with respect to my comments.

Although this is only error-path but still this applies for backporting
to stable. Please split it up and add respective fixes tags. This helps
companies/people using stable trees, including LTS.

> 
>>>
>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>> execution is not terminated, this prevents from double kfree() in
>>> OF_DYNAMIC build.
>>
>> Each iteration of for_each_compatible_node() has a check:
>> (dn = of_find_compatible_node(dn, type, compatible))
>> this increases the references to 'np'. 
> 
> Correct.
> 
>> If loop continues then previous 'np' is not of_node_put().
> 
> This I don't understand. The previous 'np' is of_node_put() on next
> iteration of the loop, i.e. if and only if loop continues. Please elaborate.

Step by step, if I get it right:
1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
1a. if (!pd->base) then we want to drop that reference.
1b. if not, then loop itself
3. increase value: dn = of_find_compatible_node(dn, type, compatible)
4. next iteration of loop, now we have 'dn' from last 'increase value'
5. if (!pd->base) then we want to drop that reference.

Best regards,
Krzysztof


> 
> --
> With best wishes,
> Vladimir
> 
>>
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>> ---
>>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>>> index 6001f1c..4a87e86 100644
>>> --- a/arch/arm/mach-exynos/pm_domains.c
>>> +++ b/arch/arm/mach-exynos/pm_domains.c
>>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>>                 pd->base = of_iomap(np, 0);
>>>                 if (!pd->base) {
>>>                         pr_warn("%s: failed to map memory\n", __func__);
>>> -                       kfree(pd->pd.name);
>>> +                       kfree_const(pd->pd.name);
>>>                         kfree(pd);
>>> -                       of_node_put(np);
>>>                         continue;
>>>                 }
>>>
>>> --
>>> 2.1.4
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>> otherwise there is a risk of kfree'ing ro memory.
>>
>> This looks good. Can you provide also Cc-stable and fixes tags?
> 
> Since the change fixes two independent issues I decided not to add a
> particular commit to Fixes tag. I can split the commit of course, but I
> feel reluctant to send a series in this particular case.
> 
> Let me know your decision with respect to my comments.

Although this is only error-path but still this applies for backporting
to stable. Please split it up and add respective fixes tags. This helps
companies/people using stable trees, including LTS.

> 
>>>
>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>> execution is not terminated, this prevents from double kfree() in
>>> OF_DYNAMIC build.
>>
>> Each iteration of for_each_compatible_node() has a check:
>> (dn = of_find_compatible_node(dn, type, compatible))
>> this increases the references to 'np'. 
> 
> Correct.
> 
>> If loop continues then previous 'np' is not of_node_put().
> 
> This I don't understand. The previous 'np' is of_node_put() on next
> iteration of the loop, i.e. if and only if loop continues. Please elaborate.

Step by step, if I get it right:
1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
1a. if (!pd->base) then we want to drop that reference.
1b. if not, then loop itself
3. increase value: dn = of_find_compatible_node(dn, type, compatible)
4. next iteration of loop, now we have 'dn' from last 'increase value'
5. if (!pd->base) then we want to drop that reference.

Best regards,
Krzysztof


> 
> --
> With best wishes,
> Vladimir
> 
>>
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>> ---
>>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>>> index 6001f1c..4a87e86 100644
>>> --- a/arch/arm/mach-exynos/pm_domains.c
>>> +++ b/arch/arm/mach-exynos/pm_domains.c
>>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>>                 pd->base = of_iomap(np, 0);
>>>                 if (!pd->base) {
>>>                         pr_warn("%s: failed to map memory\n", __func__);
>>> -                       kfree(pd->pd.name);
>>> +                       kfree_const(pd->pd.name);
>>>                         kfree(pd);
>>> -                       of_node_put(np);
>>>                         continue;
>>>                 }
>>>
>>> --
>>> 2.1.4
>>>
>>>
>>> _______________________________________________
>>> linux-arm-kernel mailing list
>>> linux-arm-kernel at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

On 30.07.2015 03:15, Krzysztof Kozlowski wrote:
> On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>>> otherwise there is a risk of kfree'ing ro memory.
>>>
>>> This looks good. Can you provide also Cc-stable and fixes tags?
>>
>> Since the change fixes two independent issues I decided not to add a
>> particular commit to Fixes tag. I can split the commit of course, but I
>> feel reluctant to send a series in this particular case.
>>
>> Let me know your decision with respect to my comments.
> 
> Although this is only error-path but still this applies for backporting
> to stable. Please split it up and add respective fixes tags. This helps
> companies/people using stable trees, including LTS.

Okay, I'll resend the split changes tomorrow.

>>
>>>>
>>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>>> execution is not terminated, this prevents from double kfree() in
>>>> OF_DYNAMIC build.
>>>
>>> Each iteration of for_each_compatible_node() has a check:
>>> (dn = of_find_compatible_node(dn, type, compatible))
>>> this increases the references to 'np'. 
>>
>> Correct.
>>
>>> If loop continues then previous 'np' is not of_node_put().
>>
>> This I don't understand. The previous 'np' is of_node_put() on next
>> iteration of the loop, i.e. if and only if loop continues. Please elaborate.
> 
> Step by step, if I get it right:
> 1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
> 1a. if (!pd->base) then we want to drop that reference.
> 1b. if not, then loop itself
> 3. increase value: dn = of_find_compatible_node(dn, type, compatible)
> 4. next iteration of loop, now we have 'dn' from last 'increase value'
> 5. if (!pd->base) then we want to drop that reference.

It is quite basic but it might be more visual, if the questionable
expression is preprocessed and some C99 magic is applied on top:


for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
...	
	continue;
...
}

stands for

for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
     dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
	continue;
...
}

stands for

for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
     dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
	goto contin;
...
contin:
}

stands for

dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
while (dn) {
	...
	goto contin;
	...
contin:
	dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")
};


then np reference counter is decremented inside closing
of_find_compatible_node() as usual, there is no need to decrement it two
times.

Do I miss something?

--
With best wishes,
Vladimir

>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>>>> index 6001f1c..4a87e86 100644
>>>> --- a/arch/arm/mach-exynos/pm_domains.c
>>>> +++ b/arch/arm/mach-exynos/pm_domains.c
>>>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>>>                 pd->base = of_iomap(np, 0);
>>>>                 if (!pd->base) {
>>>>                         pr_warn("%s: failed to map memory\n", __func__);
>>>> -                       kfree(pd->pd.name);
>>>> +                       kfree_const(pd->pd.name);
>>>>                         kfree(pd);
>>>> -                       of_node_put(np);
>>>>                         continue;
>>>>                 }
>>>>
>>>> --
>>>> 2.1.4
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Vladimir Zapolskiy]

On 30.07.2015 03:15, Krzysztof Kozlowski wrote:
> On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>>> otherwise there is a risk of kfree'ing ro memory.
>>>
>>> This looks good. Can you provide also Cc-stable and fixes tags?
>>
>> Since the change fixes two independent issues I decided not to add a
>> particular commit to Fixes tag. I can split the commit of course, but I
>> feel reluctant to send a series in this particular case.
>>
>> Let me know your decision with respect to my comments.
> 
> Although this is only error-path but still this applies for backporting
> to stable. Please split it up and add respective fixes tags. This helps
> companies/people using stable trees, including LTS.

Okay, I'll resend the split changes tomorrow.

>>
>>>>
>>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>>> execution is not terminated, this prevents from double kfree() in
>>>> OF_DYNAMIC build.
>>>
>>> Each iteration of for_each_compatible_node() has a check:
>>> (dn = of_find_compatible_node(dn, type, compatible))
>>> this increases the references to 'np'. 
>>
>> Correct.
>>
>>> If loop continues then previous 'np' is not of_node_put().
>>
>> This I don't understand. The previous 'np' is of_node_put() on next
>> iteration of the loop, i.e. if and only if loop continues. Please elaborate.
> 
> Step by step, if I get it right:
> 1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
> 1a. if (!pd->base) then we want to drop that reference.
> 1b. if not, then loop itself
> 3. increase value: dn = of_find_compatible_node(dn, type, compatible)
> 4. next iteration of loop, now we have 'dn' from last 'increase value'
> 5. if (!pd->base) then we want to drop that reference.

It is quite basic but it might be more visual, if the questionable
expression is preprocessed and some C99 magic is applied on top:


for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
...	
	continue;
...
}

stands for

for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
     dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
	continue;
...
}

stands for

for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
dn; \
     dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
...
	goto contin;
...
contin:
}

stands for

dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
while (dn) {
	...
	goto contin;
	...
contin:
	dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")
};


then np reference counter is decremented inside closing
of_find_compatible_node() as usual, there is no need to decrement it two
times.

Do I miss something?

--
With best wishes,
Vladimir

>>>>
>>>> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
>>>> ---
>>>>  arch/arm/mach-exynos/pm_domains.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
>>>> index 6001f1c..4a87e86 100644
>>>> --- a/arch/arm/mach-exynos/pm_domains.c
>>>> +++ b/arch/arm/mach-exynos/pm_domains.c
>>>> @@ -146,9 +146,8 @@ static __init int exynos4_pm_init_power_domain(void)
>>>>                 pd->base = of_iomap(np, 0);
>>>>                 if (!pd->base) {
>>>>                         pr_warn("%s: failed to map memory\n", __func__);
>>>> -                       kfree(pd->pd.name);
>>>> +                       kfree_const(pd->pd.name);
>>>>                         kfree(pd);
>>>> -                       of_node_put(np);
>>>>                         continue;
>>>>                 }
>>>>
>>>> --
>>>> 2.1.4
>>>>
>>>>
>>>> _______________________________________________
>>>> linux-arm-kernel mailing list
>>>> linux-arm-kernel at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 

Re: [PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

On 30.07.2015 09:35, Vladimir Zapolskiy wrote:
> On 30.07.2015 03:15, Krzysztof Kozlowski wrote:
>> On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
>>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>>>> otherwise there is a risk of kfree'ing ro memory.
>>>>
>>>> This looks good. Can you provide also Cc-stable and fixes tags?
>>>
>>> Since the change fixes two independent issues I decided not to add a
>>> particular commit to Fixes tag. I can split the commit of course, but I
>>> feel reluctant to send a series in this particular case.
>>>
>>> Let me know your decision with respect to my comments.
>>
>> Although this is only error-path but still this applies for backporting
>> to stable. Please split it up and add respective fixes tags. This helps
>> companies/people using stable trees, including LTS.
> 
> Okay, I'll resend the split changes tomorrow.
> 
>>>
>>>>>
>>>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>>>> execution is not terminated, this prevents from double kfree() in
>>>>> OF_DYNAMIC build.
>>>>
>>>> Each iteration of for_each_compatible_node() has a check:
>>>> (dn = of_find_compatible_node(dn, type, compatible))
>>>> this increases the references to 'np'. 
>>>
>>> Correct.
>>>
>>>> If loop continues then previous 'np' is not of_node_put().
>>>
>>> This I don't understand. The previous 'np' is of_node_put() on next
>>> iteration of the loop, i.e. if and only if loop continues. Please elaborate.
>>
>> Step by step, if I get it right:
>> 1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
>> 1a. if (!pd->base) then we want to drop that reference.
>> 1b. if not, then loop itself
>> 3. increase value: dn = of_find_compatible_node(dn, type, compatible)
>> 4. next iteration of loop, now we have 'dn' from last 'increase value'
>> 5. if (!pd->base) then we want to drop that reference.
> 
> It is quite basic but it might be more visual, if the questionable
> expression is preprocessed and some C99 magic is applied on top:
> 
> 
> for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
> ...	
> 	continue;
> ...
> }
> 
> stands for
> 
> for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> dn; \
>      dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
> ...
> 	continue;
> ...
> }
> 
> stands for
> 
> for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> dn; \
>      dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
> ...
> 	goto contin;
> ...
> contin:
> }
> 
> stands for
> 
> dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> while (dn) {
> 	...
> 	goto contin;
> 	...
> contin:
> 	dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")
> };
> 
> 
> then np reference counter is decremented inside closing
> of_find_compatible_node() as usual, there is no need to decrement it two
> times.
> 
> Do I miss something?

Yes, you are right. Thanks for patience!

Best regards,
Krzysztof

[PATCH] ARM: EXYNOS: pd: fix resource deallocation on error path

[Krzysztof Kozlowski]

On 30.07.2015 09:35, Vladimir Zapolskiy wrote:
> On 30.07.2015 03:15, Krzysztof Kozlowski wrote:
>> On 30.07.2015 09:06, Vladimir Zapolskiy wrote:
>>> On 30.07.2015 02:37, Krzysztof Kozlowski wrote:
>>>> 2015-07-30 5:15 GMT+09:00 Vladimir Zapolskiy <vz@mleia.com>:
>>>>> The change fixes a bug introduced by 2be2a3ff42a5, memory allocated
>>>>> by kstrdup_const() must be always deallocated with kfree_const(),
>>>>> otherwise there is a risk of kfree'ing ro memory.
>>>>
>>>> This looks good. Can you provide also Cc-stable and fixes tags?
>>>
>>> Since the change fixes two independent issues I decided not to add a
>>> particular commit to Fixes tag. I can split the commit of course, but I
>>> feel reluctant to send a series in this particular case.
>>>
>>> Let me know your decision with respect to my comments.
>>
>> Although this is only error-path but still this applies for backporting
>> to stable. Please split it up and add respective fixes tags. This helps
>> companies/people using stable trees, including LTS.
> 
> Okay, I'll resend the split changes tomorrow.
> 
>>>
>>>>>
>>>>> Also remove unneeded of_node_put(), if for_each_compatible_node() body
>>>>> execution is not terminated, this prevents from double kfree() in
>>>>> OF_DYNAMIC build.
>>>>
>>>> Each iteration of for_each_compatible_node() has a check:
>>>> (dn = of_find_compatible_node(dn, type, compatible))
>>>> this increases the references to 'np'. 
>>>
>>> Correct.
>>>
>>>> If loop continues then previous 'np' is not of_node_put().
>>>
>>> This I don't understand. The previous 'np' is of_node_put() on next
>>> iteration of the loop, i.e. if and only if loop continues. Please elaborate.
>>
>> Step by step, if I get it right:
>> 1. initialization: dn = of_find_compatible_node(NULL, type, compatible);
>> 1a. if (!pd->base) then we want to drop that reference.
>> 1b. if not, then loop itself
>> 3. increase value: dn = of_find_compatible_node(dn, type, compatible)
>> 4. next iteration of loop, now we have 'dn' from last 'increase value'
>> 5. if (!pd->base) then we want to drop that reference.
> 
> It is quite basic but it might be more visual, if the questionable
> expression is preprocessed and some C99 magic is applied on top:
> 
> 
> for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
> ...	
> 	continue;
> ...
> }
> 
> stands for
> 
> for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> dn; \
>      dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
> ...
> 	continue;
> ...
> }
> 
> stands for
> 
> for (dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> dn; \
>      dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")) {
> ...
> 	goto contin;
> ...
> contin:
> }
> 
> stands for
> 
> dn = of_find_compatible_node(NULL, NULL, "samsung,exynos4210-pd");
> while (dn) {
> 	...
> 	goto contin;
> 	...
> contin:
> 	dn = of_find_compatible_node(np, NULL, "samsung,exynos4210-pd")
> };
> 
> 
> then np reference counter is decremented inside closing
> of_find_compatible_node() as usual, there is no need to decrement it two
> times.
> 
> Do I miss something?

Yes, you are right. Thanks for patience!

Best regards,
Krzysztof