linux-kernel - Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train

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]

Message-ID: <160194558634.310579.5267169787902306024@swboyd.mtv.corp.google.com>
Date:   Mon, 05 Oct 2020 17:53:06 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     khsieh@...eaurora.org
Cc:     robdclark@...il.com, sean@...rly.run, tanmay@...eaurora.org,
        abhinavk@...eaurora.org, aravindh@...eaurora.org, airlied@...ux.ie,
        daniel@...ll.ch, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train

Quoting khsieh@...eaurora.org (2020-10-05 11:02:10)
> >> +       dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >> +
> >>         dp_display_disable(dp_display, 0);
> >> 
> >>         rc = dp_display_unprepare(dp);
> >>         if (rc)
> >>                 DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
> >> 
> >> -       dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> >> -
> >>         state =  atomic_read(&dp_display->hpd_state);
> >>         if (state == ST_DISCONNECT_PENDING) {
> > 
> > I don't understand the atomic nature of this hpd_state variable. Why is
> > it an atomic variable? Is taking a spinlock bad? What is to prevent the
> > atomic read here to not be interrupted and then this if condition check
> > be invalid because the variable has been updated somewhere else?
> hpd_state variable updated by multiple threads. however it was protected 
> by mutex.
> in theory, it should also work as u32. since it was declared as atomic 
> from beginning
> and it does not cause any negative effects, can we keep it as it is?
> 

It does cause negative effects by generating worse code for something
that is already protected from concurrency by a mutex. Can we make it an
enum and name the enum and then add a comment indicating that the
'event_mutex' lock protects this variable?