diff mbox series

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

Message ID 20221202094532.2925-2-quic_aiquny@quicinc.com
State Accepted
Commit 11c7f9e3131ad14b27a957496088fa488b153a48
Headers show
Series remoteproc: core: do pm relax when in | expand

Commit Message

Aiqun Yu (Maria) Dec. 2, 2022, 9:45 a.m. UTC
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(-)

Comments

Mathieu Poirier Dec. 2, 2022, 5:30 p.m. UTC | #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.
> 
> 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
>
Bjorn Andersson Dec. 2, 2022, 6:09 p.m. UTC | #2
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
>
Aiqun Yu (Maria) Dec. 6, 2022, 12:58 a.m. UTC | #3
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
>>
Aiqun Yu (Maria) Dec. 6, 2022, 1:05 a.m. UTC | #4
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 mbox series

Patch

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);
 }