* [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
@ 2021-03-05 1:31 Abhinav Kumar
2021-03-05 6:55 ` Stephen Boyd
0 siblings, 1 reply; 4+ messages in thread
From: Abhinav Kumar @ 2021-03-05 1:31 UTC (permalink / raw)
To: dri-devel
Cc: Abhinav Kumar, linux-arm-msm, freedreno, robdclark, seanpaul,
swboyd, nganji, aravindh, tanmay, khsieh, dan.carpenter
Fix the warnings reported by kbot across MSM DP driver.
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
drivers/gpu/drm/msm/dp/dp_debug.c | 33 +++++----------------------------
drivers/gpu/drm/msm/dp/dp_hpd.c | 4 ++--
drivers/gpu/drm/msm/dp/dp_power.c | 2 +-
3 files changed, 8 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index 84670bc..2f6247e 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -226,7 +226,7 @@ static int dp_test_data_show(struct seq_file *m, void *data)
debug->link->test_video.test_h_width);
seq_printf(m, "vdisplay: %d\n",
debug->link->test_video.test_v_height);
- seq_printf(m, "bpc: %u\n",
+ seq_printf(m, "bpc: %u\n",
dp_link_bit_depth_to_bpc(bpc));
} else
seq_puts(m, "0");
@@ -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);
- }
debug->root = minor->debugfs_root;
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
index 5b8fe3202..e1c90fa 100644
--- a/drivers/gpu/drm/msm/dp/dp_hpd.c
+++ b/drivers/gpu/drm/msm/dp/dp_hpd.c
@@ -34,8 +34,8 @@ int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
dp_usbpd->hpd_high = hpd;
- if (!hpd_priv->dp_cb && !hpd_priv->dp_cb->configure
- && !hpd_priv->dp_cb->disconnect) {
+ if (!hpd_priv->dp_cb || !hpd_priv->dp_cb->configure
+ || !hpd_priv->dp_cb->disconnect) {
pr_err("hpd dp_cb not initialized\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index 9c4ea00..3961ba4 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -269,7 +269,7 @@ int dp_power_clk_enable(struct dp_power *dp_power,
DRM_ERROR("failed to '%s' clks for: %s. err=%d\n",
enable ? "enable" : "disable",
dp_parser_pm_name(pm_type), rc);
- return rc;
+ return rc;
}
if (pm_type == DP_CORE_PM)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
2021-03-05 1:31 [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver Abhinav Kumar
@ 2021-03-05 6:55 ` Stephen Boyd
2021-03-05 7:23 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2021-03-05 6:55 UTC (permalink / raw)
To: Abhinav Kumar, dri-devel
Cc: Abhinav Kumar, linux-arm-msm, freedreno, robdclark, seanpaul,
nganji, aravindh, tanmay, khsieh, dan.carpenter
Maybe subject could be "Ignore debugfs failures, fix indentation, fix
logical error"?
Quoting Abhinav Kumar (2021-03-04 17:31:52)
> Fix the warnings reported by kbot across MSM DP driver.
Which warnings? Can you include them? Or at least list the three things
that are being fixed in this patch?
>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
> drivers/gpu/drm/msm/dp/dp_debug.c | 33 +++++----------------------------
> drivers/gpu/drm/msm/dp/dp_hpd.c | 4 ++--
> drivers/gpu/drm/msm/dp/dp_power.c | 2 +-
> 3 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
> index 84670bc..2f6247e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_debug.c
> +++ b/drivers/gpu/drm/msm/dp/dp_debug.c
> @@ -226,7 +226,7 @@ static int dp_test_data_show(struct seq_file *m, void *data)
> debug->link->test_video.test_h_width);
> seq_printf(m, "vdisplay: %d\n",
> debug->link->test_video.test_v_height);
> - seq_printf(m, "bpc: %u\n",
> + seq_printf(m, "bpc: %u\n",
> dp_link_bit_depth_to_bpc(bpc));
> } else
> seq_puts(m, "0");
Indentation.
> @@ -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.
>
> debug->root = minor->debugfs_root;
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
> index 5b8fe3202..e1c90fa 100644
> --- a/drivers/gpu/drm/msm/dp/dp_hpd.c
> +++ b/drivers/gpu/drm/msm/dp/dp_hpd.c
> @@ -34,8 +34,8 @@ int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
>
> dp_usbpd->hpd_high = hpd;
>
> - if (!hpd_priv->dp_cb && !hpd_priv->dp_cb->configure
> - && !hpd_priv->dp_cb->disconnect) {
> + if (!hpd_priv->dp_cb || !hpd_priv->dp_cb->configure
> + || !hpd_priv->dp_cb->disconnect) {
Logical error.
> pr_err("hpd dp_cb not initialized\n");
> return -EINVAL;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 9c4ea00..3961ba4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -269,7 +269,7 @@ int dp_power_clk_enable(struct dp_power *dp_power,
> DRM_ERROR("failed to '%s' clks for: %s. err=%d\n",
> enable ? "enable" : "disable",
> dp_parser_pm_name(pm_type), rc);
> - return rc;
> + return rc;
Indentation.
> }
>
> if (pm_type == DP_CORE_PM)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
2021-03-05 6:55 ` Stephen Boyd
@ 2021-03-05 7:23 ` Dan Carpenter
2021-03-05 18:38 ` [Freedreno] " abhinavk
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-03-05 7:23 UTC (permalink / raw)
To: Stephen Boyd
Cc: Abhinav Kumar, dri-devel, linux-arm-msm, freedreno, robdclark,
seanpaul, nganji, aravindh, tanmay, khsieh
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Freedreno] [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
2021-03-05 7:23 ` Dan Carpenter
@ 2021-03-05 18:38 ` abhinavk
0 siblings, 0 replies; 4+ messages in thread
From: abhinavk @ 2021-03-05 18:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: Stephen Boyd, tanmay, linux-arm-msm, dri-devel, khsieh,
robdclark, nganji, seanpaul, aravindh, freedreno
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-03-05 18:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-05 1:31 [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver Abhinav Kumar
2021-03-05 6:55 ` Stephen Boyd
2021-03-05 7:23 ` Dan Carpenter
2021-03-05 18:38 ` [Freedreno] " abhinavk
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).