[PATCH] remoteproc: core: check state in rproc_boot

[PATCH] remoteproc: core: check state in rproc_boot

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

If remote processor has already been in RUNNING or ATTACHED
state, report it. Not just increment the power counter and return
success.

Without this patch, if m7 is in RUNNING state, and start it again,
nothing output to console.
If wanna to stop the m7, we need write twice 'stop'.

This patch is to improve that the 2nd start would show some useful
info.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

Not sure to keep power counter or not.

 drivers/remoteproc/remoteproc_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 02a04ab34a23..f37e0758c096 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2005,6 +2005,12 @@ int rproc_boot(struct rproc *rproc)
 		goto unlock_mutex;
 	}
 
+	if (rproc->state == RPROC_RUNNING || rproc->state == RPROC_ATTACHED) {
+		ret = -EINVAL;
+		dev_err(dev, "%s already booted\n", rproc->name);
+		goto unlock_mutex;
+	}
+
 	/* skip the boot or attach process if rproc is already powered up */
 	if (atomic_inc_return(&rproc->power) > 1) {
 		ret = 0;
-- 
2.25.1

Re: [PATCH] remoteproc: core: check state in rproc_boot

[Bjorn Andersson]

On Thu 19 May 01:41 CDT 2022, Peng Fan (OSS) wrote:

> From: Peng Fan <peng.fan@nxp.com>
> 
> If remote processor has already been in RUNNING or ATTACHED
> state, report it. Not just increment the power counter and return
> success.
> 
> Without this patch, if m7 is in RUNNING state, and start it again,
> nothing output to console.
> If wanna to stop the m7, we need write twice 'stop'.
> 
> This patch is to improve that the 2nd start would show some useful
> info.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> Not sure to keep power counter or not.
> 

I did discuss this with Mathieu, whom argued in favor of keeping the
refcount mechanism.

I can see that there could be a scenario where multiple user-space
components keep the remotproc running while they are, and if there is
any such user this ABI change would be a breakage.

That said, it's more than once that I accidentally have bumped the
refcount and then assumed that a single stop would tear down the
remoteproc...

>  drivers/remoteproc/remoteproc_core.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 02a04ab34a23..f37e0758c096 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2005,6 +2005,12 @@ int rproc_boot(struct rproc *rproc)
>  		goto unlock_mutex;
>  	}
>  
> +	if (rproc->state == RPROC_RUNNING || rproc->state == RPROC_ATTACHED) {

If we were to do this would it make sense to boot it out of anything but
RPROC_OFFLINE?

Regards,
Bjorn

> +		ret = -EINVAL;
> +		dev_err(dev, "%s already booted\n", rproc->name);
> +		goto unlock_mutex;
> +	}
> +
>  	/* skip the boot or attach process if rproc is already powered up */
>  	if (atomic_inc_return(&rproc->power) > 1) {
>  		ret = 0;
> -- 
> 2.25.1
> 

Re: [PATCH] remoteproc: core: check state in rproc_boot

[Peng Fan]

On 7/17/2022 12:07 PM, Bjorn Andersson wrote:
> On Thu 19 May 01:41 CDT 2022, Peng Fan (OSS) wrote:
> 
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> If remote processor has already been in RUNNING or ATTACHED
>> state, report it. Not just increment the power counter and return
>> success.
>>
>> Without this patch, if m7 is in RUNNING state, and start it again,
>> nothing output to console.
>> If wanna to stop the m7, we need write twice 'stop'.
>>
>> This patch is to improve that the 2nd start would show some useful
>> info.
>>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>
>> Not sure to keep power counter or not.
>>
> 
> I did discuss this with Mathieu, whom argued in favor of keeping the
> refcount mechanism.
> 
> I can see that there could be a scenario where multiple user-space
> components keep the remotproc running while they are, and if there is
> any such user this ABI change would be a breakage.
> 
> That said, it's more than once that I accidentally have bumped the
> refcount and then assumed that a single stop would tear down the
> remoteproc...
> 
>>   drivers/remoteproc/remoteproc_core.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 02a04ab34a23..f37e0758c096 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2005,6 +2005,12 @@ int rproc_boot(struct rproc *rproc)
>>   		goto unlock_mutex;
>>   	}
>>   
>> +	if (rproc->state == RPROC_RUNNING || rproc->state == RPROC_ATTACHED) {
> 
> If we were to do this would it make sense to boot it out of anything but
> RPROC_OFFLINE?

It is just a bit confused if started twice, need stop twice without any 
notice.Not a critical thing, we could leave it as is.

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
>> +		ret = -EINVAL;
>> +		dev_err(dev, "%s already booted\n", rproc->name);
>> +		goto unlock_mutex;
>> +	}
>> +
>>   	/* skip the boot or attach process if rproc is already powered up */
>>   	if (atomic_inc_return(&rproc->power) > 1) {
>>   		ret = 0;
>> -- 
>> 2.25.1
>>