diff mbox series

[2/6] remoteproc: Add support for detach-only during shutdown

Message ID 20210522000309.26134-3-s-anna@ti.com (mailing list archive)
State New, archived
Headers show
Series K3 R5F & DSP IPC-only mode support | expand

Commit Message

Suman Anna May 22, 2021, 12:03 a.m. UTC
The remoteproc core has support for both stopping and detaching a
remote processor that was attached to previously, through both the
remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
unconditionally only uses the stop functionality at present. This
may not be the default desired functionality for all the remoteproc
platform drivers.

Introduce a new rproc state flag 'detach_on_shutdown' that individual
remoteproc drivers can set to only allow detach in rproc_shutdown()
that would have been invoked when the driver is uninstalled, so that
remote processor continues to run undisturbed even after the driver
removal.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
 drivers/remoteproc/remoteproc_core.c  | 5 ++++-
 drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
 include/linux/remoteproc.h            | 3 +++
 4 files changed, 20 insertions(+), 1 deletion(-)

Comments

Bjorn Andersson May 28, 2021, 4:11 a.m. UTC | #1
On Fri 21 May 19:03 CDT 2021, Suman Anna wrote:

> The remoteproc core has support for both stopping and detaching a
> remote processor that was attached to previously, through both the
> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> unconditionally only uses the stop functionality at present. This
> may not be the default desired functionality for all the remoteproc
> platform drivers.
> 
> Introduce a new rproc state flag 'detach_on_shutdown' that individual
> remoteproc drivers can set to only allow detach in rproc_shutdown()
> that would have been invoked when the driver is uninstalled, so that
> remote processor continues to run undisturbed even after the driver
> removal.
> 

I dislike the introduction of knobs for everything and would much rather
see that we define some sound defaults. Can we make shutdown just do
detach() if that's supported otherwise stop().

This still allows userspace to explicitly stop the detachable remoteproc
before shutdown, if for some reason that's what you want...

Regards,
Bjorn

