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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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