Re: [PATCH] genirq/irqdesc: hide illegible sysfs warning of kobject_del()

[Liu Shixin]

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@xxxxxxxxxx>
>> ---
>>  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
> .
>