diff mbox series

genirq/irqdesc: hide illegible sysfs warning of kobject_del()

Message ID 20221112093939.616270-1-liushixin2@huawei.com
State New
Headers
Series genirq/irqdesc: hide illegible sysfs warning of kobject_del() |

Commit Message

Liu Shixin Nov. 12, 2022, 9:39 a.m. UTC 
  If irq_sysfs_add() failed, system will report a warning but don't call
kobject_put() to release the descriptor. Then in irq_sysfs_del(), we
continue to call kobject_del(). In such situation, kobject_del() will
complains about a object with no parent like this:

 kernfs: can not remove 'actions', no directory
 WARNING: CPU: 0 PID: 277 at fs/kernfs/dir.c:1615 kernfs_remove_by_name_ns+0xd5/0xe0
[...]
 Call Trace:
  <TASK>
  remove_files.isra.0+0x3f/0xb0
  sysfs_remove_group+0x68/0xe0
  sysfs_remove_groups+0x41/0x70
  __kobject_del+0x45/0xc0
  kobject_del+0x2a/0x40
  free_desc+0x44/0x70
  irq_free_descs+0x5d/0x90
[...]

Use kobj->state_in_sysfs to check whether kobject is added succeed. And
if not, we should not call kobject_del().

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 kernel/irq/irqdesc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Greg KH Nov. 12, 2022, 8:59 a.m. UTC | #1 
On Sat, Nov 12, 2022 at 05:39:39PM +0800, Liu Shixin wrote:
> If irq_sysfs_add() failed, system will report a warning but don't call
> kobject_put() to release the descriptor.

