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
> <gre...@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
