linux-kernel - Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 14 Jul 2022 15:14:39 -0400
From:   Alex Deucher <alexdeucher@...il.com>
To:     André Almeida <andrealmeid@...lia.com>
Cc:     Maíra Canal <mairacanal@...eup.net>,
        Magali Lemes <magalilemes00@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Tales Lelo da Aparecida <tales.aparecida@...il.com>,
        xinhui pan <Xinhui.Pan@....com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        amd-gfx list <amd-gfx@...ts.freedesktop.org>,
        Christian Koenig <christian.koenig@....com>,
        Melissa Wen <mwen@...lia.com>, Leo Li <sunpeng.li@....com>,
        Dmytro Laktyushkin <Dmytro.Laktyushkin@....com>,
        Aurabindo Pillai <aurabindo.pillai@....com>,
        Daniel Vetter <daniel@...ll.ch>,
        Alex Deucher <alexander.deucher@....com>,
        Isabella Basso <isabbasso@...eup.net>, andrealmeid@...eup.net,
        Harry Wentland <harry.wentland@....com>,
        Nicholas Kazlauskas <nicholas.kazlauskas@....com>
Subject: Re: [PATCH 01/12] drm/amdgpu: Write masked value to control register

On Thu, Jul 14, 2022 at 3:05 PM André Almeida <andrealmeid@...lia.com> wrote:
>
> Hi Maíra,
>
> Thank you for your patch,
>
> Às 13:44 de 14/07/22, Maíra Canal escreveu:
> > On the dce_v6_0 and dce_v8_0 hpd tear down callback, the tmp variable
> > should be written into the control register instead of 0.
> >
>
> Why? I do see that tmp was unused before your patch, but why should we
> write it into this register? Did you manage to test this somehow?

The patch is correct.  We should only be clearing the enable bit in
this case, not the entire register.  Clearing the other fields could
cause spurious hotplug events as it affects the short and long pulse
times for the HPD pin.

Alex

>
> > Fixes: b00861b9 ("drm/amd/amdgpu: port of DCE v6 to new headers (v3)")
> > Fixes: 2285b91c ("drm/amdgpu/dce8: simplify hpd code")
>
> Checking both commits, I can see that 0 is written at `mmDC_HPD1_CONTROL
> + N` register in _hpd_fini() in them, so if you are trying to fix the
> commit that inserted that behavior, I think aren't those ones. For instance:
>
> $ git show 2285b91c
>
> [...]
>
> @@ -479,28 +372,11 @@ static void dce_v8_0_hpd_fini(struct amdgpu_device
> *adev)
>         list_for_each_entry(connector, &dev->mode_config.connector_list,
> head) {
>                 struct amdgpu_connector *amdgpu_connector =
> to_amdgpu_connector(connector);
>
> -               switch (amdgpu_connector->hpd.hpd) {
> -               case AMDGPU_HPD_1:
> -                       WREG32(mmDC_HPD1_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_2:
> -                       WREG32(mmDC_HPD2_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_3:
> -                       WREG32(mmDC_HPD3_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_4:
> -                       WREG32(mmDC_HPD4_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_5:
> -                       WREG32(mmDC_HPD5_CONTROL, 0);
> -                       break;
> -               case AMDGPU_HPD_6:
> -                       WREG32(mmDC_HPD6_CONTROL, 0);
> -                       break;
> -               default:
> -                       break;
> -               }
> +               if (amdgpu_connector->hpd.hpd >= adev->mode_info.num_hpd)
> +                       continue;
> +
> +               WREG32(mmDC_HPD1_CONTROL +
> hpd_offsets[amdgpu_connector->hpd.hpd], 0);
> +
>
> 0 was the valued written here before this commit. The commit basically
> replaces the switch case with a sum in this snippet.
>
> thanks,
>         andré