* Re: [PATCH] drm/amd/display: Fix an uninitialized index variable
2021-02-25 15:01 [PATCH] drm/amd/display: Fix an uninitialized index variable Arnd Bergmann
@ 2021-02-25 21:33 ` Nick Desaulniers
2021-02-25 22:08 ` Arnd Bergmann
2021-02-25 22:15 ` Alex Deucher
2021-03-02 17:54 ` Alex Deucher
2 siblings, 1 reply; 5+ messages in thread
From: Nick Desaulniers @ 2021-02-25 21:33 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Harry Wentland, Leo Li, Alex Deucher, Christian König,
Arnd Bergmann, David Airlie, Daniel Vetter, Nathan Chancellor,
Nicholas Kazlauskas, Rodrigo Siqueira, Aurabindo Pillai,
Stylon Wang, Eryk Brol, Bas Nieuwenhuizen, Bhawanpreet Lakha,
Simon Ser, Qingqing Zhuo, amd-gfx list, dri-devel, LKML,
clang-built-linux
On Thu, Feb 25, 2021 at 7:01 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> clang points out that the new logic uses an always-uninitialized
> array index:
>
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9810:38: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
> timing = &edid->detailed_timings[i];
> ^
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9720:7: note: initialize the variable 'i' to silence this warning
>
> My best guess is that the index should have been returned by the
> parse_hdmi_amd_vsdb() function that walks an array here, so do that.
>
> Fixes: f9b4f20c4777 ("drm/amd/display: Add Freesync HDMI support to DM")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b19b93c74bae..667c0d52dbfa 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9736,7 +9736,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> return false;
> }
>
> -static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> +static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> {
> uint8_t *edid_ext = NULL;
> @@ -9746,7 +9746,7 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> /*----- drm_find_cea_extension() -----*/
> /* No EDID or EDID extensions */
> if (edid == NULL || edid->extensions == 0)
> - return false;
> + return -ENODEV;
>
> /* Find CEA extension */
> for (i = 0; i < edid->extensions; i++) {
> @@ -9756,14 +9756,15 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> }
>
> if (i == edid->extensions)
> - return false;
> + return -ENODEV;
>
> /*----- cea_db_offsets() -----*/
> if (edid_ext[0] != CEA_EXT)
> - return false;
> + return -ENODEV;
>
> valid_vsdb_found = parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info);
> - return valid_vsdb_found;
> +
> + return valid_vsdb_found ? i : -ENODEV;
Thanks for the patch!
I don't think we need a local variable to store the return value from
one function call that's immediately returned, ie.
return parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info) ?
i : -ENODEV;
would suffice, but the patch is still fine as is.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> }
>
> void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> @@ -9781,7 +9782,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> struct amdgpu_device *adev = drm_to_adev(dev);
> bool freesync_capable = false;
> struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> - bool hdmi_valid_vsdb_found = false;
>
> if (!connector->state) {
> DRM_ERROR("%s - Connector has no state", __func__);
> @@ -9857,8 +9857,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> }
> }
> } else if (edid && amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> - hdmi_valid_vsdb_found = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> - if (hdmi_valid_vsdb_found && vsdb_info.freesync_supported) {
> + i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> + if (i >= 0 && vsdb_info.freesync_supported) {
reusing `i` here is safe, for now, but reuse of variables like this in
separate branches like this might not get noticed if the function is
amended in the future.
> timing = &edid->detailed_timings[i];
> data = &timing->data.other_data;
>
> --
> 2.29.2
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/amd/display: Fix an uninitialized index variable
2021-02-25 21:33 ` Nick Desaulniers
@ 2021-02-25 22:08 ` Arnd Bergmann
0 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2021-02-25 22:08 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Harry Wentland, Leo Li, Alex Deucher, Christian König,
Arnd Bergmann, David Airlie, Daniel Vetter, Nathan Chancellor,
Nicholas Kazlauskas, Rodrigo Siqueira, Aurabindo Pillai,
Stylon Wang, Eryk Brol, Bas Nieuwenhuizen, Bhawanpreet Lakha,
Simon Ser, Qingqing Zhuo, amd-gfx list, dri-devel, LKML,
clang-built-linux
On Thu, Feb 25, 2021 at 10:34 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
> return parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info) ? i : -ENODEV;
>
> would suffice, but the patch is still fine as is.
Right, I did not want to change more than necessary here, and the
original code already had the extra assignment instead of returning
the value.
> > @@ -9857,8 +9857,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> > }
> > }
> > } else if (edid && amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> > - hdmi_valid_vsdb_found = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> > - if (hdmi_valid_vsdb_found && vsdb_info.freesync_supported) {
> > + i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> > + if (i >= 0 && vsdb_info.freesync_supported) {
>
> reusing `i` here is safe, for now, but reuse of variables like this in
> separate branches like this might not get noticed if the function is
> amended in the future.
>
> > timing = &edid->detailed_timings[i];
> > data = &timing->data.other_data;
The entire point of the patch is that 'i' is in fact used in the following line,
but was lacking an intialization.
Arnd
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amd/display: Fix an uninitialized index variable
2021-02-25 15:01 [PATCH] drm/amd/display: Fix an uninitialized index variable Arnd Bergmann
2021-02-25 21:33 ` Nick Desaulniers
@ 2021-02-25 22:15 ` Alex Deucher
2021-03-02 17:54 ` Alex Deucher
2 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2021-02-25 22:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Harry Wentland, Leo Li, Alex Deucher, Christian König,
Stylon Wang, Eryk Brol, Arnd Bergmann, David Airlie,
Rodrigo Siqueira, Qingqing Zhuo, Nick Desaulniers, LKML,
amd-gfx list, Nathan Chancellor, clang-built-linux,
Aurabindo Pillai, Maling list - DRI developers,
Bhawanpreet Lakha, Nicholas Kazlauskas
On Thu, Feb 25, 2021 at 10:01 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> clang points out that the new logic uses an always-uninitialized
> array index:
>
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9810:38: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
> timing = &edid->detailed_timings[i];
> ^
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9720:7: note: initialize the variable 'i' to silence this warning
>
> My best guess is that the index should have been returned by the
> parse_hdmi_amd_vsdb() function that walks an array here, so do that.
>
> Fixes: f9b4f20c4777 ("drm/amd/display: Add Freesync HDMI support to DM")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
This looks correct to me. Stylon can you verify?
Thanks,
Alex
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b19b93c74bae..667c0d52dbfa 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9736,7 +9736,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> return false;
> }
>
> -static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> +static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> {
> uint8_t *edid_ext = NULL;
> @@ -9746,7 +9746,7 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> /*----- drm_find_cea_extension() -----*/
> /* No EDID or EDID extensions */
> if (edid == NULL || edid->extensions == 0)
> - return false;
> + return -ENODEV;
>
> /* Find CEA extension */
> for (i = 0; i < edid->extensions; i++) {
> @@ -9756,14 +9756,15 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> }
>
> if (i == edid->extensions)
> - return false;
> + return -ENODEV;
>
> /*----- cea_db_offsets() -----*/
> if (edid_ext[0] != CEA_EXT)
> - return false;
> + return -ENODEV;
>
> valid_vsdb_found = parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info);
> - return valid_vsdb_found;
> +
> + return valid_vsdb_found ? i : -ENODEV;
> }
>
> void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> @@ -9781,7 +9782,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> struct amdgpu_device *adev = drm_to_adev(dev);
> bool freesync_capable = false;
> struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> - bool hdmi_valid_vsdb_found = false;
>
> if (!connector->state) {
> DRM_ERROR("%s - Connector has no state", __func__);
> @@ -9857,8 +9857,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> }
> }
> } else if (edid && amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> - hdmi_valid_vsdb_found = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> - if (hdmi_valid_vsdb_found && vsdb_info.freesync_supported) {
> + i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> + if (i >= 0 && vsdb_info.freesync_supported) {
> timing = &edid->detailed_timings[i];
> data = &timing->data.other_data;
>
> --
> 2.29.2
>
> _______________________________________________
> 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/display: Fix an uninitialized index variable
2021-02-25 15:01 [PATCH] drm/amd/display: Fix an uninitialized index variable Arnd Bergmann
2021-02-25 21:33 ` Nick Desaulniers
2021-02-25 22:15 ` Alex Deucher
@ 2021-03-02 17:54 ` Alex Deucher
2 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2021-03-02 17:54 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Harry Wentland, Leo Li, Alex Deucher, Christian König,
Stylon Wang, Eryk Brol, Arnd Bergmann, David Airlie,
Rodrigo Siqueira, Qingqing Zhuo, Nick Desaulniers, LKML,
amd-gfx list, Nathan Chancellor, clang-built-linux,
Aurabindo Pillai, Maling list - DRI developers,
Bhawanpreet Lakha, Nicholas Kazlauskas
On Thu, Feb 25, 2021 at 10:01 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> clang points out that the new logic uses an always-uninitialized
> array index:
>
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9810:38: warning: variable 'i' is uninitialized when used here [-Wuninitialized]
> timing = &edid->detailed_timings[i];
> ^
> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm.c:9720:7: note: initialize the variable 'i' to silence this warning
>
> My best guess is that the index should have been returned by the
> parse_hdmi_amd_vsdb() function that walks an array here, so do that.
>
> Fixes: f9b4f20c4777 ("drm/amd/display: Add Freesync HDMI support to DM")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied. Thanks!
Alex
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index b19b93c74bae..667c0d52dbfa 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9736,7 +9736,7 @@ static bool parse_edid_cea(struct amdgpu_dm_connector *aconnector,
> return false;
> }
>
> -static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> +static int parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> struct edid *edid, struct amdgpu_hdmi_vsdb_info *vsdb_info)
> {
> uint8_t *edid_ext = NULL;
> @@ -9746,7 +9746,7 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> /*----- drm_find_cea_extension() -----*/
> /* No EDID or EDID extensions */
> if (edid == NULL || edid->extensions == 0)
> - return false;
> + return -ENODEV;
>
> /* Find CEA extension */
> for (i = 0; i < edid->extensions; i++) {
> @@ -9756,14 +9756,15 @@ static bool parse_hdmi_amd_vsdb(struct amdgpu_dm_connector *aconnector,
> }
>
> if (i == edid->extensions)
> - return false;
> + return -ENODEV;
>
> /*----- cea_db_offsets() -----*/
> if (edid_ext[0] != CEA_EXT)
> - return false;
> + return -ENODEV;
>
> valid_vsdb_found = parse_edid_cea(aconnector, edid_ext, EDID_LENGTH, vsdb_info);
> - return valid_vsdb_found;
> +
> + return valid_vsdb_found ? i : -ENODEV;
> }
>
> void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> @@ -9781,7 +9782,6 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> struct amdgpu_device *adev = drm_to_adev(dev);
> bool freesync_capable = false;
> struct amdgpu_hdmi_vsdb_info vsdb_info = {0};
> - bool hdmi_valid_vsdb_found = false;
>
> if (!connector->state) {
> DRM_ERROR("%s - Connector has no state", __func__);
> @@ -9857,8 +9857,8 @@ void amdgpu_dm_update_freesync_caps(struct drm_connector *connector,
> }
> }
> } else if (edid && amdgpu_dm_connector->dc_sink->sink_signal == SIGNAL_TYPE_HDMI_TYPE_A) {
> - hdmi_valid_vsdb_found = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> - if (hdmi_valid_vsdb_found && vsdb_info.freesync_supported) {
> + i = parse_hdmi_amd_vsdb(amdgpu_dm_connector, edid, &vsdb_info);
> + if (i >= 0 && vsdb_info.freesync_supported) {
> timing = &edid->detailed_timings[i];
> data = &timing->data.other_data;
>
> --
> 2.29.2
>
> _______________________________________________
> 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