| Message ID | 20221202094532.2925-2-quic_aiquny@quicinc.com |
|---|---|
| State | Accepted |
| Commit | 11c7f9e3131ad14b27a957496088fa488b153a48 |
| Headers | show |
| Series | remoteproc: core: do pm relax when in | expand |
On Fri, Dec 02, 2022 at 05:45:31PM +0800, Maria Yu wrote: > RPROC_OFFLINE state indicate there is no recovery process > is in progress and no chance to do the pm_relax. > Because when recovering from crash, rproc->lock is held and > state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING, > and then unlock rproc->lock. > When the state is in RPROC_OFFLINE it means separate request > of rproc_stop was done and no need to hold the wakeup source > in crash handler to recover any more. > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > --- > drivers/remoteproc/remoteproc_core.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 8768cb64f560..c2d0af048c69 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1862,11 +1862,16 @@ static void rproc_crash_handler_work(struct work_struct *work) > > mutex_lock(&rproc->lock); > > - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { > + if (rproc->state == RPROC_CRASHED) { > /* handle only the first crash detected */ > mutex_unlock(&rproc->lock); > return; > } Please add a newline here. > + if (rproc->state == RPROC_OFFLINE) { > + /* no need to recover if remote processor is offline */ > + mutex_unlock(&rproc->lock); > + goto out; > + } > > rproc->state = RPROC_CRASHED; > dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt, > @@ -1877,6 +1882,7 @@ static void rproc_crash_handler_work(struct work_struct *work) > if (!rproc->recovery_disabled) > rproc_trigger_recovery(rproc); > > +out: > pm_relax(rproc->dev.parent); > } > > -- > 2.17.1 >
On Fri, Dec 02, 2022 at 05:45:31PM +0800, Maria Yu wrote: > RPROC_OFFLINE state indicate there is no recovery process > is in progress and no chance to do the pm_relax. > Because when recovering from crash, rproc->lock is held and > state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING, > and then unlock rproc->lock. > When the state is in RPROC_OFFLINE it means separate request > of rproc_stop was done and no need to hold the wakeup source > in crash handler to recover any more. > It's not obvious to me that you're trying to say here is "make sure that pm_relax() happens even when the remoteproc is stopped before the crash handler work is scheduled". > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> > --- > drivers/remoteproc/remoteproc_core.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 8768cb64f560..c2d0af048c69 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -1862,11 +1862,16 @@ static void rproc_crash_handler_work(struct work_struct *work) > > mutex_lock(&rproc->lock); > > - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { > + if (rproc->state == RPROC_CRASHED) { > /* handle only the first crash detected */ > mutex_unlock(&rproc->lock); > return; > } > + if (rproc->state == RPROC_OFFLINE) { > + /* no need to recover if remote processor is offline */ I don't think it's correct to say "no need", I think if the user stopped the remoteproc before the recovery was scheduled recovery would undo that stop... So perhaps something like: "Don't recover if the remote processor was stopped" Regards, Bjorn > + mutex_unlock(&rproc->lock); > + goto out; > + } > > rproc->state = RPROC_CRASHED; > dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt, > @@ -1877,6 +1882,7 @@ static void rproc_crash_handler_work(struct work_struct *work) > if (!rproc->recovery_disabled) > rproc_trigger_recovery(rproc); > > +out: > pm_relax(rproc->dev.parent); > } > > -- > 2.17.1 >
On 12/3/2022 1:30 AM, Mathieu Poirier wrote: > On Fri, Dec 02, 2022 at 05:45:31PM +0800, Maria Yu wrote: >> RPROC_OFFLINE state indicate there is no recovery process >> is in progress and no chance to do the pm_relax. >> Because when recovering from crash, rproc->lock is held and >> state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING, >> and then unlock rproc->lock. >> When the state is in RPROC_OFFLINE it means separate request >> of rproc_stop was done and no need to hold the wakeup source >> in crash handler to recover any more. >> >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 8768cb64f560..c2d0af048c69 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1862,11 +1862,16 @@ static void rproc_crash_handler_work(struct work_struct *work) >> >> mutex_lock(&rproc->lock); >> >> - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { >> + if (rproc->state == RPROC_CRASHED) { >> /* handle only the first crash detected */ >> mutex_unlock(&rproc->lock); >> return; >> } > > Please add a newline here. > Will be addressed in new patchset. Thx. >> + if (rproc->state == RPROC_OFFLINE) { >> + /* no need to recover if remote processor is offline */ >> + mutex_unlock(&rproc->lock); >> + goto out; >> + } >> >> rproc->state = RPROC_CRASHED; >> dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt, >> @@ -1877,6 +1882,7 @@ static void rproc_crash_handler_work(struct work_struct *work) >> if (!rproc->recovery_disabled) >> rproc_trigger_recovery(rproc); >> >> +out: >> pm_relax(rproc->dev.parent); >> } >> >> -- >> 2.17.1 >>
On 12/3/2022 2:09 AM, Bjorn Andersson wrote: > On Fri, Dec 02, 2022 at 05:45:31PM +0800, Maria Yu wrote: >> RPROC_OFFLINE state indicate there is no recovery process >> is in progress and no chance to do the pm_relax. >> Because when recovering from crash, rproc->lock is held and >> state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING, >> and then unlock rproc->lock. >> When the state is in RPROC_OFFLINE it means separate request >> of rproc_stop was done and no need to hold the wakeup source >> in crash handler to recover any more. >> > > It's not obvious to me that you're trying to say here is "make sure that > pm_relax() happens even when the remoteproc is stopped before the crash > handler work is scheduled". > Will be addressed in new patchset. Thx. >> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> >> --- >> drivers/remoteproc/remoteproc_core.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c >> index 8768cb64f560..c2d0af048c69 100644 >> --- a/drivers/remoteproc/remoteproc_core.c >> +++ b/drivers/remoteproc/remoteproc_core.c >> @@ -1862,11 +1862,16 @@ static void rproc_crash_handler_work(struct work_struct *work) >> >> mutex_lock(&rproc->lock); >> >> - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { >> + if (rproc->state == RPROC_CRASHED) { >> /* handle only the first crash detected */ >> mutex_unlock(&rproc->lock); >> return; >> } >> + if (rproc->state == RPROC_OFFLINE) { >> + /* no need to recover if remote processor is offline */ > > I don't think it's correct to say "no need", I think if the user stopped > the remoteproc before the recovery was scheduled recovery would undo > that stop... > > So perhaps something like: > > "Don't recover if the remote processor was stopped" > Will be addressed in new patchset. Thx. > Regards, > Bjorn > >> + mutex_unlock(&rproc->lock); >> + goto out; >> + } >> >> rproc->state = RPROC_CRASHED; >> dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt, >> @@ -1877,6 +1882,7 @@ static void rproc_crash_handler_work(struct work_struct *work) >> if (!rproc->recovery_disabled) >> rproc_trigger_recovery(rproc); >> >> +out: >> pm_relax(rproc->dev.parent); >> } >> >> -- >> 2.17.1 >>
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 8768cb64f560..c2d0af048c69 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -1862,11 +1862,16 @@ static void rproc_crash_handler_work(struct work_struct *work) mutex_lock(&rproc->lock); - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) { + if (rproc->state == RPROC_CRASHED) { /* handle only the first crash detected */ mutex_unlock(&rproc->lock); return; } + if (rproc->state == RPROC_OFFLINE) { + /* no need to recover if remote processor is offline */ + mutex_unlock(&rproc->lock); + goto out; + } rproc->state = RPROC_CRASHED; dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt, @@ -1877,6 +1882,7 @@ static void rproc_crash_handler_work(struct work_struct *work) if (!rproc->recovery_disabled) rproc_trigger_recovery(rproc); +out: pm_relax(rproc->dev.parent); }
RPROC_OFFLINE state indicate there is no recovery process is in progress and no chance to do the pm_relax. Because when recovering from crash, rproc->lock is held and state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING, and then unlock rproc->lock. When the state is in RPROC_OFFLINE it means separate request of rproc_stop was done and no need to hold the wakeup source in crash handler to recover any more. Signed-off-by: Maria Yu <quic_aiquny@quicinc.com> --- drivers/remoteproc/remoteproc_core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)