> Signed-off-by: Suman Anna <s-anna@ti.com>
> ---
>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>  include/linux/remoteproc.h            | 3 +++
>  4 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 0b8a84c04f76..473467711a09 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> +		if (rproc->state == RPROC_ATTACHED &&
> +		    rproc->detach_on_shutdown) {
> +			dev_err(&rproc->dev,
> +				"stop not supported for this rproc, use detach\n");
> +			return -EINVAL;
> +		}
> +
>  		rproc_shutdown(rproc);
>  	} else if (!strncmp(cmd, "detach", len)) {
>  		if (rproc->state != RPROC_ATTACHED)
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 6019f46001c8..e8ab3eb41f00 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
>  	if (!atomic_dec_and_test(&rproc->power))
>  		goto out;
>  
> -	ret = rproc_stop(rproc, false);
> +	if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
> +		ret = __rproc_detach(rproc);
> +	else
> +		ret = rproc_stop(rproc, false);
>  	if (ret) {
>  		atomic_inc(&rproc->power);
>  		goto out;
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index ea8b89f97d7b..1785fbcb1075 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>  		    rproc->state != RPROC_ATTACHED)
>  			return -EINVAL;
>  
> +		if (rproc->state == RPROC_ATTACHED &&
> +		    rproc->detach_on_shutdown) {
> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> +			return -EINVAL;
> +		}
> +
>  		rproc_shutdown(rproc);
>  	} else if (sysfs_streq(buf, "detach")) {
>  		if (rproc->state != RPROC_ATTACHED)
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 42a1f30e33a7..35ef921676a1 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
>   * @elf_machine: firmware ELF machine
>   * @cdev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
> + *			attached state and _only_ support detach
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -569,6 +571,7 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool detach_on_shutdown;
>  };
>  
>  /**
> -- 
> 2.30.1
>
Suman Anna May 28, 2021, 4:40 p.m. UTC | #2
Hi Bjorn,

On 5/27/21 11:11 PM, Bjorn Andersson wrote:
> On Fri 21 May 19:03 CDT 2021, Suman Anna wrote:
> 
>> The remoteproc core has support for both stopping and detaching a
>> remote processor that was attached to previously, through both the
>> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
>> unconditionally only uses the stop functionality at present. This
>> may not be the default desired functionality for all the remoteproc
>> platform drivers.
>>
>> Introduce a new rproc state flag 'detach_on_shutdown' that individual
>> remoteproc drivers can set to only allow detach in rproc_shutdown()
>> that would have been invoked when the driver is uninstalled, so that
>> remote processor continues to run undisturbed even after the driver
>> removal.
>>
> 
> I dislike the introduction of knobs for everything and would much rather
> see that we define some sound defaults. Can we make shutdown just do
> detach() if that's supported otherwise stop().
> 

I maybe missing your point, but the change in remoteproc_core below exactly does
that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs?

The asymmetry did bug me as well, but it is already existing even before this
patch. I personally would have preferred a cleaner and symmetrical attach,
start, stop, detach, but existing code has overloaded attach into start (keys
off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from
stop. I have retained the meaning of stop as shutdown from userspace interface
perspective, but enforcing the checks for detach only remoteprocs.

The logic in rproc_shutdown is for driver paths.

> This still allows userspace to explicitly stop the detachable remoteproc
> before shutdown, if for some reason that's what you want...

This is the existing behavior and the difference between stop and detach. That
behavior is maintained for remoteprocs not setting the detach_on_shutdown flag.
I am only restricting the behavior for those that set it.

Mathieu,
Your thoughts on this?

regards
Suman



> 
> Regards,
> Bjorn
> 
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> ---
>>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
>>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
>>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>>  include/linux/remoteproc.h            | 3 +++
>>  4 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
>> index 0b8a84c04f76..473467711a09 100644
>> --- a/drivers/remoteproc/remoteproc_cdev.c
>> +++ b/drivers/remoteproc/remoteproc_cdev.c
>> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>>  		    rproc->state != RPROC_ATTACHED)
>>  			return -EINVAL;
>>  
>> +		if (rproc->state == RPROC_ATTACHED &&
>> +		    rproc->detach_on_shutdown) {
>> +			dev_err(&rproc->dev,
>> +				"stop not supported for this rproc, use detach\n");
>> +			return -EINVAL;
>> +		}
>> +
>>  		rproc_shutdown(rproc);
>>  	} else if (!strncmp(cmd, "detach", len)) {
>>  		if (rproc->state != RPROC_ATTACHED)
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index 6019f46001c8..e8ab3eb41f00 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
>>  	if (!atomic_dec_and_test(&rproc->power))
>>  		goto out;
>>  
>> -	ret = rproc_stop(rproc, false);
>> +	if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
>> +		ret = __rproc_detach(rproc);
>> +	else
>> +		ret = rproc_stop(rproc, false);
>>  	if (ret) {
>>  		atomic_inc(&rproc->power);
>>  		goto out;
>> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
>> index ea8b89f97d7b..1785fbcb1075 100644
>> --- a/drivers/remoteproc/remoteproc_sysfs.c
>> +++ b/drivers/remoteproc/remoteproc_sysfs.c
>> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
>>  		    rproc->state != RPROC_ATTACHED)
>>  			return -EINVAL;
>>  
>> +		if (rproc->state == RPROC_ATTACHED &&
>> +		    rproc->detach_on_shutdown) {
>> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
>> +			return -EINVAL;
>> +		}
>> +
>>  		rproc_shutdown(rproc);
>>  	} else if (sysfs_streq(buf, "detach")) {
>>  		if (rproc->state != RPROC_ATTACHED)
>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>> index 42a1f30e33a7..35ef921676a1 100644
>> --- a/include/linux/remoteproc.h
>> +++ b/include/linux/remoteproc.h
>> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
>>   * @elf_machine: firmware ELF machine
>>   * @cdev: character device of the rproc
>>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
>> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
>> + *			attached state and _only_ support detach
>>   */
>>  struct rproc {
>>  	struct list_head node;
>> @@ -569,6 +571,7 @@ struct rproc {
>>  	u16 elf_machine;
>>  	struct cdev cdev;
>>  	bool cdev_put_on_release;
>> +	bool detach_on_shutdown;
>>  };
>>  
>>  /**
>> -- 
>> 2.30.1
>>
Mathieu Poirier June 1, 2021, 5:15 p.m. UTC | #3
Hey guys,

On Fri, May 28, 2021 at 11:40:02AM -0500, Suman Anna wrote:
> Hi Bjorn,
> 
> On 5/27/21 11:11 PM, Bjorn Andersson wrote:
> > On Fri 21 May 19:03 CDT 2021, Suman Anna wrote:
> > 
> >> The remoteproc core has support for both stopping and detaching a
> >> remote processor that was attached to previously, through both the
> >> remoteproc sysfs and cdev interfaces. The rproc_shutdown() though
> >> unconditionally only uses the stop functionality at present. This
> >> may not be the default desired functionality for all the remoteproc
> >> platform drivers.
> >>
> >> Introduce a new rproc state flag 'detach_on_shutdown' that individual
> >> remoteproc drivers can set to only allow detach in rproc_shutdown()
> >> that would have been invoked when the driver is uninstalled, so that
> >> remote processor continues to run undisturbed even after the driver
> >> removal.
> >>
> > 
> > I dislike the introduction of knobs for everything and would much rather
> > see that we define some sound defaults. Can we make shutdown just do
> > detach() if that's supported otherwise stop().
> > 
> 
> I maybe missing your point, but the change in remoteproc_core below exactly does
> that, right? Are you saying drop the checks in remoteproc_cdev and remoteproc_sysfs?
> 
> The asymmetry did bug me as well, but it is already existing even before this
> patch. I personally would have preferred a cleaner and symmetrical attach,
> start, stop, detach, but existing code has overloaded attach into start (keys
> off by RPROC_OFFLINE/RPROC_DETACHED) while introducing a separate detach from
> stop. I have retained the meaning of stop as shutdown from userspace interface
> perspective, but enforcing the checks for detach only remoteprocs.
> 
> The logic in rproc_shutdown is for driver paths.
> 
> > This still allows userspace to explicitly stop the detachable remoteproc
> > before shutdown, if for some reason that's what you want...
> 
> This is the existing behavior and the difference between stop and detach. That
> behavior is maintained for remoteprocs not setting the detach_on_shutdown flag.
> I am only restricting the behavior for those that set it.
> 
> Mathieu,
> Your thoughts on this?

Introducing knobs in such a way makes the code very difficult to understand and
maintain.  It is also a matter of time before another knob is introduced to
modify the behavior of this knob.  

Function rproc_detach() is exported and should be used in the platform driver if
the state of the remote processor mandates it.  Function rproc_del() calls
rproc_shutdown() but the latter will return immediately because of rproc->power.
So calling rproc_detach() followed by rproc_del() will work as expected.  The
real fix is to de-couple rproc_shutdown from rproc_del() and do the right calls
in the platform drivers using them.

With regards to rproc_cdev_write(), the state of the remote processor is
advertised in sysfs.  As such it should be easy to write "stop" or "detach" to
the character interface.  If a command to stop the remote processor is not
supported in a scenario then rproc_ops::stop should reflect that.  If that is
the case then rproc_shutdown() should be modified to return an error code, the
same way rproc_detach() was done.

> 
> regards
> Suman
> 
> 
> 
> > 
> > Regards,
> > Bjorn
> > 
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> ---
> >>  drivers/remoteproc/remoteproc_cdev.c  | 7 +++++++
> >>  drivers/remoteproc/remoteproc_core.c  | 5 ++++-
> >>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
> >>  include/linux/remoteproc.h            | 3 +++
> >>  4 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> >> index 0b8a84c04f76..473467711a09 100644
> >> --- a/drivers/remoteproc/remoteproc_cdev.c
> >> +++ b/drivers/remoteproc/remoteproc_cdev.c
> >> @@ -42,6 +42,13 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
> >>  		    rproc->state != RPROC_ATTACHED)
> >>  			return -EINVAL;
> >>  
> >> +		if (rproc->state == RPROC_ATTACHED &&
> >> +		    rproc->detach_on_shutdown) {
> >> +			dev_err(&rproc->dev,
> >> +				"stop not supported for this rproc, use detach\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >>  		rproc_shutdown(rproc);
> >>  	} else if (!strncmp(cmd, "detach", len)) {
> >>  		if (rproc->state != RPROC_ATTACHED)
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index 6019f46001c8..e8ab3eb41f00 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -2074,7 +2074,10 @@ void rproc_shutdown(struct rproc *rproc)
> >>  	if (!atomic_dec_and_test(&rproc->power))
> >>  		goto out;
> >>  
> >> -	ret = rproc_stop(rproc, false);
> >> +	if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
> >> +		ret = __rproc_detach(rproc);
> >> +	else
> >> +		ret = rproc_stop(rproc, false);
> >>  	if (ret) {
> >>  		atomic_inc(&rproc->power);
> >>  		goto out;
> >> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> >> index ea8b89f97d7b..1785fbcb1075 100644
> >> --- a/drivers/remoteproc/remoteproc_sysfs.c
> >> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> >> @@ -206,6 +206,12 @@ static ssize_t state_store(struct device *dev,
> >>  		    rproc->state != RPROC_ATTACHED)
> >>  			return -EINVAL;
> >>  
> >> +		if (rproc->state == RPROC_ATTACHED &&
> >> +		    rproc->detach_on_shutdown) {
> >> +			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >>  		rproc_shutdown(rproc);
> >>  	} else if (sysfs_streq(buf, "detach")) {
> >>  		if (rproc->state != RPROC_ATTACHED)
> >> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >> index 42a1f30e33a7..35ef921676a1 100644
> >> --- a/include/linux/remoteproc.h
> >> +++ b/include/linux/remoteproc.h
> >> @@ -530,6 +530,8 @@ struct rproc_dump_segment {
> >>   * @elf_machine: firmware ELF machine
> >>   * @cdev: character device of the rproc
> >>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> >> + * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
> >> + *			attached state and _only_ support detach
> >>   */
> >>  struct rproc {
> >>  	struct list_head node;
> >> @@ -569,6 +571,7 @@ struct rproc {
> >>  	u16 elf_machine;
> >>  	struct cdev cdev;
> >>  	bool cdev_put_on_release;
> >> +	bool detach_on_shutdown;
> >>  };
> >>  
> >>  /**
> >> -- 
> >> 2.30.1
> >>
>
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 0b8a84c04f76..473467711a09 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -42,6 +42,13 @@  static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
+		if (rproc->state == RPROC_ATTACHED &&
+		    rproc->detach_on_shutdown) {
+			dev_err(&rproc->dev,
+				"stop not supported for this rproc, use detach\n");
+			return -EINVAL;
+		}
+
 		rproc_shutdown(rproc);
 	} else if (!strncmp(cmd, "detach", len)) {
 		if (rproc->state != RPROC_ATTACHED)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 6019f46001c8..e8ab3eb41f00 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2074,7 +2074,10 @@  void rproc_shutdown(struct rproc *rproc)
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
 
-	ret = rproc_stop(rproc, false);
+	if (rproc->detach_on_shutdown && rproc->state == RPROC_ATTACHED)
+		ret = __rproc_detach(rproc);
+	else
+		ret = rproc_stop(rproc, false);
 	if (ret) {
 		atomic_inc(&rproc->power);
 		goto out;
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index ea8b89f97d7b..1785fbcb1075 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -206,6 +206,12 @@  static ssize_t state_store(struct device *dev,
 		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
+		if (rproc->state == RPROC_ATTACHED &&
+		    rproc->detach_on_shutdown) {
+			dev_err(&rproc->dev, "stop not supported for this rproc, use detach\n");
+			return -EINVAL;
+		}
+
 		rproc_shutdown(rproc);
 	} else if (sysfs_streq(buf, "detach")) {
 		if (rproc->state != RPROC_ATTACHED)
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 42a1f30e33a7..35ef921676a1 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -530,6 +530,8 @@  struct rproc_dump_segment {
  * @elf_machine: firmware ELF machine
  * @cdev: character device of the rproc
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @detach_on_shutdown: flag to indicate if remoteproc cannot be shutdown in
+ *			attached state and _only_ support detach
  */
 struct rproc {
 	struct list_head node;
@@ -569,6 +571,7 @@  struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	bool detach_on_shutdown;
 };
 
 /**