On Mon, 11 Oct 2021, Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Oct 11, 2021 at 07:38:22PM +0300, Jani Nikula wrote:
On Mon, 11 Oct 2021, Greg KH gregkh@linuxfoundation.org wrote:
On Mon, Oct 11, 2021 at 04:19:58PM +0200, Christian König wrote:
> > And then throw it away, later, when you want to remove the directory, > > look it up with a call to debugfs_lookup() and pass that to > > debugfs_remove() (which does so recursively). > > > > There should never be a need to save, or check, the result of any > > debugfs call. If so, odds are it is being used incorrectly. Yeah, exactly that's the problem I see here.
We save the return value because the DRM subsystem is creating a debugfs directory for the drivers to use.
That's fine for now, not a big deal. And even if there is an error, again, you can always feed that error back into the debugfs subsystem on another call and it will handle it correctly.
Problem is it isn't, we have a crash because the member isn't a pointer but an ERR_PTR instead.
Again, that is fine, you can feed that into debugfs and it will "just work". Treat it as an opaque pointer, not a *dentry and you will be fine.
Hmm, some of the patches add things like:
- if (!root)
goto error;- minor->debugfs_root = debugfs_create_dir(name, root);
Superficially this seems okay, as it looks like debugfs_create_dir() doesn't actually cope with NULL values.
Yes it does, it puts things at the root of debugfs.
Oh, thanks for the correction.
But why are you checking for NULL here, as the return value of a debugfs call can never be NULL?
Just musing on what is going on in the patch, and why such changes initially seem like good ideas and get through review.
Thanks, Jani.
However, since ->debugfs_root comes from debugfs_create_dir() I presume it'll never be NULL on errors anyway but rather an error pointer!
That is correct.
So I think we probably need to go through the drm subsystem and look for existing similar patterns in fix them.
Please do. I know I made one pass at it a while ago but I think someone else went through and cleaned them up again.
thanks,
greg k-h