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

[Sibi Sankar]

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.