linux-kernel - Re:Re: [PATCH] powerpc: kernel: Change the order of of_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5814e0cc.2c68.181779d72a2.Coremail.windhl@126.com>
Date:   Sun, 19 Jun 2022 00:20:16 +0800 (CST)
From:   "Liang He" <windhl@....com>
To:     "Christophe Leroy" <christophe.leroy@...roup.eu>
Cc:     "mpe@...erman.id.au" <mpe@...erman.id.au>,
        "benh@...nel.crashing.org" <benh@...nel.crashing.org>,
        "paulus@...ba.org" <paulus@...ba.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "wangkefeng.wang@...wei.com" <wangkefeng.wang@...wei.com>,
        "gpiccoli@...lia.com" <gpiccoli@...lia.com>,
        "aneesh.kumar@...ux.ibm.com" <aneesh.kumar@...ux.ibm.com>,
        "dmitry.osipenko@...labora.com" <dmitry.osipenko@...labora.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re:Re: [PATCH] powerpc: kernel: Change the order of of_node_put()

 
  
At 2022-06-18 16:48:26, "Christophe Leroy" <christophe.leroy@...roup.eu> wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>> 
>> 
>> 
>> 
>> 
>> 在 2022-06-18 15:13:13，"Christophe Leroy" <christophe.leroy@...roup.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