Messages in this thread | | | Date | Sun, 19 Jun 2022 00:20:16 +0800 (CST) | From | "Liang He" <> | Subject | Re: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. >
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
| |