Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200306235951.214678-1-dianders@chromium.org> <20200306155707.RFT.6.Icf2213131ea652087f100129359052c83601f8b0@changeid>
In-Reply-To: <20200306155707.RFT.6.Icf2213131ea652087f100129359052c83601f8b0@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 10 Mar 2020 17:33:24 -0700
Message-ID: <CAD=FV=U0OX-HFK8jcj5HU9_f9tPq13GEZpc7er2knMYS6sdDNA@mail.gmail.com>
Subject: Re: [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use"
 for ACTIVE_ONLY
To:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Maulik Shah <mkshah@codeaurora.org>
Cc:     Rajendra Nayak <rnayak@codeaurora.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Evan Green <evgreen@chromium.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Lina Iyer <ilina@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> From trawling through the code (see the "A lot of comments" change) I
> found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs.
> ...yet tcs_is_free() was checking the variable called from
> tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY
> TCSs.
>
> Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS"
> register, which was presumably the important part.
>
> It also feels like for ACTIVE_ONLY TCSs that it probably wasn't
> important to check the "RSC_DRV_STATUS".  We'll keep doing it just in
> case but we'll add a warning if it ever actually mattered.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)

After other RPMH email threads, it's possible that this patch isn't
quite right.  ...but also the code wasn't quite right before.

Specifically if we have 0 active TCSs then it's possible that we've
used a wake TCS to send an active-only request.  That would be a case
where "tcs_in_use" might be set and we'd need to make sure that
tcs_invalidate() checks it.  However:

1. We need to add locking to tcs_invalidate() since "tcs_in_use" is
protected by drv->lock and tcs_invalidate() didn't grab that lock.

2. We should add documentation explaining the situation.


I'm still waiting on overall review / testing of my series, but I'll
put it on my list to address this for v2.

-Doug