On Mon, Dec 7, 2020 at 2:00 PM Evan Green <evgreen@xxxxxxxxxxxx> wrote:
>
> 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?
No, I prefer not to force push to the tree. I did however merge and
push out Arnd's fixup to this. You can find it here:
https://git.kernel.org/pub/scm/linux/kernel/git/andersson/remoteproc.git/commit/?h=for-next&id=9d7b4a40387d0f91512a74caed6654ffa23d5ce4
Regards,
Bjorn
