linux-kernel - RE: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Apr 2020 15:18:56 +0000
From:   "Koo, Anthony" <Anthony.Koo@....com>
To:     Colin King <colin.king@...onical.com>,
        "Wentland, Harry" <Harry.Wentland@....com>,
        "Li, Sun peng (Leo)" <Sunpeng.Li@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
CC:     "kernel-janitors@...r.kernel.org" <kernel-janitors@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH][next] drm/amd/display: fix incorrect assignment due to a
 typo

Hi Colin,

Sorry for any confusion of this code.
I think in this case, it seems like the comment is wrong (but original implementation is somewhat wrong as well). Probably the original code implementation makes it unclear.

There are three scenarios:
1. Variable refresh active, targeting a fixed rate
In this case, the min = max = fixed rate

2. Variable refresh active, with a variable range
In this case, the min = minimum refresh rate of the range.
And the max = maximum refresh rate of the range.

3. Variable refresh rate is disabled (The case you are modifying)
In the disabled case, we want to indicate to the display that the refresh rate is fixed, so we want to program min = max = the base refresh rate.
Today there seems to be an implication that max refresh = base refresh, which is not necessarily true.
I guess to make the code more clear and correct, the min and max should both be programmed equal to the base refresh rate (nominal field rate from mod_freesync_calc_nominal_field_rate)

Does that make sense?

Thanks,
Anthony

-----Original Message-----
From: Colin King <colin.king@...onical.com> 
Sent: Thursday, April 23, 2020 10:03 AM
To: Wentland, Harry <Harry.Wentland@....com>; Li, Sun peng (Leo) <Sunpeng.Li@....com>; Deucher, Alexander <Alexander.Deucher@....com>; Koenig, Christian <Christian.Koenig@....com>; Zhou, David(ChunMing) <David1.Zhou@....com>; David Airlie <airlied@...ux.ie>; Daniel Vetter <daniel@...ll.ch>; Koo, Anthony <Anthony.Koo@....com>; amd-gfx@...ts.freedesktop.org; dri-devel@...ts.freedesktop.org
Cc: kernel-janitors@...r.kernel.org; linux-kernel@...r.kernel.org
Subject: [PATCH][next] drm/amd/display: fix incorrect assignment due to a typo

From: Colin Ian King <colin.king@...onical.com>

The assignment to infopacket->sb[7] looks incorrect, the comment states it is the minimum refresh rate yet it is being assigned a value from the maximum refresh rate max_refresh_in_uhz. Fix this by using min_refresh_in_uhz instead.

Addresses-Coverity: ("Copy-paste error")
Fixes: d2bacc38f6ca ("drm/amd/display: Change infopacket type programming")
Signed-off-by: Colin Ian King <colin.king@...onical.com>
---
 drivers/gpu/drm/amd/display/modules/freesync/freesync.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
index eb7421e83b86..fe11436536e8 100644
--- a/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
+++ b/drivers/gpu/drm/amd/display/modules/freesync/freesync.c
@@ -587,7 +587,7 @@ static void build_vrr_infopacket_data_v3(const struct mod_vrr_params *vrr,
 	} else {
 		// Non-fs case, program nominal range
 		/* PB7 = FreeSync Minimum refresh rate (Hz) */
-		infopacket->sb[7] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
+		infopacket->sb[7] = (unsigned char)((vrr->min_refresh_in_uhz + 
+500000) / 1000000);
 		/* PB8 = FreeSync Maximum refresh rate (Hz) */
 		infopacket->sb[8] = (unsigned char)((vrr->max_refresh_in_uhz + 500000) / 1000000);
 	}
--
2.25.1