| Message ID | 1662712413-38233-1-git-send-email-quic_aiquny@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v1] remoteproc: core: do pm relax when not first crash | expand |
Hi Maria, On Fri, Sep 09, 2022 at 04:33:33PM +0800, Maria Yu wrote: > Even if it is not first crash, need to relax the pm > wakelock otherwise the device will stay awake. > The goal is exactly to keep the device awake... > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > --- > drivers/remoteproc/remoteproc_core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index e5279ed9a8d7..30078043e939 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1956,6 +1956,7 @@ static void rproc_crash_handler_work(struct work_struct *work) > if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { > /* handle only the first crash detected */ > mutex_unlock(&rproc->lock); > + pm_relax(rproc->dev.parent); If we are here it means that rproc_crash_handler_work() has already been called _and_ that a recovery is in process. When the first crash handler completes pm_relax() will be called and the device will go to sleep as expected. Thanks, Mathieu > return; > } > > -- > 2.7.4 >
Hi Mathieu, pm_awake and pm_relax needed to be used as a pair. There is chance that pm_relax is not being called, and make the device always in cannot suspend state. On 9/10/2022 3:23 AM, Mathieu Poirier wrote: > Hi Maria, > > On Fri, Sep 09, 2022 at 04:33:33PM +0800, Maria Yu wrote: >> Even if it is not first crash, need to relax the pm >> wakelock otherwise the device will stay awake. >> > > The goal is exactly to keep the device awake... > >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index e5279ed9a8d7..30078043e939 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1956,6 +1956,7 @@ static void rproc_crash_handler_work(struct work_struct *work) >> if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { >> /* handle only the first crash detected */ >> mutex_unlock(&rproc->lock); >> + pm_relax(rproc->dev.parent); > > If we are here it means that rproc_crash_handler_work() has already been called > _and_ that a recovery is in process. When the first crash handler completes > pm_relax() will be called and the device will go to sleep as expected. If the rproc->state cannot be changed to running state, the device will always be awake from this return. Also APROC_OFFLINE state can be given in other path like an shutdown request is issued. While this patch is not considering carefully as well, I think I need to upload a new patchset with an ordered workqueue to make each work have each pm_relax before return. what do you think? > > Thanks, > Mathieu > >> return; >> } >> >> -- >> 2.7.4 >>
On Tue, 13 Sept 2022 at 05:03, Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote: > > Hi Mathieu, > > pm_awake and pm_relax needed to be used as a pair. There is chance that > pm_relax is not being called, and make the device always in cannot > suspend state. > > On 9/10/2022 3:23 AM, Mathieu Poirier wrote: > > Hi Maria, > > > > On Fri, Sep 09, 2022 at 04:33:33PM +0800, Maria Yu wrote: > >> Even if it is not first crash, need to relax the pm > >> wakelock otherwise the device will stay awake. > >> > > > > The goal is exactly to keep the device awake... > > > >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > >> --- > >> drivers/remoteproc/remoteproc_core.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > >> index e5279ed9a8d7..30078043e939 100644 > >> --- a/drivers/remoteproc/remoteproc_core.c > >> +++ b/drivers/remoteproc/remoteproc_core.c > >> @@ -1956,6 +1956,7 @@ static void rproc_crash_handler_work(struct work_struct *work) > >> if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { > >> /* handle only the first crash detected */ > >> mutex_unlock(&rproc->lock); > >> + pm_relax(rproc->dev.parent); > > > > If we are here it means that rproc_crash_handler_work() has already been called > > _and_ that a recovery is in process. When the first crash handler completes > > pm_relax() will be called and the device will go to sleep as expected. > If the rproc->state cannot be changed to running state, the device will > always be awake from this return. > Also APROC_OFFLINE state can be given in other path like an shutdown > request is issued. > > While this patch is not considering carefully as well, I think I need to > upload a new patchset with an ordered workqueue to make each work have > each pm_relax before return. > what do you think? I was travelling this week and as such did not have the time to follow-up with this thread, something I will do next week. > > > > > Thanks, > > Mathieu > > > >> return; > >> } > >> > >> -- > >> 2.7.4 > >> > > > -- > Thx and BRs, > Aiqun(Maria) Yu
On 9/17/2022 1:05 AM, Mathieu Poirier wrote: > On Tue, 13 Sept 2022 at 05:03, Aiqun(Maria) Yu <quic_aiquny@quicinc.com> wrote: >> >> Hi Mathieu, >> >> pm_awake and pm_relax needed to be used as a pair. There is chance that >> pm_relax is not being called, and make the device always in cannot >> suspend state. >> >> On 9/10/2022 3:23 AM, Mathieu Poirier wrote: >>> Hi Maria, >>> >>> On Fri, Sep 09, 2022 at 04:33:33PM +0800, Maria Yu wrote: >>>> Even if it is not first crash, need to relax the pm >>>> wakelock otherwise the device will stay awake. >>>> >>> >>> The goal is exactly to keep the device awake... >>> >>>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >>>> --- >>>> drivers/remoteproc/remoteproc_core.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >>>> index e5279ed9a8d7..30078043e939 100644 >>>> --- a/drivers/remoteproc/remoteproc_core.c >>>> +++ b/drivers/remoteproc/remoteproc_core.c >>>> @@ -1956,6 +1956,7 @@ static void rproc_crash_handler_work(struct work_struct *work) >>>> if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { >>>> /* handle only the first crash detected */ >>>> mutex_unlock(&rproc->lock); >>>> + pm_relax(rproc->dev.parent); >>> >>> If we are here it means that rproc_crash_handler_work() has already been called >>> _and_ that a recovery is in process. When the first crash handler completes >>> pm_relax() will be called and the device will go to sleep as expected. >> If the rproc->state cannot be changed to running state, the device will >> always be awake from this return. >> Also APROC_OFFLINE state can be given in other path like an shutdown >> request is issued. >> >> While this patch is not considering carefully as well, I think I need to >> upload a new patchset with an ordered workqueue to make each work have >> each pm_relax before return. >> what do you think? > > I was travelling this week and as such did not have the time to > follow-up with this thread, something I will do next week. > Thx for follow up. I have new patchset posted on this thread. After reconsideration, extra action can be done only for RPROC_OFFLINE state. Pls check the newest v4 patchset on this thread. >> >>> >>> Thanks, >>> Mathieu >>> >>>> return; >>>> } >>>> >>>> -- >>>> 2.7.4 >>>> >> >> >> -- >> Thx and BRs, >> Aiqun(Maria) Yu
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index e5279ed9a8d7..30078043e939 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1956,6 +1956,7 @@ static void rproc_crash_handler_work(struct work_struct *work) if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { /* handle only the first crash detected */ mutex_unlock(&rproc->lock); + pm_relax(rproc->dev.parent); return; }
Even if it is not first crash, need to relax the pm wakelock otherwise the device will stay awake. Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> --- drivers/remoteproc/remoteproc_core.c | 1 + 1 file changed, 1 insertion(+)