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

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

[Greg Kroah-Hartman]

When calling debugfs functions, there is no need to ever check the
return value.  The function can work or not, but the code logic should
never do something different based on this.

Because there's no need to check, also make the return value of the
local debugfs_create_io_x64() call void, as no one ever did anything
with the return value (as they did not need to.)

Cc: Frederic Barrat <fbarrat@linux.ibm.com>
Cc: Andrew Donnellan <ajd@linux.ibm.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/misc/cxl/debugfs.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/misc/cxl/debugfs.c b/drivers/misc/cxl/debugfs.c
index 1fda22c24c93..27f3bcb7d939 100644
--- a/drivers/misc/cxl/debugfs.c
+++ b/drivers/misc/cxl/debugfs.c
@@ -26,11 +26,11 @@ static int debugfs_io_u64_set(void *data, u64 val)
 DEFINE_DEBUGFS_ATTRIBUTE(fops_io_x64, debugfs_io_u64_get, debugfs_io_u64_set,
 			 "0x%016llx\n");
 
-static struct dentry *debugfs_create_io_x64(const char *name, umode_t mode,
-					    struct dentry *parent, u64 __iomem *value)
+static void debugfs_create_io_x64(const char *name, umode_t mode,
+				  struct dentry *parent, u64 __iomem *value)
 {
-	return debugfs_create_file_unsafe(name, mode, parent,
-					  (void __force *)value, &fops_io_x64);
+	debugfs_create_file_unsafe(name, mode, parent, (void __force *)value,
+				   &fops_io_x64);
 }
 
 void cxl_debugfs_add_adapter_regs_psl9(struct cxl *adapter, struct dentry *dir)
@@ -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;
 
 	debugfs_create_io_x64("err_ivte", S_IRUSR, dir, _cxl_p1_addr(adapter, CXL_PSL_ErrIVTE));
@@ -106,8 +104,6 @@ int cxl_debugfs_afu_add(struct cxl_afu *afu)
 
 	snprintf(buf, 32, "psl%i.%i", afu->adapter->adapter_num, afu->slice);
 	dir = debugfs_create_dir(buf, afu->adapter->debugfs);
-	if (IS_ERR(dir))
-		return PTR_ERR(dir);
 	afu->debugfs = dir;
 
 	debugfs_create_io_x64("sr",         S_IRUSR, dir, _cxl_p1n_addr(afu, CXL_PSL_SR_An));
@@ -129,15 +125,10 @@ void cxl_debugfs_afu_remove(struct cxl_afu *afu)
 
 int __init cxl_debugfs_init(void)
 {
-	struct dentry *ent;
-
 	if (!cpu_has_feature(CPU_FTR_HVMODE))
 		return 0;
 
-	ent = debugfs_create_dir("cxl", NULL);
-	if (IS_ERR(ent))
-		return PTR_ERR(ent);
-	cxl_debugfs = ent;
+	cxl_debugfs = debugfs_create_dir("cxl", NULL);
 
 	return 0;
 }
-- 
2.22.0

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

[Arnd Bergmann]

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?
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:

        /* 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.

     Arnd

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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

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

[Frederic Barrat]

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
> 

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

[Greg Kroah-Hartman]

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?
> 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).

dir can only be NULL if no one has initialized it, debugfs_create_dir()
will never return a null value.  I don't really know the ordering of the
calls here, so I'll keep this as-is for now incase someone is trying to
add a "device" before a directory is initialized.

thanks,

greg k-h