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: <20201122054135.802935-1-bjorn.andersson@linaro.org> <20201122054135.802935-3-bjorn.andersson@linaro.org>
In-Reply-To: <20201122054135.802935-3-bjorn.andersson@linaro.org>
From:   Evan Green <evgreen@chromium.org>
Date:   Mon, 7 Dec 2020 11:59:43 -0800
Message-ID: <CAE=gft4P+kuAFfZEZz7nGoWEr9ZzDFtDZgtA7vCt5wFfnghuWQ@mail.gmail.com>
Subject: Re: [PATCH v3 2/4] remoteproc: sysmon: Expose the shutdown result
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Andy Gross <agross@kernel.org>, Ohad Ben-Cohen <ohad@wizery.com>,
        Siddharth Gupta <sidgup@codeaurora.org>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        linux-remoteproc@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Steev Klimaszewski <steev@kali.org>,
        Rishabh Bhatnagar <rishabhb@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Nov 21, 2020 at 9:43 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> A graceful shutdown of the Qualcomm remote processors where
> traditionally performed by invoking a shared memory state signal and
> waiting for the associated ack.
>
> This was later superseded by the "sysmon" mechanism, where some form of
> shared memory bus is used to send a "graceful shutdown request" message
> and one of more signals comes back to indicate its success.
>
> But when this newer mechanism is in effect the firmware is shut down by
> the time the older mechanism, implemented in the remoteproc drivers,
> attempts to perform a graceful shutdown - and as such it will never
> receive an ack back.
>
> This patch therefor track the success of the latest shutdown attempt in
> sysmon and exposes a new function in the API that the remoteproc driver
> can use to query the success and the necessity of invoking the older
> mechanism.
>
> Tested-by: Steev Klimaszewski <steev@kali.org>
> Reviewed-by: Rishabh Bhatnagar <rishabhb@codeaurora.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>
> Change since v2:
> - None
>
>  drivers/remoteproc/qcom_common.h |  6 +++
>  drivers/remoteproc/qcom_sysmon.c | 82 ++++++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_common.h b/drivers/remoteproc/qcom_common.h
> index dfc641c3a98b..8ba9052955bd 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -51,6 +51,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
>                                            const char *name,
>                                            int ssctl_instance);
>  void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon);
> +bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon);
>  #else
>  static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
>                                                          const char *name,
> @@ -62,6 +63,11 @@ static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
>  static inline void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon)
>  {
>  }
> +
> +static inline bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon)
> +{
> +       return false;
> +}
>  #endif
>
>  #endif
> diff --git a/drivers/remoteproc/qcom_sysmon.c b/drivers/remoteproc/qcom_sysmon.c
> index b37b111b15b3..a428b707a6de 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
> @@ -44,6 +44,7 @@ struct qcom_sysmon {
>         struct mutex lock;
>
>         bool ssr_ack;
> +       bool shutdown_acked;
>
>         struct qmi_handle qmi;
>         struct sockaddr_qrtr ssctl;
> @@ -115,10 +116,13 @@ static void sysmon_send_event(struct qcom_sysmon *sysmon,
>  /**
>   * sysmon_request_shutdown() - request graceful shutdown of remote
>   * @sysmon:    sysmon context
> + *
> + * Return: boolean indicator of the remote processor acking the request
>   */
> -static void sysmon_request_shutdown(struct qcom_sysmon *sysmon)
> +static bool sysmon_request_shutdown(struct qcom_sysmon *sysmon)
>  {
>         char *req = "ssr:shutdown";
> +       bool acked = false;
>         int ret;
>
>         mutex_lock(&sysmon->lock);
> @@ -141,9 +145,13 @@ static void sysmon_request_shutdown(struct qcom_sysmon *sysmon)
>         if (!sysmon->ssr_ack)
>                 dev_err(sysmon->dev,
>                         "unexpected response to sysmon shutdown request\n");
> +       else
> +               acked = true;
>
>  out_unlock:
>         mutex_unlock(&sysmon->lock);
> +
> +       return acked;
>  }
>
>  static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
> @@ -297,14 +305,33 @@ static struct qmi_msg_handler qmi_indication_handler[] = {
>         {}
>  };
>
> +static bool ssctl_request_shutdown_wait(struct qcom_sysmon *sysmon)
> +{
> +       int ret;
> +
> +       ret = wait_for_completion_timeout(&sysmon->shutdown_comp, 10 * HZ);
> +       if (ret)
> +               return true;
> +
> +       ret = try_wait_for_completion(&sysmon->ind_comp);
> +       if (ret)
> +               return true;
> +
> +       dev_err(sysmon->dev, "timeout waiting for shutdown ack\n");
> +       return false;
> +}
> +
>  /**
>   * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
>   * @sysmon:    sysmon context
> + *
> + * Return: boolean indicator of the remote processor acking the request
>   */
> -static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
> +static bool ssctl_request_shutdown(struct qcom_sysmon *sysmon)
>  {
>         struct ssctl_shutdown_resp resp;
>         struct qmi_txn txn;
> +       bool acked = false;
>         int ret;
>
>         reinit_completion(&sysmon->ind_comp);
> @@ -312,7 +339,7 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
>         ret = qmi_txn_init(&sysmon->qmi, &txn, ssctl_shutdown_resp_ei, &resp);
>         if (ret < 0) {
>                 dev_err(sysmon->dev, "failed to allocate QMI txn\n");
> -               return;
> +               return false;
>         }
>
>         ret = qmi_send_request(&sysmon->qmi, &sysmon->ssctl, &txn,
> @@ -320,27 +347,23 @@ static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
>         if (ret < 0) {
>                 dev_err(sysmon->dev, "failed to send shutdown request\n");
>                 qmi_txn_cancel(&txn);
> -               return;
> +               return false;
>         }
>
>         ret = qmi_txn_wait(&txn, 5 * HZ);
> -       if (ret < 0)
> +       if (ret < 0) {
>                 dev_err(sysmon->dev, "failed receiving QMI response\n");
> -       else if (resp.resp.result)
> +       } else if (resp.resp.result) {
>                 dev_err(sysmon->dev, "shutdown request failed\n");
> -       else
> +       } else {
>                 dev_dbg(sysmon->dev, "shutdown request completed\n");
> -
> -       if (sysmon->shutdown_irq > 0) {
> -               ret = wait_for_completion_timeout(&sysmon->shutdown_comp,
> -                                                 10 * HZ);
> -               if (!ret) {
> -                       ret = try_wait_for_completion(&sysmon->ind_comp);
> -                       if (!ret)
> -                               dev_err(sysmon->dev,
> -                                       "timeout waiting for shutdown ack\n");
> -               }
> +               acked = true;
>         }
> +
> +       if (sysmon->shutdown_irq > 0)
> +               return ssctl_request_shutdown_wait(sysmon);
> +
> +       return acked;
>  }
>
>  /**
> @@ -510,6 +533,9 @@ static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
>                 .subsys_name = sysmon->name,
>                 .ssr_event = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN
>         };
> +       bool acked;
> +
> +       sysmon->shutdown_acked = false;
>
>         mutex_lock(&sysmon->state_lock);
>         sysmon->state = SSCTL_SSR_EVENT_BEFORE_SHUTDOWN;
> @@ -521,9 +547,11 @@ static void sysmon_stop(struct rproc_subdev *subdev, bool crashed)
>                 return;
>
>         if (sysmon->ssctl_version)

> -               ssctl_request_shutdown(sysmon);
> +               acked = ssctl_request_shutdown(sysmon);
>         else if (sysmon->ept)
> -               sysmon_request_shutdown(sysmon);
> +               acked = sysmon_request_shutdown(sysmon);
> +
> +       sysmon->shutdown_acked = acked;

Guenter noticed that the 0-day bot complains about acked being
potentially uninitialized here. He put a fix for us into Chrome OS:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2577656

Bjorn, do you want to tweak the patch in your tree?
-Evan