From: Doug Anderson <dianders@chromium.org>
To: Maulik Shah <mkshah@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
Andy Gross <agross@kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Todd Kjos <tkjos@google.com>, Lina Iyer <ilina@codeaurora.org>,
Srinivas Rao L <lsrao@codeaurora.org>
Subject: Re: [PATCH v2 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()
Date: Wed, 3 Feb 2021 10:36:56 -0800 [thread overview]
Message-ID: <CAD=FV=WwufqMRRpT6Kk4=d9+S7xHLnNqH4+vOrRK2eW+y1ht-w@mail.gmail.com> (raw)
In-Reply-To: <1611555637-7688-3-git-send-email-mkshah@codeaurora.org>
Hi,
On Sun, Jan 24, 2021 at 10:21 PM Maulik Shah <mkshah@codeaurora.org> wrote:
>
> @@ -136,6 +136,6 @@ void rpmh_rsc_invalidate(struct rsc_drv *drv);
> int rpmh_rsc_mode_solver_set(struct rsc_drv *drv, bool enable);
>
> void rpmh_tx_done(const struct tcs_request *msg, int r);
> -int rpmh_flush(struct rpmh_ctrlr *ctrlr);
> +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu);
Given that you're touching this code, why not do the cleanup you
promised Stephen you'd do in April of 2020 [1]. Specifically rename
this function to something other than rpmh_flush().
[1] https://lore.kernel.org/r/11c37c89-aa1f-7297-9ecf-4d77a20deebd@codeaurora.org/
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 1c1f5b0..a67bcd6 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -841,7 +841,8 @@ static int rpmh_rsc_cpu_pm_callback(struct notifier_block *nfb,
> * CPU.
> */
> if (spin_trylock(&drv->lock)) {
> - if (rpmh_rsc_ctrlr_is_busy(drv) || rpmh_flush(&drv->client))
> + if (rpmh_rsc_ctrlr_is_busy(drv) ||
> + rpmh_flush(&drv->client, true))
I'll channel the spirit of Bjorn and say that it's better to blow over
the 80 column limit here and avoid wrapping to a new line.
> @@ -458,22 +458,33 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
> * rpmh_flush() - Flushes the buffered sleep and wake sets to TCSes
> *
> * @ctrlr: Controller making request to flush cached data
> + * @from_last_cpu: Set if invoked from last cpu with irqs disabled
> *
> * Return:
> * * 0 - Success
> * * Error code - Otherwise
> */
> -int rpmh_flush(struct rpmh_ctrlr *ctrlr)
> +int rpmh_flush(struct rpmh_ctrlr *ctrlr, bool from_last_cpu)
> {
> struct cache_req *p;
> int ret = 0;
>
> - lockdep_assert_irqs_disabled();
> + /*
> + * rpmh_flush() can be called when we think we're running
> + * on the last CPU with irqs_disabled or when RPMH client
> + * explicitly requests to write sleep and wake data.
> + * (for e.g. when in solver mode clients can requests to flush)
> + *
> + * Conditionally check for irqs_disabled only when called
> + * from last cpu.
> + */
> +
> + if (from_last_cpu)
> + lockdep_assert_irqs_disabled();
>
> /*
> - * Currently rpmh_flush() is only called when we think we're running
> - * on the last processor. If the lock is busy it means another
> - * processor is up and it's better to abort than spin.
> + * If the lock is busy it means another transaction is on going,
> + * in such case it's better to abort than spin.
> */
> if (!spin_trylock(&ctrlr->cache_lock))
> return -EBUSY;
I think logically here you should only do the trylock if
"from_last_cpu". If you're not called "from_last_cpu" you should do a
normal spinlock.
Also: if you're not "from_last_cpu" you need to use the "irq" or
"irqsave" variants of the spinlock.
Also: if you're not "from_last_cpu" I think you somehow confirm that
we're in solver mode. The only time it's legal to call this when not
"from_last_cpu" is when we've previously set solver mode, right?
That's the thing that makes everything OK and fulfills all the
requirements talked about in rpmh-rsc.c like in the comments for
rpmh_rsc_write_ctrl_data() where we say:
* The caller must ensure that no other RPMH actions are happening and the
* controller is idle when this function is called since it runs lockless.
I know I told you in patch #1 that we shouldn't have two copies of the
"in_solver_mode" state variable and, on the surface, it seems like the
check I'm requesting would be hard to do. I _think_ the right thing
to do is actually to combine your rpmh_write_sleep_and_wake() and
rpmh_mode_solver_set() functions. One way to do this would be to just
have rpmh_write_sleep_and_wake() implicitly enter solver mode. Then
you could change rpmh_mode_solver_set() to just
rpmh_mode_solver_exit() and have it only used to exit solver mode.
-Doug
next prev parent reply other threads:[~2021-02-03 18:38 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-25 6:20 [PATCH v2 1/3] drivers: qcom: rpmh: Disallow active requests in solver mode Maulik Shah
2021-01-25 6:20 ` [PATCH v2 2/3] soc: qcom: rpmh: Add rpmh_write_sleep_and_wake() function Maulik Shah
2021-02-03 18:36 ` Doug Anderson
2021-01-25 6:20 ` [PATCH v2 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled() Maulik Shah
2021-02-03 18:36 ` Doug Anderson [this message]
2021-02-03 18:35 ` [PATCH v2 1/3] drivers: qcom: rpmh: Disallow active requests in solver mode Doug Anderson
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=WwufqMRRpT6Kk4=d9+S7xHLnNqH4+vOrRK2eW+y1ht-w@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=ilina@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lsrao@codeaurora.org \
--cc=mkshah@codeaurora.org \
--cc=tkjos@google.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).