Smatch complains that:
mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error
pointer or valid
There is no need to check the return value of the debugfs_create_dir()
function, just delete the dead code.
Fixes: 2b01b293f359 ("scsi: mpt3sas: Capture IOC data for debugging purposes")
Signed-off-by: Jing Xu <[email protected]>
Reviewed-by: Dongliang Mu <[email protected]>
---
drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 2 --
1 file changed, 2 deletions(-)
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");
}
/*
--
2.25.1
On 4/23/23 14:25, Jing Xu wrote:
> Smatch complains that:
> mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error
> pointer or valid
>
> There is no need to check the return value of the debugfs_create_dir()
> function, just delete the dead code.
>
> Fixes: 2b01b293f359 ("scsi: mpt3sas: Capture IOC data for debugging purposes")
> Signed-off-by: Jing Xu <[email protected]>
> Reviewed-by: Dongliang Mu <[email protected]>
> ---
> drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 2 --
> 1 file changed, 2 deletions(-)
>
> 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?
Also there are other similar issues in this file.
Regards,
Tomas
> }
>
> /*
On Tue, May 02, 2023 at 05:53:10PM +0200, Tomas Henzl wrote:
> On 4/23/23 14:25, Jing Xu wrote:
> > Smatch complains that:
> > mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error
> > pointer or valid
> >
> > There is no need to check the return value of the debugfs_create_dir()
> > function, just delete the dead code.
> >
> > Fixes: 2b01b293f359 ("scsi: mpt3sas: Capture IOC data for debugging purposes")
> > Signed-off-by: Jing Xu <[email protected]>
> > Reviewed-by: Dongliang Mu <[email protected]>
> > ---
> > drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > 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.
regards,
dan carpenter
On 5/3/23 01:06, Dan Carpenter wrote:
> On Tue, May 02, 2023 at 05:53:10PM +0200, Tomas Henzl wrote:
>> On 4/23/23 14:25, Jing Xu wrote:
>>> Smatch complains that:
>>> mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error
>>> pointer or valid
>>>
>>> There is no need to check the return value of the debugfs_create_dir()
>>> function, just delete the dead code.
>>>
>>> Fixes: 2b01b293f359 ("scsi: mpt3sas: Capture IOC data for debugging purposes")
>>> Signed-off-by: Jing Xu <[email protected]>
>>> Reviewed-by: Dongliang Mu <[email protected]>
>>> ---
>>> drivers/scsi/mpt3sas/mpt3sas_debugfs.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> 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.
>
>
> regards,
> dan carpenter
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
On 5/8/23 16:38, Dan Carpenter wrote:
> 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);
I'm fine with this as well, I could wish we get a fix for the exact same
case of debugfs_create_dir in mpt3sas_setup_debugfs and ideally all the
debugfs_create* in mpt3sas_debugfs.c in a single patch. But this patch
is ok even if that wasn't possible.
tomash
>
> 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
>
On Tue, May 23, 2023 at 04:48:12PM +0200, Tomas Henzl wrote:
> On 5/8/23 16:38, Dan Carpenter wrote:
> > 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);
> I'm fine with this as well, I could wish we get a fix for the exact same
> case of debugfs_create_dir in mpt3sas_setup_debugfs and ideally all the
> debugfs_create* in mpt3sas_debugfs.c in a single patch. But this patch
> is ok even if that wasn't possible.
> tomash
No, you didn't read until the end. That will break the driver badly.
This *used* to be a correct-ish way that *used* to work but it was never
the what Greg wanted. So to discourage people from doing it, Greg made
it *impossible* to check for if debugfs has failed. Literally, the only
correct thing to do now is to delete the debugfs checking.
regards,
dan carpenter
On 5/23/23 16:57, Dan Carpenter wrote:
> On Tue, May 23, 2023 at 04:48:12PM +0200, Tomas Henzl wrote:
>> On 5/8/23 16:38, Dan Carpenter wrote:
>>> 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);
>> I'm fine with this as well, I could wish we get a fix for the exact same
>> case of debugfs_create_dir in mpt3sas_setup_debugfs and ideally all the
>> debugfs_create* in mpt3sas_debugfs.c in a single patch. But this patch
>> is ok even if that wasn't possible.
>> tomash
>
> No, you didn't read until the end. That will break the driver badly.
>
> This *used* to be a correct-ish way that *used* to work but it was never
> the what Greg wanted. So to discourage people from doing it, Greg made
> it *impossible* to check for if debugfs has failed. Literally, the only
> correct thing to do now is to delete the debugfs checking.
I put my comment in on a wrong place I meant the original patch without
the check, sorry for the confusion. The only thing I'd like to see is to
have corrected all debugfs_create* and that is also optional.
>
> regards,
> dan carpenter
>
>
On Tue, May 23, 2023 at 07:56:02PM +0200, Tomas Henzl wrote:
> The only thing I'd like to see is to have corrected all debugfs_create*
> and that is also optional.
Yeah. We should fix everything in the file at once.
drivers/scsi/mpt3sas/mpt3sas_debugfs.c:102 mpt3sas_init_debugfs() warn: 'mpt3sas_debugfs_root' is an error pointer or valid
drivers/scsi/mpt3sas/mpt3sas_debugfs.c:127 mpt3sas_setup_debugfs() warn: 'ioc->debugfs_root' is an error pointer or valid
drivers/scsi/mpt3sas/mpt3sas_debugfs.c:137 mpt3sas_setup_debugfs() warn: 'ioc->ioc_dump' is an error pointer or valid
Ideally, someone would go through the kernel and do a mass update of
debugfs checking. CC Greg KH. Write a good cover letter that explains
all the background. That's how you get stuff like this merged with a
minimum fuss. It's probably too much to ask of the HUST students.
I have 120 of these warnings and most are debugfs related. I could
silence them but I'm hoping the iritation will make people fix it.
regards,
dan carpenter