* [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
@ 2020-02-14 8:26 Pi-Hsun Shih
2020-02-14 15:10 ` Enric Balletbo i Serra
2020-03-03 10:03 ` Enric Balletbo i Serra
0 siblings, 2 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2020-02-14 8:26 UTC (permalink / raw)
Cc: Pi-Hsun Shih, Benson Leung, Enric Balletbo i Serra,
Guenter Roeck, open list
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.
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
--
2.25.0.265.gbab2e86ba0-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-14 8:26 [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event Pi-Hsun Shih
@ 2020-02-14 15:10 ` Enric Balletbo i Serra
2020-02-15 3:56 ` Pi-Hsun Shih
2020-03-03 10:03 ` Enric Balletbo i Serra
1 sibling, 1 reply; 7+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-14 15:10 UTC (permalink / raw)
To: Pi-Hsun Shih; +Cc: Benson Leung, Guenter Roeck, open list
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
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-14 15:10 ` Enric Balletbo i Serra
@ 2020-02-15 3:56 ` Pi-Hsun Shih
2020-02-17 15:55 ` Enric Balletbo i Serra
0 siblings, 1 reply; 7+ messages in thread
From: Pi-Hsun Shih @ 2020-02-15 3:56 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: Benson Leung, Guenter Roeck, open list
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-15 3:56 ` Pi-Hsun Shih
@ 2020-02-17 15:55 ` Enric Balletbo i Serra
2020-02-28 8:52 ` Enric Balletbo i Serra
0 siblings, 1 reply; 7+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-17 15:55 UTC (permalink / raw)
To: Pi-Hsun Shih
Cc: Benson Leung, Guenter Roeck, open list, ohad, Bjorn Andersson,
Matthias Brugger, linux-remoteproc
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
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-17 15:55 ` Enric Balletbo i Serra
@ 2020-02-28 8:52 ` Enric Balletbo i Serra
2020-03-02 5:59 ` Pi-Hsun Shih
0 siblings, 1 reply; 7+ messages in thread
From: Enric Balletbo i Serra @ 2020-02-28 8:52 UTC (permalink / raw)
To: Pi-Hsun Shih
Cc: Benson Leung, Guenter Roeck, open list, ohad, Bjorn Andersson,
Matthias Brugger, linux-remoteproc
Hi Pi-Hsun,
On 17/2/20 16:55, Enric Balletbo i Serra wrote:
> 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.
>
I'll pick this patch, just I want to request a minor change.
> 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;
Could you try if just calling driver_probe_done() when needed works, so we don't
need to add a new boolean flag for this?
Thanks,
Enric
>>>> };
>>>>
>>>> /**
>>>> @@ -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
>>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-28 8:52 ` Enric Balletbo i Serra
@ 2020-03-02 5:59 ` Pi-Hsun Shih
0 siblings, 0 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2020-03-02 5:59 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: Benson Leung, Guenter Roeck, open list, Ohad Ben-Cohen,
Bjorn Andersson, Matthias Brugger,
open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM
(Resending since I forgot to use plain text mode in the previous mail,
and got blocked by mailing lists. Sorry for the duplicate email.)
Hi Enric,
On Fri, Feb 28, 2020 at 4:52 PM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Pi-Hsun,
>
> On 17/2/20 16:55, Enric Balletbo i Serra wrote:
> > 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.
> >
>
> I'll pick this patch, just I want to request a minor change.
>
> > 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;
>
>
> Could you try if just calling driver_probe_done() when needed works, so we don't
> need to add a new boolean flag for this?
Changing from "if (ec_rpmsg->probe_done)" to "if (driver_probe_done()
== 0)" works in my testing.
But since driver_probe_done() returns 0 after all driver probes are
done, not after the probe of cros_ec_rpmsg driver, I think it's
possible that we got a host event after the cros_ec_rpmsg driver probe
is done (ec_rpmsg->probe_done is true), but before all driver probe
done (driver_probe_done() is still -EBUSY). In this case the host
event would be lost since we would set the has_pending_host_event flag
but no one would be processing it.
>
> Thanks,
> Enric
>
> >>>> };
> >>>>
> >>>> /**
> >>>> @@ -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
> >>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event.
2020-02-14 8:26 [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event Pi-Hsun Shih
2020-02-14 15:10 ` Enric Balletbo i Serra
@ 2020-03-03 10:03 ` Enric Balletbo i Serra
1 sibling, 0 replies; 7+ messages in thread
From: Enric Balletbo i Serra @ 2020-03-03 10:03 UTC (permalink / raw)
To: Pi-Hsun Shih; +Cc: Benson Leung, Guenter Roeck, open list
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.
>
> 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>
> ---
Applied for 5.7
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-03 10:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-14 8:26 [PATCH] platform/chrome: cros_ec_rpmsg: Fix race with host event Pi-Hsun Shih
2020-02-14 15:10 ` Enric Balletbo i Serra
2020-02-15 3:56 ` Pi-Hsun Shih
2020-02-17 15:55 ` Enric Balletbo i Serra
2020-02-28 8:52 ` Enric Balletbo i Serra
2020-03-02 5:59 ` Pi-Hsun Shih
2020-03-03 10:03 ` Enric Balletbo i Serra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).