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

["Liang He" <windhl@126.com>]

 
  
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