* [PATCH v5 0/2] remoteproc: core: do pm relax when in
@ 2022-12-02 9:45 Maria Yu
2022-12-02 9:45 ` [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE Maria Yu
2022-12-02 9:45 ` [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler Maria Yu
0 siblings, 2 replies; 12+ messages in thread
From: Maria Yu @ 2022-12-02 9:45 UTC (permalink / raw)
To: mathieu.poirier
Cc: Maria Yu, arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
If the remote processor is offline, no need to recover anything, and
pm_relax is needed to be called.
Only the first detected crash needed to be handled, so change
to ordered workqueue to avoid unnecessary multi active work at
the same time. This will reduce the pm_relax unnecessary concurrency.
summary from some discussion points:
pm_stay_awake() is needed to stop and reverse the suspend process that
is currently underway.
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.
Changelog
===
V5
---
Use goto out instead of directly call pm_relax and return. Suggested by
Mathieu.
Add new change with allocate ordered workqueue for allow only 1 active
work.
V4
---
Add pm relax when in RPROC_OFFLINE.
previous discussion here:
[1] https://lore.kernel.org/lkml/1bcd3fe8-f68d-ea7f-c0f9-68771e3421d5@quicinc.com/
Maria Yu (2):
remoteproc: core: do pm relax when in RPROC_OFFLINE
remoteproc: core: change to ordered workqueue for crash handler
drivers/remoteproc/remoteproc_core.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE
2022-12-02 9:45 [PATCH v5 0/2] remoteproc: core: do pm relax when in Maria Yu
@ 2022-12-02 9:45 ` Maria Yu
2022-12-02 17:30 ` Mathieu Poirier
2022-12-02 18:09 ` Bjorn Andersson
2022-12-02 9:45 ` [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler Maria Yu
1 sibling, 2 replies; 12+ messages in thread
From: Maria Yu @ 2022-12-02 9:45 UTC (permalink / raw)
To: mathieu.poirier
Cc: Maria Yu, arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
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;
}
+ 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-02 9:45 [PATCH v5 0/2] remoteproc: core: do pm relax when in Maria Yu
2022-12-02 9:45 ` [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE Maria Yu
@ 2022-12-02 9:45 ` Maria Yu
2022-12-02 17:34 ` Mathieu Poirier
2022-12-02 18:16 ` Bjorn Andersson
1 sibling, 2 replies; 12+ messages in thread
From: Maria Yu @ 2022-12-02 9:45 UTC (permalink / raw)
To: mathieu.poirier
Cc: Maria Yu, arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
Only the first detected crash needed to be handled, so change
to ordered workqueue to avoid unnecessary multi active work at
the same time. This will reduce the pm_relax unnecessary concurrency.
Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
---
drivers/remoteproc/remoteproc_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c2d0af048c69..4b973eea10bb 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
static int __init remoteproc_init(void)
{
- rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
- WQ_UNBOUND | WQ_FREEZABLE, 0);
+ rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
+ WQ_FREEZABLE, 0);
if (!rproc_recovery_wq) {
pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
return -ENOMEM;
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE
2022-12-02 9:45 ` [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE Maria Yu
@ 2022-12-02 17:30 ` Mathieu Poirier
2022-12-06 0:58 ` Aiqun(Maria) Yu
2022-12-02 18:09 ` Bjorn Andersson
1 sibling, 1 reply; 12+ messages in thread
From: Mathieu Poirier @ 2022-12-02 17:30 UTC (permalink / raw)
To: Maria Yu; +Cc: arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-02 9:45 ` [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler Maria Yu
@ 2022-12-02 17:34 ` Mathieu Poirier
2022-12-06 1:28 ` Aiqun(Maria) Yu
2022-12-02 18:16 ` Bjorn Andersson
1 sibling, 1 reply; 12+ messages in thread
From: Mathieu Poirier @ 2022-12-02 17:34 UTC (permalink / raw)
To: Maria Yu; +Cc: arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote:
> Only the first detected crash needed to be handled, so change
> to ordered workqueue to avoid unnecessary multi active work at
> the same time. This will reduce the pm_relax unnecessary concurrency.
>
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c2d0af048c69..4b973eea10bb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
>
> static int __init remoteproc_init(void)
> {
> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
> - WQ_UNBOUND | WQ_FREEZABLE, 0);
> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
> + WQ_FREEZABLE, 0);
There is an indentation issue with the second line and this patch doesn't
compile:
CC drivers/remoteproc/imx_dsp_rproc.o
AR drivers/hwspinlock/built-in.a
In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15,
from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7,
from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5,
from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5,
from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15,
from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23,
from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19:
/home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’:
/home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args]
2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
| ^~~~~~~~~~~~~~~~~~~
/home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’
419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \
| ^~~
Last but not least, please use the get_maintainer.pl script to make sure the
right people are CC'ed on your patchsets.
Thanks,
Mathieu
> if (!rproc_recovery_wq) {
> pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
> return -ENOMEM;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE
2022-12-02 9:45 ` [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE Maria Yu
2022-12-02 17:30 ` Mathieu Poirier
@ 2022-12-02 18:09 ` Bjorn Andersson
2022-12-06 1:05 ` Aiqun(Maria) Yu
1 sibling, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2022-12-02 18:09 UTC (permalink / raw)
To: Maria Yu
Cc: mathieu.poirier, arnaud.pouliquen, linux-arm-msm,
linux-remoteproc, quic_clew
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-02 9:45 ` [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler Maria Yu
2022-12-02 17:34 ` Mathieu Poirier
@ 2022-12-02 18:16 ` Bjorn Andersson
2022-12-06 1:42 ` Aiqun(Maria) Yu
1 sibling, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2022-12-02 18:16 UTC (permalink / raw)
To: Maria Yu
Cc: mathieu.poirier, arnaud.pouliquen, linux-arm-msm,
linux-remoteproc, quic_clew
On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote:
> Only the first detected crash needed to be handled, so change
> to ordered workqueue to avoid unnecessary multi active work at
> the same time.
In cab8300b5621 ("remoteproc: Use unbounded workqueue for recovery
work") Mukesh specifically said that it was required that multiple
remoteproc instances should be allowed to recover concurrently.
Is this no longer the case? Or am I perhaps misunderstanding the
nuances of the different work queue models?
> This will reduce the pm_relax unnecessary concurrency.
I'm not sure I understand this sentence, unless I remove the word
"pm_relax", was it added by mistake?
If so, is the support for concurrent recovery really unnecessary?
I know we have cases where we spend time in the recovery process just
waiting for things to happen, so allowing recovery to run concurrent
between instances sounds like a good idea.
Regards,
Bjorn
>
> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c2d0af048c69..4b973eea10bb 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
>
> static int __init remoteproc_init(void)
> {
> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
> - WQ_UNBOUND | WQ_FREEZABLE, 0);
> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
> + WQ_FREEZABLE, 0);
> if (!rproc_recovery_wq) {
> pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
> return -ENOMEM;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE
2022-12-02 17:30 ` Mathieu Poirier
@ 2022-12-06 0:58 ` Aiqun(Maria) Yu
0 siblings, 0 replies; 12+ messages in thread
From: Aiqun(Maria) Yu @ 2022-12-06 0:58 UTC (permalink / raw)
To: Mathieu Poirier
Cc: arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
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
>>
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE
2022-12-02 18:09 ` Bjorn Andersson
@ 2022-12-06 1:05 ` Aiqun(Maria) Yu
0 siblings, 0 replies; 12+ messages in thread
From: Aiqun(Maria) Yu @ 2022-12-06 1:05 UTC (permalink / raw)
To: Bjorn Andersson
Cc: mathieu.poirier, arnaud.pouliquen, linux-arm-msm,
linux-remoteproc, quic_clew
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
>>
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-02 17:34 ` Mathieu Poirier
@ 2022-12-06 1:28 ` Aiqun(Maria) Yu
2022-12-07 18:16 ` Mathieu Poirier
0 siblings, 1 reply; 12+ messages in thread
From: Aiqun(Maria) Yu @ 2022-12-06 1:28 UTC (permalink / raw)
To: Mathieu Poirier
Cc: arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
On 12/3/2022 1:34 AM, Mathieu Poirier wrote:
> On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote:
>> Only the first detected crash needed to be handled, so change
>> to ordered workqueue to avoid unnecessary multi active work at
>> the same time. This will reduce the pm_relax unnecessary concurrency.
>>
>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index c2d0af048c69..4b973eea10bb 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
>>
>> static int __init remoteproc_init(void)
>> {
>> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
>> - WQ_UNBOUND | WQ_FREEZABLE, 0);
>> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
>> + WQ_FREEZABLE, 0);
>
> There is an indentation issue with the second line and this patch doesn't
> compile:
>
My Clang 14.0.7 didn't have such kind of compilation error.
what's your CC version pls? Maybe I can have a try to reproduce.
Anyway, I will double confirm if there is any difference with current
patchset with my compile tested patchset as well.
> CC drivers/remoteproc/imx_dsp_rproc.o
> AR drivers/hwspinlock/built-in.a
> In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15,
> from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7,
> from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5,
> from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5,
> from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15,
> from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23,
> from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19:
> /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’:
> /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args]
> 2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
> | ^~~~~~~~~~~~~~~~~~~
> /home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’
> 419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \
> | ^~~
>
> Last but not least, please use the get_maintainer.pl script to make sure the
> right people are CC'ed on your patchsets.get_maintainer.pl will be re-run for next patchset uploading.
Thank you for reminder!
>
> Thanks,
> Mathieu
>
>> if (!rproc_recovery_wq) {
>> pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
>> return -ENOMEM;
>> --
>> 2.17.1
>>
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-02 18:16 ` Bjorn Andersson
@ 2022-12-06 1:42 ` Aiqun(Maria) Yu
0 siblings, 0 replies; 12+ messages in thread
From: Aiqun(Maria) Yu @ 2022-12-06 1:42 UTC (permalink / raw)
To: Bjorn Andersson
Cc: mathieu.poirier, arnaud.pouliquen, linux-arm-msm,
linux-remoteproc, quic_clew
On 12/3/2022 2:16 AM, Bjorn Andersson wrote:
> On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote:
>> Only the first detected crash needed to be handled, so change
>> to ordered workqueue to avoid unnecessary multi active work at
>> the same time.
>
> In cab8300b5621 ("remoteproc: Use unbounded workqueue for recovery
> work") Mukesh specifically said that it was required that multiple
> remoteproc instances should be allowed to recover concurrently.
>
> Is this no longer the case? Or am I perhaps misunderstanding the
> nuances of the different work queue models?
>
>> This will reduce the pm_relax unnecessary concurrency.
>
> I'm not sure I understand this sentence, unless I remove the word
> "pm_relax", was it added by mistake?
>
>
> If so, is the support for concurrent recovery really unnecessary?
>
> I know we have cases where we spend time in the recovery process just
> waiting for things to happen, so allowing recovery to run concurrent
> between instances sounds like a good idea.
>
Agree with you.
Allowing recovery to run concurrent for different subsystem still seems
a good idea currently. Let's drop this change.
> Regards,
> Bjorn
>
>>
>> Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
>> ---
>> drivers/remoteproc/remoteproc_core.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index c2d0af048c69..4b973eea10bb 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
>>
>> static int __init remoteproc_init(void)
>> {
>> - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
>> - WQ_UNBOUND | WQ_FREEZABLE, 0);
>> + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
>> + WQ_FREEZABLE, 0);
>> if (!rproc_recovery_wq) {
>> pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
>> return -ENOMEM;
>> --
>> 2.17.1
>>
--
Thx and BRs,
Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler
2022-12-06 1:28 ` Aiqun(Maria) Yu
@ 2022-12-07 18:16 ` Mathieu Poirier
0 siblings, 0 replies; 12+ messages in thread
From: Mathieu Poirier @ 2022-12-07 18:16 UTC (permalink / raw)
To: Aiqun(Maria) Yu
Cc: arnaud.pouliquen, linux-arm-msm, linux-remoteproc, quic_clew
On Tue, Dec 06, 2022 at 09:28:23AM +0800, Aiqun(Maria) Yu wrote:
> On 12/3/2022 1:34 AM, Mathieu Poirier wrote:
> > On Fri, Dec 02, 2022 at 05:45:32PM +0800, Maria Yu wrote:
> > > Only the first detected crash needed to be handled, so change
> > > to ordered workqueue to avoid unnecessary multi active work at
> > > the same time. This will reduce the pm_relax unnecessary concurrency.
> > >
> > > Signed-off-by: Maria Yu <quic_aiquny@quicinc.com>
> > > ---
> > > drivers/remoteproc/remoteproc_core.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > > index c2d0af048c69..4b973eea10bb 100644
> > > --- a/drivers/remoteproc/remoteproc_core.c
> > > +++ b/drivers/remoteproc/remoteproc_core.c
> > > @@ -2728,8 +2728,8 @@ static void __exit rproc_exit_panic(void)
> > > static int __init remoteproc_init(void)
> > > {
> > > - rproc_recovery_wq = alloc_workqueue("rproc_recovery_wq",
> > > - WQ_UNBOUND | WQ_FREEZABLE, 0);
> > > + rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
> > > + WQ_FREEZABLE, 0);
> >
> > There is an indentation issue with the second line and this patch doesn't
> > compile:
> >
> My Clang 14.0.7 didn't have such kind of compilation error.
> what's your CC version pls? Maybe I can have a try to reproduce.
I was either:
arm-linux-gnueabihf-gcc 9.4.0
or
aarch64-linux-gnu-gcc 9.4.0
I can't remember if I was compiling for 32 or 64 bit.
>
> Anyway, I will double confirm if there is any difference with current
> patchset with my compile tested patchset as well.
>
> > CC drivers/remoteproc/imx_dsp_rproc.o
> > AR drivers/hwspinlock/built-in.a
> > In file included from /home/mpoirier/work/remoteproc/kernel-review/include/linux/rhashtable-types.h:15,
> > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/ipc.h:7,
> > from /home/mpoirier/work/remoteproc/kernel-review/include/uapi/linux/sem.h:5,
> > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sem.h:5,
> > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/sched.h:15,
> > from /home/mpoirier/work/remoteproc/kernel-review/include/linux/delay.h:23,
> > from /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:19:
> > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c: In function ‘remoteproc_init’:
> > /home/mpoirier/work/remoteproc/kernel-review/drivers/remoteproc/remoteproc_core.c:2738:46: warning: too many arguments for format [-Wformat-extra-args]
> > 2738 | rproc_recovery_wq = alloc_ordered_workqueue("rproc_recovery_wq",
> > | ^~~~~~~~~~~~~~~~~~~
> > /home/mpoirier/work/remoteproc/kernel-review/include/linux/workqueue.h:419:18: note: in definition of macro ‘alloc_ordered_workqueue’
> > 419 | alloc_workqueue(fmt, WQ_UNBOUND | __WQ_ORDERED | \
> > | ^~~
> >
> > Last but not least, please use the get_maintainer.pl script to make sure the
> > right people are CC'ed on your patchsets.get_maintainer.pl will be re-run for next patchset uploading.
> Thank you for reminder!
> >
> > Thanks,
> > Mathieu
> >
> > > if (!rproc_recovery_wq) {
> > > pr_err("remoteproc: creation of rproc_recovery_wq failed\n");
> > > return -ENOMEM;
> > > --
> > > 2.17.1
> > >
>
>
> --
> Thx and BRs,
> Aiqun(Maria) Yu
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-12-07 18:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-02 9:45 [PATCH v5 0/2] remoteproc: core: do pm relax when in Maria Yu
2022-12-02 9:45 ` [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE Maria Yu
2022-12-02 17:30 ` Mathieu Poirier
2022-12-06 0:58 ` Aiqun(Maria) Yu
2022-12-02 18:09 ` Bjorn Andersson
2022-12-06 1:05 ` Aiqun(Maria) Yu
2022-12-02 9:45 ` [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler Maria Yu
2022-12-02 17:34 ` Mathieu Poirier
2022-12-06 1:28 ` Aiqun(Maria) Yu
2022-12-07 18:16 ` Mathieu Poirier
2022-12-02 18:16 ` Bjorn Andersson
2022-12-06 1:42 ` Aiqun(Maria) Yu
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).