* re: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
@ 2016-10-11 7:11 Dan Carpenter
2016-10-11 11:17 ` Zhu, Rex
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2016-10-11 7:11 UTC (permalink / raw)
To: Rex.Zhu; +Cc: dri-devel
Hello Rex Zhu,
This is a semi-automatic email about new static checker warnings.
The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to
manager asics with smu ip version 7." from Sep 9, 2016, leads to the
following Smatch complaint:
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:3809 smu7_check_states_equal()
warn: variable dereferenced before check 'pstate1' (see line 3805)
drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c
3804 {
3805 const struct smu7_power_state *psa = cast_const_phw_smu7_power_state(pstate1);
^^^^^^^
3806 const struct smu7_power_state *psb = cast_const_phw_smu7_power_state(pstate2);
^^^^^^^
New dereferences.
3807 int i;
3808
3809 if (pstate1 == NULL || pstate2 == NULL || equal == NULL)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Checked too late.
Just as an aside. People really don't review code in initializers.
I've commented on this before, and other people have noted it as well
but other people are like "In that case, those people shouldn't be
kernel devs!" which is so ignorant. Everyone consistently makes the
same mistakes. I've spent so many hours looking at basically this same
bug over and over and everyone does it.
That's nothing to do with your code, in particular, I just wanted to
update you on how my morning has been going. (Pretty well, thanks,
although I'm due for a second cup of coffee).
3810 return -EINVAL;
3811
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7. 2016-10-11 7:11 drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7 Dan Carpenter @ 2016-10-11 11:17 ` Zhu, Rex 2016-10-11 11:55 ` Christian König 0 siblings, 1 reply; 3+ messages in thread From: Zhu, Rex @ 2016-10-11 11:17 UTC (permalink / raw) To: Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org [-- Attachment #1: Type: text/plain, Size: 2022 bytes --] Thanks. Please review the attached patches. Best Regards Rex -----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Tuesday, October 11, 2016 3:11 PM To: Zhu, Rex Cc: dri-devel@lists.freedesktop.org Subject: re: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7. Hello Rex Zhu, This is a semi-automatic email about new static checker warnings. The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch complaint: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:3809 smu7_check_states_equal() warn: variable dereferenced before check 'pstate1' (see line 3805) drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c 3804 { 3805 const struct smu7_power_state *psa = cast_const_phw_smu7_power_state(pstate1); ^^^^^^^ 3806 const struct smu7_power_state *psb = cast_const_phw_smu7_power_state(pstate2); ^^^^^^^ New dereferences. 3807 int i; 3808 3809 if (pstate1 == NULL || pstate2 == NULL || equal == NULL) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Checked too late. Just as an aside. People really don't review code in initializers. I've commented on this before, and other people have noted it as well but other people are like "In that case, those people shouldn't be kernel devs!" which is so ignorant. Everyone consistently makes the same mistakes. I've spent so many hours looking at basically this same bug over and over and everyone does it. That's nothing to do with your code, in particular, I just wanted to update you on how my morning has been going. (Pretty well, thanks, although I'm due for a second cup of coffee). 3810 return -EINVAL; 3811 regards, dan carpenter [-- Attachment #2: 0001-drm-amd-powerplay-add-array-length-check-to-avoid-bu.patch --] [-- Type: application/octet-stream, Size: 1189 bytes --] From d8530e76a53f9a340305f7fb4bd59949d680e2ee Mon Sep 17 00:00:00 2001 From: Rex Zhu <Rex.Zhu@amd.com> Date: Tue, 11 Oct 2016 18:44:46 +0800 Subject: [PATCH 1/2] drm/amd/powerplay: add array length check to avoid buffer overflow. Change-Id: I14a4c7e365fddc14ddfbaec62b3e27e100f2b36f Signed-off-by: Rex Zhu <Rex.Zhu@amd.com> --- drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c index eda802b..8c889ca 100644 --- a/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c +++ b/drivers/gpu/drm/amd/powerplay/smumgr/iceland_smc.c @@ -2458,7 +2458,7 @@ static int iceland_set_mc_special_registers(struct pp_hwmgr *hwmgr, PP_ASSERT_WITH_CODE((j <= SMU71_DISCRETE_MC_REGISTER_ARRAY_SIZE), "Invalid VramInfo table.", return -EINVAL); - if (!data->is_memory_gddr5) { + if (!data->is_memory_gddr5 && j < SMU71_DISCRETE_MC_REGISTER_ARRAY_SIZE) { table->mc_reg_address[j].s1 = mmMC_PMG_AUTO_CMD; table->mc_reg_address[j].s0 = mmMC_PMG_AUTO_CMD; for (k = 0; k < table->num_entries; k++) { -- 1.9.1 [-- Attachment #3: 0002-drm-amd-powerplay-fix-bug-variable-dereferenced-befo.patch --] [-- Type: application/octet-stream, Size: 1518 bytes --] From bd50449ce9b19c574354be286f11e85d1e5ededc Mon Sep 17 00:00:00 2001 From: Rex Zhu <Rex.Zhu@amd.com> Date: Tue, 11 Oct 2016 18:51:16 +0800 Subject: [PATCH 2/2] drm/amd/powerplay: fix bug variable dereferenced before check it Change-Id: I23c6aded68ab9b20671ad99382b71a910465e5da Signed-off-by: Rex Zhu <Rex.Zhu@amd.com> --- drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c index f6afa6a..d6b3fd1 100644 --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c @@ -3821,13 +3821,15 @@ static int smu7_check_states_equal(struct pp_hwmgr *hwmgr, const struct pp_hw_power_state *pstate1, const struct pp_hw_power_state *pstate2, bool *equal) { - const struct smu7_power_state *psa = cast_const_phw_smu7_power_state(pstate1); - const struct smu7_power_state *psb = cast_const_phw_smu7_power_state(pstate2); + const struct smu7_power_state *psa; + const struct smu7_power_state *psb; int i; if (pstate1 == NULL || pstate2 == NULL || equal == NULL) return -EINVAL; + psa = cast_const_phw_smu7_power_state(pstate1); + psb = cast_const_phw_smu7_power_state(pstate2); /* If the two states don't even have the same number of performance levels they cannot be the same state. */ if (psa->performance_level_count != psb->performance_level_count) { *equal = false; -- 1.9.1 [-- Attachment #4: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7. 2016-10-11 11:17 ` Zhu, Rex @ 2016-10-11 11:55 ` Christian König 0 siblings, 0 replies; 3+ messages in thread From: Christian König @ 2016-10-11 11:55 UTC (permalink / raw) To: Zhu, Rex, Dan Carpenter; +Cc: dri-devel@lists.freedesktop.org [-- Attachment #1.1: Type: text/plain, Size: 2578 bytes --] Some commit messages would be nice. You could make the subject line shorter (something like "fix some warning in xyz.c") and actually use the current subject line as commit message. With that fixed the patches are Reviewed-by: Christian König <christian.koenig@amd.com>. Regards, Christian. Am 11.10.2016 um 13:17 schrieb Zhu, Rex: > Thanks. > Please review the attached patches. > > > Best Regards > Rex > > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: Tuesday, October 11, 2016 3:11 PM > To: Zhu, Rex > Cc: dri-devel@lists.freedesktop.org > Subject: re: drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7. > > Hello Rex Zhu, > > This is a semi-automatic email about new static checker warnings. > > The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch complaint: > > drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:3809 smu7_check_states_equal() > warn: variable dereferenced before check 'pstate1' (see line 3805) > > drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c > 3804 { > 3805 const struct smu7_power_state *psa = cast_const_phw_smu7_power_state(pstate1); > ^^^^^^^ > 3806 const struct smu7_power_state *psb = cast_const_phw_smu7_power_state(pstate2); > ^^^^^^^ New dereferences. > > 3807 int i; > 3808 > 3809 if (pstate1 == NULL || pstate2 == NULL || equal == NULL) > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Checked too late. > > Just as an aside. People really don't review code in initializers. > I've commented on this before, and other people have noted it as well but other people are like "In that case, those people shouldn't be kernel devs!" which is so ignorant. Everyone consistently makes the same mistakes. I've spent so many hours looking at basically this same bug over and over and everyone does it. > > That's nothing to do with your code, in particular, I just wanted to update you on how my morning has been going. (Pretty well, thanks, although I'm due for a second cup of coffee). > > 3810 return -EINVAL; > 3811 > > regards, > dan carpenter > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel [-- Attachment #1.2: Type: text/html, Size: 3513 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-11 11:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-11 7:11 drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7 Dan Carpenter 2016-10-11 11:17 ` Zhu, Rex 2016-10-11 11:55 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.