[PATCH] powerpc: kernel: Change the order of of_node_put()

[PATCH] powerpc: kernel: Change the order of of_node_put()

[Liang He]

In add_pcspkr(), it is better to call of_node_put() after the
'if(!np)' check.

Signed-off-by: Liang He <windhl@126.com>
---
 arch/powerpc/kernel/setup-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index eb0077b302e2..761817d1f4db 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
 	int ret;
 
 	np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
-	of_node_put(np);
 	if (!np)
 		return -ENODEV;
+	of_node_put(np);
 
 	pd = platform_device_alloc("pcspkr", -1);
 	if (!pd)
-- 
2.25.1

Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Christophe Leroy]

Le 17/06/2022 à 13:26, Liang He a écrit :
> In add_pcspkr(), it is better to call of_node_put() after the
> 'if(!np)' check.

Why is it better ?



/**
  * of_node_put() - Decrement refcount of a node
  * @node:	Node to dec refcount, NULL is supported to simplify writing of
  *		callers
  */
void of_node_put(struct device_node *node)
{
	if (node)
		kobject_put(&node->kobj);
}
EXPORT_SYMBOL(of_node_put);



Christophe


> 
> Signed-off-by: Liang He <windhl@126.com>
> ---
>   arch/powerpc/kernel/setup-common.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
> index eb0077b302e2..761817d1f4db 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
>   	int ret;
>   
>   	np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
> -	of_node_put(np);
>   	if (!np)
>   		return -ENODEV;
> +	of_node_put(np);
>   
>   	pd = platform_device_alloc("pcspkr", -1);
>   	if (!pd)

Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Liang He]

在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.leroy@csgroup.eu> 写道：
>
>
>Le 17/06/2022 à 13:26, Liang He a écrit :
>> In add_pcspkr(), it is better to call of_node_put() after the
>> 'if(!np)' check.
>
>Why is it better ?
>
>
>
>/**
>  * of_node_put() - Decrement refcount of a node
>  * @node:	Node to dec refcount, NULL is supported to simplify writing of
>  *		callers
>  */
>void of_node_put(struct device_node *node)
>{
>	if (node)
>		kobject_put(&node->kobj);
>}
>EXPORT_SYMBOL(of_node_put);
>
>
>
>Christophe

Hi, Christophe.

Thanks for your reply and I want to have a discussion.

In my thought, xxx_put(pointer)'s semantic usually means 
this reference has been used done and will not be used 
anymore. Is this semantic more reasonable, right?

Besides, if the np is NULL, we can just return and save a cpu 
time for the xxx_put() call.

Otherwise, I prefer to call it 'use(check)-after-put'.  

In fact, I have meet many other 'use(check)-after-put' instances
after I send this patch-commit, so I am waiting for this 
discussion.

This is just my thought, it may be wrong.

Anyway, thanks for your reply.

Liang

>
>
>> 
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>   arch/powerpc/kernel/setup-common.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
>> index eb0077b302e2..761817d1f4db 100644
>> --- a/arch/powerpc/kernel/setup-common.c
>> +++ b/arch/powerpc/kernel/setup-common.c
>> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void)
>>   	int ret;
>>   
>>   	np = of_find_compatible_node(NULL, NULL, "pnpPNP,100");
>> -	of_node_put(np);
>>   	if (!np)
>>   		return -ENODEV;
>> +	of_node_put(np);
>>   
>>   	pd = platform_device_alloc("pcspkr", -1);
>>   	if (!pd)

Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Christophe Leroy]

Le 18/06/2022 à 10:03, Liang He a écrit :
> 
> 
> 
> 
> 
> 在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.leroy@csgroup.eu> 写道：
>>
>>
>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>> In add_pcspkr(), it is better to call of_node_put() after the
>>> 'if(!np)' check.
>>
>> Why is it better ?
>>
>>
>>
>> /**
>>   * of_node_put() - Decrement refcount of a node
>>   * @node:	Node to dec refcount, NULL is supported to simplify writing of
>>   *		callers
>>   */
>> void of_node_put(struct device_node *node)
>> {
>> 	if (node)
>> 		kobject_put(&node->kobj);
>> }
>> EXPORT_SYMBOL(of_node_put);
>>
>>
>>
>> Christophe
> 
> Hi, Christophe.
> 
> Thanks for your reply and I want to have a discussion.
> 
> In my thought, xxx_put(pointer)'s semantic usually means
> this reference has been used done and will not be used
> anymore. Is this semantic more reasonable, right?
> 
> Besides, if the np is NULL, we can just return and save a cpu
> time for the xxx_put() call.
> 
> Otherwise, I prefer to call it 'use(check)-after-put'.
> 
> In fact, I have meet many other 'use(check)-after-put' instances
> after I send this patch-commit, so I am waiting for this
> discussion.
> 
> This is just my thought, it may be wrong.
> 
> Anyway, thanks for your reply.

