When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Tomas Winkler <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: [email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
v2: break the patch up properly
drivers/misc/mei/debugfs.c | 47 +++++++++-----------------------------
drivers/misc/mei/main.c | 8 +------
drivers/misc/mei/mei_dev.h | 7 ++----
3 files changed, 14 insertions(+), 48 deletions(-)
diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
index 0970142bcace..df6bf8b81936 100644
--- a/drivers/misc/mei/debugfs.c
+++ b/drivers/misc/mei/debugfs.c
@@ -233,47 +233,22 @@ void mei_dbgfs_deregister(struct mei_device *dev)
*
* @dev: the mei device structure
* @name: the mei device name
- *
- * Return: 0 on success, <0 on failure.
*/
-int mei_dbgfs_register(struct mei_device *dev, const char *name)
+void mei_dbgfs_register(struct mei_device *dev, const char *name)
{
- struct dentry *dir, *f;
+ struct dentry *dir;
dir = debugfs_create_dir(name, NULL);
- if (!dir)
- return -ENOMEM;
-
dev->dbgfs_dir = dir;
- f = debugfs_create_file("meclients", S_IRUSR, dir,
- dev, &mei_dbgfs_fops_meclients);
- if (!f) {
- dev_err(dev->dev, "meclients: registration failed\n");
- goto err;
- }
- f = debugfs_create_file("active", S_IRUSR, dir,
- dev, &mei_dbgfs_fops_active);
- if (!f) {
- dev_err(dev->dev, "active: registration failed\n");
- goto err;
- }
- f = debugfs_create_file("devstate", S_IRUSR, dir,
- dev, &mei_dbgfs_fops_devstate);
- if (!f) {
- dev_err(dev->dev, "devstate: registration failed\n");
- goto err;
- }
- f = debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR, dir,
- &dev->allow_fixed_address,
- &mei_dbgfs_fops_allow_fa);
- if (!f) {
- dev_err(dev->dev, "allow_fixed_address: registration failed\n");
- goto err;
- }
- return 0;
-err:
- mei_dbgfs_deregister(dev);
- return -ENODEV;
+ debugfs_create_file("meclients", S_IRUSR, dir, dev,
+ &mei_dbgfs_fops_meclients);
+ debugfs_create_file("active", S_IRUSR, dir, dev,
+ &mei_dbgfs_fops_active);
+ debugfs_create_file("devstate", S_IRUSR, dir, dev,
+ &mei_dbgfs_fops_devstate);
+ debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR, dir,
+ &dev->allow_fixed_address,
+ &mei_dbgfs_fops_allow_fa);
}
diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index ad02097d7fee..f894d1f8a53e 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -984,16 +984,10 @@ int mei_register(struct mei_device *dev, struct device *parent)
goto err_dev_create;
}
- ret = mei_dbgfs_register(dev, dev_name(clsdev));
- if (ret) {
- dev_err(clsdev, "cannot register debugfs ret = %d\n", ret);
- goto err_dev_dbgfs;
- }
+ mei_dbgfs_register(dev, dev_name(clsdev));
return 0;
-err_dev_dbgfs:
- device_destroy(mei_class, devno);
err_dev_create:
cdev_del(&dev->cdev);
err_dev_add:
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index fca832fcac57..f71a023aed3c 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -718,13 +718,10 @@ bool mei_hbuf_acquire(struct mei_device *dev);
bool mei_write_is_idle(struct mei_device *dev);
#if IS_ENABLED(CONFIG_DEBUG_FS)
-int mei_dbgfs_register(struct mei_device *dev, const char *name);
+void mei_dbgfs_register(struct mei_device *dev, const char *name);
void mei_dbgfs_deregister(struct mei_device *dev);
#else
-static inline int mei_dbgfs_register(struct mei_device *dev, const char *name)
-{
- return 0;
-}
+static inline void mei_dbgfs_register(struct mei_device *dev, const char *name) {}
static inline void mei_dbgfs_deregister(struct mei_device *dev) {}
#endif /* CONFIG_DEBUG_FS */
--
2.22.0
>
> When calling debugfs functions, there is no need to ever check the return
> value. The function can work or not, but the code logic should never do
> something different based on this.
Maybe need to mention that API has changed in patch ' ff9fb72bc07705c00795ca48631f7fffe24d2c6b' in 5.0
and create_dir() doesn't return NULL but ERR_PTR() and proper checking is done inside the debugfs functions.
Not sure how critical is that but, but this should go probably to stable 5.0+ as well.
Ack otherwise.
>
> Cc: Tomas Winkler <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: [email protected]
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> v2: break the patch up properly
>
> drivers/misc/mei/debugfs.c | 47 +++++++++-----------------------------
> drivers/misc/mei/main.c | 8 +------
> drivers/misc/mei/mei_dev.h | 7 ++----
> 3 files changed, 14 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c index
> 0970142bcace..df6bf8b81936 100644
> --- a/drivers/misc/mei/debugfs.c
> +++ b/drivers/misc/mei/debugfs.c
> @@ -233,47 +233,22 @@ void mei_dbgfs_deregister(struct mei_device *dev)
> *
> * @dev: the mei device structure
> * @name: the mei device name
> - *
> - * Return: 0 on success, <0 on failure.
> */
> -int mei_dbgfs_register(struct mei_device *dev, const char *name)
> +void mei_dbgfs_register(struct mei_device *dev, const char *name)
> {
> - struct dentry *dir, *f;
> + struct dentry *dir;
>
> dir = debugfs_create_dir(name, NULL);
> - if (!dir)
> - return -ENOMEM;
> -
> dev->dbgfs_dir = dir;
>
> - f = debugfs_create_file("meclients", S_IRUSR, dir,
> - dev, &mei_dbgfs_fops_meclients);
> - if (!f) {
> - dev_err(dev->dev, "meclients: registration failed\n");
> - goto err;
> - }
> - f = debugfs_create_file("active", S_IRUSR, dir,
> - dev, &mei_dbgfs_fops_active);
> - if (!f) {
> - dev_err(dev->dev, "active: registration failed\n");
> - goto err;
> - }
> - f = debugfs_create_file("devstate", S_IRUSR, dir,
> - dev, &mei_dbgfs_fops_devstate);
> - if (!f) {
> - dev_err(dev->dev, "devstate: registration failed\n");
> - goto err;
> - }
> - f = debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR,
> dir,
> - &dev->allow_fixed_address,
> - &mei_dbgfs_fops_allow_fa);
> - if (!f) {
> - dev_err(dev->dev, "allow_fixed_address: registration
> failed\n");
> - goto err;
> - }
> - return 0;
> -err:
> - mei_dbgfs_deregister(dev);
> - return -ENODEV;
> + debugfs_create_file("meclients", S_IRUSR, dir, dev,
> + &mei_dbgfs_fops_meclients);
> + debugfs_create_file("active", S_IRUSR, dir, dev,
> + &mei_dbgfs_fops_active);
> + debugfs_create_file("devstate", S_IRUSR, dir, dev,
> + &mei_dbgfs_fops_devstate);
> + debugfs_create_file("allow_fixed_address", S_IRUSR | S_IWUSR, dir,
> + &dev->allow_fixed_address,
> + &mei_dbgfs_fops_allow_fa);
> }
>
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c index
> ad02097d7fee..f894d1f8a53e 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -984,16 +984,10 @@ int mei_register(struct mei_device *dev, struct
> device *parent)
> goto err_dev_create;
> }
>
> - ret = mei_dbgfs_register(dev, dev_name(clsdev));
> - if (ret) {
> - dev_err(clsdev, "cannot register debugfs ret = %d\n", ret);
> - goto err_dev_dbgfs;
> - }
> + mei_dbgfs_register(dev, dev_name(clsdev));
>
> return 0;
>
> -err_dev_dbgfs:
> - device_destroy(mei_class, devno);
> err_dev_create:
> cdev_del(&dev->cdev);
> err_dev_add:
> diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h index
> fca832fcac57..f71a023aed3c 100644
> --- a/drivers/misc/mei/mei_dev.h
> +++ b/drivers/misc/mei/mei_dev.h
> @@ -718,13 +718,10 @@ bool mei_hbuf_acquire(struct mei_device *dev);
> bool mei_write_is_idle(struct mei_device *dev);
>
> #if IS_ENABLED(CONFIG_DEBUG_FS)
> -int mei_dbgfs_register(struct mei_device *dev, const char *name);
> +void mei_dbgfs_register(struct mei_device *dev, const char *name);
> void mei_dbgfs_deregister(struct mei_device *dev); #else -static inline int
> mei_dbgfs_register(struct mei_device *dev, const char *name) -{
> - return 0;
> -}
> +static inline void mei_dbgfs_register(struct mei_device *dev, const
> +char *name) {}
> static inline void mei_dbgfs_deregister(struct mei_device *dev) {} #endif /*
> CONFIG_DEBUG_FS */
>
> --
> 2.22.0
On Wed, Jun 19, 2019 at 08:25:58AM +0000, Winkler, Tomas wrote:
> >
> > When calling debugfs functions, there is no need to ever check the return
> > value. The function can work or not, but the code logic should never do
> > something different based on this.
>
> Maybe need to mention that API has changed in patch ' ff9fb72bc07705c00795ca48631f7fffe24d2c6b' in 5.0
> and create_dir() doesn't return NULL but ERR_PTR() and proper checking is done inside the debugfs functions.
> Not sure how critical is that but, but this should go probably to stable 5.0+ as well.
It's not critical at all, the odds of an error ever returning, or NULL,
from debugfs before is just as rare as it is today :)
> Ack otherwise.
great, thanks for reviewing it.
greg k-h