| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Nov 2020 17:01:34 -0800
From: rishabhb@...eaurora.org
To: Bjorn Andersson <bjorn.andersson@...aro.org>
Cc: Andy Gross <agross@...nel.org>, Ohad Ben-Cohen <ohad@...ery.com>,
Siddharth Gupta <sidgup@...eaurora.org>,
Sibi Sankar <sibis@...eaurora.org>,
linux-arm-msm@...r.kernel.org, linux-remoteproc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] remoteproc: sysmon: Expose the shutdown result
On 2020-11-04 20:50, Bjorn Andersson 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@...aro.org>
> ---
>
> Changes since v1:
> - New patch
>
> 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 38f63c968fa8..1c42f00010d3 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;
> }
>
> /**
> @@ -508,6 +531,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;
> @@ -519,9 +545,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;
> }
>
> static void sysmon_unprepare(struct rproc_subdev *subdev)
> @@ -679,6 +707,22 @@ void qcom_remove_sysmon_subdev(struct qcom_sysmon
> *sysmon)
> }
> EXPORT_SYMBOL_GPL(qcom_remove_sysmon_subdev);
>
> +/**
> + * qcom_sysmon_shutdown_acked() - query the success of the last
> shutdown
> + * @sysmon: sysmon context
> + *
> + * When sysmon is used to request a graceful shutdown of the remote
> processor
> + * this can be used by the remoteproc driver to query the success, in
> order to
> + * know if it should fall back to other means of requesting a
> shutdown.
> + *
> + * Return: boolean indicator of the success of the last shutdown
> request
> + */
> +bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon)
> +{
> + return sysmon && sysmon->shutdown_acked;
> +}
> +EXPORT_SYMBOL_GPL(qcom_sysmon_shutdown_acked);
> +
> /**
> * sysmon_probe() - probe sys_mon channel
> * @rpdev: rpmsg device handle
Reviewed-by: Rishabh Bhatnagar <rishabhb@...eaurora.org>
Powered by blists - more mailing lists