Well in principle you are right, in an ideal world it should be like 
that. However, you have to wonder if it is worth the churn. The CPU 
cycle argument is valid only if that function is used in a hot path. But 
as we are talking about error handling, it can't be a hot path.

Taking into account the comment associated of of_node_put : "NULL is 
supported to simplify writing of callers", it means that usage is valid, 
just like it is with function kfree() after a kmalloc().

So in a new developpement, or when doing real modifications to a driver, 
that kind of change can be done ideally. However for drivers that have 
been there for years without any change, ask yourself if it is worth the 
churn. You spend time on it, you require other people to spend time on 
it for reviewing and applying your patches and during that time they 
don't do other things that could have been more usefull.

So unless this change is part of a more global patch, I think it is not 
worth the effort.

By the way, also for all your other patches, I think you should start 
doing all the changes locally on your side, and when you are finished 
try to group things together in bigger patches per area instead of 
sending one by one. I see you have already started doing that for 
opal/powernv for instance, but there are still individual powernv/opal 
in the queue. I think you should group all together in a single patch. 
And same for other areas, please try to minimise the number of patches. 
We don't link huge bombs that modify all the kernel at once, but you can 
group things together, one patch for powerpc core parts, one patch for 
each platform in arch/powerpc/platforms/ etc ...


Christophe

Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Liang He]

 
  
At 2022-06-18 16:48:26, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>> 
>> 
>> 
>> 
>> 
>> 在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.leroy@csgroup.eu> 写道：
>>>
>>>
>>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>>> In add_pcspkr(), it is better to call of_node_put() after the
>>>> 'if(!np)' check.
>>>
>>> Why is it better ?
>>>
>>>
>>>
>>> /**
>>>   * of_node_put() - Decrement refcount of a node
>>>   * @node:	Node to dec refcount, NULL is supported to simplify writing of
>>>   *		callers
>>>   */
>>> void of_node_put(struct device_node *node)
>>> {
>>> 	if (node)
>>> 		kobject_put(&node->kobj);
>>> }
>>> EXPORT_SYMBOL(of_node_put);
>>>
>>>
>>>
>>> Christophe
>> 
>> Hi, Christophe.
>> 
>> Thanks for your reply and I want to have a discussion.
>> 
>> In my thought, xxx_put(pointer)'s semantic usually means
>> this reference has been used done and will not be used
>> anymore. Is this semantic more reasonable, right?
>> 
>> Besides, if the np is NULL, we can just return and save a cpu
>> time for the xxx_put() call.
>> 
>> Otherwise, I prefer to call it 'use(check)-after-put'.
>> 
>> In fact, I have meet many other 'use(check)-after-put' instances
>> after I send this patch-commit, so I am waiting for this
>> discussion.
>> 
>> This is just my thought, it may be wrong.
>> 
>> Anyway, thanks for your reply.
>
>Well in principle you are right, in an ideal world it should be like 
>that. However, you have to wonder if it is worth the churn. The CPU 
>cycle argument is valid only if that function is used in a hot path. But 
>as we are talking about error handling, it can't be a hot path.
>

Thanks very much for this valuable lesson.

>Taking into account the comment associated of of_node_put : "NULL is 
>supported to simplify writing of callers", it means that usage is valid, 
>just like it is with function kfree() after a kmalloc().
>
>So in a new developpement, or when doing real modifications to a driver, 
>that kind of change can be done ideally. However for drivers that have 
>been there for years without any change, ask yourself if it is worth the 
>churn. You spend time on it, you require other people to spend time on 
>it for reviewing and applying your patches and during that time they 
>don't do other things that could have been more usefull.
>

Thanks for you advice, I will keep it in my mind before I send a new patch.

>So unless this change is part of a more global patch, I think it is not 
>worth the effort.
>
>By the way, also for all your other patches, I think you should start 
>doing all the changes locally on your side, and when you are finished 
>try to group things together in bigger patches per area instead of 
>sending one by one. I see you have already started doing that for 
>opal/powernv for instance, but there are still individual powernv/opal 
>in the queue. I think you should group all together in a single patch. 
>And same for other areas, please try to minimise the number of patches. 
>We don't link huge bombs that modify all the kernel at once, but you can 
>group things together, one patch for powerpc core parts, one patch for 
>each platform in arch/powerpc/platforms/ etc ...
>

You are right and I will follow this principle in future patching work.
While It is too exciting for me to begin the patching work , I should have 
grouped my patches.

>
>Christophe

Thanks again, Christophe.

Liang

Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Liang He]

