Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Dec 09, 2024 at 09:02:10AM +0800, Jijie Shao wrote:
> 
> on 2024/12/6 19:40, Greg KH wrote:
> > On Fri, Dec 06, 2024 at 07:16:22PM +0800, Jijie Shao wrote:
> > > Add debugfs_create_devm_dir() helper
> > > 
> > > Signed-off-by: Jijie Shao <shaojijie@huawei.com>
> > > ---
> > >   fs/debugfs/inode.c      | 36 ++++++++++++++++++++++++++++++++++++
> > >   include/linux/debugfs.h | 10 ++++++++++
> > >   2 files changed, 46 insertions(+)
> > > 
> > > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > > index 38a9c7eb97e6..f682c4952a27 100644
> > > --- a/fs/debugfs/inode.c
> > > +++ b/fs/debugfs/inode.c
> > > @@ -610,6 +610,42 @@ struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
> > >   }
> > >   EXPORT_SYMBOL_GPL(debugfs_create_dir);
> > > +static void debugfs_remove_devm(void *dentry_rwa)
> > > +{
> > > +	struct dentry *dentry = dentry_rwa;
> > > +
> > > +	debugfs_remove(dentry);
> > > +}
> > > +
> > > +/**
> > > + * debugfs_create_devm_dir - Managed debugfs_create_dir()
> > > + * @dev: Device that owns the action
> > > + * @name: a pointer to a string containing the name of the directory to
> > > + *        create.
> > > + * @parent: a pointer to the parent dentry for this file.  This should be a
> > > + *          directory dentry if set.  If this parameter is NULL, then the
> > > + *          directory will be created in the root of the debugfs filesystem.
> > > + * Managed debugfs_create_dir(). dentry will automatically be remove on
> > > + * driver detach.
> > > + */
> > > +struct dentry *debugfs_create_devm_dir(struct device *dev, const char *name,
> > > +				       struct dentry *parent)
> > > +{
> > > +	struct dentry *dentry;
> > > +	int ret;
> > > +
> > > +	dentry = debugfs_create_dir(name, parent);
> > > +	if (IS_ERR(dentry))
> > > +		return dentry;
> > > +
> > > +	ret = devm_add_action_or_reset(dev, debugfs_remove_devm, dentry);
> > > +	if (ret)
> > > +		ERR_PTR(ret);
> > You don't clean up the directory you created if this failed?  Why not?
> 
> Don't need to clean up.
> in devm_add_action_or_reset(), if failed, will call action: debugfs_remove_devm(),
> So, not clean up again.
> 
> #define devm_add_action_or_reset(dev, action, data) \
> 	__devm_add_action_or_reset(dev, action, data, #action)
> 
> static inline int __devm_add_action_or_reset(struct device *dev, void (*action)(void *),
> 					     void *data, const char *name)
> {
> 	int ret;
> 
> 	ret = __devm_add_action(dev, action, data, name);
> 	if (ret)
> 		action(data);
> 
> 	return ret;
> }
> 
> But there's a problem with this, I missed return.
> I will add return in v6.

As this did not even compile, how did you test any of this?

I'm now loath to add this at all, please let's just keep this "open
coded" in your driver for now until there are multiple users that need
this and then convert them all to use the function when you add it.

thanks,

greg k-h