1. Patchew
  2. linux
  3. [PATCH V5 net-next 0/8] Support some features for the HIBMCGE driver
  4. View patch

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

Jijie Shao posted 8 patches 1 year ago
There is a newer version of this series
[PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by Jijie Shao 1 year ago 
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);
+
+	return dentry;
+}
+EXPORT_SYMBOL_GPL(debugfs_create_devm_dir);
+
 /**
  * debugfs_create_automount - create automount point in the debugfs filesystem
  * @name: a pointer to a string containing the name of the file to create.
diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
index 59444b495d49..19d8c322debe 100644
--- a/include/linux/debugfs.h
+++ b/include/linux/debugfs.h
@@ -139,6 +139,9 @@ void debugfs_create_file_size(const char *name, umode_t mode,
 
 struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);
 
+struct dentry *debugfs_create_devm_dir(struct device *dev, const char *name,
+				       struct dentry *parent);
+
 struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 				      const char *dest);
 
@@ -286,6 +289,13 @@ static inline struct dentry *debugfs_create_dir(const char *name,
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct dentry *debugfs_create_devm_dir(struct device *dev,
+						     const char *name,
+						     struct dentry *parent)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct dentry *debugfs_create_symlink(const char *name,
 						    struct dentry *parent,
 						    const char *dest)
-- 
2.33.0
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by kernel test robot 1 year ago 
Hi Jijie,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jijie-Shao/debugfs-Add-debugfs_create_devm_dir-helper/20241206-192734
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241206111629.3521865-2-shaojijie%40huawei.com
patch subject: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
config: x86_64-buildonly-randconfig-003-20241206 (https://download.01.org/0day-ci/archive/20241207/202412070055.uUO1oKY8-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412070055.uUO1oKY8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412070055.uUO1oKY8-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/debugfs/inode.c: In function 'debugfs_create_devm_dir':
>> fs/debugfs/inode.c:643:17: warning: ignoring return value of 'ERR_PTR' declared with attribute 'warn_unused_result' [-Wunused-result]
     643 |                 ERR_PTR(ret);
         |                 ^~~~~~~~~~~~


vim +643 fs/debugfs/inode.c

   619	
   620	/**
   621	 * debugfs_create_devm_dir - Managed debugfs_create_dir()
   622	 * @dev: Device that owns the action
   623	 * @name: a pointer to a string containing the name of the directory to
   624	 *        create.
   625	 * @parent: a pointer to the parent dentry for this file.  This should be a
   626	 *          directory dentry if set.  If this parameter is NULL, then the
   627	 *          directory will be created in the root of the debugfs filesystem.
   628	 * Managed debugfs_create_dir(). dentry will automatically be remove on
   629	 * driver detach.
   630	 */
   631	struct dentry *debugfs_create_devm_dir(struct device *dev, const char *name,
   632					       struct dentry *parent)
   633	{
   634		struct dentry *dentry;
   635		int ret;
   636	
   637		dentry = debugfs_create_dir(name, parent);
   638		if (IS_ERR(dentry))
   639			return dentry;
   640	
   641		ret = devm_add_action_or_reset(dev, debugfs_remove_devm, dentry);
   642		if (ret)
 > 643			ERR_PTR(ret);
   644	
   645		return dentry;
   646	}
   647	EXPORT_SYMBOL_GPL(debugfs_create_devm_dir);
   648	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by kernel test robot 1 year ago 
Hi Jijie,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Jijie-Shao/debugfs-Add-debugfs_create_devm_dir-helper/20241206-192734
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241206111629.3521865-2-shaojijie%40huawei.com
patch subject: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
config: powerpc-ebony_defconfig (https://download.01.org/0day-ci/archive/20241206/202412062221.GPVtNG5v-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241206/202412062221.GPVtNG5v-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412062221.GPVtNG5v-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> fs/debugfs/inode.c:643:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
     643 |                 ERR_PTR(ret);
         |                 ^~~~~~~ ~~~
   1 warning generated.


vim +/warn_unused_result +643 fs/debugfs/inode.c

   619	
   620	/**
   621	 * debugfs_create_devm_dir - Managed debugfs_create_dir()
   622	 * @dev: Device that owns the action
   623	 * @name: a pointer to a string containing the name of the directory to
   624	 *        create.
   625	 * @parent: a pointer to the parent dentry for this file.  This should be a
   626	 *          directory dentry if set.  If this parameter is NULL, then the
   627	 *          directory will be created in the root of the debugfs filesystem.
   628	 * Managed debugfs_create_dir(). dentry will automatically be remove on
   629	 * driver detach.
   630	 */
   631	struct dentry *debugfs_create_devm_dir(struct device *dev, const char *name,
   632					       struct dentry *parent)
   633	{
   634		struct dentry *dentry;
   635		int ret;
   636	
   637		dentry = debugfs_create_dir(name, parent);
   638		if (IS_ERR(dentry))
   639			return dentry;
   640	
   641		ret = devm_add_action_or_reset(dev, debugfs_remove_devm, dentry);
   642		if (ret)
 > 643			ERR_PTR(ret);
   644	
   645		return dentry;
   646	}
   647	EXPORT_SYMBOL_GPL(debugfs_create_devm_dir);
   648	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by Greg KH 1 year ago 
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?

thanks,

greg k-h
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by Jijie Shao 1 year ago 
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.

Thanks,
Jijie Shao
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by Greg KH 1 year ago 
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
Re: [PATCH V5 net-next 1/8] debugfs: Add debugfs_create_devm_dir() helper
Posted by Jijie Shao 1 year ago 
on 2024/12/9 14:31, Greg KH wrote:
> 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

I've tested and it's ok,

But, there is a warning(warn_unused_result)..
My test command does not check warning other than the hibmcge driver.
This was caused by my carelessness. I'm very sorry.

I've looked at other driver codes, and quite a few others have similar codes.
As you suggest, this patchset still uses open code in next version.
In the future, I will try to send a new independent patchset to introduce it.

Thanks,
Jijie Shao

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.