At 2022-06-18 16:48:26, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>> 
>> 
>> 
>> 
>> 
>> 在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.leroy@csgroup.eu> 写道：
>>>
>>>
>>> Le 17/06/2022 à 13:26, Liang He a écrit :
>>>> In add_pcspkr(), it is better to call of_node_put() after the
>>>> 'if(!np)' check.
>>>
>>> Why is it better ?
>>>
>>>
>>>
>>> /**
>>>   * of_node_put() - Decrement refcount of a node
>>>   * @node:	Node to dec refcount, NULL is supported to simplify writing of
>>>   *		callers
>>>   */
>>> void of_node_put(struct device_node *node)
>>> {
>>> 	if (node)
>>> 		kobject_put(&node->kobj);
>>> }
>>> EXPORT_SYMBOL(of_node_put);
>>>
>>>
>>>
>>> Christophe
>> 
>> Hi, Christophe.
>> 
>> Thanks for your reply and I want to have a discussion.
>> 
>> In my thought, xxx_put(pointer)'s semantic usually means
>> this reference has been used done and will not be used
>> anymore. Is this semantic more reasonable, right?
>> 
>> Besides, if the np is NULL, we can just return and save a cpu
>> time for the xxx_put() call.
>> 
>> Otherwise, I prefer to call it 'use(check)-after-put'.
>> 
>> In fact, I have meet many other 'use(check)-after-put' instances
>> after I send this patch-commit, so I am waiting for this
>> discussion.
>> 
>> This is just my thought, it may be wrong.
>> 
>> Anyway, thanks for your reply.
>
>Well in principle you are right, in an ideal world it should be like 
>that. However, you have to wonder if it is worth the churn. The CPU 
>cycle argument is valid only if that function is used in a hot path. But 
>as we are talking about error handling, it can't be a hot path.
>
>Taking into account the comment associated of of_node_put : "NULL is 
>supported to simplify writing of callers", it means that usage is valid, 
>just like it is with function kfree() after a kmalloc().
>
>So in a new developpement, or when doing real modifications to a driver, 
>that kind of change can be done ideally. However for drivers that have 
>been there for years without any change, ask yourself if it is worth the 
>churn. You spend time on it, you require other people to spend time on 
>it for reviewing and applying your patches and during that time they 
>don't do other things that could have been more usefull.
>
>So unless this change is part of a more global patch, I think it is not 
>worth the effort.
>
>By the way, also for all your other patches, I think you should start 
>doing all the changes locally on your side, and when you are finished 
>try to group things together in bigger patches per area instead of 
>sending one by one. I see you have already started doing that for 
>opal/powernv for instance, but there are still individual powernv/opal 
>in the queue. I think you should group all together in a single patch. 
>And same for other areas, please try to minimise the number of patches. 
>We don't link huge bombs that modify all the kernel at once, but you can 
>group things together, one patch for powerpc core parts, one patch for 
>each platform in arch/powerpc/platforms/ etc ...
>
>
>Christophe


Hi, Christophe.

Sorry to trobule you again.

Now I have found other bugs in same directories (i.e., arch/powerpc/sysdev), 
with the ones I have sent but not recieved acked-by or confirmed email.

So I need to merge the old ones into the new ones as a PATCH-v2 and then resend the 
old ones ?
or just use a new PATCH to send only new ones?

I am afraid to make new trouble for maintainers, so can you share your valuable 
experience?

Thanks very much.

Liang

Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Christophe Leroy]

Hi,

Le 20/06/2022 à 11:23, Liang He a écrit :
> 
> Hi, Christophe.
> 
> Sorry to trobule you again.
> 
> Now I have found other bugs in same directories (i.e., arch/powerpc/sysdev),
> with the ones I have sent but not recieved acked-by or confirmed email.
> 
> So I need to merge the old ones into the new ones as a PATCH-v2 and then resend the
> old ones ?
> or just use a new PATCH to send only new ones?
> 
> I am afraid to make new trouble for maintainers, so can you share your valuable
> experience?
> 

Here is the list of your patches : 
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=84258

 From my point of view, for all the patches that are still in status 
"new" it is better than you send a v2 with more things into a single 
patch. When the patch is in "under review" state, it is better to not 
update it anymore.

So in the list there are for instance several patches for powernv, so it 
would be good if you can regroup all of them in a single v2 patch.

Christophe

Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

[Liang He]

At 2022-06-20 19:11:33, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote:
>Hi,
>
>Le 20/06/2022 à 11:23, Liang He a écrit :
>> 
>> Hi, Christophe.
>> 
>> Sorry to trobule you again.
>> 
>> Now I have found other bugs in same directories (i.e., arch/powerpc/sysdev),
>> with the ones I have sent but not recieved acked-by or confirmed email.
>> 
>> So I need to merge the old ones into the new ones as a PATCH-v2 and then resend the
>> old ones ?
>> or just use a new PATCH to send only new ones?
>> 
>> I am afraid to make new trouble for maintainers, so can you share your valuable
>> experience?
>> 
>
>Here is the list of your patches : 
>https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=84258
>
> From my point of view, for all the patches that are still in status 
>"new" it is better than you send a v2 with more things into a single 
>patch. When the patch is in "under review" state, it is better to not 
>update it anymore.
>
>So in the list there are for instance several patches for powernv, so it 
>would be good if you can regroup all of them in a single v2 patch.
>
>Christophe

Thanks, Christophe.

I will follow your rules and try to group the 'new' state ones.