Re: [PATCH] cxl: no need to check return value of debugfs_create functions

[Frederic Barrat <fbarrat@linux.ibm.com>]

Le 12/06/2019 à 12:02, Greg Kroah-Hartman a écrit :
> On Wed, Jun 12, 2019 at 11:51:21AM +0200, Arnd Bergmann wrote:
>> On Tue, Jun 11, 2019 at 8:13 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>>
>>> @@ -64,8 +64,6 @@ int cxl_debugfs_adapter_add(struct cxl *adapter)
>>>
>>>          snprintf(buf, 32, "card%i", adapter->adapter_num);
>>>          dir = debugfs_create_dir(buf, cxl_debugfs);
>>> -       if (IS_ERR(dir))
>>> -               return PTR_ERR(dir);
>>>          adapter->debugfs = dir;
>>>
>>
>> Should the check for 'cxl_debugfs' get removed here as well?
> 
> Maybe, I could not determine the logic if those functions could be
> called before cxl_debugfs was ever set.
> 
> And debugfs_create_dir() will not return a NULL value if an error
> happens, so no need to worry about files being created in the wrong
> place.
> 
>> If that is null, we might put the subdir in the wrong place in the
>> tree, but that would otherwise be harmless as well, and the
>> same thing happens if 'dir' is NULL above and we add the
>> files in the debugfs root later (losing the ability to clean up
>> afterwards).
>>
>> int cxl_debugfs_adapter_add(struct cxl *adapter)
>> {
>>          struct dentry *dir;
>>          char buf[32];
>>
>>          if (!cxl_debugfs)
>>                  return -ENODEV;
>>
>> It's still a bit odd to return an error, since the caller then just
>> ignores the return code anway:
> 
> Then let's just return nothing.
> 
>>          /* Don't care if this one fails: */
>>          cxl_debugfs_adapter_add(adapter);
>>
>> It would seem best to change the return type to 'void' here for
>> consistency.
> 
> I agree, let me go do that.


I don't see any problems with turning all those function return types to 
'void'. Thanks for pointing it out and the clean up!

   Fred



> thanks,
> 
> greg k-h
>