Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1611555637-7688-1-git-send-email-mkshah@codeaurora.org> <1611555637-7688-3-git-send-email-mkshah@codeaurora.org>
In-Reply-To: <1611555637-7688-3-git-send-email-mkshah@codeaurora.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Wed, 3 Feb 2021 10:36:56 -0800
Message-ID: <CAD=FV=WwufqMRRpT6Kk4=d9+S7xHLnNqH4+vOrRK2eW+y1ht-w@mail.gmail.com>
Subject: Re: [PATCH v2 3/3] soc: qcom: rpmh: Conditionally check lockdep_assert_irqs_disabled()
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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