* [PATCH] drm/amd/powerplay: return false instead of -EINVAL
@ 2016-09-02 9:55 Andrew Shadura
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Shadura @ 2016-09-02 9:55 UTC (permalink / raw)
To: linux-kernel, dri-devel
Returning -EINVAL from a bool-returning function
phm_check_smc_update_required_for_display_configuration has an unexpected
effect of returning true, which is probably not what was intended.
Replace -EINVAL by false.
The only place this function is called from is
psm_adjust_power_state_dynamic in
drivers/gpu/drm/amd/powerplay/eventmgr/psm.c:106:
if (!equal || phm_check_smc_update_required_for_display_configuration(hwmgr)) {
phm_apply_state_adjust_rules(hwmgr, requested, pcurrent);
phm_set_power_state(hwmgr, &pcurrent->hardware, &requested->hardware);
hwmgr->current_ps = requested;
}
It seems to expect a boolean value here.
This issue has been found using the following Coccinelle semantic patch
written by Peter Senna Tschudin:
<smpl>
@@
identifier f;
constant C;
typedef bool;
@@
bool f (...){
<+...
* return -C;
...+>
}
</smpl>
Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
---
drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
index 789f98a..82038b08 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
@@ -306,7 +306,7 @@ bool phm_check_smc_update_required_for_display_configuration(struct pp_hwmgr *hw
PHM_FUNC_CHECK(hwmgr);
if (hwmgr->hwmgr_func->check_smc_update_required_for_display_configuration == NULL)
- return -EINVAL;
+ return false;
return hwmgr->hwmgr_func->check_smc_update_required_for_display_configuration(hwmgr);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] drm/amd/powerplay: return false instead of -EINVAL
@ 2016-11-03 10:09 Andrew Shadura
2016-11-03 16:39 ` Deucher, Alexander
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Shadura @ 2016-11-03 10:09 UTC (permalink / raw)
To: linux-kernel, dri-devel, Alex Deucher, Christian König,
David Airlie
Cc: Rex Zhu, Jammy Zhou
Returning -EINVAL from a bool-returning function
phm_check_smc_update_required_for_display_configuration has an unexpected
effect of returning true, which is probably not what was intended.
Replace -EINVAL by false.
The only place this function is called from is
psm_adjust_power_state_dynamic in
drivers/gpu/drm/amd/powerplay/eventmgr/psm.c:106:
if (!equal || phm_check_smc_update_required_for_display_configuration(hwmgr)) {
phm_apply_state_adjust_rules(hwmgr, requested, pcurrent);
phm_set_power_state(hwmgr, &pcurrent->hardware, &requested->hardware);
hwmgr->current_ps = requested;
}
It seems to expect a boolean value here.
This issue has been found using the following Coccinelle semantic patch
written by Peter Senna Tschudin:
<smpl>
@@
identifier f;
constant C;
typedef bool;
@@
bool f (...){
<+...
* return -C;
...+>
}
</smpl>
Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
---
drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
index 789f98a..82038b08 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
@@ -306,7 +306,7 @@ bool phm_check_smc_update_required_for_display_configuration(struct pp_hwmgr *hw
PHM_FUNC_CHECK(hwmgr);
if (hwmgr->hwmgr_func->check_smc_update_required_for_display_configuration == NULL)
- return -EINVAL;
+ return false;
return hwmgr->hwmgr_func->check_smc_update_required_for_display_configuration(hwmgr);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread* RE: [PATCH] drm/amd/powerplay: return false instead of -EINVAL
2016-11-03 10:09 Andrew Shadura
@ 2016-11-03 16:39 ` Deucher, Alexander
2016-11-03 16:41 ` Andrew Shadura
0 siblings, 1 reply; 5+ messages in thread
From: Deucher, Alexander @ 2016-11-03 16:39 UTC (permalink / raw)
To: 'Andrew Shadura', linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Koenig, Christian, David Airlie
Cc: Zhu, Rex, Jammy Zhou
> -----Original Message-----
> From: Andrew Shadura [mailto:andrew.shadura@collabora.co.uk]
> Sent: Thursday, November 03, 2016 6:09 AM
> To: linux-kernel@vger.kernel.org; dri-devel@lists.freedesktop.org; Deucher,
> Alexander; Koenig, Christian; David Airlie
> Cc: Zhu, Rex; Jammy Zhou
> Subject: [PATCH] drm/amd/powerplay: return false instead of -EINVAL
>
> Returning -EINVAL from a bool-returning function
> phm_check_smc_update_required_for_display_configuration has an
> unexpected
> effect of returning true, which is probably not what was intended.
> Replace -EINVAL by false.
>
> The only place this function is called from is
> psm_adjust_power_state_dynamic in
> drivers/gpu/drm/amd/powerplay/eventmgr/psm.c:106:
>
> if (!equal ||
> phm_check_smc_update_required_for_display_configuration(hwmgr)) {
> phm_apply_state_adjust_rules(hwmgr, requested,
> pcurrent);
> phm_set_power_state(hwmgr, &pcurrent->hardware,
> &requested->hardware);
> hwmgr->current_ps = requested;
> }
>
> It seems to expect a boolean value here.
>
> This issue has been found using the following Coccinelle semantic patch
> written by Peter Senna Tschudin:
> <smpl>
> @@
> identifier f;
> constant C;
> typedef bool;
> @@
> bool f (...){
> <+...
> * return -C;
> ...+>
> }
> </smpl>
>
> Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
I'll pick this up shortly. Thanks!
Alex
> ---
> drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> index 789f98a..82038b08 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hardwaremanager.c
> @@ -306,7 +306,7 @@ bool
> phm_check_smc_update_required_for_display_configuration(struct
> pp_hwmgr *hw
> PHM_FUNC_CHECK(hwmgr);
>
> if (hwmgr->hwmgr_func-
> >check_smc_update_required_for_display_configuration == NULL)
> - return -EINVAL;
> + return false;
>
> return hwmgr->hwmgr_func-
> >check_smc_update_required_for_display_configuration(hwmgr);
> }
> --
> 2.7.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amd/powerplay: return false instead of -EINVAL
2016-11-03 16:39 ` Deucher, Alexander
@ 2016-11-03 16:41 ` Andrew Shadura
2016-11-03 16:46 ` Deucher, Alexander
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Shadura @ 2016-11-03 16:41 UTC (permalink / raw)
To: Deucher, Alexander, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Koenig, Christian, David Airlie
Cc: Zhu, Rex, Jammy Zhou
[-- Attachment #1.1: Type: text/plain, Size: 1337 bytes --]
On 03/11/16 17:39, Deucher, Alexander wrote:
>> Returning -EINVAL from a bool-returning function
>> phm_check_smc_update_required_for_display_configuration has an
>> unexpected
>> effect of returning true, which is probably not what was intended.
>> Replace -EINVAL by false.
>>
>> The only place this function is called from is
>> psm_adjust_power_state_dynamic in
>> drivers/gpu/drm/amd/powerplay/eventmgr/psm.c:106:
>>
>> if (!equal ||
>> phm_check_smc_update_required_for_display_configuration(hwmgr)) {
>> phm_apply_state_adjust_rules(hwmgr, requested,
>> pcurrent);
>> phm_set_power_state(hwmgr, &pcurrent->hardware,
>> &requested->hardware);
>> hwmgr->current_ps = requested;
>> }
>>
>> It seems to expect a boolean value here.
>>
>> This issue has been found using the following Coccinelle semantic patch
>> written by Peter Senna Tschudin:
>> <smpl>
>> @@
>> identifier f;
>> constant C;
>> typedef bool;
>> @@
>> bool f (...){
>> <+...
>> * return -C;
>> ...+>
>> }
>> </smpl>
>>
>> Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
>
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> I'll pick this up shortly. Thanks!
Great, thanks! I wonder who should I prod for this one:
https://patchwork.kernel.org/patch/7206081/
--
Cheers,
Andrew
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] drm/amd/powerplay: return false instead of -EINVAL
2016-11-03 16:41 ` Andrew Shadura
@ 2016-11-03 16:46 ` Deucher, Alexander
0 siblings, 0 replies; 5+ messages in thread
From: Deucher, Alexander @ 2016-11-03 16:46 UTC (permalink / raw)
To: 'Andrew Shadura', linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, Koenig, Christian, David Airlie
Cc: Zhu, Rex, Jammy Zhou
> -----Original Message-----
> From: Andrew Shadura [mailto:andrew.shadura@collabora.co.uk]
> Sent: Thursday, November 03, 2016 12:42 PM
> To: Deucher, Alexander; linux-kernel@vger.kernel.org; dri-
> devel@lists.freedesktop.org; Koenig, Christian; David Airlie
> Cc: Zhu, Rex; Jammy Zhou
> Subject: Re: [PATCH] drm/amd/powerplay: return false instead of -EINVAL
>
> On 03/11/16 17:39, Deucher, Alexander wrote:
> >> Returning -EINVAL from a bool-returning function
> >> phm_check_smc_update_required_for_display_configuration has an
> >> unexpected
> >> effect of returning true, which is probably not what was intended.
> >> Replace -EINVAL by false.
> >>
> >> The only place this function is called from is
> >> psm_adjust_power_state_dynamic in
> >> drivers/gpu/drm/amd/powerplay/eventmgr/psm.c:106:
> >>
> >> if (!equal ||
> >> phm_check_smc_update_required_for_display_configuration(hwmgr)) {
> >> phm_apply_state_adjust_rules(hwmgr, requested,
> >> pcurrent);
> >> phm_set_power_state(hwmgr, &pcurrent->hardware,
> >> &requested->hardware);
> >> hwmgr->current_ps = requested;
> >> }
> >>
> >> It seems to expect a boolean value here.
> >>
> >> This issue has been found using the following Coccinelle semantic patch
> >> written by Peter Senna Tschudin:
> >> <smpl>
> >> @@
> >> identifier f;
> >> constant C;
> >> typedef bool;
> >> @@
> >> bool f (...){
> >> <+...
> >> * return -C;
> >> ...+>
> >> }
> >> </smpl>
> >>
> >> Signed-off-by: Andrew Shadura <andrew.shadura@collabora.co.uk>
> >
> > Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> >
> > I'll pick this up shortly. Thanks!
>
> Great, thanks! I wonder who should I prod for this one:
> https://patchwork.kernel.org/patch/7206081/
PowerPC developers?
Alex
>
> --
> Cheers,
> Andrew
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-03 16:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-02 9:55 [PATCH] drm/amd/powerplay: return false instead of -EINVAL Andrew Shadura
-- strict thread matches above, loose matches on Subject: below --
2016-11-03 10:09 Andrew Shadura
2016-11-03 16:39 ` Deucher, Alexander
2016-11-03 16:41 ` Andrew Shadura
2016-11-03 16:46 ` Deucher, Alexander
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).