| Message ID | 20201122054135.802935-3-bjorn.andersson@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5c212aaf5457ca5bd99aba3ad29a4a17f8129939 |
| Headers | show |
| Series | remoteproc: Improvement for the Qualcomm sysmon | expand |
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
On Mon, Dec 7, 2020 at 2:00 PM Evan Green <evgreen@chromium.org> wrote: > > 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? 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
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; } static void sysmon_unprepare(struct rproc_subdev *subdev) @@ -681,6 +709,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