| Message ID | 20190611181309.GA17098@kroah.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | cxl: no need to check return value of debugfs_create functions | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (a3bf9fbdad600b1e4335dd90979f8d6072e4f602) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 47 lines checked |
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
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
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 >
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
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; }
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(-)