Re: [PATCH v4 04/13] remoteproc: qcom: q6v5: Use qmp_send to update co-processor load state

Sibi Sankar
2021-07-21T17:27:33Z

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.