I can not parse this sentance :(

> Then in irq_sysfs_del(), we continue to call kobject_del(). In such
> situation, kobject_del() will complains about a object with no parent
> like this:

Then we should not be calling irq_sysfs_del() if the call failed.  That
is the real fix here.

> 
>  kernfs: can not remove 'actions', no directory
>  WARNING: CPU: 0 PID: 277 at fs/kernfs/dir.c:1615 kernfs_remove_by_name_ns+0xd5/0xe0
> [...]
>  Call Trace:
>   <TASK>
>   remove_files.isra.0+0x3f/0xb0
>   sysfs_remove_group+0x68/0xe0
>   sysfs_remove_groups+0x41/0x70
>   __kobject_del+0x45/0xc0
>   kobject_del+0x2a/0x40
>   free_desc+0x44/0x70
>   irq_free_descs+0x5d/0x90
> [...]
> 
> Use kobj->state_in_sysfs to check whether kobject is added succeed. And
> if not, we should not call kobject_del().

That does not describe what you are doing here at all.

> 
> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> ---
>  kernel/irq/irqdesc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> index a91f9001103c..a820d96210d4 100644
> --- a/kernel/irq/irqdesc.c
> +++ b/kernel/irq/irqdesc.c
> @@ -300,10 +300,11 @@ static void irq_sysfs_del(struct irq_desc *desc)
>  	/*
>  	 * If irq_sysfs_init() has not yet been invoked (early boot), then
>  	 * irq_kobj_base is NULL and the descriptor was never added.
> +	 * And the descriptor may be added failed.
>  	 * kobject_del() complains about a object with no parent, so make
>  	 * it conditional.
>  	 */
> -	if (irq_kobj_base)
> +	if (irq_kobj_base && desc->kobj.parent)

How would the parent be NULL?  Parent devices always stick around until
the child is removed, otherwise something is really wrong here.  You
should never have to look at the parent.

thanks,

greg k-h
Liu Shixin Nov. 12, 2022, 9:19 a.m. UTC | #2 
On 2022/11/12 16:59, Greg Kroah-Hartman wrote:
> On Sat, Nov 12, 2022 at 05:39:39PM +0800, Liu Shixin wrote:
>> If irq_sysfs_add() failed, system will report a warning but don't call
>> kobject_put() to release the descriptor.
> I can not parse this sentance :(
irq_sysfs_add() call kobject_add(). If kobject_add() failed, will print "Failed to add kobject for irq".
But not call kobject_put().
>
>> Then in irq_sysfs_del(), we continue to call kobject_del(). In such
>> situation, kobject_del() will complains about a object with no parent
>> like this:
> Then we should not be calling irq_sysfs_del() if the call failed.  That
> is the real fix here.
If so, should I add a variable to record whether kobject has alreadly added or not?
>>  kernfs: can not remove 'actions', no directory
>>  WARNING: CPU: 0 PID: 277 at fs/kernfs/dir.c:1615 kernfs_remove_by_name_ns+0xd5/0xe0
>> [...]
>>  Call Trace:
>>   <TASK>
>>   remove_files.isra.0+0x3f/0xb0
>>   sysfs_remove_group+0x68/0xe0
>>   sysfs_remove_groups+0x41/0x70
>>   __kobject_del+0x45/0xc0
>>   kobject_del+0x2a/0x40
>>   free_desc+0x44/0x70
>>   irq_free_descs+0x5d/0x90
>> [...]
>>
>> Use kobj->state_in_sysfs to check whether kobject is added succeed. And
>> if not, we should not call kobject_del().
> That does not describe what you are doing here at all.
Sorry, I forget to update...
>
>> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
>> ---
>>  kernel/irq/irqdesc.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
>> index a91f9001103c..a820d96210d4 100644
>> --- a/kernel/irq/irqdesc.c
>> +++ b/kernel/irq/irqdesc.c
>> @@ -300,10 +300,11 @@ static void irq_sysfs_del(struct irq_desc *desc)
>>  	/*
>>  	 * If irq_sysfs_init() has not yet been invoked (early boot), then
>>  	 * irq_kobj_base is NULL and the descriptor was never added.
>> +	 * And the descriptor may be added failed.
>>  	 * kobject_del() complains about a object with no parent, so make
>>  	 * it conditional.
>>  	 */
>> -	if (irq_kobj_base)
>> +	if (irq_kobj_base && desc->kobj.parent)
> How would the parent be NULL?  Parent devices always stick around until
> the child is removed, otherwise something is really wrong here.  You
> should never have to look at the parent.
irq_sysfs_add() call kobject_add(). If kobject_add() failed, the parent will be NULL.
You can find the same check of kobj->parent in cpuid_cpu_offline().

thanks,

Liu Shixin
.
>
> thanks,
>
> greg k-h
> .
>
Greg KH Nov. 12, 2022, 9:34 a.m. UTC | #3 
On Sat, Nov 12, 2022 at 05:19:50PM +0800, Liu Shixin wrote:
> 
> 
> On 2022/11/12 16:59, Greg Kroah-Hartman wrote:
> > On Sat, Nov 12, 2022 at 05:39:39PM +0800, Liu Shixin wrote:
> >> If irq_sysfs_add() failed, system will report a warning but don't call
> >> kobject_put() to release the descriptor.
> > I can not parse this sentance :(
> irq_sysfs_add() call kobject_add(). If kobject_add() failed, will print "Failed to add kobject for irq".
> But not call kobject_put().

Then fix that.

> >> Then in irq_sysfs_del(), we continue to call kobject_del(). In such
> >> situation, kobject_del() will complains about a object with no parent
> >> like this:
> > Then we should not be calling irq_sysfs_del() if the call failed.  That
> > is the real fix here.
> If so, should I add a variable to record whether kobject has alreadly added or not?

The code itself knows what just failed, handle the error case there
properly.

> >>  kernfs: can not remove 'actions', no directory
> >>  WARNING: CPU: 0 PID: 277 at fs/kernfs/dir.c:1615 kernfs_remove_by_name_ns+0xd5/0xe0
> >> [...]
> >>  Call Trace:
> >>   <TASK>
> >>   remove_files.isra.0+0x3f/0xb0
> >>   sysfs_remove_group+0x68/0xe0
> >>   sysfs_remove_groups+0x41/0x70
> >>   __kobject_del+0x45/0xc0
> >>   kobject_del+0x2a/0x40
> >>   free_desc+0x44/0x70
> >>   irq_free_descs+0x5d/0x90
> >> [...]
> >>
> >> Use kobj->state_in_sysfs to check whether kobject is added succeed. And
> >> if not, we should not call kobject_del().
> > That does not describe what you are doing here at all.
> Sorry, I forget to update...
> >
> >> Signed-off-by: Liu Shixin <liushixin2@huawei.com>
> >> ---
> >>  kernel/irq/irqdesc.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
> >> index a91f9001103c..a820d96210d4 100644
> >> --- a/kernel/irq/irqdesc.c
> >> +++ b/kernel/irq/irqdesc.c
> >> @@ -300,10 +300,11 @@ static void irq_sysfs_del(struct irq_desc *desc)
> >>  	/*
> >>  	 * If irq_sysfs_init() has not yet been invoked (early boot), then
> >>  	 * irq_kobj_base is NULL and the descriptor was never added.
> >> +	 * And the descriptor may be added failed.
> >>  	 * kobject_del() complains about a object with no parent, so make
> >>  	 * it conditional.
> >>  	 */
> >> -	if (irq_kobj_base)
> >> +	if (irq_kobj_base && desc->kobj.parent)
> > How would the parent be NULL?  Parent devices always stick around until
> > the child is removed, otherwise something is really wrong here.  You
> > should never have to look at the parent.
> irq_sysfs_add() call kobject_add(). If kobject_add() failed, the parent will be NULL.
> You can find the same check of kobj->parent in cpuid_cpu_offline().

And it is wrong there as well.  Do not copy bad patterns please.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index a91f9001103c..a820d96210d4 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -300,10 +300,11 @@  static void irq_sysfs_del(struct irq_desc *desc)
 	/*
 	 * If irq_sysfs_init() has not yet been invoked (early boot), then
 	 * irq_kobj_base is NULL and the descriptor was never added.
+	 * And the descriptor may be added failed.
 	 * kobject_del() complains about a object with no parent, so make
 	 * it conditional.
 	 */
-	if (irq_kobj_base)
+	if (irq_kobj_base && desc->kobj.parent)
 		kobject_del(&desc->kobj);
 }