On Sat, Nov 21, 2020 at 9:43 PM Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> 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@xxxxxxxx>
> Reviewed-by: Rishabh Bhatnagar <rishabhb@xxxxxxxxxxxxxx>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> ---
>
> 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
