lkml.org 
[lkml]   [2022]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()



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


\
 
 \ /
  Last update: 2022-06-20 11:31    [W:0.054 / U:2.660 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site