LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
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>
Subject: Re: [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY
Date: Tue, 10 Mar 2020 17:33:24 -0700	[thread overview]
Message-ID: <CAD=FV=U0OX-HFK8jcj5HU9_f9tPq13GEZpc7er2knMYS6sdDNA@mail.gmail.com> (raw)
In-Reply-To: <20200306155707.RFT.6.Icf2213131ea652087f100129359052c83601f8b0@changeid>

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

  reply	other threads:[~2020-03-11  0:33 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-06 23:59 [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 1/9] drivers: qcom: rpmh-rsc: Clean code reading/writing regs/cmds Douglas Anderson
2020-03-11  8:47   ` Maulik Shah
2020-03-11 15:03     ` Doug Anderson
2020-03-11 16:17       ` Matthias Kaehlcke
2020-03-11 19:30         ` Stephen Boyd
2020-03-06 23:59 ` [RFT PATCH 2/9] drivers: qcom: rpmh-rsc: Document the register layout better Douglas Anderson
2020-03-11  9:35   ` Maulik Shah
2020-03-11 15:27     ` Doug Anderson
2020-03-11 18:49       ` Evan Green
2020-03-11 20:08       ` Stephen Boyd
2020-03-11 22:35         ` Doug Anderson
2020-03-06 23:59 ` [RFT PATCH 3/9] drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller Douglas Anderson
2020-03-11  9:50   ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 4/9] drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction Douglas Anderson
2020-03-11 12:02   ` Maulik Shah
2020-03-06 23:59 ` [RFT PATCH 5/9] drivers: qcom: rpmh-rsc: A lot of comments Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY Douglas Anderson
2020-03-11  0:33   ` Doug Anderson [this message]
2020-03-06 23:59 ` [RFT PATCH 7/9] drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 8/9] drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate() Douglas Anderson
2020-03-06 23:59 ` [RFT PATCH 9/9] drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire Douglas Anderson
2020-03-11  0:35   ` Doug Anderson
2020-03-11  9:48 ` [RFT PATCH 0/9] drivers: qcom: rpmh-rsc: Cleanup / add lots of comments Maulik Shah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=U0OX-HFK8jcj5HU9_f9tPq13GEZpc7er2knMYS6sdDNA@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=mkshah@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --cc=swboyd@chromium.org \
    --subject='Re: [RFT PATCH 6/9] drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).