Re: [PATCH] irqdomain: remove debugfs_file from struct irq_domain

[Greg KH]

On Wed, Feb 17, 2021 at 09:50:38PM +0000, Marc Zyngier wrote:
> On Wed, 17 Feb 2021 20:10:50 +0000,
> Michael Walle <michael@xxxxxxxx> wrote:
> > 
> > Am 2021-02-17 21:02, schrieb Marc Zyngier:
> > > On 2021-02-17 19:57, Michael Walle wrote:
> > >> Hi Greg,
> > >> 
> > >>> There's no need to keep around a dentry pointer to a simple file that
> > >>> debugfs itself can look up when we need to remove it from the system.
> > >>> So simplify the code by deleting the variable and cleaning up the
> > >>> logic
> > >>> around the debugfs file.
> > >> 
> > >> This will generate the following oops on my board (arm64,
> > >> freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts). In debugfs_lookup()
> > >> debugfs_mount is NULL.
> > > 
> > > That's odd. I gave it a go yesterday, and nothing blew up.
> > > Which makes me wonder whether I had the debug stuff enabled
> > > the first place...
> > > 
> > > I've dropped the patch from -next for now until I figure it out
> > > (probably tomorrow).
> > 
> > Mh, maybe its my .config, I've attached it. I also noticed that
> > the board boots just fine in our kernel-ci [1].
> 
> I reproduced here. I had disabled GENERIC_IRQ_DEBUGFS for obscure
> reasons, and it caught fire as I re-enabled it.
> 
> Adding this fixes it for me:
> 
> diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
> index 367ff1c35f75..d8a14cf1a7b6 100644
> --- a/kernel/irq/irqdomain.c
> +++ b/kernel/irq/irqdomain.c
> @@ -1904,7 +1904,8 @@ static void debugfs_add_domain_dir(struct irq_domain *d)
>  
>  static void debugfs_remove_domain_dir(struct irq_domain *d)
>  {
> -	debugfs_remove(debugfs_lookup(d->name, domain_dir));
> +	if (domain_dir)
> +		debugfs_remove(debugfs_lookup(d->name, domain_dir));
>  }
>  
>  void __init irq_domain_debugfs_init(struct dentry *root)
> 
> 
> Could you please check whether it works for you?

Can you try this debugfs core change instead?  Callers to debugfs should
not have to do the above type of checking as debugfs should be much more
robust than that.

thanks,

greg k-h


diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 2fcf66473436..5aa798f52b6e 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -297,7 +297,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
 {
 	struct dentry *dentry;
 
-	if (IS_ERR(parent))
+	if (IS_ERR_OR_NULL(name) || IS_ERR(parent))
 		return NULL;
 
 	if (!parent)