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

[abhinavk@codeaurora.org]

Hi Stephen

Thanks for the review.
I will break this up into patches according to the class of warning to 
show the warning in the commit text
and resend the patches.

Abhinav
On 2021-03-04 23:23, Dan Carpenter wrote:
> 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
> 
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno