[PATCH v3 00/15] remoteproc: Add support for detaching from rproc

[PATCH v3 00/15] remoteproc: Add support for detaching from rproc

[Mathieu Poirier]

Following the work done here [1], this set provides support for the
remoteproc core to release resources associated with a remote processor
without having to switch it off. That way a platform driver can be removed
or the application processor power cycled while the remote processor is
still operating.

Applies cleanly on rproc-next (c3c21b356505).

Thanks,
Mathieu

[1]. https://lkml.org/lkml/2020/7/14/1600

----
New for V3:
- Added RB from Arnaud where applicable.
- Reformatted comments about "detach" operation in struct rproc_ops.
- Fixed error path in rproc_shutdown().
- Fixed processing of "start" command in state_store() and rproc_cdev_write().
- Changed binding from "autonomous-on-core-reboot" to
  "autonomous-on-core-shutdown".
- Wrote a proper YAML file for the binding.

Mathieu Poirier (15):
  dt-bindings: remoteproc: Add bindind to support autonomous processors
  remoteproc: Re-check state in rproc_shutdown()
  remoteproc: Remove useless check in rproc_del()
  remoteproc: Add new RPROC_ATTACHED state
  remoteproc: Properly represent the attached state
  remoteproc: Properly deal with a kernel panic when attached
  remoteproc: Add new detach() remoteproc operation
  remoteproc: Introduce function __rproc_detach()
  remoteproc: Introduce function rproc_detach()
  remoteproc: Rename function rproc_actuate()
  remoteproc: Add return value to function rproc_shutdown()
  remoteproc: Properly deal with a stop request when attached
  remoteproc: Properly deal with a start request when attached
  remoteproc: Properly deal with detach request
  remoteproc: Refactor rproc delete and cdev release path

 .../bindings/remoteproc/remoteproc-core.yaml  |  25 +++
 drivers/remoteproc/remoteproc_cdev.c          |  27 ++-
 drivers/remoteproc/remoteproc_core.c          | 182 +++++++++++++++---
 drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
 include/linux/remoteproc.h                    |  18 +-
 5 files changed, 225 insertions(+), 47 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml

-- 
2.25.1

[PATCH v3 01/15] dt-bindings: remoteproc: Add bindind to support autonomous processors

[Mathieu Poirier]

This patch adds a binding to guide the remoteproc core on how to deal with
remote processors in two cases:

1) When an application holding a reference to a remote processor character
   device interface crashes.

2) when the platform driver for a remote processor is removed.

In both cases if "autonomous-on-core-reboot" is specified in the remote
processor DT node, the remoteproc core will detach the remote processor
rather than switching it off.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 .../bindings/remoteproc/remoteproc-core.yaml  | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
new file mode 100644
index 000000000000..3032734f42a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/remoteproc/remoteproc-core.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Binding for the remoteproc core applicable to all remote processors
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+  - Mathieu Poirier <mathieu.poirier@linaro.org>
+
+description:
+  This document defines the binding recognised by the remoteproc core that can
+  be used by any remote processor in the subsystem.
+
+properties:
+  autonomous-on-core-reboot:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Used in two situations, i.e when a user space application releases the
+      handle it has on the remote processor's character driver interface and
+      when a remote processor's platform driver is being removed.  If defined,
+      this flag instructs the remoteproc core to detach the remote processor
+      rather than turning it off.
-- 
2.25.1

Re: [PATCH v3 01/15] dt-bindings: remoteproc: Add bindind to support autonomous processors

[Rob Herring]

On Thu, 26 Nov 2020 14:06:28 -0700, Mathieu Poirier wrote:
> This patch adds a binding to guide the remoteproc core on how to deal with
> remote processors in two cases:
> 
> 1) When an application holding a reference to a remote processor character
>    device interface crashes.
> 
> 2) when the platform driver for a remote processor is removed.
> 
> In both cases if "autonomous-on-core-reboot" is specified in the remote
> processor DT node, the remoteproc core will detach the remote processor
> rather than switching it off.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  .../bindings/remoteproc/remoteproc-core.yaml  | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml: 'additionalProperties' is a required property
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml


See https://patchwork.ozlabs.org/patch/1406889

The base for the patch is generally the last rc1. Any dependencies
should be noted.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 01/15] dt-bindings: remoteproc: Add bindind to support autonomous processors

[Rob Herring]

On Thu, Nov 26, 2020 at 02:06:28PM -0700, Mathieu Poirier wrote:
> This patch adds a binding to guide the remoteproc core on how to deal with
> remote processors in two cases:
> 
> 1) When an application holding a reference to a remote processor character
>    device interface crashes.
> 
> 2) when the platform driver for a remote processor is removed.
> 
> In both cases if "autonomous-on-core-reboot" is specified in the remote
> processor DT node, the remoteproc core will detach the remote processor
> rather than switching it off.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  .../bindings/remoteproc/remoteproc-core.yaml  | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> new file mode 100644
> index 000000000000..3032734f42a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/remoteproc/remoteproc-core.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Binding for the remoteproc core applicable to all remote processors
> +
> +maintainers:
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> +
> +description:
> +  This document defines the binding recognised by the remoteproc core that can
> +  be used by any remote processor in the subsystem.
> +
> +properties:
> +  autonomous-on-core-reboot:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Used in two situations, i.e when a user space application releases the
> +      handle it has on the remote processor's character driver interface and
> +      when a remote processor's platform driver is being removed.  If defined,
> +      this flag instructs the remoteproc core to detach the remote processor
> +      rather than turning it off.

Userspace? character driver? platform driver? remoteproc core? Please 
explain this without OS specific terms.

Seems to me this would be implied by functionality the remote proc 
provides.

Rob

Re: [PATCH v3 01/15] dt-bindings: remoteproc: Add bindind to support autonomous processors

[Mathieu Poirier]

Hi Rob,

On Mon, Nov 30, 2020 at 10:33:21AM -0700, Rob Herring wrote:
> On Thu, Nov 26, 2020 at 02:06:28PM -0700, Mathieu Poirier wrote:
> > This patch adds a binding to guide the remoteproc core on how to deal with
> > remote processors in two cases:
> > 
> > 1) When an application holding a reference to a remote processor character
> >    device interface crashes.
> > 
> > 2) when the platform driver for a remote processor is removed.
> > 
> > In both cases if "autonomous-on-core-reboot" is specified in the remote
> > processor DT node, the remoteproc core will detach the remote processor
> > rather than switching it off.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  .../bindings/remoteproc/remoteproc-core.yaml  | 25 +++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > new file mode 100644
> > index 000000000000..3032734f42a3
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > @@ -0,0 +1,25 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/remoteproc/remoteproc-core.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Binding for the remoteproc core applicable to all remote processors
> > +
> > +maintainers:
> > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> > +
> > +description:
> > +  This document defines the binding recognised by the remoteproc core that can
> > +  be used by any remote processor in the subsystem.
> > +
> > +properties:
> > +  autonomous-on-core-reboot:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      Used in two situations, i.e when a user space application releases the
> > +      handle it has on the remote processor's character driver interface and
> > +      when a remote processor's platform driver is being removed.  If defined,
> > +      this flag instructs the remoteproc core to detach the remote processor
> > +      rather than turning it off.
> 
> Userspace? character driver? platform driver? remoteproc core? Please 
> explain this without OS specific terms.

The remoteproc state machine is gaining in complexity and having technical terms
in the binding's description helps understand when and how it should be used.  I
could make it more generic but that will lead to confusion and abuse.  Should I
make it "rproc,autonomous-on-core-reboot" ?

> 
> Seems to me this would be implied by functionality the remote proc 
> provides.

Exactly - this binding is used by the remoteproc core itself.  It is specified
in the remote processor DT nodes but the individual platform drivers don't do
anything with it - the core takes care of enacting the desired behavior on their
behalf.  Otherwise each platform driver would end up adding the same code, which
nobody wants to see happening.

How do you want me to proceed?

Thanks,
Mathieu

> 
> Rob

Re: [PATCH v3 01/15] dt-bindings: remoteproc: Add bindind to support autonomous processors

[Rob Herring]

On Tue, Dec 1, 2020 at 5:43 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> Hi Rob,
>
> On Mon, Nov 30, 2020 at 10:33:21AM -0700, Rob Herring wrote:
> > On Thu, Nov 26, 2020 at 02:06:28PM -0700, Mathieu Poirier wrote:
> > > This patch adds a binding to guide the remoteproc core on how to deal with
> > > remote processors in two cases:
> > >
> > > 1) When an application holding a reference to a remote processor character
> > >    device interface crashes.
> > >
> > > 2) when the platform driver for a remote processor is removed.
> > >
> > > In both cases if "autonomous-on-core-reboot" is specified in the remote
> > > processor DT node, the remoteproc core will detach the remote processor
> > > rather than switching it off.
> > >
> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > ---
> > >  .../bindings/remoteproc/remoteproc-core.yaml  | 25 +++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > > new file mode 100644
> > > index 000000000000..3032734f42a3
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > > @@ -0,0 +1,25 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/remoteproc/remoteproc-core.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Binding for the remoteproc core applicable to all remote processors
> > > +
> > > +maintainers:
> > > +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> > > +  - Mathieu Poirier <mathieu.poirier@linaro.org>
> > > +
> > > +description:
> > > +  This document defines the binding recognised by the remoteproc core that can
> > > +  be used by any remote processor in the subsystem.
> > > +
> > > +properties:
> > > +  autonomous-on-core-reboot:
> > > +    $ref: /schemas/types.yaml#/definitions/flag
> > > +    description:
> > > +      Used in two situations, i.e when a user space application releases the
> > > +      handle it has on the remote processor's character driver interface and
> > > +      when a remote processor's platform driver is being removed.  If defined,
> > > +      this flag instructs the remoteproc core to detach the remote processor
> > > +      rather than turning it off.
> >
> > Userspace? character driver? platform driver? remoteproc core? Please
> > explain this without OS specific terms.
>
> The remoteproc state machine is gaining in complexity and having technical terms
> in the binding's description helps understand when and how it should be used.  I
> could make it more generic but that will lead to confusion and abuse.

Explaining in Linux specific terms will confuse any other OS user.

>  Should I
> make it "rproc,autonomous-on-core-reboot" ?

No, 'rproc' is not a vendor.

> >
> > Seems to me this would be implied by functionality the remote proc
> > provides.
>
> Exactly - this binding is used by the remoteproc core itself.  It is specified
> in the remote processor DT nodes but the individual platform drivers don't do
> anything with it - the core takes care of enacting the desired behavior on their
> behalf.  Otherwise each platform driver would end up adding the same code, which
> nobody wants to see happening.

The platform drivers just need to set a flag to enable some behavior
that the core code checks and handles. That should be 1 to a few lines
in the drivers. It's not really any different, just a question of
where the flag lives.

Rob

[PATCH v3 02/15] remoteproc: Re-check state in rproc_shutdown()

[Mathieu Poirier]

The state of the remote processor may have changed between the
time a call to rproc_shutdown() was made and the time it is
executed.  To avoid moving forward with an operation that may
have been cancelled, recheck while holding the mutex.

Cc: <stable@vger.kernel.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 46c2937ebea9..e8b901f34c91 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1857,6 +1857,9 @@ void rproc_shutdown(struct rproc *rproc)
 		return;
 	}
 
+	if (rproc->state != RPROC_RUNNING)
+		goto out;
+
 	/* if the remote proc is still needed, bail out */
 	if (!atomic_dec_and_test(&rproc->power))
 		goto out;
-- 
2.25.1

[PATCH v3 03/15] remoteproc: Remove useless check in rproc_del()

[Mathieu Poirier]

Whether started at probe() time or thereafter from the command
line, a remote processor needs to be shutdown before the final
cleanup phases can happen.  Otherwise the system may be left in
an unpredictable state where the remote processor is expecting
the remoteproc core to be providing services when in fact it
no longer exist.

Invariably calling rproc_shutdown() is fine since it will return
immediately if the remote processor has already been switched
off.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e8b901f34c91..a2b9cd541315 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2346,10 +2346,8 @@ int rproc_del(struct rproc *rproc)
 	if (!rproc)
 		return -EINVAL;
 
-	/* if rproc is marked always-on, rproc_add() booted it */
 	/* TODO: make sure this works with rproc->power > 1 */
-	if (rproc->auto_boot)
-		rproc_shutdown(rproc);
+	rproc_shutdown(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
-- 
2.25.1

[PATCH v3 04/15] remoteproc: Add new RPROC_ATTACHED state

[Mathieu Poirier]

Add a new RPROC_ATTACHED state to take into account scenarios
where the remoteproc core needs to attach to a remote processor
that is booted by another entity.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_sysfs.c | 1 +
 include/linux/remoteproc.h            | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 1dbef895e65e..4b4aab0d4c4b 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -172,6 +172,7 @@ static const char * const rproc_state_string[] = {
 	[RPROC_RUNNING]		= "running",
 	[RPROC_CRASHED]		= "crashed",
 	[RPROC_DELETED]		= "deleted",
+	[RPROC_ATTACHED]	= "attached",
 	[RPROC_DETACHED]	= "detached",
 	[RPROC_LAST]		= "invalid",
 };
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e8ac041c64d9..71d531c64dfd 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -403,6 +403,8 @@ struct rproc_ops {
  * @RPROC_RUNNING:	device is up and running
  * @RPROC_CRASHED:	device has crashed; need to start recovery
  * @RPROC_DELETED:	device is deleted
+ * @RPROC_ATTACHED:	device has been booted by another entity and the core
+ *			has attached to it
  * @RPROC_DETACHED:	device has been booted by another entity and waiting
  *			for the core to attach to it
  * @RPROC_LAST:		just keep this one at the end
@@ -419,8 +421,9 @@ enum rproc_state {
 	RPROC_RUNNING	= 2,
 	RPROC_CRASHED	= 3,
 	RPROC_DELETED	= 4,
-	RPROC_DETACHED	= 5,
-	RPROC_LAST	= 6,
+	RPROC_ATTACHED	= 5,
+	RPROC_DETACHED	= 6,
+	RPROC_LAST	= 7,
 };
 
 /**
-- 
2.25.1

[PATCH v3 05/15] remoteproc: Properly represent the attached state

[Mathieu Poirier]

There is a need to know when a remote processor has been attached
to rather than booted by the remoteproc core.  In order to avoid
manipulating two variables, i.e rproc::autonomous and
rproc::state, get rid of the former and simply use the newly
introduced RPROC_ATTACHED state.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c  | 20 +-------------------
 drivers/remoteproc/remoteproc_sysfs.c |  5 +----
 include/linux/remoteproc.h            |  2 --
 3 files changed, 2 insertions(+), 25 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index a2b9cd541315..300785a18f03 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1444,7 +1444,7 @@ static int rproc_attach(struct rproc *rproc)
 		goto stop_rproc;
 	}
 
-	rproc->state = RPROC_RUNNING;
+	rproc->state = RPROC_ATTACHED;
 
 	dev_info(dev, "remote processor %s is now attached\n", rproc->name);
 
@@ -1659,14 +1659,6 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 
 	rproc->state = RPROC_OFFLINE;
 
-	/*
-	 * The remote processor has been stopped and is now offline, which means
-	 * that the next time it is brought back online the remoteproc core will
-	 * be responsible to load its firmware.  As such it is no longer
-	 * autonomous.
-	 */
-	rproc->autonomous = false;
-
 	dev_info(dev, "stopped remote processor %s\n", rproc->name);
 
 	return 0;
@@ -2080,16 +2072,6 @@ int rproc_add(struct rproc *rproc)
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Remind ourselves the remote processor has been attached to rather
-	 * than booted by the remoteproc core.  This is important because the
-	 * RPROC_DETACHED state will be lost as soon as the remote processor
-	 * has been attached to.  Used in firmware_show() and reset in
-	 * rproc_stop().
-	 */
-	if (rproc->state == RPROC_DETACHED)
-		rproc->autonomous = true;
-
 	/* if rproc is marked always-on, request it to boot */
 	if (rproc->auto_boot) {
 		ret = rproc_trigger_auto_boot(rproc);
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 4b4aab0d4c4b..f9694def9b54 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -138,11 +138,8 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
 	 * If the remote processor has been started by an external
 	 * entity we have no idea of what image it is running.  As such
 	 * simply display a generic string rather then rproc->firmware.
-	 *
-	 * Here we rely on the autonomous flag because a remote processor
-	 * may have been attached to and currently in a running state.
 	 */
-	if (rproc->autonomous)
+	if (rproc->state == RPROC_ATTACHED)
 		firmware = "unknown";
 
 	return sprintf(buf, "%s\n", firmware);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 71d531c64dfd..9be112b5c09d 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -510,7 +510,6 @@ struct rproc_dump_segment {
  * @table_sz: size of @cached_table
  * @has_iommu: flag to indicate if remote processor is behind an MMU
  * @auto_boot: flag to indicate if remote processor should be auto-started
- * @autonomous: true if an external entity has booted the remote processor
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @char_dev: character device of the rproc
@@ -547,7 +546,6 @@ struct rproc {
 	size_t table_sz;
 	bool has_iommu;
 	bool auto_boot;
-	bool autonomous;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;
-- 
2.25.1

[PATCH v3 06/15] remoteproc: Properly deal with a kernel panic when attached

[Mathieu Poirier]

The panic handler operation of registered remote processors
should also be called when remote processors have been
attached to.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 300785a18f03..539667948bc8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2471,7 +2471,11 @@ static int rproc_panic_handler(struct notifier_block *nb, unsigned long event,
 
 	rcu_read_lock();
 	list_for_each_entry_rcu(rproc, &rproc_list, node) {
-		if (!rproc->ops->panic || rproc->state != RPROC_RUNNING)
+		if (!rproc->ops->panic)
+			continue;
+
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			continue;
 
 		d = rproc->ops->panic(rproc);
-- 
2.25.1

[PATCH v3 07/15] remoteproc: Add new detach() remoteproc operation

[Mathieu Poirier]

Add an new detach() operation in order to support scenarios where
the remoteproc core is going away but the remote processor is
kept operating.  This could be the case when the system is
rebooted or when the platform driver is removed.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 include/linux/remoteproc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 9be112b5c09d..da15b77583d3 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -361,6 +361,7 @@ enum rsc_handling_status {
  * @start:	power on the device and boot it
  * @stop:	power off the device
  * @attach:	attach to a device that his already powered up
+ * @detach:	detach from a device, leaving it powered up
  * @kick:	kick a virtqueue (virtqueue id given as a parameter)
  * @da_to_va:	optional platform hook to perform address translations
  * @parse_fw:	parse firmware to extract information (e.g. resource table)
@@ -382,6 +383,7 @@ struct rproc_ops {
 	int (*start)(struct rproc *rproc);
 	int (*stop)(struct rproc *rproc);
 	int (*attach)(struct rproc *rproc);
+	int (*detach)(struct rproc *rproc);
 	void (*kick)(struct rproc *rproc, int vqid);
 	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len);
 	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
-- 
2.25.1

Re: [PATCH v3 07/15] remoteproc: Add new detach() remoteproc operation

[Arnaud POULIQUEN]

Hi Mathieu

On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> Add an new detach() operation in order to support scenarios where
> the remoteproc core is going away but the remote processor is
> kept operating.  This could be the case when the system is
> rebooted or when the platform driver is removed.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud
> ---
>  include/linux/remoteproc.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 9be112b5c09d..da15b77583d3 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -361,6 +361,7 @@ enum rsc_handling_status {
>   * @start:	power on the device and boot it
>   * @stop:	power off the device
>   * @attach:	attach to a device that his already powered up
> + * @detach:	detach from a device, leaving it powered up
>   * @kick:	kick a virtqueue (virtqueue id given as a parameter)
>   * @da_to_va:	optional platform hook to perform address translations
>   * @parse_fw:	parse firmware to extract information (e.g. resource table)
> @@ -382,6 +383,7 @@ struct rproc_ops {
>  	int (*start)(struct rproc *rproc);
>  	int (*stop)(struct rproc *rproc);
>  	int (*attach)(struct rproc *rproc);
> +	int (*detach)(struct rproc *rproc);
>  	void (*kick)(struct rproc *rproc, int vqid);
>  	void * (*da_to_va)(struct rproc *rproc, u64 da, size_t len);
>  	int (*parse_fw)(struct rproc *rproc, const struct firmware *fw);
> 

[PATCH v3 08/15] remoteproc: Introduce function __rproc_detach()

[Mathieu Poirier]

Introduce function __rproc_detach() to perform the same kind of
operation as rproc_stop(), but instead of switching off the
remote processor using rproc->ops->stop(), it uses
rproc->ops->detach().  That way it is possible for the core
to release the resources associated with a remote processor while
the latter is kept operating.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 31 ++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 539667948bc8..928b3f975798 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1664,6 +1664,37 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 	return 0;
 }
 
+/*
+ * __rproc_detach(): Does the opposite of rproc_attach()
+ */
+static int __maybe_unused __rproc_detach(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	/* No need to continue if a detach() operation has not been provided */
+	if (!rproc->ops->detach)
+		return -EINVAL;
+
+	/* Stop any subdevices for the remote processor */
+	rproc_stop_subdevices(rproc, false);
+
+	/* Tell the remote processor the core isn't available anymore */
+	ret = rproc->ops->detach(rproc);
+	if (ret) {
+		dev_err(dev, "can't detach from rproc: %d\n", ret);
+		rproc_start_subdevices(rproc);
+		return ret;
+	}
+
+	rproc_unprepare_subdevices(rproc);
+
+	rproc->state = RPROC_DETACHED;
+
+	dev_info(dev, "detached remote processor %s\n", rproc->name);
+
+	return 0;
+}
 
 /**
  * rproc_trigger_recovery() - recover a remoteproc
-- 
2.25.1

[PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

Introduce function rproc_detach() to enable the remoteproc
core to release the resources associated with a remote processor
without stopping its operation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
 include/linux/remoteproc.h           |  1 +
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 928b3f975798..f5adf05762e9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 /*
  * __rproc_detach(): Does the opposite of rproc_attach()
  */
-static int __maybe_unused __rproc_detach(struct rproc *rproc)
+static int __rproc_detach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
+/**
+ * rproc_detach() - Detach the remote processor from the
+ * remoteproc core
+ *
+ * @rproc: the remote processor
+ *
+ * Detach a remote processor (previously attached to with rproc_actuate()).
+ *
+ * In case @rproc is still being used by an additional user(s), then
+ * this function will just decrement the power refcount and exit,
+ * without disconnecting the device.
+ *
+ * Function rproc_detach() calls __rproc_detach() in order to let a remote
+ * processor know that services provided by the application processor are
+ * no longer available.  From there it should be possible to remove the
+ * platform driver and even power cycle the application processor (if the HW
+ * supports it) without needing to switch off the remote processor.
+ */
+int rproc_detach(struct rproc *rproc)
+{
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	ret = mutex_lock_interruptible(&rproc->lock);
+	if (ret) {
+		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
+		return ret;
+	}
+
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
+		ret = -EPERM;
+		goto out;
+	}
+
+	/* if the remote proc is still needed, bail out */
+	if (!atomic_dec_and_test(&rproc->power)) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	ret = __rproc_detach(rproc);
+	if (ret) {
+		atomic_inc(&rproc->power);
+		goto out;
+	}
+
+	/* clean up all acquired resources */
+	rproc_resource_cleanup(rproc);
+
+	rproc_disable_iommu(rproc);
+
+	/*
+	 * Set the remote processor's table pointer to NULL.  Since mapping
+	 * of the resource table to a virtual address is done in the platform
+	 * driver, unmapping should also be done there.
+	 */
+	rproc->table_ptr = NULL;
+out:
+	mutex_unlock(&rproc->lock);
+	return ret;
+}
+EXPORT_SYMBOL(rproc_detach);
+
 /**
  * rproc_get_by_phandle() - find a remote processor by phandle
  * @phandle: phandle to the rproc
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index da15b77583d3..329c1c071dcf 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
 
 int rproc_boot(struct rproc *rproc);
 void rproc_shutdown(struct rproc *rproc);
+int rproc_detach(struct rproc *rproc);
 int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
 int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
-- 
2.25.1

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Arnaud POULIQUEN]

Hi Mathieu,


On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> Introduce function rproc_detach() to enable the remoteproc
> core to release the resources associated with a remote processor
> without stopping its operation.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  1 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 928b3f975798..f5adf05762e9 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>  /*
>   * __rproc_detach(): Does the opposite of rproc_attach()
>   */
> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> +static int __rproc_detach(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> +/**
> + * rproc_detach() - Detach the remote processor from the
> + * remoteproc core
> + *
> + * @rproc: the remote processor
> + *
> + * Detach a remote processor (previously attached to with rproc_actuate()).
> + *
> + * In case @rproc is still being used by an additional user(s), then
> + * this function will just decrement the power refcount and exit,
> + * without disconnecting the device.
> + *
> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> + * processor know that services provided by the application processor are
> + * no longer available.  From there it should be possible to remove the
> + * platform driver and even power cycle the application processor (if the HW
> + * supports it) without needing to switch off the remote processor.
> + */
> +int rproc_detach(struct rproc *rproc)
> +{
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&rproc->lock);
> +	if (ret) {
> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> +		return ret;
> +	}
> +
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	/* if the remote proc is still needed, bail out */
> +	if (!atomic_dec_and_test(&rproc->power)) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	ret = __rproc_detach(rproc);
> +	if (ret) {
> +		atomic_inc(&rproc->power);
> +		goto out;
> +	}
> +
> +	/* clean up all acquired resources */
> +	rproc_resource_cleanup(rproc);

I started to test the series, I found 2 problems testing in STM32P1 board.

1) the resource_table pointer is unmapped if the firmware has been booted by the
Linux, generating a crash in rproc_free_vring.
I attached a fix at the end of the mail.

2) After the detach, the rproc state is "detached"
but it is no longer possible to re-attach to it correctly.
Neither if the firmware is standalone, nor if it has been booted
by the Linux.

I did not investigate, but the issue is probably linked to the resource
table address which is set to NULL.

So we either have to fix the problem in order to attach or forbid the transition.


Regards,
Arnaud

> +
> +	rproc_disable_iommu(rproc);
> +
> +	/*
> +	 * Set the remote processor's table pointer to NULL.  Since mapping
> +	 * of the resource table to a virtual address is done in the platform
> +	 * driver, unmapping should also be done there.
> +	 */
> +	rproc->table_ptr = NULL;
> +out:
> +	mutex_unlock(&rproc->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(rproc_detach);
> +
>  /**
>   * rproc_get_by_phandle() - find a remote processor by phandle
>   * @phandle: phandle to the rproc
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index da15b77583d3..329c1c071dcf 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>  
>  int rproc_boot(struct rproc *rproc);
>  void rproc_shutdown(struct rproc *rproc);
> +int rproc_detach(struct rproc *rproc);
>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> 

From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
Date: Tue, 8 Dec 2020 18:54:51 +0100
Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr

If the firmware has been loaded and started by the kernel, the
resource table has probably been mapped by the carveout allocation
(see rproc_elf_find_loaded_rsc_table).
In this case the memory can have been unmapped before the vrings are free.
The result is a crash that occurs in rproc_free_vring while try to use the
unmapped pointer.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
---
 drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index 2b0a52fb3398..3508ffba4a2a 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
 		goto out;
 	}

+	/*
+	 * Prevent case that the installed resource table is no longer
+	 * accessible (e.g. memory unmapped), use the cache if available
+	 */
+	if (rproc->cached_table)
+		rproc->table_ptr = rproc->cached_table;
+
 	ret = __rproc_detach(rproc);
 	if (ret) {
 		atomic_inc(&rproc->power);
@@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)

 	rproc_disable_iommu(rproc);

+	/* Free the chached table memory that can has been allocated*/
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
 	/*
-	 * Set the remote processor's table pointer to NULL.  Since mapping
-	 * of the resource table to a virtual address is done in the platform
-	 * driver, unmapping should also be done there.
+	 * Set the remote processor's table pointer to NULL. If mapping
+	 * of the resource table to a virtual address has been done in the
+	 * platform driver(attachment to an existing firmware),
+	 * unmapping should also be done there.
 	 */
 	rproc->table_ptr = NULL;
 out:
-- 
2.17.1

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

On Tue, Dec 08, 2020 at 07:35:18PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> 
> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> > Introduce function rproc_detach() to enable the remoteproc
> > core to release the resources associated with a remote processor
> > without stopping its operation.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h           |  1 +
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 928b3f975798..f5adf05762e9 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >  /*
> >   * __rproc_detach(): Does the opposite of rproc_attach()
> >   */
> > -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> > +static int __rproc_detach(struct rproc *rproc)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> > @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
> >  }
> >  EXPORT_SYMBOL(rproc_shutdown);
> >  
> > +/**
> > + * rproc_detach() - Detach the remote processor from the
> > + * remoteproc core
> > + *
> > + * @rproc: the remote processor
> > + *
> > + * Detach a remote processor (previously attached to with rproc_actuate()).
> > + *
> > + * In case @rproc is still being used by an additional user(s), then
> > + * this function will just decrement the power refcount and exit,
> > + * without disconnecting the device.
> > + *
> > + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> > + * processor know that services provided by the application processor are
> > + * no longer available.  From there it should be possible to remove the
> > + * platform driver and even power cycle the application processor (if the HW
> > + * supports it) without needing to switch off the remote processor.
> > + */
> > +int rproc_detach(struct rproc *rproc)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	ret = mutex_lock_interruptible(&rproc->lock);
> > +	if (ret) {
> > +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> > +		return ret;
> > +	}
> > +
> > +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> > +		ret = -EPERM;
> > +		goto out;
> > +	}
> > +
> > +	/* if the remote proc is still needed, bail out */
> > +	if (!atomic_dec_and_test(&rproc->power)) {
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	ret = __rproc_detach(rproc);
> > +	if (ret) {
> > +		atomic_inc(&rproc->power);
> > +		goto out;
> > +	}
> > +
> > +	/* clean up all acquired resources */
> > +	rproc_resource_cleanup(rproc);
> 
> I started to test the series, I found 2 problems testing in STM32P1 board.
> 
> 1) the resource_table pointer is unmapped if the firmware has been booted by the
> Linux, generating a crash in rproc_free_vring.
> I attached a fix at the end of the mail.
> 
> 2) After the detach, the rproc state is "detached"
> but it is no longer possible to re-attach to it correctly.
> Neither if the firmware is standalone, nor if it has been booted
> by the Linux.

Thanks for the report - I thought both problems had been fixed...

> 
> I did not investigate, but the issue is probably linked to the resource
> table address which is set to NULL.
> 
> So we either have to fix the problem in order to attach or forbid the transition.
> 

Perfect timing on your side as I was contemplating sending another revision.
Let me look at things and I will get back to you.

> 
> Regards,
> Arnaud
> 
> > +
> > +	rproc_disable_iommu(rproc);
> > +
> > +	/*
> > +	 * Set the remote processor's table pointer to NULL.  Since mapping
> > +	 * of the resource table to a virtual address is done in the platform
> > +	 * driver, unmapping should also be done there.
> > +	 */
> > +	rproc->table_ptr = NULL;
> > +out:
> > +	mutex_unlock(&rproc->lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(rproc_detach);
> > +
> >  /**
> >   * rproc_get_by_phandle() - find a remote processor by phandle
> >   * @phandle: phandle to the rproc
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index da15b77583d3..329c1c071dcf 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> >  
> >  int rproc_boot(struct rproc *rproc);
> >  void rproc_shutdown(struct rproc *rproc);
> > +int rproc_detach(struct rproc *rproc);
> >  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
> >  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> >  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> > 
> 
> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> Date: Tue, 8 Dec 2020 18:54:51 +0100
> Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr
> 
> If the firmware has been loaded and started by the kernel, the
> resource table has probably been mapped by the carveout allocation
> (see rproc_elf_find_loaded_rsc_table).
> In this case the memory can have been unmapped before the vrings are free.
> The result is a crash that occurs in rproc_free_vring while try to use the
> unmapped pointer.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 2b0a52fb3398..3508ffba4a2a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
>  		goto out;
>  	}
> 
> +	/*
> +	 * Prevent case that the installed resource table is no longer
> +	 * accessible (e.g. memory unmapped), use the cache if available
> +	 */
> +	if (rproc->cached_table)
> +		rproc->table_ptr = rproc->cached_table;
> +
>  	ret = __rproc_detach(rproc);
>  	if (ret) {
>  		atomic_inc(&rproc->power);
> @@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)
> 
>  	rproc_disable_iommu(rproc);
> 
> +	/* Free the chached table memory that can has been allocated*/
> +	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
>  	/*
> -	 * Set the remote processor's table pointer to NULL.  Since mapping
> -	 * of the resource table to a virtual address is done in the platform
> -	 * driver, unmapping should also be done there.
> +	 * Set the remote processor's table pointer to NULL. If mapping
> +	 * of the resource table to a virtual address has been done in the
> +	 * platform driver(attachment to an existing firmware),
> +	 * unmapping should also be done there.
>  	 */
>  	rproc->table_ptr = NULL;
>  out:
> -- 
> 2.17.1
> 
> 
> 

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

On Tue, Dec 08, 2020 at 07:35:18PM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> 
> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> > Introduce function rproc_detach() to enable the remoteproc
> > core to release the resources associated with a remote processor
> > without stopping its operation.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h           |  1 +
> >  2 files changed, 65 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 928b3f975798..f5adf05762e9 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >  /*
> >   * __rproc_detach(): Does the opposite of rproc_attach()
> >   */
> > -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> > +static int __rproc_detach(struct rproc *rproc)
> >  {
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> > @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
> >  }
> >  EXPORT_SYMBOL(rproc_shutdown);
> >  
> > +/**
> > + * rproc_detach() - Detach the remote processor from the
> > + * remoteproc core
> > + *
> > + * @rproc: the remote processor
> > + *
> > + * Detach a remote processor (previously attached to with rproc_actuate()).
> > + *
> > + * In case @rproc is still being used by an additional user(s), then
> > + * this function will just decrement the power refcount and exit,
> > + * without disconnecting the device.
> > + *
> > + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> > + * processor know that services provided by the application processor are
> > + * no longer available.  From there it should be possible to remove the
> > + * platform driver and even power cycle the application processor (if the HW
> > + * supports it) without needing to switch off the remote processor.
> > + */
> > +int rproc_detach(struct rproc *rproc)
> > +{
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	ret = mutex_lock_interruptible(&rproc->lock);
> > +	if (ret) {
> > +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> > +		return ret;
> > +	}
> > +
> > +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> > +		ret = -EPERM;
> > +		goto out;
> > +	}
> > +
> > +	/* if the remote proc is still needed, bail out */
> > +	if (!atomic_dec_and_test(&rproc->power)) {
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	ret = __rproc_detach(rproc);
> > +	if (ret) {
> > +		atomic_inc(&rproc->power);
> > +		goto out;
> > +	}
> > +
> > +	/* clean up all acquired resources */
> > +	rproc_resource_cleanup(rproc);
> 
> I started to test the series, I found 2 problems testing in STM32P1 board.
> 
> 1) the resource_table pointer is unmapped if the firmware has been booted by the
> Linux, generating a crash in rproc_free_vring.
> I attached a fix at the end of the mail.
> 

I have reproduced the condition on my side and confirm that your solution is
correct.  See below for a minor comment. 

> 2) After the detach, the rproc state is "detached"
> but it is no longer possible to re-attach to it correctly.
> Neither if the firmware is standalone, nor if it has been booted
> by the Linux.
> 

Did you update your FW image?  If so, I need to run the same one.

> I did not investigate, but the issue is probably linked to the resource
> table address which is set to NULL.
> 
> So we either have to fix the problem in order to attach or forbid the transition.
> 
> 
> Regards,
> Arnaud
> 
> > +
> > +	rproc_disable_iommu(rproc);
> > +
> > +	/*
> > +	 * Set the remote processor's table pointer to NULL.  Since mapping
> > +	 * of the resource table to a virtual address is done in the platform
> > +	 * driver, unmapping should also be done there.
> > +	 */
> > +	rproc->table_ptr = NULL;
> > +out:
> > +	mutex_unlock(&rproc->lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(rproc_detach);
> > +
> >  /**
> >   * rproc_get_by_phandle() - find a remote processor by phandle
> >   * @phandle: phandle to the rproc
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index da15b77583d3..329c1c071dcf 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> >  
> >  int rproc_boot(struct rproc *rproc);
> >  void rproc_shutdown(struct rproc *rproc);
> > +int rproc_detach(struct rproc *rproc);
> >  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
> >  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> >  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> > 
> 
> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> Date: Tue, 8 Dec 2020 18:54:51 +0100
> Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr
> 
> If the firmware has been loaded and started by the kernel, the
> resource table has probably been mapped by the carveout allocation
> (see rproc_elf_find_loaded_rsc_table).
> In this case the memory can have been unmapped before the vrings are free.
> The result is a crash that occurs in rproc_free_vring while try to use the
> unmapped pointer.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index 2b0a52fb3398..3508ffba4a2a 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
>  		goto out;
>  	}
> 
> +	/*
> +	 * Prevent case that the installed resource table is no longer
> +	 * accessible (e.g. memory unmapped), use the cache if available
> +	 */
> +	if (rproc->cached_table)
> +		rproc->table_ptr = rproc->cached_table;

I don't think there is an explicit need to check ->cached_table.  If the remote
processor has been started by the remoteproc core it is valid anyway.  And below
kfree() is called invariably. 

So that problem is fixed.  Let me know about your FW image and we'll pick it up
from there.

Mathieu

> +
>  	ret = __rproc_detach(rproc);
>  	if (ret) {
>  		atomic_inc(&rproc->power);
> @@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)
> 
>  	rproc_disable_iommu(rproc);
> 
> +	/* Free the chached table memory that can has been allocated*/
> +	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
>  	/*
> -	 * Set the remote processor's table pointer to NULL.  Since mapping
> -	 * of the resource table to a virtual address is done in the platform
> -	 * driver, unmapping should also be done there.
> +	 * Set the remote processor's table pointer to NULL. If mapping
> +	 * of the resource table to a virtual address has been done in the
> +	 * platform driver(attachment to an existing firmware),
> +	 * unmapping should also be done there.
>  	 */
>  	rproc->table_ptr = NULL;
>  out:
> -- 
> 2.17.1
> 
> 
> 

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Arnaud POULIQUEN]

On 12/9/20 1:53 AM, Mathieu Poirier wrote:
> On Tue, Dec 08, 2020 at 07:35:18PM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu,
>>
>>
>> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
>>> Introduce function rproc_detach() to enable the remoteproc
>>> core to release the resources associated with a remote processor
>>> without stopping its operation.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
>>>  include/linux/remoteproc.h           |  1 +
>>>  2 files changed, 65 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>> index 928b3f975798..f5adf05762e9 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>>>  /*
>>>   * __rproc_detach(): Does the opposite of rproc_attach()
>>>   */
>>> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
>>> +static int __rproc_detach(struct rproc *rproc)
>>>  {
>>>  	struct device *dev = &rproc->dev;
>>>  	int ret;
>>> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
>>>  }
>>>  EXPORT_SYMBOL(rproc_shutdown);
>>>  
>>> +/**
>>> + * rproc_detach() - Detach the remote processor from the
>>> + * remoteproc core
>>> + *
>>> + * @rproc: the remote processor
>>> + *
>>> + * Detach a remote processor (previously attached to with rproc_actuate()).
>>> + *
>>> + * In case @rproc is still being used by an additional user(s), then
>>> + * this function will just decrement the power refcount and exit,
>>> + * without disconnecting the device.
>>> + *
>>> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
>>> + * processor know that services provided by the application processor are
>>> + * no longer available.  From there it should be possible to remove the
>>> + * platform driver and even power cycle the application processor (if the HW
>>> + * supports it) without needing to switch off the remote processor.
>>> + */
>>> +int rproc_detach(struct rproc *rproc)
>>> +{
>>> +	struct device *dev = &rproc->dev;
>>> +	int ret;
>>> +
>>> +	ret = mutex_lock_interruptible(&rproc->lock);
>>> +	if (ret) {
>>> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
>>> +		ret = -EPERM;
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* if the remote proc is still needed, bail out */
>>> +	if (!atomic_dec_and_test(&rproc->power)) {
>>> +		ret = -EBUSY;
>>> +		goto out;
>>> +	}
>>> +
>>> +	ret = __rproc_detach(rproc);
>>> +	if (ret) {
>>> +		atomic_inc(&rproc->power);
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* clean up all acquired resources */
>>> +	rproc_resource_cleanup(rproc);
>>
>> I started to test the series, I found 2 problems testing in STM32P1 board.
>>
>> 1) the resource_table pointer is unmapped if the firmware has been booted by the
>> Linux, generating a crash in rproc_free_vring.
>> I attached a fix at the end of the mail.
>>
> 
> I have reproduced the condition on my side and confirm that your solution is
> correct.  See below for a minor comment. 
> 
>> 2) After the detach, the rproc state is "detached"
>> but it is no longer possible to re-attach to it correctly.
>> Neither if the firmware is standalone, nor if it has been booted
>> by the Linux.
>>
> 
> Did you update your FW image?  If so, I need to run the same one.
> 
>> I did not investigate, but the issue is probably linked to the resource
>> table address which is set to NULL.
>>
>> So we either have to fix the problem in order to attach or forbid the transition.
>>
>>
>> Regards,
>> Arnaud
>>
>>> +
>>> +	rproc_disable_iommu(rproc);
>>> +
>>> +	/*
>>> +	 * Set the remote processor's table pointer to NULL.  Since mapping
>>> +	 * of the resource table to a virtual address is done in the platform
>>> +	 * driver, unmapping should also be done there.
>>> +	 */
>>> +	rproc->table_ptr = NULL;
>>> +out:
>>> +	mutex_unlock(&rproc->lock);
>>> +	return ret;
>>> +}
>>> +EXPORT_SYMBOL(rproc_detach);
>>> +
>>>  /**
>>>   * rproc_get_by_phandle() - find a remote processor by phandle
>>>   * @phandle: phandle to the rproc
>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>> index da15b77583d3..329c1c071dcf 100644
>>> --- a/include/linux/remoteproc.h
>>> +++ b/include/linux/remoteproc.h
>>> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>>>  
>>>  int rproc_boot(struct rproc *rproc);
>>>  void rproc_shutdown(struct rproc *rproc);
>>> +int rproc_detach(struct rproc *rproc);
>>>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>>>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>>>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
>>>
>>
>> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
>> Date: Tue, 8 Dec 2020 18:54:51 +0100
>> Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr
>>
>> If the firmware has been loaded and started by the kernel, the
>> resource table has probably been mapped by the carveout allocation
>> (see rproc_elf_find_loaded_rsc_table).
>> In this case the memory can have been unmapped before the vrings are free.
>> The result is a crash that occurs in rproc_free_vring while try to use the
>> unmapped pointer.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index 2b0a52fb3398..3508ffba4a2a 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
>>  		goto out;
>>  	}
>>
>> +	/*
>> +	 * Prevent case that the installed resource table is no longer
>> +	 * accessible (e.g. memory unmapped), use the cache if available
>> +	 */
>> +	if (rproc->cached_table)
>> +		rproc->table_ptr = rproc->cached_table;
> 
> I don't think there is an explicit need to check ->cached_table.  If the remote
> processor has been started by the remoteproc core it is valid anyway.  And below
> kfree() is called invariably. 

The condition is needed, the  rproc->cached_table is null if the firmware as
been preloaded and the Linux remote proc just attaches to it.
The cached is used only when Linux loads the firmware, as the resource table is
extracted from the elf file to parse resource before the load of the firmware.

> 
> So that problem is fixed.  Let me know about your FW image and we'll pick it up
> from there.

I use the following example available on the stm32mp1 image:
/usr/local/Cube-M4-examples/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo_wakeup/lib/firmware/
This exemple use the RPMsg and also blink a LED when while running.

Don't hesitate if you need me to send it to you by mail.

Thank,
Arnaud

> 
> Mathieu
> 
>> +
>>  	ret = __rproc_detach(rproc);
>>  	if (ret) {
>>  		atomic_inc(&rproc->power);
>> @@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)
>>
>>  	rproc_disable_iommu(rproc);
>>
>> +	/* Free the chached table memory that can has been allocated*/
>> +	kfree(rproc->cached_table);
>> +	rproc->cached_table = NULL;
>>  	/*
>> -	 * Set the remote processor's table pointer to NULL.  Since mapping
>> -	 * of the resource table to a virtual address is done in the platform
>> -	 * driver, unmapping should also be done there.
>> +	 * Set the remote processor's table pointer to NULL. If mapping
>> +	 * of the resource table to a virtual address has been done in the
>> +	 * platform driver(attachment to an existing firmware),
>> +	 * unmapping should also be done there.
>>  	 */
>>  	rproc->table_ptr = NULL;
>>  out:
>> -- 
>> 2.17.1
>>
>>
>>

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

On Wed, Dec 09, 2020 at 09:45:32AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 12/9/20 1:53 AM, Mathieu Poirier wrote:
> > On Tue, Dec 08, 2020 at 07:35:18PM +0100, Arnaud POULIQUEN wrote:
> >> Hi Mathieu,
> >>
> >>
> >> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> >>> Introduce function rproc_detach() to enable the remoteproc
> >>> core to release the resources associated with a remote processor
> >>> without stopping its operation.
> >>>
> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
> >>>  include/linux/remoteproc.h           |  1 +
> >>>  2 files changed, 65 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >>> index 928b3f975798..f5adf05762e9 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >>>  /*
> >>>   * __rproc_detach(): Does the opposite of rproc_attach()
> >>>   */
> >>> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
> >>> +static int __rproc_detach(struct rproc *rproc)
> >>>  {
> >>>  	struct device *dev = &rproc->dev;
> >>>  	int ret;
> >>> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
> >>>  }
> >>>  EXPORT_SYMBOL(rproc_shutdown);
> >>>  
> >>> +/**
> >>> + * rproc_detach() - Detach the remote processor from the
> >>> + * remoteproc core
> >>> + *
> >>> + * @rproc: the remote processor
> >>> + *
> >>> + * Detach a remote processor (previously attached to with rproc_actuate()).
> >>> + *
> >>> + * In case @rproc is still being used by an additional user(s), then
> >>> + * this function will just decrement the power refcount and exit,
> >>> + * without disconnecting the device.
> >>> + *
> >>> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
> >>> + * processor know that services provided by the application processor are
> >>> + * no longer available.  From there it should be possible to remove the
> >>> + * platform driver and even power cycle the application processor (if the HW
> >>> + * supports it) without needing to switch off the remote processor.
> >>> + */
> >>> +int rproc_detach(struct rproc *rproc)
> >>> +{
> >>> +	struct device *dev = &rproc->dev;
> >>> +	int ret;
> >>> +
> >>> +	ret = mutex_lock_interruptible(&rproc->lock);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
> >>> +		ret = -EPERM;
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	/* if the remote proc is still needed, bail out */
> >>> +	if (!atomic_dec_and_test(&rproc->power)) {
> >>> +		ret = -EBUSY;
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	ret = __rproc_detach(rproc);
> >>> +	if (ret) {
> >>> +		atomic_inc(&rproc->power);
> >>> +		goto out;
> >>> +	}
> >>> +
> >>> +	/* clean up all acquired resources */
> >>> +	rproc_resource_cleanup(rproc);
> >>
> >> I started to test the series, I found 2 problems testing in STM32P1 board.
> >>
> >> 1) the resource_table pointer is unmapped if the firmware has been booted by the
> >> Linux, generating a crash in rproc_free_vring.
> >> I attached a fix at the end of the mail.
> >>
> > 
> > I have reproduced the condition on my side and confirm that your solution is
> > correct.  See below for a minor comment. 
> > 
> >> 2) After the detach, the rproc state is "detached"
> >> but it is no longer possible to re-attach to it correctly.
> >> Neither if the firmware is standalone, nor if it has been booted
> >> by the Linux.
> >>
> > 
> > Did you update your FW image?  If so, I need to run the same one.
> > 
> >> I did not investigate, but the issue is probably linked to the resource
> >> table address which is set to NULL.
> >>
> >> So we either have to fix the problem in order to attach or forbid the transition.
> >>
> >>
> >> Regards,
> >> Arnaud
> >>
> >>> +
> >>> +	rproc_disable_iommu(rproc);
> >>> +
> >>> +	/*
> >>> +	 * Set the remote processor's table pointer to NULL.  Since mapping
> >>> +	 * of the resource table to a virtual address is done in the platform
> >>> +	 * driver, unmapping should also be done there.
> >>> +	 */
> >>> +	rproc->table_ptr = NULL;
> >>> +out:
> >>> +	mutex_unlock(&rproc->lock);
> >>> +	return ret;
> >>> +}
> >>> +EXPORT_SYMBOL(rproc_detach);
> >>> +
> >>>  /**
> >>>   * rproc_get_by_phandle() - find a remote processor by phandle
> >>>   * @phandle: phandle to the rproc
> >>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> >>> index da15b77583d3..329c1c071dcf 100644
> >>> --- a/include/linux/remoteproc.h
> >>> +++ b/include/linux/remoteproc.h
> >>> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
> >>>  
> >>>  int rproc_boot(struct rproc *rproc);
> >>>  void rproc_shutdown(struct rproc *rproc);
> >>> +int rproc_detach(struct rproc *rproc);
> >>>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
> >>>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> >>>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
> >>>
> >>
> >> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> >> Date: Tue, 8 Dec 2020 18:54:51 +0100
> >> Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr
> >>
> >> If the firmware has been loaded and started by the kernel, the
> >> resource table has probably been mapped by the carveout allocation
> >> (see rproc_elf_find_loaded_rsc_table).
> >> In this case the memory can have been unmapped before the vrings are free.
> >> The result is a crash that occurs in rproc_free_vring while try to use the
> >> unmapped pointer.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index 2b0a52fb3398..3508ffba4a2a 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
> >>  		goto out;
> >>  	}
> >>
> >> +	/*
> >> +	 * Prevent case that the installed resource table is no longer
> >> +	 * accessible (e.g. memory unmapped), use the cache if available
> >> +	 */
> >> +	if (rproc->cached_table)
> >> +		rproc->table_ptr = rproc->cached_table;
> > 
> > I don't think there is an explicit need to check ->cached_table.  If the remote
> > processor has been started by the remoteproc core it is valid anyway.  And below
> > kfree() is called invariably. 
> 
> The condition is needed, the  rproc->cached_table is null if the firmware as
> been preloaded and the Linux remote proc just attaches to it.
> The cached is used only when Linux loads the firmware, as the resource table is
> extracted from the elf file to parse resource before the load of the firmware.

I have taken another look at this and you are correct. The if() condition is
needed because ->table_ptr is set only once when the platform driver is
probed.  See further down...

> 
> > 
> > So that problem is fixed.  Let me know about your FW image and we'll pick it up
> > from there.
> 
> I use the following example available on the stm32mp1 image:
> /usr/local/Cube-M4-examples/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo_wakeup/lib/firmware/
> This exemple use the RPMsg and also blink a LED when while running.
> 
> Don't hesitate if you need me to send it to you by mail.
> 
> Thank,
> Arnaud
> 
> > 
> > Mathieu
> > 
> >> +
> >>  	ret = __rproc_detach(rproc);
> >>  	if (ret) {
> >>  		atomic_inc(&rproc->power);
> >> @@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)
> >>
> >>  	rproc_disable_iommu(rproc);
> >>
> >> +	/* Free the chached table memory that can has been allocated*/
> >> +	kfree(rproc->cached_table);
> >> +	rproc->cached_table = NULL;
> >>  	/*
> >> -	 * Set the remote processor's table pointer to NULL.  Since mapping
> >> -	 * of the resource table to a virtual address is done in the platform
> >> -	 * driver, unmapping should also be done there.
> >> +	 * Set the remote processor's table pointer to NULL. If mapping
> >> +	 * of the resource table to a virtual address has been done in the
> >> +	 * platform driver(attachment to an existing firmware),
> >> +	 * unmapping should also be done there.
> >>  	 */
> >>  	rproc->table_ptr = NULL;

With the above in mind we can't to that, otherwise trying to re-attach with
rproc_attach() won't work because ->table_ptr will be NULL.

I wasn't able to test that code path because I didn't have the FW that supported
detaching.  Now that the feature is maturing it needs to be done.  

> >>  out:
> >> -- 
> >> 2.17.1
> >>
> >>
> >>

Re: [PATCH v3 09/15] remoteproc: Introduce function rproc_detach()

[Arnaud POULIQUEN]

On 12/9/20 10:18 PM, Mathieu Poirier wrote:
> On Wed, Dec 09, 2020 at 09:45:32AM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 12/9/20 1:53 AM, Mathieu Poirier wrote:
>>> On Tue, Dec 08, 2020 at 07:35:18PM +0100, Arnaud POULIQUEN wrote:
>>>> Hi Mathieu,
>>>>
>>>>
>>>> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
>>>>> Introduce function rproc_detach() to enable the remoteproc
>>>>> core to release the resources associated with a remote processor
>>>>> without stopping its operation.
>>>>>
>>>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>>>>> ---
>>>>>  drivers/remoteproc/remoteproc_core.c | 65 +++++++++++++++++++++++++++-
>>>>>  include/linux/remoteproc.h           |  1 +
>>>>>  2 files changed, 65 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>> index 928b3f975798..f5adf05762e9 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -1667,7 +1667,7 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
>>>>>  /*
>>>>>   * __rproc_detach(): Does the opposite of rproc_attach()
>>>>>   */
>>>>> -static int __maybe_unused __rproc_detach(struct rproc *rproc)
>>>>> +static int __rproc_detach(struct rproc *rproc)
>>>>>  {
>>>>>  	struct device *dev = &rproc->dev;
>>>>>  	int ret;
>>>>> @@ -1910,6 +1910,69 @@ void rproc_shutdown(struct rproc *rproc)
>>>>>  }
>>>>>  EXPORT_SYMBOL(rproc_shutdown);
>>>>>  
>>>>> +/**
>>>>> + * rproc_detach() - Detach the remote processor from the
>>>>> + * remoteproc core
>>>>> + *
>>>>> + * @rproc: the remote processor
>>>>> + *
>>>>> + * Detach a remote processor (previously attached to with rproc_actuate()).
>>>>> + *
>>>>> + * In case @rproc is still being used by an additional user(s), then
>>>>> + * this function will just decrement the power refcount and exit,
>>>>> + * without disconnecting the device.
>>>>> + *
>>>>> + * Function rproc_detach() calls __rproc_detach() in order to let a remote
>>>>> + * processor know that services provided by the application processor are
>>>>> + * no longer available.  From there it should be possible to remove the
>>>>> + * platform driver and even power cycle the application processor (if the HW
>>>>> + * supports it) without needing to switch off the remote processor.
>>>>> + */
>>>>> +int rproc_detach(struct rproc *rproc)
>>>>> +{
>>>>> +	struct device *dev = &rproc->dev;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = mutex_lock_interruptible(&rproc->lock);
>>>>> +	if (ret) {
>>>>> +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
>>>>> +		ret = -EPERM;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	/* if the remote proc is still needed, bail out */
>>>>> +	if (!atomic_dec_and_test(&rproc->power)) {
>>>>> +		ret = -EBUSY;
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	ret = __rproc_detach(rproc);
>>>>> +	if (ret) {
>>>>> +		atomic_inc(&rproc->power);
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	/* clean up all acquired resources */
>>>>> +	rproc_resource_cleanup(rproc);
>>>>
>>>> I started to test the series, I found 2 problems testing in STM32P1 board.
>>>>
>>>> 1) the resource_table pointer is unmapped if the firmware has been booted by the
>>>> Linux, generating a crash in rproc_free_vring.
>>>> I attached a fix at the end of the mail.
>>>>
>>>
>>> I have reproduced the condition on my side and confirm that your solution is
>>> correct.  See below for a minor comment. 
>>>
>>>> 2) After the detach, the rproc state is "detached"
>>>> but it is no longer possible to re-attach to it correctly.
>>>> Neither if the firmware is standalone, nor if it has been booted
>>>> by the Linux.
>>>>
>>>
>>> Did you update your FW image?  If so, I need to run the same one.
>>>
>>>> I did not investigate, but the issue is probably linked to the resource
>>>> table address which is set to NULL.
>>>>
>>>> So we either have to fix the problem in order to attach or forbid the transition.
>>>>
>>>>
>>>> Regards,
>>>> Arnaud
>>>>
>>>>> +
>>>>> +	rproc_disable_iommu(rproc);
>>>>> +
>>>>> +	/*
>>>>> +	 * Set the remote processor's table pointer to NULL.  Since mapping
>>>>> +	 * of the resource table to a virtual address is done in the platform
>>>>> +	 * driver, unmapping should also be done there.
>>>>> +	 */
>>>>> +	rproc->table_ptr = NULL;
>>>>> +out:
>>>>> +	mutex_unlock(&rproc->lock);
>>>>> +	return ret;
>>>>> +}
>>>>> +EXPORT_SYMBOL(rproc_detach);
>>>>> +
>>>>>  /**
>>>>>   * rproc_get_by_phandle() - find a remote processor by phandle
>>>>>   * @phandle: phandle to the rproc
>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>> index da15b77583d3..329c1c071dcf 100644
>>>>> --- a/include/linux/remoteproc.h
>>>>> +++ b/include/linux/remoteproc.h
>>>>> @@ -656,6 +656,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>>>>>  
>>>>>  int rproc_boot(struct rproc *rproc);
>>>>>  void rproc_shutdown(struct rproc *rproc);
>>>>> +int rproc_detach(struct rproc *rproc);
>>>>>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>>>>>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
>>>>>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
>>>>>
>>>>
>>>> From: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
>>>> Date: Tue, 8 Dec 2020 18:54:51 +0100
>>>> Subject: [PATCH] remoteproc: core: fix detach for unmapped table_ptr
>>>>
>>>> If the firmware has been loaded and started by the kernel, the
>>>> resource table has probably been mapped by the carveout allocation
>>>> (see rproc_elf_find_loaded_rsc_table).
>>>> In this case the memory can have been unmapped before the vrings are free.
>>>> The result is a crash that occurs in rproc_free_vring while try to use the
>>>> unmapped pointer.
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
>>>> ---
>>>>  drivers/remoteproc/remoteproc_core.c | 17 ++++++++++++++---
>>>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>> b/drivers/remoteproc/remoteproc_core.c
>>>> index 2b0a52fb3398..3508ffba4a2a 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1964,6 +1964,13 @@ int rproc_detach(struct rproc *rproc)
>>>>  		goto out;
>>>>  	}
>>>>
>>>> +	/*
>>>> +	 * Prevent case that the installed resource table is no longer
>>>> +	 * accessible (e.g. memory unmapped), use the cache if available
>>>> +	 */
>>>> +	if (rproc->cached_table)
>>>> +		rproc->table_ptr = rproc->cached_table;
>>>
>>> I don't think there is an explicit need to check ->cached_table.  If the remote
>>> processor has been started by the remoteproc core it is valid anyway.  And below
>>> kfree() is called invariably. 
>>
>> The condition is needed, the  rproc->cached_table is null if the firmware as
>> been preloaded and the Linux remote proc just attaches to it.
>> The cached is used only when Linux loads the firmware, as the resource table is
>> extracted from the elf file to parse resource before the load of the firmware.
> 
> I have taken another look at this and you are correct. The if() condition is
> needed because ->table_ptr is set only once when the platform driver is
> probed.  See further down...
> 
>>
>>>
>>> So that problem is fixed.  Let me know about your FW image and we'll pick it up
>>> from there.
>>
>> I use the following example available on the stm32mp1 image:
>> /usr/local/Cube-M4-examples/STM32MP157C-DK2/Applications/OpenAMP/OpenAMP_TTY_echo_wakeup/lib/firmware/
>> This exemple use the RPMsg and also blink a LED when while running.
>>
>> Don't hesitate if you need me to send it to you by mail.
>>
>> Thank,
>> Arnaud
>>
>>>
>>> Mathieu
>>>
>>>> +
>>>>  	ret = __rproc_detach(rproc);
>>>>  	if (ret) {
>>>>  		atomic_inc(&rproc->power);
>>>> @@ -1975,10 +1982,14 @@ int rproc_detach(struct rproc *rproc)
>>>>
>>>>  	rproc_disable_iommu(rproc);
>>>>
>>>> +	/* Free the chached table memory that can has been allocated*/
>>>> +	kfree(rproc->cached_table);
>>>> +	rproc->cached_table = NULL;
>>>>  	/*
>>>> -	 * Set the remote processor's table pointer to NULL.  Since mapping
>>>> -	 * of the resource table to a virtual address is done in the platform
>>>> -	 * driver, unmapping should also be done there.
>>>> +	 * Set the remote processor's table pointer to NULL. If mapping
>>>> +	 * of the resource table to a virtual address has been done in the
>>>> +	 * platform driver(attachment to an existing firmware),
>>>> +	 * unmapping should also be done there.
>>>>  	 */
>>>>  	rproc->table_ptr = NULL;
> 
> With the above in mind we can't to that, otherwise trying to re-attach with
> rproc_attach() won't work because ->table_ptr will be NULL.

Yes, or an alternative would be to call find_loaded_rsc_table on attach. I did
not test it but could make sense to call the ops instead of expecting that the
platform has set table_ptr.

> 
> I wasn't able to test that code path because I didn't have the FW that supported
> detaching.  Now that the feature is maturing it needs to be done.  
> 
>>>>  out:
>>>> -- 
>>>> 2.17.1
>>>>
>>>>
>>>>

[PATCH v3 10/15] remoteproc: Rename function rproc_actuate()

[Mathieu Poirier]

Align what was done for rproc_detach() by renaming function
rproc_actuate().  That way it is easier to figure out the
opposite of each functions.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f5adf05762e9..b54f60cc3cbd 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1416,7 +1416,7 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
 	return ret;
 }
 
-static int rproc_attach(struct rproc *rproc)
+static int __rproc_attach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1541,7 +1541,7 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
  * Attach to remote processor - similar to rproc_fw_boot() but without
  * the steps that deal with the firmware image.
  */
-static int rproc_actuate(struct rproc *rproc)
+static int rproc_attach(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1581,7 +1581,7 @@ static int rproc_actuate(struct rproc *rproc)
 		goto clean_up_resources;
 	}
 
-	ret = rproc_attach(rproc);
+	ret = __rproc_attach(rproc);
 	if (ret)
 		goto clean_up_resources;
 
@@ -1825,7 +1825,7 @@ int rproc_boot(struct rproc *rproc)
 	if (rproc->state == RPROC_DETACHED) {
 		dev_info(dev, "attaching to %s\n", rproc->name);
 
-		ret = rproc_actuate(rproc);
+		ret = rproc_attach(rproc);
 	} else {
 		dev_info(dev, "powering up %s\n", rproc->name);
 
@@ -1916,7 +1916,7 @@ EXPORT_SYMBOL(rproc_shutdown);
  *
  * @rproc: the remote processor
  *
- * Detach a remote processor (previously attached to with rproc_actuate()).
+ * Detach a remote processor (previously attached to with rproc_attach()).
  *
  * In case @rproc is still being used by an additional user(s), then
  * this function will just decrement the power refcount and exit,
-- 
2.25.1

[PATCH v3 11/15] remoteproc: Add return value to function rproc_shutdown()

[Mathieu Poirier]

Add a return value to function rproc_shutdown() in order to
properly deal with error conditions that may occur.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 19 ++++++++++++++-----
 include/linux/remoteproc.h           |  2 +-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index b54f60cc3cbd..51275107eb1f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1869,7 +1869,7 @@ EXPORT_SYMBOL(rproc_boot);
  *   returns, and users can still use it with a subsequent rproc_boot(), if
  *   needed.
  */
-void rproc_shutdown(struct rproc *rproc)
+int rproc_shutdown(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
 	int ret;
@@ -1877,15 +1877,19 @@ void rproc_shutdown(struct rproc *rproc)
 	ret = mutex_lock_interruptible(&rproc->lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
-		return;
+		return ret;
 	}
 
-	if (rproc->state != RPROC_RUNNING)
+	if (rproc->state != RPROC_RUNNING) {
+		ret = -EPERM;
 		goto out;
+	}
 
 	/* if the remote proc is still needed, bail out */
-	if (!atomic_dec_and_test(&rproc->power))
+	if (!atomic_dec_and_test(&rproc->power)) {
+		ret = -EBUSY;
 		goto out;
+	}
 
 	ret = rproc_stop(rproc, false);
 	if (ret) {
@@ -1897,7 +1901,11 @@ void rproc_shutdown(struct rproc *rproc)
 	rproc_resource_cleanup(rproc);
 
 	/* release HW resources if needed */
-	rproc_unprepare_device(rproc);
+	ret = rproc_unprepare_device(rproc);
+	if (ret) {
+		atomic_inc(&rproc->power);
+		goto out;
+	}
 
 	rproc_disable_iommu(rproc);
 
@@ -1907,6 +1915,7 @@ void rproc_shutdown(struct rproc *rproc)
 	rproc->table_ptr = NULL;
 out:
 	mutex_unlock(&rproc->lock);
+	return ret;
 }
 EXPORT_SYMBOL(rproc_shutdown);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 329c1c071dcf..02312096d59f 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -655,7 +655,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
 			     u32 da, const char *name, ...);
 
 int rproc_boot(struct rproc *rproc);
-void rproc_shutdown(struct rproc *rproc);
+int rproc_shutdown(struct rproc *rproc);
 int rproc_detach(struct rproc *rproc);
 int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
-- 
2.25.1

Re: [PATCH v3 11/15] remoteproc: Add return value to function rproc_shutdown()

[Arnaud POULIQUEN]

On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> Add a return value to function rproc_shutdown() in order to
> properly deal with error conditions that may occur.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud
> ---
>  drivers/remoteproc/remoteproc_core.c | 19 ++++++++++++++-----
>  include/linux/remoteproc.h           |  2 +-
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index b54f60cc3cbd..51275107eb1f 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1869,7 +1869,7 @@ EXPORT_SYMBOL(rproc_boot);
>   *   returns, and users can still use it with a subsequent rproc_boot(), if
>   *   needed.
>   */
> -void rproc_shutdown(struct rproc *rproc)
> +int rproc_shutdown(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
>  	int ret;
> @@ -1877,15 +1877,19 @@ void rproc_shutdown(struct rproc *rproc)
>  	ret = mutex_lock_interruptible(&rproc->lock);
>  	if (ret) {
>  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
> -		return;
> +		return ret;
>  	}
>  
> -	if (rproc->state != RPROC_RUNNING)
> +	if (rproc->state != RPROC_RUNNING) {
> +		ret = -EPERM;
>  		goto out;
> +	}
>  
>  	/* if the remote proc is still needed, bail out */
> -	if (!atomic_dec_and_test(&rproc->power))
> +	if (!atomic_dec_and_test(&rproc->power)) {
> +		ret = -EBUSY;
>  		goto out;
> +	}
>  
>  	ret = rproc_stop(rproc, false);
>  	if (ret) {
> @@ -1897,7 +1901,11 @@ void rproc_shutdown(struct rproc *rproc)
>  	rproc_resource_cleanup(rproc);
>  
>  	/* release HW resources if needed */
> -	rproc_unprepare_device(rproc);
> +	ret = rproc_unprepare_device(rproc);
> +	if (ret) {
> +		atomic_inc(&rproc->power);
> +		goto out;
> +	}
>  
>  	rproc_disable_iommu(rproc);
>  
> @@ -1907,6 +1915,7 @@ void rproc_shutdown(struct rproc *rproc)
>  	rproc->table_ptr = NULL;
>  out:
>  	mutex_unlock(&rproc->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_shutdown);
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 329c1c071dcf..02312096d59f 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -655,7 +655,7 @@ rproc_of_resm_mem_entry_init(struct device *dev, u32 of_resm_idx, size_t len,
>  			     u32 da, const char *name, ...);
>  
>  int rproc_boot(struct rproc *rproc);
> -void rproc_shutdown(struct rproc *rproc);
> +int rproc_shutdown(struct rproc *rproc);
>  int rproc_detach(struct rproc *rproc);
>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> 

[PATCH v3 12/15] remoteproc: Properly deal with a stop request when attached

[Mathieu Poirier]

This patch introduces the capability to stop a remote processor
that has been attached to by the remoteproc core.  For that to
happen a rproc::ops::stop() operation need to be available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/remoteproc/remoteproc_cdev.c  | 5 +++--
 drivers/remoteproc/remoteproc_core.c  | 6 +++++-
 drivers/remoteproc/remoteproc_sysfs.c | 5 +++--
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index b19ea3057bde..d06f8d4919c7 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -37,10 +37,11 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 
 		ret = rproc_boot(rproc);
 	} else if (!strncmp(cmd, "stop", len)) {
-		if (rproc->state != RPROC_RUNNING)
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
-		rproc_shutdown(rproc);
+		ret = rproc_shutdown(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognized option\n");
 		ret = -EINVAL;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 51275107eb1f..3d7d245edc4e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1642,6 +1642,10 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
 	struct device *dev = &rproc->dev;
 	int ret;
 
+	/* No need to continue if a stop() operation has not been provided */
+	if (!rproc->ops->stop)
+		return -EINVAL;
+
 	/* Stop any subdevices for the remote processor */
 	rproc_stop_subdevices(rproc, crashed);
 
@@ -1880,7 +1884,7 @@ int rproc_shutdown(struct rproc *rproc)
 		return ret;
 	}
 
-	if (rproc->state != RPROC_RUNNING) {
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED) {
 		ret = -EPERM;
 		goto out;
 	}
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index f9694def9b54..3696f2ccc785 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -201,10 +201,11 @@ static ssize_t state_store(struct device *dev,
 		if (ret)
 			dev_err(&rproc->dev, "Boot failed: %d\n", ret);
 	} else if (sysfs_streq(buf, "stop")) {
-		if (rproc->state != RPROC_RUNNING)
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
 			return -EINVAL;
 
-		rproc_shutdown(rproc);
+		ret = rproc_shutdown(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.25.1

[PATCH v3 13/15] remoteproc: Properly deal with a start request when attached

[Mathieu Poirier]

This patch takes into account scenarios where a remote processor
has been attached to when receiving a "start" command from sysfs.

As with the "running" case, the command can't be carried out if the
remote processor is already in operation.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_cdev.c  | 3 ++-
 drivers/remoteproc/remoteproc_sysfs.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index d06f8d4919c7..61541bc7d26c 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -32,7 +32,8 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 		return -EFAULT;
 
 	if (!strncmp(cmd, "start", len)) {
-		if (rproc->state == RPROC_RUNNING)
+		if (rproc->state == RPROC_RUNNING ||
+		    rproc->state == RPROC_ATTACHED)
 			return -EBUSY;
 
 		ret = rproc_boot(rproc);
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 3696f2ccc785..7d281cfe3e03 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -194,7 +194,8 @@ static ssize_t state_store(struct device *dev,
 	int ret = 0;
 
 	if (sysfs_streq(buf, "start")) {
-		if (rproc->state == RPROC_RUNNING)
+		if (rproc->state == RPROC_RUNNING ||
+		    rproc->state == RPROC_ATTACHED)
 			return -EBUSY;
 
 		ret = rproc_boot(rproc);
-- 
2.25.1

Re: [PATCH v3 13/15] remoteproc: Properly deal with a start request when attached

[Arnaud POULIQUEN]

On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> This patch takes into account scenarios where a remote processor
> has been attached to when receiving a "start" command from sysfs.
> 
> As with the "running" case, the command can't be carried out if the
> remote processor is already in operation.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>


Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud

> ---
>  drivers/remoteproc/remoteproc_cdev.c  | 3 ++-
>  drivers/remoteproc/remoteproc_sysfs.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index d06f8d4919c7..61541bc7d26c 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -32,7 +32,8 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  		return -EFAULT;
>  
>  	if (!strncmp(cmd, "start", len)) {
> -		if (rproc->state == RPROC_RUNNING)
> +		if (rproc->state == RPROC_RUNNING ||
> +		    rproc->state == RPROC_ATTACHED)
>  			return -EBUSY;
>  
>  		ret = rproc_boot(rproc);
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 3696f2ccc785..7d281cfe3e03 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -194,7 +194,8 @@ static ssize_t state_store(struct device *dev,
>  	int ret = 0;
>  
>  	if (sysfs_streq(buf, "start")) {
> -		if (rproc->state == RPROC_RUNNING)
> +		if (rproc->state == RPROC_RUNNING ||
> +		    rproc->state == RPROC_ATTACHED)
>  			return -EBUSY;
>  
>  		ret = rproc_boot(rproc);
> 

[PATCH v3 14/15] remoteproc: Properly deal with detach request

[Mathieu Poirier]

This patch introduces the capability to detach a remote processor
that has been attached to or booted by the remoteproc core.  For
that to happen a rproc::ops::detach() operation need to be
available.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_cdev.c  | 6 ++++++
 drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index 61541bc7d26c..f7645f289563 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -43,6 +43,12 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
 			return -EINVAL;
 
 		ret = rproc_shutdown(rproc);
+	} else if (!strncmp(cmd, "detach", len)) {
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
+			return -EINVAL;
+
+		ret = rproc_detach(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognized option\n");
 		ret = -EINVAL;
diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
index 7d281cfe3e03..5a239df5877e 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -207,6 +207,12 @@ static ssize_t state_store(struct device *dev,
 			return -EINVAL;
 
 		ret = rproc_shutdown(rproc);
+	} else if (sysfs_streq(buf, "detach")) {
+		if (rproc->state != RPROC_RUNNING &&
+		    rproc->state != RPROC_ATTACHED)
+			return -EINVAL;
+
+		ret = rproc_detach(rproc);
 	} else {
 		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
 		ret = -EINVAL;
-- 
2.25.1

Re: [PATCH v3 14/15] remoteproc: Properly deal with detach request

[Arnaud POULIQUEN]

On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> This patch introduces the capability to detach a remote processor
> that has been attached to or booted by the remoteproc core.  For
> that to happen a rproc::ops::detach() operation need to be
> available.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>

Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Thanks,
Arnaud
> ---
>  drivers/remoteproc/remoteproc_cdev.c  | 6 ++++++
>  drivers/remoteproc/remoteproc_sysfs.c | 6 ++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index 61541bc7d26c..f7645f289563 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -43,6 +43,12 @@ static ssize_t rproc_cdev_write(struct file *filp, const char __user *buf, size_
>  			return -EINVAL;
>  
>  		ret = rproc_shutdown(rproc);
> +	} else if (!strncmp(cmd, "detach", len)) {
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
> +			return -EINVAL;
> +
> +		ret = rproc_detach(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognized option\n");
>  		ret = -EINVAL;
> diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> index 7d281cfe3e03..5a239df5877e 100644
> --- a/drivers/remoteproc/remoteproc_sysfs.c
> +++ b/drivers/remoteproc/remoteproc_sysfs.c
> @@ -207,6 +207,12 @@ static ssize_t state_store(struct device *dev,
>  			return -EINVAL;
>  
>  		ret = rproc_shutdown(rproc);
> +	} else if (sysfs_streq(buf, "detach")) {
> +		if (rproc->state != RPROC_RUNNING &&
> +		    rproc->state != RPROC_ATTACHED)
> +			return -EINVAL;
> +
> +		ret = rproc_detach(rproc);
>  	} else {
>  		dev_err(&rproc->dev, "Unrecognised option: %s\n", buf);
>  		ret = -EINVAL;
> 

[PATCH v3 15/15] remoteproc: Refactor rproc delete and cdev release path

[Mathieu Poirier]

Refactor function rproc_del() and rproc_cdev_release() to take
into account the policy specified in the device tree.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_cdev.c | 13 +++++++++++-
 drivers/remoteproc/remoteproc_core.c | 30 ++++++++++++++++++++++++++--
 include/linux/remoteproc.h           |  4 ++++
 3 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index f7645f289563..3dfe555dfc07 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -88,7 +88,18 @@ static int rproc_cdev_release(struct inode *inode, struct file *filp)
 {
 	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
 
-	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
+	if (!rproc->cdev_put_on_release)
+		return 0;
+
+	/*
+	 * The application has crashed or is releasing its file handle.  Detach
+	 * or shutdown the remote processor based on the policy specified in the
+	 * DT.  No need to check rproc->state right away, it will be done
+	 * in either rproc_detach() or rproc_shutdown().
+	 */
+	if (rproc->autonomous_on_core_shutdown)
+		rproc_detach(rproc);
+	else
 		rproc_shutdown(rproc);
 
 	return 0;
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 3d7d245edc4e..1a170103bf27 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2294,6 +2294,22 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
 	return 0;
 }
 
+static void rproc_set_automation_flags(struct rproc *rproc)
+{
+	struct device *dev = rproc->dev.parent;
+	struct device_node *np = dev->of_node;
+	bool core_shutdown;
+
+	/*
+	 * When function rproc_cdev_release() or rproc_del() are called and
+	 * the remote processor has been attached to, it will be detached from
+	 * (rather than turned off) if "autonomous-on-core-shutdown is specified
+	 * in the DT.
+	 */
+	core_shutdown = of_property_read_bool(np, "autonomous-on-core-shutdown");
+	rproc->autonomous_on_core_shutdown = core_shutdown;
+}
+
 /**
  * rproc_alloc() - allocate a remote processor handle
  * @dev: the underlying device
@@ -2352,6 +2368,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
 	if (rproc_alloc_ops(rproc, ops))
 		goto put_device;
 
+	rproc_set_automation_flags(rproc);
+
 	/* Assign a unique device index and name */
 	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
 	if (rproc->index < 0) {
@@ -2435,8 +2453,16 @@ int rproc_del(struct rproc *rproc)
 	if (!rproc)
 		return -EINVAL;
 
-	/* TODO: make sure this works with rproc->power > 1 */
-	rproc_shutdown(rproc);
+	/*
+	 * TODO: make sure this works with rproc->power > 1
+	 *
+	 * No need to check rproc->state right away, it will be done in either
+	 * rproc_detach() or rproc_shutdown().
+	 */
+	if (rproc->autonomous_on_core_shutdown)
+		rproc_detach(rproc);
+	else
+		rproc_shutdown(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 02312096d59f..5702f630d810 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -516,6 +516,9 @@ struct rproc_dump_segment {
  * @nb_vdev: number of vdev currently handled by rproc
  * @char_dev: character device of the rproc
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @autonomous_on_core_shutdown: true if the remote processor should be detached
+ *				 from (rather than turned off) when the remoteproc
+ *				 core goes away.
  */
 struct rproc {
 	struct list_head node;
@@ -554,6 +557,7 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	bool autonomous_on_core_shutdown;
 };
 
 /**
-- 
2.25.1

Re: [PATCH v3 15/15] remoteproc: Refactor rproc delete and cdev release path

[Arnaud POULIQUEN]

On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> Refactor function rproc_del() and rproc_cdev_release() to take
> into account the policy specified in the device tree.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_cdev.c | 13 +++++++++++-
>  drivers/remoteproc/remoteproc_core.c | 30 ++++++++++++++++++++++++++--
>  include/linux/remoteproc.h           |  4 ++++
>  3 files changed, 44 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index f7645f289563..3dfe555dfc07 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -88,7 +88,18 @@ static int rproc_cdev_release(struct inode *inode, struct file *filp)
>  {
>  	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
>  
> -	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
> +	if (!rproc->cdev_put_on_release)
> +		return 0;
> +
> +	/*
> +	 * The application has crashed or is releasing its file handle.  Detach
> +	 * or shutdown the remote processor based on the policy specified in the
> +	 * DT.  No need to check rproc->state right away, it will be done
> +	 * in either rproc_detach() or rproc_shutdown().
> +	 */
> +	if (rproc->autonomous_on_core_shutdown)
> +		rproc_detach(rproc);
> +	else
>  		rproc_shutdown(rproc);

A reason to not propagate the return of functions?

>  
>  	return 0;
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 3d7d245edc4e..1a170103bf27 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2294,6 +2294,22 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
>  	return 0;
>  }
>  
> +static void rproc_set_automation_flags(struct rproc *rproc)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct device_node *np = dev->of_node;
> +	bool core_shutdown;
> +
> +	/*
> +	 * When function rproc_cdev_release() or rproc_del() are called and
> +	 * the remote processor has been attached to, it will be detached from
> +	 * (rather than turned off) if "autonomous-on-core-shutdown is specified
> +	 * in the DT.
> +	 */
> +	core_shutdown = of_property_read_bool(np, "autonomous-on-core-shutdown");
> +	rproc->autonomous_on_core_shutdown = core_shutdown;
> +}
> +
>  /**
>   * rproc_alloc() - allocate a remote processor handle
>   * @dev: the underlying device
> @@ -2352,6 +2368,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
>  	if (rproc_alloc_ops(rproc, ops))
>  		goto put_device;
>  
> +	rproc_set_automation_flags(rproc);
> +
>  	/* Assign a unique device index and name */
>  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
>  	if (rproc->index < 0) {
> @@ -2435,8 +2453,16 @@ int rproc_del(struct rproc *rproc)
>  	if (!rproc)
>  		return -EINVAL;
>  
> -	/* TODO: make sure this works with rproc->power > 1 */
> -	rproc_shutdown(rproc);
> +	/*
> +	 * TODO: make sure this works with rproc->power > 1
> +	 *
> +	 * No need to check rproc->state right away, it will be done in either
> +	 * rproc_detach() or rproc_shutdown().
> +	 */
> +	if (rproc->autonomous_on_core_shutdown)
> +		rproc_detach(rproc);
> +	else
> +		rproc_shutdown(rproc);

same here

>  
>  	mutex_lock(&rproc->lock);
>  	rproc->state = RPROC_DELETED;
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 02312096d59f..5702f630d810 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -516,6 +516,9 @@ struct rproc_dump_segment {
>   * @nb_vdev: number of vdev currently handled by rproc
>   * @char_dev: character device of the rproc
>   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> + * @autonomous_on_core_shutdown: true if the remote processor should be detached
> + *				 from (rather than turned off) when the remoteproc
> + *				 core goes away.
>   */
>  struct rproc {
>  	struct list_head node;
> @@ -554,6 +557,7 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool autonomous_on_core_shutdown;
>  };
>  
>  /**
> 

Re: [PATCH v3 15/15] remoteproc: Refactor rproc delete and cdev release path

[Mathieu Poirier]

On Wed, Dec 09, 2020 at 11:13:07AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 11/26/20 10:06 PM, Mathieu Poirier wrote:
> > Refactor function rproc_del() and rproc_cdev_release() to take
> > into account the policy specified in the device tree.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_cdev.c | 13 +++++++++++-
> >  drivers/remoteproc/remoteproc_core.c | 30 ++++++++++++++++++++++++++--
> >  include/linux/remoteproc.h           |  4 ++++
> >  3 files changed, 44 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> > index f7645f289563..3dfe555dfc07 100644
> > --- a/drivers/remoteproc/remoteproc_cdev.c
> > +++ b/drivers/remoteproc/remoteproc_cdev.c
> > @@ -88,7 +88,18 @@ static int rproc_cdev_release(struct inode *inode, struct file *filp)
> >  {
> >  	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
> >  
> > -	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
> > +	if (!rproc->cdev_put_on_release)
> > +		return 0;
> > +
> > +	/*
> > +	 * The application has crashed or is releasing its file handle.  Detach
> > +	 * or shutdown the remote processor based on the policy specified in the
> > +	 * DT.  No need to check rproc->state right away, it will be done
> > +	 * in either rproc_detach() or rproc_shutdown().
> > +	 */
> > +	if (rproc->autonomous_on_core_shutdown)
> > +		rproc_detach(rproc);
> > +	else
> >  		rproc_shutdown(rproc);
> 
> A reason to not propagate the return of functions?

A valid observation...  I'll fix it.

> 
> >  
> >  	return 0;
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 3d7d245edc4e..1a170103bf27 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2294,6 +2294,22 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops)
> >  	return 0;
> >  }
> >  
> > +static void rproc_set_automation_flags(struct rproc *rproc)
> > +{
> > +	struct device *dev = rproc->dev.parent;
> > +	struct device_node *np = dev->of_node;
> > +	bool core_shutdown;
> > +
> > +	/*
> > +	 * When function rproc_cdev_release() or rproc_del() are called and
> > +	 * the remote processor has been attached to, it will be detached from
> > +	 * (rather than turned off) if "autonomous-on-core-shutdown is specified
> > +	 * in the DT.
> > +	 */
> > +	core_shutdown = of_property_read_bool(np, "autonomous-on-core-shutdown");
> > +	rproc->autonomous_on_core_shutdown = core_shutdown;
> > +}
> > +
> >  /**
> >   * rproc_alloc() - allocate a remote processor handle
> >   * @dev: the underlying device
> > @@ -2352,6 +2368,8 @@ struct rproc *rproc_alloc(struct device *dev, const char *name,
> >  	if (rproc_alloc_ops(rproc, ops))
> >  		goto put_device;
> >  
> > +	rproc_set_automation_flags(rproc);
> > +
> >  	/* Assign a unique device index and name */
> >  	rproc->index = ida_simple_get(&rproc_dev_index, 0, 0, GFP_KERNEL);
> >  	if (rproc->index < 0) {
> > @@ -2435,8 +2453,16 @@ int rproc_del(struct rproc *rproc)
> >  	if (!rproc)
> >  		return -EINVAL;
> >  
> > -	/* TODO: make sure this works with rproc->power > 1 */
> > -	rproc_shutdown(rproc);
> > +	/*
> > +	 * TODO: make sure this works with rproc->power > 1
> > +	 *
> > +	 * No need to check rproc->state right away, it will be done in either
> > +	 * rproc_detach() or rproc_shutdown().
> > +	 */
> > +	if (rproc->autonomous_on_core_shutdown)
> > +		rproc_detach(rproc);
> > +	else
> > +		rproc_shutdown(rproc);
> 
> same here
> 
> >  
> >  	mutex_lock(&rproc->lock);
> >  	rproc->state = RPROC_DELETED;
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 02312096d59f..5702f630d810 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -516,6 +516,9 @@ struct rproc_dump_segment {
> >   * @nb_vdev: number of vdev currently handled by rproc
> >   * @char_dev: character device of the rproc
> >   * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
> > + * @autonomous_on_core_shutdown: true if the remote processor should be detached
> > + *				 from (rather than turned off) when the remoteproc
> > + *				 core goes away.
> >   */
> >  struct rproc {
> >  	struct list_head node;
> > @@ -554,6 +557,7 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	bool autonomous_on_core_shutdown;
> >  };
> >  
> >  /**
> >