Re: [PATCH] drivers: mpt3sas: mpt3sas_debugfs: return value check of `mpt3sas_debugfs_root`

[Dan Carpenter <dan.carpenter@linaro.org>]

On Mon, May 08, 2023 at 09:40:41PM +0800, Dongliang Mu wrote:
> > > > diff --git a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
> > > > index a6ab1db81167..c92e08c130b9 100644
> > > > --- a/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
> > > > +++ b/drivers/scsi/mpt3sas/mpt3sas_debugfs.c
> > > > @@ -99,8 +99,6 @@ static const struct file_operations mpt3sas_debugfs_iocdump_fops = {
> > > >   void mpt3sas_init_debugfs(void)
> > > >   {
> > > >   	mpt3sas_debugfs_root = debugfs_create_dir("mpt3sas", NULL);
> > > > -	if (!mpt3sas_debugfs_root)
> > > > -		pr_info("mpt3sas: Cannot create debugfs root\n");
> > > Hi Jing,
> > > most drivers just ignore the return value but here the author wanted to
> > > have the information logged.
> > > Can you instead of removing the message modify the 'if' condition so it
> > > suits the author's intention?
> > 
> > This code was always just wrong.
> > 
> > The history of this is slightly complicated and boring.  These days it's
> > harmless dead code so I guess it's less bad than before.
> 
> Hi Dan and Tomas,
> 
> Any conclusion about this patch? The student Jing Xu is not sure about how
> to revise this patch.

The correct fix is to delete the code.

Debugfs code has error checking built in and was never supposed to be
checked for errors in normal driver code.

Originally, debugfs returned a mix of error pointers and NULL.  In the
kernel, when you have a mix of error pointers and NULL, then the NULL
means that the feature has been disabled deliberately.  It's not an
error, we should not print a message.

So a different, correct-ish way to write write debugfs error handling
was to say:

	mpt3sas_debugfs_root = debugfs_create_dir("mpt3sas", NULL);
	if (IS_ERR(mpt3sas_debugfs_root))
		return PTR_ERR(mpt3sas_debugfs_root);

However, in those days, a lot of people didn't understand error pointers
and thought that "if (IS_ERR_OR_NULL(mpt3sas_debugfs_root)) {" was a
super secure way to check for errors.  Or they just got it wrong and
checked for NULL instead of error pointers.  Any of the checks are
wrong, but if (IS_ERR()) check was at least correct-ish.

I dealt with this a lot because of my work with Smatch.  I used to be
happy if I could persuade someone to write at least correct-ish code,
but it was pretty painful to try explain this over and over and very few
people deleted the checks.

Eventually Greg changed the code to never return NULL and mass deleted
the IS_ERR() checks.  Not returning NULL makes it simpler to understand.
And it makes it impossible to check in the correct-ish way so it kind of
forces people to just delete the error handling.

regards,
dan carpenter