netdev - Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024120925-dreamt-immorally-f9f8@gregkh>
Date: Mon, 9 Dec 2024 07:31:40 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Jijie Shao <shaojijie@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	shenjian15@...wei.com, wangpeiyang1@...wei.com,
	liuyonglong@...wei.com, chenhao418@...wei.com,
	sudongming1@...wei.com, xujunsheng@...wei.com,
	shiyongbang@...wei.com, libaihan@...wei.com,
	jonathan.cameron@...wei.com, shameerali.kolothum.thodi@...wei.com,
	salil.mehta@...wei.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, hkelam@...vell.com
Subject: Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir()
 helper

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