| Message ID | 20220617112636.4041671-1-windhl@126.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | powerpc: kernel: Change the order of of_node_put() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 23 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
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 > > Signed-off-by: Liang He <windhl@126.com> > --- > arch/powerpc/kernel/setup-common.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c > index eb0077b302e2..761817d1f4db 100644 > --- a/arch/powerpc/kernel/setup-common.c > +++ b/arch/powerpc/kernel/setup-common.c > @@ -563,9 +563,9 @@ static __init int add_pcspkr(void) > int ret; > > np = of_find_compatible_node(NULL, NULL, "pnpPNP,100"); > - of_node_put(np); > if (!np) > return -ENODEV; > + of_node_put(np); > > pd = platform_device_alloc("pcspkr", -1); > if (!pd)
在 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. Liang > > >> >> Signed-off-by: Liang He <windhl@126.com> >> --- >> arch/powerpc/kernel/setup-common.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c >> index eb0077b302e2..761817d1f4db 100644 >> --- a/arch/powerpc/kernel/setup-common.c >> +++ b/arch/powerpc/kernel/setup-common.c >> @@ -563,9 +563,9 @@ static __init int add_pcspkr(void) >> int ret; >> >> np = of_find_compatible_node(NULL, NULL, "pnpPNP,100"); >> - of_node_put(np); >> if (!np) >> return -ENODEV; >> + of_node_put(np); >> >> pd = platform_device_alloc("pcspkr", -1); >> if (!pd)
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
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
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
Hi, Le 20/06/2022 à 11:23, Liang He a écrit : > > 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? > Here is the list of your patches : https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=84258 From my point of view, for all the patches that are still in status "new" it is better than you send a v2 with more things into a single patch. When the patch is in "under review" state, it is better to not update it anymore. So in the list there are for instance several patches for powernv, so it would be good if you can regroup all of them in a single v2 patch. Christophe
At 2022-06-20 19:11:33, "Christophe Leroy" <christophe.leroy@csgroup.eu> wrote: >Hi, > >Le 20/06/2022 à 11:23, Liang He a écrit : >> >> 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? >> > >Here is the list of your patches : >https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=84258 > > From my point of view, for all the patches that are still in status >"new" it is better than you send a v2 with more things into a single >patch. When the patch is in "under review" state, it is better to not >update it anymore. > >So in the list there are for instance several patches for powernv, so it >would be good if you can regroup all of them in a single v2 patch. > >Christophe Thanks, Christophe. I will follow your rules and try to group the 'new' state ones.
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c index eb0077b302e2..761817d1f4db 100644 --- a/arch/powerpc/kernel/setup-common.c +++ b/arch/powerpc/kernel/setup-common.c @@ -563,9 +563,9 @@ static __init int add_pcspkr(void) int ret; np = of_find_compatible_node(NULL, NULL, "pnpPNP,100"); - of_node_put(np); if (!np) return -ENODEV; + of_node_put(np); pd = platform_device_alloc("pcspkr", -1); if (!pd)
In add_pcspkr(), it is better to call of_node_put() after the 'if(!np)' check. Signed-off-by: Liang He <windhl@126.com> --- arch/powerpc/kernel/setup-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)