Le 18/06/2022 à 10:03, Liang He a écrit :
>
>
>
>
>
> 在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.le...@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
