[PATCH v5 0/2] remoteproc: core: do pm relax when in

[PATCH v5 0/2] remoteproc: core: do pm relax when in

[Maria Yu]

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

[PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE

[Maria Yu]

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

Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE

[Mathieu Poirier]

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
> 

Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE

[Aiqun(Maria) Yu]

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

Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE

[Bjorn Andersson]

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
> 

Re: [PATCH v5 1/2] remoteproc: core: do pm relax when in RPROC_OFFLINE

[Aiqun(Maria) Yu]

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

[PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Maria Yu]

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

Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Mathieu Poirier]

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
> 

Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Aiqun(Maria) Yu]

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

Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Mathieu Poirier]

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

Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Bjorn Andersson]

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
> 

Re: [PATCH v5 2/2] remoteproc: core: change to ordered workqueue for crash handler

[Aiqun(Maria) Yu]

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