Re: [PATCH 1/2] bna: Fix return value check for debugfs create APIs

[Simon Horman <horms@kernel.org>]

On Thu, Oct 24, 2024 at 09:26:30PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/10/24 20:13, Simon Horman wrote:
> > On Wed, Oct 23, 2024 at 04:09:20PM +0800, Zhen Lei wrote:
> >> Fix the incorrect return value check for debugfs_create_dir() and
> >> debugfs_create_file(), which returns ERR_PTR(-ERROR) instead of NULL
> >> when it fails.
> >>
> >> Commit 4ad23d2368cc ("bna: Remove error checking for
> >> debugfs_create_dir()") allows the program to continue execution if the
> >> creation of bnad->port_debugfs_root fails, which causes the atomic count
> >> bna_debugfs_port_count to be unbalanced. The corresponding error check
> >> need to be added back.
> > 
> > Hi Zhen Lei,
> > 
> > The documentation for debugfs_create_dir states:
> > 
> >  * NOTE: it's expected that most callers should _ignore_ the errors returned
> >  * by this function. Other debugfs functions handle the fact that the "dentry"
> >  * passed to them could be an error and they don't crash in that case.
> >  * Drivers should generally work fine even if debugfs fails to init anyway.
> > 
> > Which makes me wonder why we are checking the return value of
> > debugfs_create_dir() at all. Can't we just take advantage of
> > it not mattering, to debugfs functions, if the return value
> > is an error or not?
> 
> Do you want to ignore all the return values of debugfs_create_dir() and debugfs_create_file()?
> "bna_debugfs_root = debugfs_create_dir("bna", NULL);" and debugfs_create_file() is OK.
> I've carefully analyzed the current code, and "bnad->port_debugfs_root = debugfs_create_dir(...);"
> is also OK for now.

What I'm saying is that it is unusual to depend on the return value of
debugfs_create_dir() for anything. And it would be best to avoid doing so.

But perhaps that isn't possible for some reason?

> 
> bnad_debugfs_init():
> 	bnad->port_debugfs_root = debugfs_create_dir(name, bna_debugfs_root);	//IS_ERR() if fails
> (1)
> 	atomic_inc(&bna_debugfs_port_count);
> 
> bnad_debugfs_uninit():
> (2)	if (bnad->port_debugfs_root)						//It still works when it's IS_ERR()
> 		atomic_dec(&bna_debugfs_port_count);
> 
> 	if (atomic_read(&bna_debugfs_port_count) == 0)
> 		debugfs_remove(bna_debugfs_root);
> 
> If we want the code to be more robust or easier to understand, it is better
> to modify (1) and (2) above as follows:
> (1) if (IS_ERR(bnad->port_debugfs_root))
> 	return;
> (2) if (!IS_ERR_OR_NULL(bnad->port_debugfs_root))
> 
> > 
> >> Fixes: 4ad23d2368cc ("bna: Remove error checking for debugfs_create_dir()")
> >> Fixes: 7afc5dbde091 ("bna: Add debugfs interface.")
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> > 
> > ...
> > .
> > 
> 
> -- 
> Regards,
>   Zhen Lei
>