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

[Simon Horman <horms@kernel.org>]

On Fri, Oct 25, 2024 at 12:17:17PM +0800, Leizhen (ThunderTown) wrote:
> 
> 
> On 2024/10/24 23:27, Simon Horman wrote:
> > 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?
> 
> OK, I understand now. Please forgive my poor English. Combine Andrew's reply
> and my analysis above. The return value check for the remaining two places
> should now be removed.

Thanks. Sorry that I was not clearer.

...