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@xxxxxxxxxx> 写道：
>>
>>
>> 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