| Message ID | 20190613132829.GB4863@kroah.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | malidp: no need to check return value of debugfs_create functions | expand |
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 >
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
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
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
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
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; }
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(-)