[PATCH] malidp: no need to check return value of debugfs_create functions

[PATCH] malidp: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

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: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/gpu/drm/arm/malidp_drv.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 21725c9b9f5e..7d732423d70d 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -542,19 +542,12 @@ static const struct file_operations malidp_debugfs_fops = {
 static int malidp_debugfs_init(struct drm_minor *minor)
 {
 	struct malidp_drm *malidp = minor->dev->dev_private;
-	struct dentry *dentry = NULL;
 
 	malidp_error_stats_init(&malidp->de_errors);
 	malidp_error_stats_init(&malidp->se_errors);
 	spin_lock_init(&malidp->errors_lock);
-	dentry = debugfs_create_file("debug",
-				     S_IRUGO | S_IWUSR,
-				     minor->debugfs_root, minor->dev,
-				     &malidp_debugfs_fops);
-	if (!dentry) {
-		DRM_ERROR("Cannot create debug file\n");
-		return -ENOMEM;
-	}
+	debugfs_create_file("debug", S_IRUGO | S_IWUSR, minor->debugfs_root,
+			    minor->dev, &malidp_debugfs_fops);
 	return 0;
 }
 
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] malidp: no need to check return value of debugfs_create functions

[Liviu Dudau]

On Thu, Jun 13, 2019 at 03:28:29PM +0200, Greg Kroah-Hartman 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.

I remember when writing this code and testing not fully complete code that left
nodes around on removing the module that there were errors being returned by
debugfs_create_file(). Has that changed since 2 years ago? :)

> 
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Acked-by: Liviu Dudau <liviu.dudau@arm.com>

I'll pull it into the malidp tree.

Best regards,
Liviu

> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 21725c9b9f5e..7d732423d70d 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -542,19 +542,12 @@ static const struct file_operations malidp_debugfs_fops = {
>  static int malidp_debugfs_init(struct drm_minor *minor)
>  {
>  	struct malidp_drm *malidp = minor->dev->dev_private;
> -	struct dentry *dentry = NULL;
>  
>  	malidp_error_stats_init(&malidp->de_errors);
>  	malidp_error_stats_init(&malidp->se_errors);
>  	spin_lock_init(&malidp->errors_lock);
> -	dentry = debugfs_create_file("debug",
> -				     S_IRUGO | S_IWUSR,
> -				     minor->debugfs_root, minor->dev,
> -				     &malidp_debugfs_fops);
> -	if (!dentry) {
> -		DRM_ERROR("Cannot create debug file\n");
> -		return -ENOMEM;
> -	}
> +	debugfs_create_file("debug", S_IRUGO | S_IWUSR, minor->debugfs_root,
> +			    minor->dev, &malidp_debugfs_fops);
>  	return 0;
>  }
>  
> -- 
> 2.22.0
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] malidp: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Thu, Jun 13, 2019 at 03:52:22PM +0100, Liviu Dudau wrote:
> On Thu, Jun 13, 2019 at 03:28:29PM +0200, Greg Kroah-Hartman 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.
> 
> I remember when writing this code and testing not fully complete code that left
> nodes around on removing the module that there were errors being returned by
> debugfs_create_file(). Has that changed since 2 years ago? :)

Errors can be returned if you do something foolish:
	- pass an error as a parent pointer
	- pass a name that is already present
or if the system is out of resources
	- can not increment superblock reference
	- out of memory to create an inode

If those last two things are happening, then your system is crashing
already, debugfs is the least of your worries :)

> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Brian Starkey <brian.starkey@arm.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> 
> I'll pull it into the malidp tree.

Wonderful, thanks!

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] malidp: no need to check return value of debugfs_create functions

[Liviu Dudau]

On Thu, Jun 13, 2019 at 05:57:13PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 03:52:22PM +0100, Liviu Dudau wrote:
> > On Thu, Jun 13, 2019 at 03:28:29PM +0200, Greg Kroah-Hartman 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.
> > 
> > I remember when writing this code and testing not fully complete code that left
> > nodes around on removing the module that there were errors being returned by
> > debugfs_create_file(). Has that changed since 2 years ago? :)
> 
> Errors can be returned if you do something foolish:
> 	- pass an error as a parent pointer
> 	- pass a name that is already present

That is what I was hitting previously. If we follow the new advice of not
checking for errors does this mean I can now start to hide debugfs entries
by touching some debugfs files before modules get loaded?

Best regards,
Liviu

> or if the system is out of resources
> 	- can not increment superblock reference
> 	- out of memory to create an inode
> 
> If those last two things are happening, then your system is crashing
> already, debugfs is the least of your worries :)
> 
> > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > Cc: Brian Starkey <brian.starkey@arm.com>
> > > Cc: David Airlie <airlied@linux.ie>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > Acked-by: Liviu Dudau <liviu.dudau@arm.com>
> > 
> > I'll pull it into the malidp tree.
> 
> Wonderful, thanks!
> 
> greg k-h

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] malidp: no need to check return value of debugfs_create functions

[Greg Kroah-Hartman]

On Thu, Jun 13, 2019 at 05:27:55PM +0100, Liviu Dudau wrote:
> On Thu, Jun 13, 2019 at 05:57:13PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 03:52:22PM +0100, Liviu Dudau wrote:
> > > On Thu, Jun 13, 2019 at 03:28:29PM +0200, Greg Kroah-Hartman 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.
> > > 
> > > I remember when writing this code and testing not fully complete code that left
> > > nodes around on removing the module that there were errors being returned by
> > > debugfs_create_file(). Has that changed since 2 years ago? :)
> > 
> > Errors can be returned if you do something foolish:
> > 	- pass an error as a parent pointer
> > 	- pass a name that is already present
> 
> That is what I was hitting previously. If we follow the new advice of not
> checking for errors does this mean I can now start to hide debugfs entries
> by touching some debugfs files before modules get loaded?

How can you "touch" a debugfs file today?

And even if there is a duplicate debugfs file, you shouldn't care as
your driver should keep going even if creation of it fails as no
functionality of the code should ever rely on debugfs.

thanks,

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] malidp: no need to check return value of debugfs_create functions

[Liviu Dudau]

On Thu, Jun 13, 2019 at 07:42:15PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jun 13, 2019 at 05:27:55PM +0100, Liviu Dudau wrote:
> > On Thu, Jun 13, 2019 at 05:57:13PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jun 13, 2019 at 03:52:22PM +0100, Liviu Dudau wrote:
> > > > On Thu, Jun 13, 2019 at 03:28:29PM +0200, Greg Kroah-Hartman 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.
> > > > 
> > > > I remember when writing this code and testing not fully complete code that left
> > > > nodes around on removing the module that there were errors being returned by
> > > > debugfs_create_file(). Has that changed since 2 years ago? :)
> > > 
> > > Errors can be returned if you do something foolish:
> > > 	- pass an error as a parent pointer
> > > 	- pass a name that is already present
> > 
> > That is what I was hitting previously. If we follow the new advice of not
> > checking for errors does this mean I can now start to hide debugfs entries
> > by touching some debugfs files before modules get loaded?
> 
> How can you "touch" a debugfs file today?

Touché! Yes, last time it was through my sloppy coding :)

> 
> And even if there is a duplicate debugfs file, you shouldn't care as
> your driver should keep going even if creation of it fails as no
> functionality of the code should ever rely on debugfs.

Agree and understood. Thanks for taking the time to explain it to me!

Best regards,
Liviu

> 
> thanks,
> 
> greg k-h

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel