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

From: Stephen Boyd <swboyd@chromium.org>
To: Sibi Sankar <sibis@codeaurora.org>,
	bjorn.andersson@linaro.org, mka@chromium.org, robh+dt@kernel.org
Cc: ulf.hansson@linaro.org, rjw@rjwysocki.net, agross@kernel.org,
	ohad@wizery.com, mathieu.poirier@linaro.org,
	linux-arm-msm@vger.kernel.org, linux-remoteproc@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	dianders@chromium.org, rishabhb@codeaurora.org,
	sidgup@codeaurora.org
Subject: Re: [PATCH v4 04/13] remoteproc: qcom: q6v5: Use qmp_send to update co-processor load state
Date: Wed, 21 Jul 2021 05:26:19 +0000	[thread overview]
Message-ID: <CAE-0n518x-W8kCdtrLjw0kwsbEnLzk9OmnKara_B=et0j9+ScA@mail.gmail.com> (raw)
In-Reply-To: <1626755807-11865-5-git-send-email-sibis@codeaurora.org>

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?

> +       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()?

> +                       kfree_const(q6v5->load_state);
> +                       return PTR_ERR(q6v5->qmp);
> +               }
> +       } else {
> +               if (!q6v5->load_state) {

Use else if and deindent?

> +                       dev_err(&pdev->dev, "load state resource string empty\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(qcom_q6v5_init);
>