In add_pcspkr(), it is better to call of_node_put() after the
'if(!np)' check.
Signed-off-by: Liang He <[email protected]>
---
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)
--
2.25.1
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 <[email protected]>
> ---
> 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" <[email protected]> 写道：
>
>
>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 <[email protected]>
>> ---
>> 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" <[email protected]> 写道：
>>
>>
>> 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" <[email protected]> wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>>
>>
>>
>>
>>
>> 在 2022-06-18 15:13:13，"Christophe Leroy" <[email protected]> 写道：
>>>
>>>
>>> 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" <[email protected]> wrote:
>
>
>Le 18/06/2022 à 10:03, Liang He a écrit :
>>
>>
>>
>>
>>
>> 在 2022-06-18 15:13:13，"Christophe Leroy" <[email protected]> 写道：
>>>
>>>
>>> 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" <[email protected]> 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.