Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

Dear remoteproc experts,

cc'ing you for if we can have your feedback on this change.

Thanks Pi-Hsun, for your quick answer, makes sense but I'm still feeling that I
miss something (probably because I'm not a remoteproc expert), so I added the
Remoteproc people for if they can comment this patch. We have time as we're in
rc2 only, so I'd like to wait a bit in case they can take a look.

If no answer is received I'll take a second look and apply the patch.

Thanks,
 Enric

On 15/2/20 4:56, Pi-Hsun Shih wrote:
> Hi Enric,
> 
> On Fri, Feb 14, 2020 at 11:10 PM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Pi-Hsun,
>>
>> On 14/2/20 9:26, Pi-Hsun Shih wrote:
>>> Host event can be sent by remoteproc by any time, and
>>> cros_ec_rpmsg_callback would be called after cros_ec_rpmsg_create_ept.
>>> But the cros_ec_device is initialized after that, which cause host event
>>> handler to use cros_ec_device that are not initialized properly yet.
>>>
>>
>> I don't have the hardware to test but, can't we call first cros_ec_register and
>> then cros_ec_rpmsg_create_ept?
>>
>> Start receiving driver callbacks before finishing to probe the drivers itself
>> sounds weird to me.
>>
>> Thanks,
>>  Enric
> 
> Since cros_ec_register calls cros_ec_query_all, which sends message to
> remoteproc using cros_ec_pkt_xfer_rpmsg (to query protocol version),
> the ec_rpmsg->ept need to be ready before calling cros_ec_register.
> 
>>
>>> Fix this by don't schedule host event handler before cros_ec_register
>>> returns. Instead, remember that we have a pending host event, and
>>> schedule host event handler after cros_ec_register.
>>>
>>> Fixes: 71cddb7097e2 ("platform/chrome: cros_ec_rpmsg: Fix race with host command when probe failed.")
>>> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
>>> ---
>>>  drivers/platform/chrome/cros_ec_rpmsg.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_rpmsg.c b/drivers/platform/chrome/cros_ec_rpmsg.c
>>> index dbc3f5523b83..7e8629e3db74 100644
>>> --- a/drivers/platform/chrome/cros_ec_rpmsg.c
>>> +++ b/drivers/platform/chrome/cros_ec_rpmsg.c
>>> @@ -44,6 +44,8 @@ struct cros_ec_rpmsg {
>>>       struct completion xfer_ack;
>>>       struct work_struct host_event_work;
>>>       struct rpmsg_endpoint *ept;
>>> +     bool has_pending_host_event;
>>> +     bool probe_done;
>>>  };
>>>
>>>  /**
>>> @@ -177,7 +179,14 @@ static int cros_ec_rpmsg_callback(struct rpmsg_device *rpdev, void *data,
>>>               memcpy(ec_dev->din, resp->data, len);
>>>               complete(&ec_rpmsg->xfer_ack);
>>>       } else if (resp->type == HOST_EVENT_MARK) {
>>> -             schedule_work(&ec_rpmsg->host_event_work);
>>> +             /*
>>> +              * If the host event is sent before cros_ec_register is
>>> +              * finished, queue the host event.
>>> +              */
>>> +             if (ec_rpmsg->probe_done)
>>> +                     schedule_work(&ec_rpmsg->host_event_work);
>>> +             else
>>> +                     ec_rpmsg->has_pending_host_event = true;
>>>       } else {
>>>               dev_warn(ec_dev->dev, "rpmsg received invalid type = %d",
>>>                        resp->type);
>>> @@ -240,6 +249,11 @@ static int cros_ec_rpmsg_probe(struct rpmsg_device *rpdev)
>>>               return ret;
>>>       }
>>>
>>> +     ec_rpmsg->probe_done = true;
>>> +
>>> +     if (ec_rpmsg->has_pending_host_event)
>>> +             schedule_work(&ec_rpmsg->host_event_work);
>>> +
>>>       return 0;
>>>  }
>>>
>>>
>>> base-commit: b19e8c68470385dd2c5440876591fddb02c8c402
>>>