All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] remoteproc: core: check state in rproc_boot
@ 2022-05-19  6:41 ` Peng Fan (OSS)
  0 siblings, 0 replies; 6+ messages in thread
From: Peng Fan (OSS) @ 2022-05-19  6:41 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, linux-imx, linux-remoteproc,
	linux-arm-kernel, linux-kernel
  Cc: Peng Fan

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH] remoteproc: core: check state in rproc_boot
@ 2022-05-19  6:41 ` Peng Fan (OSS)
  0 siblings, 0 replies; 6+ messages in thread
From: Peng Fan (OSS) @ 2022-05-19  6:41 UTC (permalink / raw)
  To: bjorn.andersson, mathieu.poirier, linux-imx, linux-remoteproc,
	linux-arm-kernel, linux-kernel
  Cc: Peng Fan

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] remoteproc: core: check state in rproc_boot
  2022-05-19  6:41 ` Peng Fan (OSS)
@ 2022-07-17  4:07   ` Bjorn Andersson
  -1 siblings, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2022-07-17  4:07 UTC (permalink / raw)
  To: Peng Fan (OSS)
  Cc: mathieu.poirier, linux-imx, linux-remoteproc, linux-arm-kernel,
	linux-kernel, Peng Fan

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] remoteproc: core: check state in rproc_boot
@ 2022-07-17  4:07   ` Bjorn Andersson
  0 siblings, 0 replies; 6+ messages in thread
From: Bjorn Andersson @ 2022-07-17  4:07 UTC (permalink / raw)
  To: Peng Fan (OSS)
  Cc: mathieu.poirier, linux-imx, linux-remoteproc, linux-arm-kernel,
	linux-kernel, Peng Fan

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] remoteproc: core: check state in rproc_boot
  2022-07-17  4:07   ` Bjorn Andersson
@ 2022-07-20  0:48     ` Peng Fan
  -1 siblings, 0 replies; 6+ messages in thread
From: Peng Fan @ 2022-07-20  0:48 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: mathieu.poirier, linux-imx, linux-remoteproc, linux-arm-kernel,
	linux-kernel, 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
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] remoteproc: core: check state in rproc_boot
@ 2022-07-20  0:48     ` Peng Fan
  0 siblings, 0 replies; 6+ messages in thread
From: Peng Fan @ 2022-07-20  0:48 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: mathieu.poirier, linux-imx, linux-remoteproc, linux-arm-kernel,
	linux-kernel, 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
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-07-20  0:50 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-19  6:41 [PATCH] remoteproc: core: check state in rproc_boot Peng Fan (OSS)
2022-05-19  6:41 ` Peng Fan (OSS)
2022-07-17  4:07 ` Bjorn Andersson
2022-07-17  4:07   ` Bjorn Andersson
2022-07-20  0:48   ` Peng Fan
2022-07-20  0:48     ` Peng Fan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.