linux-kernel - Re: [PATCH v5 4/5] soc: qcom: rpmh-rsc: Simplify locking by eliminating the per-TCS lock

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <158774945643.135303.4651711262492851591@swboyd.mtv.corp.google.com>
Date:   Fri, 24 Apr 2020 10:30:56 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Douglas Anderson <dianders@...omium.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        rafael.j.wysocki@...el.com
Cc:     evgreen@...omium.org, mka@...omium.org, mkshah@...eaurora.org,
        Douglas Anderson <dianders@...omium.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 4/5] soc: qcom: rpmh-rsc: Simplify locking by eliminating the per-TCS lock

Quoting Douglas Anderson (2020-04-24 09:46:56)
> The rpmh-rsc code had both a driver-level lock (sometimes referred to
> in comments as drv->lock) and a lock per-TCS.  The idea was supposed
> to be that there would be times where you could get by with just
> locking a TCS lock and therefor other RPMH users wouldn't be blocked.
> 
> The above didn't work out so well.
> 
> Looking at tcs_write() the bigger drv->lock was held for most of the
> function anyway.  Only the __tcs_buffer_write() and
> __tcs_set_trigger() calls were called without holding the drv->lock.
> It actually turns out that in tcs_write() we don't need to hold the
> drv->lock for those function calls anyway even if the per-TCS lock
> isn't there anymore.  From the newly added comments in the code, this
> is because:
> - We marked "tcs_in_use" under lock.
> - Once "tcs_in_use" has been marked nobody else could be writing
>   to these registers until the interrupt goes off.
> - The interrupt can't go off until we trigger w/ the last line
>   of __tcs_set_trigger().
> Thus, from a tcs_write() point of view, the per-TCS lock was useless.
> 
> Looking at rpmh_rsc_write_ctrl_data(), only the per-TCS lock was held.
> It turns out, though, that this function already needs to be called
> with the equivalent of the drv->lock held anyway (we either need to
> hold drv->lock as we will in a future patch or we need to know no
> other CPUs could be running as happens today).  Specifically
> rpmh_rsc_write_ctrl_data() might be writing to a TCS that has been
> borrowed for writing an active transation but it never checks this.
> 
> Let's eliminate this extra overhead and avoid possible AB BA locking
> headaches.
> 
> Suggested-by: Maulik Shah <mkshah@...eaurora.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@...omium.org>