[Freedreno] [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
abhinavk at codeaurora.org
abhinavk at codeaurora.org
Fri Mar 5 18:38:14 UTC 2021
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno
More information about the dri-devel
mailing list