Messages in this thread | | | Date | Wed, 21 Jul 2021 22:57:20 +0530 | From | Sibi Sankar <> | Subject | Re: [PATCH v4 04/13] remoteproc: qcom: q6v5: Use qmp_send to update co-processor load state |
| |
On 2021-07-21 10:56, Stephen Boyd wrote: > Quoting Sibi Sankar (2021-07-19 21:36:38) >> diff --git a/drivers/remoteproc/qcom_q6v5.c >> b/drivers/remoteproc/qcom_q6v5.c >> index 7e9244c748da..997ff21271f7 100644 >> --- a/drivers/remoteproc/qcom_q6v5.c >> +++ b/drivers/remoteproc/qcom_q6v5.c >> @@ -16,8 +16,28 @@ >> #include "qcom_common.h" >> #include "qcom_q6v5.h" >> >> +#define Q6V5_LOAD_STATE_MSG_LEN 64 >> #define Q6V5_PANIC_DELAY_MS 200 >> >> +static int q6v5_load_state_toggle(struct qcom_q6v5 *q6v5, bool >> enable) >> +{ >> + char buf[Q6V5_LOAD_STATE_MSG_LEN] = {}; > > Just to confirm, we want to set the whole buffer to zero before writing > it? Sounds good to not send stack junk over to to the other side but > maybe we could skip initializing to zero for the first part of the > buffer that isn't used?
Sure, it makes sense to incorporate a warn_on and memset the remainder of the buffer after populating it.
> >> + int ret; >> + >> + if (IS_ERR(q6v5->qmp)) >> + return 0; >> + >> + snprintf(buf, sizeof(buf), >> + "{class: image, res: load_state, name: %s, val: %s}", >> + q6v5->load_state, enable ? "on" : "off"); > > Should we WARN_ON() if the message doesn't fit into the 64-bytes? > >> + >> + ret = qmp_send(q6v5->qmp, buf, sizeof(buf)); >> + if (ret) >> + dev_err(q6v5->dev, "failed to toggle load state\n"); >> + >> + return ret; >> +} >> + >> /** >> * qcom_q6v5_prepare() - reinitialize the qcom_q6v5 context before >> start >> * @q6v5: reference to qcom_q6v5 context to be reinitialized >> @@ -196,12 +223,13 @@ EXPORT_SYMBOL_GPL(qcom_q6v5_panic); >> * @pdev: platform_device reference for acquiring resources >> * @rproc: associated remoteproc instance >> * @crash_reason: SMEM id for crash reason string, or 0 if none >> + * @load_state: load state resource string >> * @handover: function to be called when proxy resources should be >> released >> * >> * Return: 0 on success, negative errno on failure >> */ >> int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct platform_device >> *pdev, >> - struct rproc *rproc, int crash_reason, >> + struct rproc *rproc, int crash_reason, const char >> *load_state, >> void (*handover)(struct qcom_q6v5 *q6v5)) >> { >> int ret; >> @@ -286,9 +314,36 @@ int qcom_q6v5_init(struct qcom_q6v5 *q6v5, struct >> platform_device *pdev, >> return PTR_ERR(q6v5->state); >> } >> >> + q6v5->load_state = kstrdup_const(load_state, GFP_KERNEL); >> + q6v5->qmp = qmp_get(&pdev->dev); >> + if (IS_ERR(q6v5->qmp)) { >> + if (PTR_ERR(q6v5->qmp) != -ENODEV) { >> + if (PTR_ERR(q6v5->qmp) != -EPROBE_DEFER) >> + dev_err(&pdev->dev, "failed to acquire >> load state\n"); > > Use dev_err_probe()?
sure I'll use it.
> >> + kfree_const(q6v5->load_state); >> + return PTR_ERR(q6v5->qmp); >> + } >> + } else { >> + if (!q6v5->load_state) { > > Use else if and deindent?
lol, my bad.
> >> + dev_err(&pdev->dev, "load state resource >> string empty\n"); >> + return -EINVAL; >> + } >> + } >> + >> return 0; >> } >> EXPORT_SYMBOL_GPL(qcom_q6v5_init); >>
-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.
| |