Re: [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Mar 04, 2021 at 10:55:58PM -0800, Stephen Boyd wrote:
> > @@ -368,44 +368,21 @@ static int dp_debug_init(struct dp_debug *dp_debug, struct drm_minor *minor)
> >         int rc = 0;
> >         struct dp_debug_private *debug = container_of(dp_debug,
> >                         struct dp_debug_private, dp_debug);
> > -       struct dentry *file;
> > -       struct dentry *test_active;
> > -       struct dentry *test_data, *test_type;
> >  
> > -       file = debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
> > +       debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
> >                         debug, &dp_debug_fops);
> > -       if (IS_ERR_OR_NULL(file)) {
> > -               rc = PTR_ERR(file);
> > -               DRM_ERROR("[%s] debugfs create file failed, rc=%d\n",
> > -                                 DEBUG_NAME, rc);
> > -       }
> >  
> > -       test_active = debugfs_create_file("msm_dp_test_active", 0444,
> > +       debugfs_create_file("msm_dp_test_active", 0444,
> >                         minor->debugfs_root,
> >                         debug, &test_active_fops);
> > -       if (IS_ERR_OR_NULL(test_active)) {
> > -               rc = PTR_ERR(test_active);
> > -               DRM_ERROR("[%s] debugfs test_active failed, rc=%d\n",
> > -                                 DEBUG_NAME, rc);
> > -       }
> >  
> > -       test_data = debugfs_create_file("msm_dp_test_data", 0444,
> > +       debugfs_create_file("msm_dp_test_data", 0444,
> >                         minor->debugfs_root,
> >                         debug, &dp_test_data_fops);
> > -       if (IS_ERR_OR_NULL(test_data)) {
> > -               rc = PTR_ERR(test_data);
> > -               DRM_ERROR("[%s] debugfs test_data failed, rc=%d\n",
> > -                                 DEBUG_NAME, rc);
> > -       }
> >  
> > -       test_type = debugfs_create_file("msm_dp_test_type", 0444,
> > +       debugfs_create_file("msm_dp_test_type", 0444,
> >                         minor->debugfs_root,
> >                         debug, &dp_test_type_fops);
> > -       if (IS_ERR_OR_NULL(test_type)) {
> > -               rc = PTR_ERR(test_type);
> > -               DRM_ERROR("[%s] debugfs test_type failed, rc=%d\n",
> > -                                 DEBUG_NAME, rc);
> > -       }
> 
> Debugfs failures.

[ Update.  I misunderstood what you were saying, and initially thought
  you were critiquing the patch instead of the commit message.  The
  patch looks okay.  Probably a lot of maintainers would prefer it
  broken multiple chunks with one patch per class of warning.  But I
  already wrote this email and I love the sound of my own voice so I'm
  sending it.  - dan ]

The Smatch warning for this was that the error handling was slightly
off because debugfs_create_file() doesn't return NULL these days.  But
really these functions are not supposed to be error checked in the
normal case.

If you do a `git grep -w debugfs_create_file` there are 1472 callers
and only 192 check.  This is partly because Greg went through and did a
mass delete of error handling.

The way that debugfs works is if you fail to create a directory then
the debugfs_create_file will check if the root is an error pointer.  So
passing it "handles" errors itself.

The one time where I've seen that checking for errors is essential is
if they driver dereferences the "test_data" dentry itself.  That's
pretty uncommon.

[ So probably the commit message for this chunk should be:

  Delete unnecessary debugfs error handling

  Debugfs functions are not supposed to be checked in the normal case
  so delete this code.  Also it silences a Smatch warning that we're
  checking for NULL when these functions only return error pointers.  ]

regards,
dan carpenter