[PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[PATCH v4 00/17] remoteproc: Add support for detaching a 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.

Of special interest in this series are patches 5 and 6 where getting the
address of the resource table installed by an eternal entity if moved to
the core.  This is to support scenarios where a remote process has been
booted by the core but is being detached.  To re-attach the remote
processor, the address of the resource table needs to be known at a later
time than the platform driver's probe() function.

Applies cleanly on v5.10

Thanks,
Mathieu

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

----
New for v4:
- Made binding description OS agnostic (Rob)
- Added functionality to set the external resource table in the core
- Fixed a crash when detaching (Arnaud)
- Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
- Added RB tags

Mathieu Poirier (17):
  dt-bindings: remoteproc: Add bindind to support autonomous processors
  remoteproc: Re-check state in rproc_shutdown()
  remoteproc: Remove useless check in rproc_del()
  remoteproc: Rename function rproc_actuate()
  remoteproc: Add new get_loaded_rsc_table() remoteproc operation
  remoteproc: stm32: Move resource table setup to rproc_ops
  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: 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  |  27 +++
 drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
 drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
 drivers/remoteproc/remoteproc_internal.h      |   8 +
 drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
 drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
 include/linux/remoteproc.h                    |  24 +-
 7 files changed, 344 insertions(+), 125 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml

-- 
2.25.1

[PATCH v4 01/17] 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  | 27 +++++++++++++++++++
 1 file changed, 27 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..e8bb8ef9031a
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
@@ -0,0 +1,27 @@
+# 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(s) for a primary processor applicable to all ancillary
+       processors
+
+maintainers:
+  - Bjorn Andersson <bjorn.andersson@linaro.org>
+  - Mathieu Poirier <mathieu.poirier@linaro.org>
+
+description:
+  This document defines the bindings used by a primary processor to determine
+  the state it should leave an ancillary processor when the former is no longer
+  functioning.
+
+properties:
+  autonomous-on-core-reboot:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      When specified the ancillary processor should be left operational when
+      the primary processor is no longer available.  Otherwise the ancillary
+      processor should be made inoperative.
+
+additionalProperties: true
-- 
2.25.1

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

[Rob Herring]

On Fri, 18 Dec 2020 10:32:12 -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  | 27 +++++++++++++++++++
>  1 file changed, 27 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v4 02/17] 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 dab2c0f5caf0..e55568d1e7e2 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 v4 03/17] 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 e55568d1e7e2..f36786b47a4f 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2283,10 +2283,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 v4 04/17] remoteproc: Rename function rproc_actuate()

[Mathieu Poirier]

Rename function rproc_actuate() to rproc_attach().  That way it is
easy to understand that it does the opposite of rproc_detach().

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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f36786b47a4f..d0f6b39b56f9 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;
 
@@ -1802,7 +1802,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);
 
-- 
2.25.1

[PATCH v4 05/17] remoteproc: Add new get_loaded_rsc_table() remoteproc operation

[Mathieu Poirier]

Add an new get_loaded_rsc_table() operation in order to support
scenarios where the remoteproc core has booted a remote processor
and detaches from it.  When re-attaching to the remote processor,
the core needs to know where the resource table has been placed
in memory.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_core.c     | 6 ++++++
 drivers/remoteproc/remoteproc_internal.h | 8 ++++++++
 include/linux/remoteproc.h               | 5 ++++-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index d0f6b39b56f9..3d87c910aca7 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1556,6 +1556,12 @@ static int rproc_attach(struct rproc *rproc)
 		return ret;
 	}
 
+	ret = rproc_get_loaded_rsc_table(rproc);
+	if (ret) {
+		dev_err(dev, "can't load resource table: %d\n", ret);
+		goto disable_iommu;
+	}
+
 	/* reset max_notifyid */
 	rproc->max_notifyid = -1;
 
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index c34002888d2c..c48b301d6ad1 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -177,6 +177,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
 	return NULL;
 }
 
+static inline int rproc_get_loaded_rsc_table(struct rproc *rproc)
+{
+	if (rproc->ops->get_loaded_rsc_table)
+		return rproc->ops->get_loaded_rsc_table(rproc);
+
+	return 0;
+}
+
 static inline
 bool rproc_u64_fit_in_size_t(u64 val)
 {
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 3fa3ba6498e8..571615e77e6f 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -368,7 +368,9 @@ enum rsc_handling_status {
  * RSC_HANDLED if resource was handled, RSC_IGNORED if not handled and a
  * negative value on error
  * @load_rsc_table:	load resource table from firmware image
- * @find_loaded_rsc_table: find the loaded resouce table
+ * @find_loaded_rsc_table: find the loaded resource table from firmware image
+ * @get_loaded_rsc_table: get resource table installed in memory
+ *			  by external entity
  * @load:		load firmware to memory, where the remote processor
  *			expects to find it
  * @sanity_check:	sanity check the fw image
@@ -389,6 +391,7 @@ struct rproc_ops {
 			  int offset, int avail);
 	struct resource_table *(*find_loaded_rsc_table)(
 				struct rproc *rproc, const struct firmware *fw);
+	int (*get_loaded_rsc_table)(struct rproc *rproc);
 	int (*load)(struct rproc *rproc, const struct firmware *fw);
 	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
 	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
-- 
2.25.1

Re: [PATCH v4 05/17] remoteproc: Add new get_loaded_rsc_table() remoteproc operation

[Arnaud POULIQUEN]

Hi Mathieu,

Come back on you series...

On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> Add an new get_loaded_rsc_table() operation in order to support
> scenarios where the remoteproc core has booted a remote processor
> and detaches from it.  When re-attaching to the remote processor,
> the core needs to know where the resource table has been placed
> in memory.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_core.c     | 6 ++++++
>  drivers/remoteproc/remoteproc_internal.h | 8 ++++++++
>  include/linux/remoteproc.h               | 5 ++++-
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index d0f6b39b56f9..3d87c910aca7 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1556,6 +1556,12 @@ static int rproc_attach(struct rproc *rproc)
>  		return ret;
>  	}
>  
> +	ret = rproc_get_loaded_rsc_table(rproc);
> +	if (ret) {
> +		dev_err(dev, "can't load resource table: %d\n", ret);
> +		goto disable_iommu;
> +	}
> +

This function is rather ambiguous. Without the example of stm32, it is not
obvious what the platform driver has to do in this ops. And the update of rproc
in the in the core instead of in platform driver seems to me more reliable.

Here is a suggestion considering that ->cached_table is always NULL:


struct resource_table *rproc_get_loaded_rsc_table(struct rproc *rproc,
                                                  size_t* size)
{

	if (rproc->ops->get_loaded_rsc_table) {
		return rproc->ops->get_loaded_rsc_table(rproc, size);

	*size = 0;
	return NULL;
}

then in rproc_attach:

	table_ptr = rproc_get_loaded_rsc_table(rproc, &tab_size);
	if (PTR_ERR(table_ptr) {
		dev_err(dev, "can't load resource table: %d\n", ret);
		goto disable_iommu;
	}
 	rproc->cached_table = NULL;
 	rproc->table_ptr = table_ptr;
 	rproc->table_sz = table_sz;


Thanks,
Arnaud

>  	/* reset max_notifyid */
>  	rproc->max_notifyid = -1;
>  
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index c34002888d2c..c48b301d6ad1 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -177,6 +177,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
>  	return NULL;
>  }
>  
> +static inline int rproc_get_loaded_rsc_table(struct rproc *rproc)
> +{
> +	if (rproc->ops->get_loaded_rsc_table)
> +		return rproc->ops->get_loaded_rsc_table(rproc);
> +
> +	return 0;
> +}
> +
>  static inline
>  bool rproc_u64_fit_in_size_t(u64 val)
>  {
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 3fa3ba6498e8..571615e77e6f 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -368,7 +368,9 @@ enum rsc_handling_status {
>   * RSC_HANDLED if resource was handled, RSC_IGNORED if not handled and a
>   * negative value on error
>   * @load_rsc_table:	load resource table from firmware image
> - * @find_loaded_rsc_table: find the loaded resouce table
> + * @find_loaded_rsc_table: find the loaded resource table from firmware image
> + * @get_loaded_rsc_table: get resource table installed in memory
> + *			  by external entity
>   * @load:		load firmware to memory, where the remote processor
>   *			expects to find it
>   * @sanity_check:	sanity check the fw image
> @@ -389,6 +391,7 @@ struct rproc_ops {
>  			  int offset, int avail);
>  	struct resource_table *(*find_loaded_rsc_table)(
>  				struct rproc *rproc, const struct firmware *fw);
> +	int (*get_loaded_rsc_table)(struct rproc *rproc);
>  	int (*load)(struct rproc *rproc, const struct firmware *fw);
>  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
>  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
> 

Re: [PATCH v4 05/17] remoteproc: Add new get_loaded_rsc_table() remoteproc operation

[Mathieu Poirier]

On Wed, Jan 27, 2021 at 09:44:28AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu,
> 
> Come back on you series...
> 
> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> > Add an new get_loaded_rsc_table() operation in order to support
> > scenarios where the remoteproc core has booted a remote processor
> > and detaches from it.  When re-attaching to the remote processor,
> > the core needs to know where the resource table has been placed
> > in memory.
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_core.c     | 6 ++++++
> >  drivers/remoteproc/remoteproc_internal.h | 8 ++++++++
> >  include/linux/remoteproc.h               | 5 ++++-
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index d0f6b39b56f9..3d87c910aca7 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1556,6 +1556,12 @@ static int rproc_attach(struct rproc *rproc)
> >  		return ret;
> >  	}
> >  
> > +	ret = rproc_get_loaded_rsc_table(rproc);
> > +	if (ret) {
> > +		dev_err(dev, "can't load resource table: %d\n", ret);
> > +		goto disable_iommu;
> > +	}
> > +
> 
> This function is rather ambiguous. Without the example of stm32, it is not
> obvious what the platform driver has to do in this ops. And the update of rproc
> in the in the core instead of in platform driver seems to me more reliable.
> 
> Here is a suggestion considering that ->cached_table is always NULL:
> 
> 
> struct resource_table *rproc_get_loaded_rsc_table(struct rproc *rproc,
>                                                   size_t* size)
> {
> 
> 	if (rproc->ops->get_loaded_rsc_table) {
> 		return rproc->ops->get_loaded_rsc_table(rproc, size);
> 
> 	*size = 0;
> 	return NULL;
> }
> 
> then in rproc_attach:
> 
> 	table_ptr = rproc_get_loaded_rsc_table(rproc, &tab_size);
> 	if (PTR_ERR(table_ptr) {
> 		dev_err(dev, "can't load resource table: %d\n", ret);
> 		goto disable_iommu;
> 	}
>  	rproc->cached_table = NULL;
>  	rproc->table_ptr = table_ptr;
>  	rproc->table_sz = table_sz;
>

Much better yes, thanks for the suggestion.
 
> 
> Thanks,
> Arnaud
> 
> >  	/* reset max_notifyid */
> >  	rproc->max_notifyid = -1;
> >  
> > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > index c34002888d2c..c48b301d6ad1 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -177,6 +177,14 @@ struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> >  	return NULL;
> >  }
> >  
> > +static inline int rproc_get_loaded_rsc_table(struct rproc *rproc)
> > +{
> > +	if (rproc->ops->get_loaded_rsc_table)
> > +		return rproc->ops->get_loaded_rsc_table(rproc);
> > +
> > +	return 0;
> > +}
> > +
> >  static inline
> >  bool rproc_u64_fit_in_size_t(u64 val)
> >  {
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index 3fa3ba6498e8..571615e77e6f 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -368,7 +368,9 @@ enum rsc_handling_status {
> >   * RSC_HANDLED if resource was handled, RSC_IGNORED if not handled and a
> >   * negative value on error
> >   * @load_rsc_table:	load resource table from firmware image
> > - * @find_loaded_rsc_table: find the loaded resouce table
> > + * @find_loaded_rsc_table: find the loaded resource table from firmware image
> > + * @get_loaded_rsc_table: get resource table installed in memory
> > + *			  by external entity
> >   * @load:		load firmware to memory, where the remote processor
> >   *			expects to find it
> >   * @sanity_check:	sanity check the fw image
> > @@ -389,6 +391,7 @@ struct rproc_ops {
> >  			  int offset, int avail);
> >  	struct resource_table *(*find_loaded_rsc_table)(
> >  				struct rproc *rproc, const struct firmware *fw);
> > +	int (*get_loaded_rsc_table)(struct rproc *rproc);
> >  	int (*load)(struct rproc *rproc, const struct firmware *fw);
> >  	int (*sanity_check)(struct rproc *rproc, const struct firmware *fw);
> >  	u64 (*get_boot_addr)(struct rproc *rproc, const struct firmware *fw);
> > 

[PATCH v4 06/17] remoteproc: stm32: Move resource table setup to rproc_ops

[Mathieu Poirier]

Move the setting of the resource table installed by an external
entity to rproc_ops::get_loaded_rsc_table().  This is to support
scenarios where a remote processor has been started by the core
but is detached at a later stage.  To re-attach the remote
processor, the address of the resource table needs to be available
at a later time than the platform driver's probe() function.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/stm32_rproc.c | 147 ++++++++++++++++---------------
 1 file changed, 74 insertions(+), 73 deletions(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index d2414cc1d90d..c635f746e44a 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -541,6 +541,79 @@ static void stm32_rproc_kick(struct rproc *rproc, int vqid)
 	}
 }
 
+static int stm32_rproc_da_to_pa(struct rproc *rproc,
+				u64 da, phys_addr_t *pa)
+{
+	struct stm32_rproc *ddata = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	struct stm32_rproc_mem *p_mem;
+	unsigned int i;
+
+	for (i = 0; i < ddata->nb_rmems; i++) {
+		p_mem = &ddata->rmems[i];
+
+		if (da < p_mem->dev_addr ||
+		    da >= p_mem->dev_addr + p_mem->size)
+			continue;
+
+		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
+		dev_dbg(dev, "da %llx to pa %#x\n", da, *pa);
+
+		return 0;
+	}
+
+	dev_err(dev, "can't translate da %llx\n", da);
+
+	return -EINVAL;
+}
+
+static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
+{
+	struct stm32_rproc *ddata = rproc->priv;
+	struct device *dev = rproc->dev.parent;
+	phys_addr_t rsc_pa;
+	u32 rsc_da;
+	int err;
+
+	/* The resource table has already been mapped, nothing to do */
+	if (ddata->rsc_va)
+		goto done;
+
+	err = regmap_read(ddata->rsctbl.map, ddata->rsctbl.reg, &rsc_da);
+	if (err) {
+		dev_err(dev, "failed to read rsc tbl addr\n");
+		return err;
+	}
+
+	if (!rsc_da)
+		/* no rsc table */
+		return 0;
+
+	err = stm32_rproc_da_to_pa(rproc, rsc_da, &rsc_pa);
+	if (err)
+		return err;
+
+	ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
+	if (IS_ERR_OR_NULL(ddata->rsc_va)) {
+		dev_err(dev, "Unable to map memory region: %pa+%zx\n",
+			&rsc_pa, RSC_TBL_SIZE);
+		ddata->rsc_va = NULL;
+		return -ENOMEM;
+	}
+
+done:
+	/*
+	 * The resource table is already loaded in device memory, no need
+	 * to work with a cached table.
+	 */
+	rproc->cached_table = NULL;
+	/* Assuming the resource table fits in 1kB is fair */
+	rproc->table_sz = RSC_TBL_SIZE;
+	rproc->table_ptr = (struct resource_table *)ddata->rsc_va;
+
+	return 0;
+}
+
 static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
@@ -549,6 +622,7 @@ static struct rproc_ops st_rproc_ops = {
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= stm32_rproc_parse_fw,
 	.find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
+	.get_loaded_rsc_table = stm32_rproc_get_loaded_rsc_table,
 	.sanity_check	= rproc_elf_sanity_check,
 	.get_boot_addr	= rproc_elf_get_boot_addr,
 };
@@ -692,75 +766,6 @@ static int stm32_rproc_get_m4_status(struct stm32_rproc *ddata,
 	return regmap_read(ddata->m4_state.map, ddata->m4_state.reg, state);
 }
 
-static int stm32_rproc_da_to_pa(struct platform_device *pdev,
-				struct stm32_rproc *ddata,
-				u64 da, phys_addr_t *pa)
-{
-	struct device *dev = &pdev->dev;
-	struct stm32_rproc_mem *p_mem;
-	unsigned int i;
-
-	for (i = 0; i < ddata->nb_rmems; i++) {
-		p_mem = &ddata->rmems[i];
-
-		if (da < p_mem->dev_addr ||
-		    da >= p_mem->dev_addr + p_mem->size)
-			continue;
-
-		*pa = da - p_mem->dev_addr + p_mem->bus_addr;
-		dev_dbg(dev, "da %llx to pa %#x\n", da, *pa);
-
-		return 0;
-	}
-
-	dev_err(dev, "can't translate da %llx\n", da);
-
-	return -EINVAL;
-}
-
-static int stm32_rproc_get_loaded_rsc_table(struct platform_device *pdev,
-					    struct rproc *rproc,
-					    struct stm32_rproc *ddata)
-{
-	struct device *dev = &pdev->dev;
-	phys_addr_t rsc_pa;
-	u32 rsc_da;
-	int err;
-
-	err = regmap_read(ddata->rsctbl.map, ddata->rsctbl.reg, &rsc_da);
-	if (err) {
-		dev_err(dev, "failed to read rsc tbl addr\n");
-		return err;
-	}
-
-	if (!rsc_da)
-		/* no rsc table */
-		return 0;
-
-	err = stm32_rproc_da_to_pa(pdev, ddata, rsc_da, &rsc_pa);
-	if (err)
-		return err;
-
-	ddata->rsc_va = devm_ioremap_wc(dev, rsc_pa, RSC_TBL_SIZE);
-	if (IS_ERR_OR_NULL(ddata->rsc_va)) {
-		dev_err(dev, "Unable to map memory region: %pa+%zx\n",
-			&rsc_pa, RSC_TBL_SIZE);
-		ddata->rsc_va = NULL;
-		return -ENOMEM;
-	}
-
-	/*
-	 * The resource table is already loaded in device memory, no need
-	 * to work with a cached table.
-	 */
-	rproc->cached_table = NULL;
-	/* Assuming the resource table fits in 1kB is fair */
-	rproc->table_sz = RSC_TBL_SIZE;
-	rproc->table_ptr = (struct resource_table *)ddata->rsc_va;
-
-	return 0;
-}
-
 static int stm32_rproc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -800,10 +805,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)
 		ret = stm32_rproc_parse_memory_regions(rproc);
 		if (ret)
 			goto free_resources;
-
-		ret = stm32_rproc_get_loaded_rsc_table(pdev, rproc, ddata);
-		if (ret)
-			goto free_resources;
 	}
 
 	rproc->has_iommu = false;
-- 
2.25.1

[PATCH v4 07/17] 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 d1cf7bf277c4..1167adcf8741 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -201,6 +201,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 571615e77e6f..f02958a6c001 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -406,6 +406,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
@@ -422,8 +424,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 v4 08/17] 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 3d87c910aca7..19545b7925e2 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);
 
@@ -1665,14 +1665,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;
@@ -2023,16 +2015,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 1167adcf8741..99ff51fd9707 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 f02958a6c001..257a5005f93e 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -513,7 +513,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
@@ -550,7 +549,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 v4 09/17] 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 19545b7925e2..fc28053c7f89 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2414,7 +2414,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 v4 10/17] 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>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 include/linux/remoteproc.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 257a5005f93e..9bb34c3eb847 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)
@@ -384,6 +385,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

[PATCH v4 11/17] 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 | 42 ++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index fc28053c7f89..e665ed4776c3 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1670,6 +1670,48 @@ 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);
+
+	/*
+	 * If the remote processors was started by the core then a cached_table
+	 * is present and we must follow the same cleanup sequence as we would
+	 * for a shutdown().  As it is in rproc_stop(), use the cached resource
+	 * table for the rest of the detach process since ->table_ptr will
+	 * become invalid as soon as carveouts are released in
+	 * rproc_resource_cleanup().
+	 */
+	if (rproc->cached_table)
+		rproc->table_ptr = rproc->cached_table;
+
+	/* 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

Re: [PATCH v4 11/17] remoteproc: Introduce function __rproc_detach()

[Arnaud POULIQUEN]

On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> 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 | 42 ++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index fc28053c7f89..e665ed4776c3 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1670,6 +1670,48 @@ 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;

I wonder if this ops should be optional.

> +
> +	/* Stop any subdevices for the remote processor */
> +	rproc_stop_subdevices(rproc, false);
> +
> +	/*
> +	 * If the remote processors was started by the core then a cached_table
> +	 * is present and we must follow the same cleanup sequence as we would
> +	 * for a shutdown().  As it is in rproc_stop(), use the cached resource
> +	 * table for the rest of the detach process since ->table_ptr will
> +	 * become invalid as soon as carveouts are released in
> +	 * rproc_resource_cleanup().
> +	 */
> +	if (rproc->cached_table)
> +		rproc->table_ptr = rproc->cached_table;
> +
> +	/* 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);

Not sure that this would be possible in all cases, without a unprepare and
prepare. What about having the same behavior as the rproc_stop failure?

Thanks
Arnaud.

> +		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
> 

Re: [PATCH v4 11/17] remoteproc: Introduce function __rproc_detach()

[Mathieu Poirier]

On Wed, Jan 27, 2021 at 09:46:58AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> > 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 | 42 ++++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index fc28053c7f89..e665ed4776c3 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1670,6 +1670,48 @@ 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;
> 
> I wonder if this ops should be optional.

Function rproc_validate() doesn't check for it so it is optional.  Returning an
error is to indicate to sysfs the operation is not supported if someone tries to
do a "detach" when rproc::ops doesn't provide it.

> 
> > +
> > +	/* Stop any subdevices for the remote processor */
> > +	rproc_stop_subdevices(rproc, false);
> > +
> > +	/*
> > +	 * If the remote processors was started by the core then a cached_table
> > +	 * is present and we must follow the same cleanup sequence as we would
> > +	 * for a shutdown().  As it is in rproc_stop(), use the cached resource
> > +	 * table for the rest of the detach process since ->table_ptr will
> > +	 * become invalid as soon as carveouts are released in
> > +	 * rproc_resource_cleanup().
> > +	 */
> > +	if (rproc->cached_table)
> > +		rproc->table_ptr = rproc->cached_table;
> > +
> > +	/* 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);
> 
> Not sure that this would be possible in all cases, without a unprepare and
> prepare. What about having the same behavior as the rproc_stop failure?

I thought rproc_stop()'s failure path was buggy and could be improved but as you
say, there might be other ramifications to take into account.  I agree that it
is more prudent to follow the current behavior from rproc_stop() and leave
enhancements for another patchset.

> 
> Thanks
> Arnaud.
> 
> > +		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
> > 

[PATCH v4 12/17] 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 | 71 +++++++++++++++++++++++++++-
 include/linux/remoteproc.h           |  2 +
 2 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index e665ed4776c3..ece3f15070b9 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1673,7 +1673,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;
@@ -1927,6 +1927,75 @@ 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);
+
+	/*
+	 * If the remote processor was booted by the core the cached table needs
+	 * to be freed and ->table_ptr set to NULL because it will be
+	 * invalidated by rproc_resource_cleanup().  If the remote processor was
+	 * attached to ->cached_table is NULL and kfree() returns right away.
+	 *
+	 * In either case ->table_ptr has to be set to NULL.  It will be set
+	 * again when the remote processor is re-attached to.
+	 */
+	kfree(rproc->cached_table);
+	rproc->cached_table = NULL;
+	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 9bb34c3eb847..65ece6f177b7 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -659,6 +659,8 @@ 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);
 int rproc_coredump_add_custom_segment(struct rproc *rproc,
-- 
2.25.1

Re: [PATCH v4 12/17] remoteproc: Introduce function rproc_detach()

[Arnaud POULIQUEN]

On 12/18/20 6:32 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 | 71 +++++++++++++++++++++++++++-
>  include/linux/remoteproc.h           |  2 +
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index e665ed4776c3..ece3f15070b9 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1673,7 +1673,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;
> @@ -1927,6 +1927,75 @@ 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()).

You rename the function to rproc_attach in you patch 04/17.

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

Thanks,
Arnaud

> + *
> + * 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);
> +
> +	/*
> +	 * If the remote processor was booted by the core the cached table needs
> +	 * to be freed and ->table_ptr set to NULL because it will be
> +	 * invalidated by rproc_resource_cleanup().  If the remote processor was
> +	 * attached to ->cached_table is NULL and kfree() returns right away.
> +	 *
> +	 * In either case ->table_ptr has to be set to NULL.  It will be set
> +	 * again when the remote processor is re-attached to.
> +	 */
> +	kfree(rproc->cached_table);
> +	rproc->cached_table = NULL;
> +	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 9bb34c3eb847..65ece6f177b7 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -659,6 +659,8 @@ 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);
>  int rproc_coredump_add_custom_segment(struct rproc *rproc,
> 

Re: [PATCH v4 12/17] remoteproc: Introduce function rproc_detach()

[Mathieu Poirier]

On Wed, Jan 27, 2021 at 09:50:31AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 12/18/20 6:32 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 | 71 +++++++++++++++++++++++++++-
> >  include/linux/remoteproc.h           |  2 +
> >  2 files changed, 72 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index e665ed4776c3..ece3f15070b9 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1673,7 +1673,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;
> > @@ -1927,6 +1927,75 @@ 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()).
> 
> You rename the function to rproc_attach in you patch 04/17.
> 

Yes, good catch.

> Then Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> 
> Thanks,
> Arnaud
> 
> > + *
> > + * 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);
> > +
> > +	/*
> > +	 * If the remote processor was booted by the core the cached table needs
> > +	 * to be freed and ->table_ptr set to NULL because it will be
> > +	 * invalidated by rproc_resource_cleanup().  If the remote processor was
> > +	 * attached to ->cached_table is NULL and kfree() returns right away.
> > +	 *
> > +	 * In either case ->table_ptr has to be set to NULL.  It will be set
> > +	 * again when the remote processor is re-attached to.
> > +	 */
> > +	kfree(rproc->cached_table);
> > +	rproc->cached_table = NULL;
> > +	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 9bb34c3eb847..65ece6f177b7 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -659,6 +659,8 @@ 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);
> >  int rproc_coredump_add_custom_segment(struct rproc *rproc,
> > 

[PATCH v4 13/17] 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>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.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 ece3f15070b9..90057ad1bb6c 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1886,7 +1886,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;
@@ -1894,15 +1894,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) {
@@ -1914,7 +1918,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);
 
@@ -1924,6 +1932,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 65ece6f177b7..aa5bceb72015 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -658,7 +658,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

[PATCH v4 14/17] 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 90057ad1bb6c..2fe42ac7ca89 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1648,6 +1648,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);
 
@@ -1897,7 +1901,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 99ff51fd9707..96751c087585 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -230,10 +230,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 v4 15/17] 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>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 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 96751c087585..298052c9d068 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -223,7 +223,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

[PATCH v4 16/17] 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>
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.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 298052c9d068..ea7b067fe89a 100644
--- a/drivers/remoteproc/remoteproc_sysfs.c
+++ b/drivers/remoteproc/remoteproc_sysfs.c
@@ -236,6 +236,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

[PATCH v4 17/17] 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 | 18 +++++++++++---
 drivers/remoteproc/remoteproc_core.c | 36 ++++++++++++++++++++++++----
 include/linux/remoteproc.h           |  4 ++++
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
index f7645f289563..9b2fb6fbf8e7 100644
--- a/drivers/remoteproc/remoteproc_cdev.c
+++ b/drivers/remoteproc/remoteproc_cdev.c
@@ -87,11 +87,23 @@ static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned l
 static int rproc_cdev_release(struct inode *inode, struct file *filp)
 {
 	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
+	int ret;
 
-	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
-		rproc_shutdown(rproc);
+	if (!rproc->cdev_put_on_release)
+		return 0;
 
-	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)
+		ret = rproc_detach(rproc);
+	else
+		ret = rproc_shutdown(rproc);
+
+	return ret;
 }
 
 static const struct file_operations rproc_fops = {
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 2fe42ac7ca89..9f47a4ec0ec6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -2254,6 +2254,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
@@ -2312,6 +2328,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) {
@@ -2388,15 +2406,25 @@ EXPORT_SYMBOL(rproc_put);
  * of the outstanding reference created by rproc_alloc. To decrement that
  * one last refcount, one still needs to call rproc_free().
  *
- * Returns 0 on success and -EINVAL if @rproc isn't valid.
+ * Returns 0 on success and a negative error code on failure.
  */
 int rproc_del(struct rproc *rproc)
 {
+	int ret;
+
 	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)
+		ret = rproc_detach(rproc);
+	else
+		ret = rproc_shutdown(rproc);
 
 	mutex_lock(&rproc->lock);
 	rproc->state = RPROC_DELETED;
@@ -2415,7 +2443,7 @@ int rproc_del(struct rproc *rproc)
 
 	device_del(&rproc->dev);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(rproc_del);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index aa5bceb72015..012bebbd324b 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -519,6 +519,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;
@@ -557,6 +560,7 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	bool autonomous_on_core_shutdown;
 };
 
 /**
-- 
2.25.1

Re: [PATCH v4 17/17] remoteproc: Refactor rproc delete and cdev release path

[Arnaud POULIQUEN]

look good to me
Reviewed-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>

On 12/18/20 6:32 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 | 18 +++++++++++---
>  drivers/remoteproc/remoteproc_core.c | 36 ++++++++++++++++++++++++----
>  include/linux/remoteproc.h           |  4 ++++
>  3 files changed, 51 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_cdev.c b/drivers/remoteproc/remoteproc_cdev.c
> index f7645f289563..9b2fb6fbf8e7 100644
> --- a/drivers/remoteproc/remoteproc_cdev.c
> +++ b/drivers/remoteproc/remoteproc_cdev.c
> @@ -87,11 +87,23 @@ static long rproc_device_ioctl(struct file *filp, unsigned int ioctl, unsigned l
>  static int rproc_cdev_release(struct inode *inode, struct file *filp)
>  {
>  	struct rproc *rproc = container_of(inode->i_cdev, struct rproc, cdev);
> +	int ret;
>  
> -	if (rproc->cdev_put_on_release && rproc->state == RPROC_RUNNING)
> -		rproc_shutdown(rproc);
> +	if (!rproc->cdev_put_on_release)
> +		return 0;
>  
> -	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)
> +		ret = rproc_detach(rproc);
> +	else
> +		ret = rproc_shutdown(rproc);
> +
> +	return ret;
>  }
>  
>  static const struct file_operations rproc_fops = {
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 2fe42ac7ca89..9f47a4ec0ec6 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -2254,6 +2254,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
> @@ -2312,6 +2328,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) {
> @@ -2388,15 +2406,25 @@ EXPORT_SYMBOL(rproc_put);
>   * of the outstanding reference created by rproc_alloc. To decrement that
>   * one last refcount, one still needs to call rproc_free().
>   *
> - * Returns 0 on success and -EINVAL if @rproc isn't valid.
> + * Returns 0 on success and a negative error code on failure.
>   */
>  int rproc_del(struct rproc *rproc)
>  {
> +	int ret;
> +
>  	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)
> +		ret = rproc_detach(rproc);
> +	else
> +		ret = rproc_shutdown(rproc);
>  
>  	mutex_lock(&rproc->lock);
>  	rproc->state = RPROC_DELETED;
> @@ -2415,7 +2443,7 @@ int rproc_del(struct rproc *rproc)
>  
>  	device_del(&rproc->dev);
>  
> -	return 0;
> +	return ret;
>  }
>  EXPORT_SYMBOL(rproc_del);
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index aa5bceb72015..012bebbd324b 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -519,6 +519,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;
> @@ -557,6 +560,7 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	bool autonomous_on_core_shutdown;
>  };
>  
>  /**
> 

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Arnaud POULIQUEN]

Hi Mathieu

On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> 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.
> 
> Of special interest in this series are patches 5 and 6 where getting the
> address of the resource table installed by an eternal entity if moved to
> the core.  This is to support scenarios where a remote process has been
> booted by the core but is being detached.  To re-attach the remote
> processor, the address of the resource table needs to be known at a later
> time than the platform driver's probe() function.
> 
> Applies cleanly on v5.10
> 
> Thanks,
> Mathieu
> 
> [1]. https://lkml.org/lkml/2020/7/14/1600
> 
> ----
> New for v4:
> - Made binding description OS agnostic (Rob)
> - Added functionality to set the external resource table in the core
> - Fixed a crash when detaching (Arnaud)
> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
> - Added RB tags


I tested you series, attach and  detach is working well.

Then I faced issue when tried to re-attach after a detach.

But I don't know if this feature has to be supported in this step.

The 2 issues I found are:

1) memory carveouts are released on detach so need to be reinitialized.
The use of prepare/unprepare for the attach and detach would solve the issue but
probably need to add parameter to differentiate a start/stop from a attach/detach.

2) The vrings in the loaded resource table (so no cached) has to be properly
reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
considered as a fixed address.

Here is a fix which works on the stm32 platform

@@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	 */
 	if (rproc->table_ptr) {
 		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
-		rsc->vring[idx].da = 0;
+		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
 		rsc->vring[idx].notifyid = -1;
 	}
 }

Here, perhaps a better alternative would be to make a cached copy on attach
before updating it. On the next attach, the cached copy would be copied as it is
done in rproc_start.

Thanks,
Arnaud


> 
> Mathieu Poirier (17):
>   dt-bindings: remoteproc: Add bindind to support autonomous processors
>   remoteproc: Re-check state in rproc_shutdown()
>   remoteproc: Remove useless check in rproc_del()
>   remoteproc: Rename function rproc_actuate()
>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
>   remoteproc: stm32: Move resource table setup to rproc_ops
>   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: 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  |  27 +++
>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
>  drivers/remoteproc/remoteproc_internal.h      |   8 +
>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
>  include/linux/remoteproc.h                    |  24 +-
>  7 files changed, 344 insertions(+), 125 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> 

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Mathieu Poirier]

On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
> Hi Mathieu
> 
> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> > 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.
> > 
> > Of special interest in this series are patches 5 and 6 where getting the
> > address of the resource table installed by an eternal entity if moved to
> > the core.  This is to support scenarios where a remote process has been
> > booted by the core but is being detached.  To re-attach the remote
> > processor, the address of the resource table needs to be known at a later
> > time than the platform driver's probe() function.
> > 
> > Applies cleanly on v5.10
> > 
> > Thanks,
> > Mathieu
> > 
> > [1]. https://lkml.org/lkml/2020/7/14/1600
> > 
> > ----
> > New for v4:
> > - Made binding description OS agnostic (Rob)
> > - Added functionality to set the external resource table in the core
> > - Fixed a crash when detaching (Arnaud)
> > - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
> > - Added RB tags
> 
> 
> I tested you series, attach and  detach is working well.
> 
> Then I faced issue when tried to re-attach after a detach.
>

Right, in this case don't expect the re-attach to work properly because function
stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
in "wait-for-attach" mode as it does when booted by the boot loader.  If I
remember correctly we talked about that during an earlier conversation and we
agreed FW support would be needed to properly test the re-attach.
 
> But I don't know if this feature has to be supported in this step.
> 
> The 2 issues I found are:
> 
> 1) memory carveouts are released on detach so need to be reinitialized.
> The use of prepare/unprepare for the attach and detach would solve the issue but
> probably need to add parameter to differentiate a start/stop from a attach/detach.
> 
> 2) The vrings in the loaded resource table (so no cached) has to be properly
> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
> considered as a fixed address.
> 
> Here is a fix which works on the stm32 platform
> 
> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>  	 */
>  	if (rproc->table_ptr) {
>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> -		rsc->vring[idx].da = 0;
> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>  		rsc->vring[idx].notifyid = -1;
>  	}
>  }

In light of the above let me know if these two issues are still relevant.  If
so I'll investigate further.

Thanks,
Mathieu

> 
> Here, perhaps a better alternative would be to make a cached copy on attach
> before updating it. On the next attach, the cached copy would be copied as it is
> done in rproc_start.
> 
> Thanks,
> Arnaud
> 
> 
> > 
> > Mathieu Poirier (17):
> >   dt-bindings: remoteproc: Add bindind to support autonomous processors
> >   remoteproc: Re-check state in rproc_shutdown()
> >   remoteproc: Remove useless check in rproc_del()
> >   remoteproc: Rename function rproc_actuate()
> >   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
> >   remoteproc: stm32: Move resource table setup to rproc_ops
> >   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: 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  |  27 +++
> >  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
> >  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
> >  drivers/remoteproc/remoteproc_internal.h      |   8 +
> >  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
> >  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
> >  include/linux/remoteproc.h                    |  24 +-
> >  7 files changed, 344 insertions(+), 125 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> > 

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Arnaud POULIQUEN]

On 2/2/21 1:49 AM, Mathieu Poirier wrote:
> On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
>> Hi Mathieu
>>
>> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
>>> 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.
>>>
>>> Of special interest in this series are patches 5 and 6 where getting the
>>> address of the resource table installed by an eternal entity if moved to
>>> the core.  This is to support scenarios where a remote process has been
>>> booted by the core but is being detached.  To re-attach the remote
>>> processor, the address of the resource table needs to be known at a later
>>> time than the platform driver's probe() function.
>>>
>>> Applies cleanly on v5.10
>>>
>>> Thanks,
>>> Mathieu
>>>
>>> [1]. https://lkml.org/lkml/2020/7/14/1600
>>>
>>> ----
>>> New for v4:
>>> - Made binding description OS agnostic (Rob)
>>> - Added functionality to set the external resource table in the core
>>> - Fixed a crash when detaching (Arnaud)
>>> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
>>> - Added RB tags
>>
>>
>> I tested you series, attach and  detach is working well.
>>
>> Then I faced issue when tried to re-attach after a detach.
>>
> 
> Right, in this case don't expect the re-attach to work properly because function
> stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
> in "wait-for-attach" mode as it does when booted by the boot loader.  If I
> remember correctly we talked about that during an earlier conversation and we
> agreed FW support would be needed to properly test the re-attach.

Yes you are right the remote firmware needs to be inform about the detach, and
this is the purpose of the detach ops.
But also some actions are missing on local side as some resources have also to
be reinitialized as described in my previous mail.
For instance the resource table is handled by the remoteproc framework. The
remote firmware should only have a read access to this table.

>  
>> But I don't know if this feature has to be supported in this step.
>>
>> The 2 issues I found are:
>>
>> 1) memory carveouts are released on detach so need to be reinitialized.
>> The use of prepare/unprepare for the attach and detach would solve the issue but
>> probably need to add parameter to differentiate a start/stop from a attach/detach.
>>
>> 2) The vrings in the loaded resource table (so no cached) has to be properly
>> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
>> considered as a fixed address.
>>
>> Here is a fix which works on the stm32 platform
>>
>> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>>  	 */
>>  	if (rproc->table_ptr) {
>>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
>> -		rsc->vring[idx].da = 0;
>> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>>  		rsc->vring[idx].notifyid = -1;
>>  	}
>>  }
> 
> In light of the above let me know if these two issues are still relevant.  If
> so I'll investigate further.

To highlight the issue just test attach/detach/attch  with a firmware that
implements a RPMsg communication. On the second attach the virtio framework is
not properly restarted.

Then please find at the end of the mail 3 patches for test I added on top of
your series,that allow me to reattach. Of course the RPMsg channels are not
re-created as i don't implement the remote FW part, but the Linux virtio and
RPmsg frameworks are restarted.

- [PATCH 1/3] remoteproc: stm32: add capability to detach from the remoteproc
  => Add a dummy function in stm32_rproc for test.
- [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
  => Add prepare/unprepare on attach/detach + implement attach in stm32mp1 to
     reinitialize the memory region that as been cleaned on detach.
- [PATCH 3/3] remoteproc: virtio: set to vring address to FW_RSC_ADDR_ANY on free
  => Reinitialize the vring addresses on detach. For this one a better
     implementation would be to use a cached resource table to fully
     reinitialize it on re-attach.

Thanks,
Arnaud

> 
> Thanks,
> Mathieu
> 
>>
>> Here, perhaps a better alternative would be to make a cached copy on attach
>> before updating it. On the next attach, the cached copy would be copied as it is
>> done in rproc_start.
>>
>> Thanks,
>> Arnaud
>>
>>
>>>
>>> Mathieu Poirier (17):
>>>   dt-bindings: remoteproc: Add bindind to support autonomous processors
>>>   remoteproc: Re-check state in rproc_shutdown()
>>>   remoteproc: Remove useless check in rproc_del()
>>>   remoteproc: Rename function rproc_actuate()
>>>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
>>>   remoteproc: stm32: Move resource table setup to rproc_ops
>>>   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: 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  |  27 +++
>>>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
>>>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
>>>  drivers/remoteproc/remoteproc_internal.h      |   8 +
>>>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
>>>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
>>>  include/linux/remoteproc.h                    |  24 +-
>>>  7 files changed, 344 insertions(+), 125 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
>>>

Subject: [PATCH 1/3] remoteproc: stm32: add capability to detach from the
 remoteproc

Add a dummy function to allow to detach. No specific action is needed

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

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 2c949725b91e..b325d28f627c 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -590,6 +590,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
 	return reset_control_assert(ddata->hold_boot);
 }

+static int stm32_rproc_detach(struct rproc *rproc)
+{
+	/* Nothing to do but ops mandatory to support the detach feature */
+	return 0;
+}
+
 static int stm32_rproc_stop(struct rproc *rproc)
 {
 	struct stm32_rproc *ddata = rproc->priv;
@@ -712,6 +718,7 @@ static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
 	.attach		= stm32_rproc_attach,
+	.detach		= stm32_rproc_detach,
 	.kick		= stm32_rproc_kick,
 	.load		= rproc_elf_load_segments,
 	.parse_fw	= stm32_rproc_parse_fw,
-- 
2.17.1


------------------------------------------------------------------------


Subject: [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach

Some actions such as memory resources reallocation are needed when try
to reattach. Use the prepare ops for these actions.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
 drivers/remoteproc/stm32_rproc.c     | 14 +++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index f1f51ad1a1d6..f177561b8863 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1557,6 +1557,13 @@ static int rproc_attach(struct rproc *rproc)
 		return ret;
 	}

+	/* Prepare rproc for firmware loading if needed */
+	ret = rproc_prepare_device(rproc);
+	if (ret) {
+		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
+		goto disable_iommu;
+	}
+
 	ret = rproc_get_loaded_rsc_table(rproc);
 	if (ret) {
 		dev_err(dev, "can't load resource table: %d\n", ret);
@@ -1990,6 +1997,13 @@ int rproc_detach(struct rproc *rproc)
 	/* clean up all acquired resources */
 	rproc_resource_cleanup(rproc);

+	/* Release HW resources if needed */
+	ret = rproc_unprepare_device(rproc);
+	if (ret) {
+		atomic_inc(&rproc->power);
+		goto out;
+	}
+
 	rproc_disable_iommu(rproc);

 	/*
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index b325d28f627c..bf50d79b1f09 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -413,9 +413,6 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const
struct firmware *fw)
 	struct stm32_rproc *ddata = rproc->priv;
 	int ret;

-	ret  = stm32_rproc_parse_memory_regions(rproc);
-	if (ret)
-		return ret;

 	if (ddata->trproc)
 		ret = rproc_tee_get_rsc_table(ddata->trproc);
@@ -580,6 +577,12 @@ static int stm32_rproc_start(struct rproc *rproc)

 	return reset_control_assert(ddata->hold_boot);
 }
+static int stm32_rproc_prepare(struct rproc *rproc)
+{
+	dev_err(&rproc->dev, "%s: %d\n", __func__, __LINE__);
+
+	return stm32_rproc_parse_memory_regions(rproc);
+}

 static int stm32_rproc_attach(struct rproc *rproc)
 {
@@ -717,6 +720,7 @@ static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
 static struct rproc_ops st_rproc_ops = {
 	.start		= stm32_rproc_start,
 	.stop		= stm32_rproc_stop,
+	.prepare	= stm32_rproc_prepare,
 	.attach		= stm32_rproc_attach,
 	.detach		= stm32_rproc_detach,
 	.kick		= stm32_rproc_kick,
@@ -921,10 +925,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)

 	if (state == M4_STATE_CRUN) {
 		rproc->state = RPROC_DETACHED;
-
-		ret = stm32_rproc_parse_memory_regions(rproc);
-		if (ret)
-			goto free_resources;
 	}

 	rproc->has_iommu = false;
-- 
2.17.1


------------------------------------------------------------------------

Subject: [PATCH 3/3] remoteproc: virtio: set to vring address to
 FW_RSC_ADDR_ANY on free

The resource table vring structure is cleaned on free. But value is set
to 0. This value is considered as a valid address. Set the value
to  FW_RSC_ADDR_ANY instead.
This is needed to allow to reattach to an autonomous firmware.
An alternative would be to save the resource table before updating it.
On free the value would be reset to initial value.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/remoteproc/remoteproc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c
b/drivers/remoteproc/remoteproc_core.c
index f177561b8863..5b5de4db3981 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
 	 */
 	if (rproc->table_ptr) {
 		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
-		rsc->vring[idx].da = 0;
+		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
 		rsc->vring[idx].notifyid = -1;
 	}
 }
-- 
2.17.1

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Mathieu Poirier]

On Tue, Feb 02, 2021 at 09:54:13AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 2/2/21 1:49 AM, Mathieu Poirier wrote:
> > On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
> >> Hi Mathieu
> >>
> >> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> >>> 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.
> >>>
> >>> Of special interest in this series are patches 5 and 6 where getting the
> >>> address of the resource table installed by an eternal entity if moved to
> >>> the core.  This is to support scenarios where a remote process has been
> >>> booted by the core but is being detached.  To re-attach the remote
> >>> processor, the address of the resource table needs to be known at a later
> >>> time than the platform driver's probe() function.
> >>>
> >>> Applies cleanly on v5.10
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>> [1]. https://lkml.org/lkml/2020/7/14/1600
> >>>
> >>> ----
> >>> New for v4:
> >>> - Made binding description OS agnostic (Rob)
> >>> - Added functionality to set the external resource table in the core
> >>> - Fixed a crash when detaching (Arnaud)
> >>> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
> >>> - Added RB tags
> >>
> >>
> >> I tested you series, attach and  detach is working well.
> >>
> >> Then I faced issue when tried to re-attach after a detach.
> >>
> > 
> > Right, in this case don't expect the re-attach to work properly because function
> > stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
> > in "wait-for-attach" mode as it does when booted by the boot loader.  If I
> > remember correctly we talked about that during an earlier conversation and we
> > agreed FW support would be needed to properly test the re-attach.
> 
> Yes you are right the remote firmware needs to be inform about the detach, and
> this is the purpose of the detach ops.
> But also some actions are missing on local side as some resources have also to
> be reinitialized as described in my previous mail.
> For instance the resource table is handled by the remoteproc framework. The
> remote firmware should only have a read access to this table.
> 
> >  
> >> But I don't know if this feature has to be supported in this step.
> >>
> >> The 2 issues I found are:
> >>
> >> 1) memory carveouts are released on detach so need to be reinitialized.
> >> The use of prepare/unprepare for the attach and detach would solve the issue but
> >> probably need to add parameter to differentiate a start/stop from a attach/detach.

I am in agreement with you assesment and the patch you have proposed to fix it.
Right now carveouts are properly handled between start and stop operations
because their parsing and allocation is done as part for ops:parse_fw(), which
is called for each iteration.  Moving the parsing and allocation to
rproc_prepare_device() ends up doing the same thing.

I am not sure we absolutely need an extra parameter to differentiate between
start/stop and attach/detach.  Typically the rproc_prepare_device() is used to
do some kind of setup like providing power to a memory bank.  I suppose that if
a memory bank is already powered by the boot loader, asking to power it up again
won't do anything.

As such I suggest we keep the parameters as they are now.  We can revisit if a
use case comes up at a later time. 

> >>
> >> 2) The vrings in the loaded resource table (so no cached) has to be properly
> >> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
> >> considered as a fixed address.
> >>
> >> Here is a fix which works on the stm32 platform
> >>
> >> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
> >>  	 */
> >>  	if (rproc->table_ptr) {
> >>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> >> -		rsc->vring[idx].da = 0;
> >> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
> >>  		rsc->vring[idx].notifyid = -1;
> >>  	}
> >>  }

I see why this could be needed.  I initially assumed the remote processor would
install a new resource table in memory upon receiving notification the core is
going away.  But upon reflection the remote processor may not even have access
to the image it is running.

Let me think about this further - I want to make sure we don't introduce a
regression for current implementations.

> > 
> > In light of the above let me know if these two issues are still relevant.  If
> > so I'll investigate further.
> 
> To highlight the issue just test attach/detach/attch  with a firmware that
> implements a RPMsg communication. On the second attach the virtio framework is
> not properly restarted.
> 
> Then please find at the end of the mail 3 patches for test I added on top of
> your series,that allow me to reattach. Of course the RPMsg channels are not
> re-created as i don't implement the remote FW part, but the Linux virtio and
> RPmsg frameworks are restarted.
> 
> - [PATCH 1/3] remoteproc: stm32: add capability to detach from the remoteproc
>   => Add a dummy function in stm32_rproc for test.
> - [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
>   => Add prepare/unprepare on attach/detach + implement attach in stm32mp1 to
>      reinitialize the memory region that as been cleaned on detach.
> - [PATCH 3/3] remoteproc: virtio: set to vring address to FW_RSC_ADDR_ANY on free
>   => Reinitialize the vring addresses on detach. For this one a better
>      implementation would be to use a cached resource table to fully
>      reinitialize it on re-attach.
> 
> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Mathieu
> > 
> >>
> >> Here, perhaps a better alternative would be to make a cached copy on attach
> >> before updating it. On the next attach, the cached copy would be copied as it is
> >> done in rproc_start.
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>
> >>>
> >>> Mathieu Poirier (17):
> >>>   dt-bindings: remoteproc: Add bindind to support autonomous processors
> >>>   remoteproc: Re-check state in rproc_shutdown()
> >>>   remoteproc: Remove useless check in rproc_del()
> >>>   remoteproc: Rename function rproc_actuate()
> >>>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
> >>>   remoteproc: stm32: Move resource table setup to rproc_ops
> >>>   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: 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  |  27 +++
> >>>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
> >>>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
> >>>  drivers/remoteproc/remoteproc_internal.h      |   8 +
> >>>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
> >>>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
> >>>  include/linux/remoteproc.h                    |  24 +-
> >>>  7 files changed, 344 insertions(+), 125 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> >>>
> 
> Subject: [PATCH 1/3] remoteproc: stm32: add capability to detach from the
>  remoteproc
> 
> Add a dummy function to allow to detach. No specific action is needed
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> ---
>  drivers/remoteproc/stm32_rproc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 2c949725b91e..b325d28f627c 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -590,6 +590,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
>  	return reset_control_assert(ddata->hold_boot);
>  }
> 
> +static int stm32_rproc_detach(struct rproc *rproc)
> +{
> +	/* Nothing to do but ops mandatory to support the detach feature */
> +	return 0;
> +}
> +
>  static int stm32_rproc_stop(struct rproc *rproc)
>  {
>  	struct stm32_rproc *ddata = rproc->priv;
> @@ -712,6 +718,7 @@ static struct rproc_ops st_rproc_ops = {
>  	.start		= stm32_rproc_start,
>  	.stop		= stm32_rproc_stop,
>  	.attach		= stm32_rproc_attach,
> +	.detach		= stm32_rproc_detach,
>  	.kick		= stm32_rproc_kick,
>  	.load		= rproc_elf_load_segments,
>  	.parse_fw	= stm32_rproc_parse_fw,
> -- 
> 2.17.1
> 
> 
> ------------------------------------------------------------------------
> 
> 
> Subject: [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
> 
> Some actions such as memory resources reallocation are needed when try
> to reattach. Use the prepare ops for these actions.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
>  drivers/remoteproc/stm32_rproc.c     | 14 +++++++-------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index f1f51ad1a1d6..f177561b8863 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1557,6 +1557,13 @@ static int rproc_attach(struct rproc *rproc)
>  		return ret;
>  	}
> 
> +	/* Prepare rproc for firmware loading if needed */
> +	ret = rproc_prepare_device(rproc);
> +	if (ret) {
> +		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
> +		goto disable_iommu;
> +	}
> +
>  	ret = rproc_get_loaded_rsc_table(rproc);
>  	if (ret) {
>  		dev_err(dev, "can't load resource table: %d\n", ret);
> @@ -1990,6 +1997,13 @@ int rproc_detach(struct rproc *rproc)
>  	/* clean up all acquired resources */
>  	rproc_resource_cleanup(rproc);
> 
> +	/* Release HW resources if needed */
> +	ret = rproc_unprepare_device(rproc);
> +	if (ret) {
> +		atomic_inc(&rproc->power);
> +		goto out;
> +	}
> +
>  	rproc_disable_iommu(rproc);
> 
>  	/*
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index b325d28f627c..bf50d79b1f09 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -413,9 +413,6 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const
> struct firmware *fw)
>  	struct stm32_rproc *ddata = rproc->priv;
>  	int ret;
> 
> -	ret  = stm32_rproc_parse_memory_regions(rproc);
> -	if (ret)
> -		return ret;
> 
>  	if (ddata->trproc)
>  		ret = rproc_tee_get_rsc_table(ddata->trproc);
> @@ -580,6 +577,12 @@ static int stm32_rproc_start(struct rproc *rproc)
> 
>  	return reset_control_assert(ddata->hold_boot);
>  }
> +static int stm32_rproc_prepare(struct rproc *rproc)
> +{
> +	dev_err(&rproc->dev, "%s: %d\n", __func__, __LINE__);
> +
> +	return stm32_rproc_parse_memory_regions(rproc);
> +}
> 
>  static int stm32_rproc_attach(struct rproc *rproc)
>  {
> @@ -717,6 +720,7 @@ static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
>  static struct rproc_ops st_rproc_ops = {
>  	.start		= stm32_rproc_start,
>  	.stop		= stm32_rproc_stop,
> +	.prepare	= stm32_rproc_prepare,
>  	.attach		= stm32_rproc_attach,
>  	.detach		= stm32_rproc_detach,
>  	.kick		= stm32_rproc_kick,
> @@ -921,10 +925,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> 
>  	if (state == M4_STATE_CRUN) {
>  		rproc->state = RPROC_DETACHED;
> -
> -		ret = stm32_rproc_parse_memory_regions(rproc);
> -		if (ret)
> -			goto free_resources;
>  	}
> 
>  	rproc->has_iommu = false;
> -- 
> 2.17.1
> 
> 
> ------------------------------------------------------------------------
> 
> Subject: [PATCH 3/3] remoteproc: virtio: set to vring address to
>  FW_RSC_ADDR_ANY on free
> 
> The resource table vring structure is cleaned on free. But value is set
> to 0. This value is considered as a valid address. Set the value
> to  FW_RSC_ADDR_ANY instead.
> This is needed to allow to reattach to an autonomous firmware.
> An alternative would be to save the resource table before updating it.
> On free the value would be reset to initial value.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c
> b/drivers/remoteproc/remoteproc_core.c
> index f177561b8863..5b5de4db3981 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>  	 */
>  	if (rproc->table_ptr) {
>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> -		rsc->vring[idx].da = 0;
> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>  		rsc->vring[idx].notifyid = -1;
>  	}
>  }
> -- 
> 2.17.1
> 
> 
> 
> 
> 
> 

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Arnaud POULIQUEN]

On 2/2/21 11:42 PM, Mathieu Poirier wrote:
> On Tue, Feb 02, 2021 at 09:54:13AM +0100, Arnaud POULIQUEN wrote:
>>
>>
>> On 2/2/21 1:49 AM, Mathieu Poirier wrote:
>>> On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
>>>> Hi Mathieu
>>>>
>>>> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
>>>>> 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.
>>>>>
>>>>> Of special interest in this series are patches 5 and 6 where getting the
>>>>> address of the resource table installed by an eternal entity if moved to
>>>>> the core.  This is to support scenarios where a remote process has been
>>>>> booted by the core but is being detached.  To re-attach the remote
>>>>> processor, the address of the resource table needs to be known at a later
>>>>> time than the platform driver's probe() function.
>>>>>
>>>>> Applies cleanly on v5.10
>>>>>
>>>>> Thanks,
>>>>> Mathieu
>>>>>
>>>>> [1]. https://lkml.org/lkml/2020/7/14/1600
>>>>>
>>>>> ----
>>>>> New for v4:
>>>>> - Made binding description OS agnostic (Rob)
>>>>> - Added functionality to set the external resource table in the core
>>>>> - Fixed a crash when detaching (Arnaud)
>>>>> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
>>>>> - Added RB tags
>>>>
>>>>
>>>> I tested you series, attach and  detach is working well.
>>>>
>>>> Then I faced issue when tried to re-attach after a detach.
>>>>
>>>
>>> Right, in this case don't expect the re-attach to work properly because function
>>> stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
>>> in "wait-for-attach" mode as it does when booted by the boot loader.  If I
>>> remember correctly we talked about that during an earlier conversation and we
>>> agreed FW support would be needed to properly test the re-attach.
>>
>> Yes you are right the remote firmware needs to be inform about the detach, and
>> this is the purpose of the detach ops.
>> But also some actions are missing on local side as some resources have also to
>> be reinitialized as described in my previous mail.
>> For instance the resource table is handled by the remoteproc framework. The
>> remote firmware should only have a read access to this table.
>>
>>>  
>>>> But I don't know if this feature has to be supported in this step.
>>>>
>>>> The 2 issues I found are:
>>>>
>>>> 1) memory carveouts are released on detach so need to be reinitialized.
>>>> The use of prepare/unprepare for the attach and detach would solve the issue but
>>>> probably need to add parameter to differentiate a start/stop from a attach/detach.
> 
> I am in agreement with you assesment and the patch you have proposed to fix it.
> Right now carveouts are properly handled between start and stop operations
> because their parsing and allocation is done as part for ops:parse_fw(), which
> is called for each iteration.  Moving the parsing and allocation to
> rproc_prepare_device() ends up doing the same thing.
> 
> I am not sure we absolutely need an extra parameter to differentiate between
> start/stop and attach/detach.  Typically the rproc_prepare_device() is used to
> do some kind of setup like providing power to a memory bank.  I suppose that if
> a memory bank is already powered by the boot loader, asking to power it up again
> won't do anything.
> 
> As such I suggest we keep the parameters as they are now.  We can revisit if a
> use case comes up at a later time. 
> 

Your suggestion sound good to me.

>>>>
>>>> 2) The vrings in the loaded resource table (so no cached) has to be properly
>>>> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
>>>> considered as a fixed address.
>>>>
>>>> Here is a fix which works on the stm32 platform
>>>>
>>>> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>>>>  	 */
>>>>  	if (rproc->table_ptr) {
>>>>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
>>>> -		rsc->vring[idx].da = 0;
>>>> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>>>>  		rsc->vring[idx].notifyid = -1;
>>>>  	}
>>>>  }
> 
> I see why this could be needed.  I initially assumed the remote processor would
> install a new resource table in memory upon receiving notification the core is
> going away.  But upon reflection the remote processor may not even have access
> to the image it is running.

The risk here is that both processors try to access this table at the same time.

> 
> Let me think about this further - I want to make sure we don't introduce a
> regression for current implementations.

Just a precision: the vring DA can be fixed by the coprocessor firmware to
impose the address, my patch is not sufficent in such case for the reattachment.
That's why i suggested a copy of the resource table before updating it.

Thanks,
Arnaud

> 
>>>
>>> In light of the above let me know if these two issues are still relevant.  If
>>> so I'll investigate further.
>>
>> To highlight the issue just test attach/detach/attch  with a firmware that
>> implements a RPMsg communication. On the second attach the virtio framework is
>> not properly restarted.
>>
>> Then please find at the end of the mail 3 patches for test I added on top of
>> your series,that allow me to reattach. Of course the RPMsg channels are not
>> re-created as i don't implement the remote FW part, but the Linux virtio and
>> RPmsg frameworks are restarted.
>>
>> - [PATCH 1/3] remoteproc: stm32: add capability to detach from the remoteproc
>>   => Add a dummy function in stm32_rproc for test.
>> - [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
>>   => Add prepare/unprepare on attach/detach + implement attach in stm32mp1 to
>>      reinitialize the memory region that as been cleaned on detach.
>> - [PATCH 3/3] remoteproc: virtio: set to vring address to FW_RSC_ADDR_ANY on free
>>   => Reinitialize the vring addresses on detach. For this one a better
>>      implementation would be to use a cached resource table to fully
>>      reinitialize it on re-attach.
>>
>> Thanks,
>> Arnaud
>>
>>>
>>> Thanks,
>>> Mathieu
>>>
>>>>
>>>> Here, perhaps a better alternative would be to make a cached copy on attach
>>>> before updating it. On the next attach, the cached copy would be copied as it is
>>>> done in rproc_start.
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>
>>>>
>>>>>
>>>>> Mathieu Poirier (17):
>>>>>   dt-bindings: remoteproc: Add bindind to support autonomous processors
>>>>>   remoteproc: Re-check state in rproc_shutdown()
>>>>>   remoteproc: Remove useless check in rproc_del()
>>>>>   remoteproc: Rename function rproc_actuate()
>>>>>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
>>>>>   remoteproc: stm32: Move resource table setup to rproc_ops
>>>>>   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: 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  |  27 +++
>>>>>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
>>>>>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
>>>>>  drivers/remoteproc/remoteproc_internal.h      |   8 +
>>>>>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
>>>>>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
>>>>>  include/linux/remoteproc.h                    |  24 +-
>>>>>  7 files changed, 344 insertions(+), 125 deletions(-)
>>>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
>>>>>
>>
>> Subject: [PATCH 1/3] remoteproc: stm32: add capability to detach from the
>>  remoteproc
>>
>> Add a dummy function to allow to detach. No specific action is needed
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
>> ---
>>  drivers/remoteproc/stm32_rproc.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index 2c949725b91e..b325d28f627c 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -590,6 +590,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
>>  	return reset_control_assert(ddata->hold_boot);
>>  }
>>
>> +static int stm32_rproc_detach(struct rproc *rproc)
>> +{
>> +	/* Nothing to do but ops mandatory to support the detach feature */
>> +	return 0;
>> +}
>> +
>>  static int stm32_rproc_stop(struct rproc *rproc)
>>  {
>>  	struct stm32_rproc *ddata = rproc->priv;
>> @@ -712,6 +718,7 @@ static struct rproc_ops st_rproc_ops = {
>>  	.start		= stm32_rproc_start,
>>  	.stop		= stm32_rproc_stop,
>>  	.attach		= stm32_rproc_attach,
>> +	.detach		= stm32_rproc_detach,
>>  	.kick		= stm32_rproc_kick,
>>  	.load		= rproc_elf_load_segments,
>>  	.parse_fw	= stm32_rproc_parse_fw,
>> -- 
>> 2.17.1
>>
>>
>> ------------------------------------------------------------------------
>>
>>
>> Subject: [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
>>
>> Some actions such as memory resources reallocation are needed when try
>> to reattach. Use the prepare ops for these actions.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
>>  drivers/remoteproc/stm32_rproc.c     | 14 +++++++-------
>>  2 files changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index f1f51ad1a1d6..f177561b8863 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1557,6 +1557,13 @@ static int rproc_attach(struct rproc *rproc)
>>  		return ret;
>>  	}
>>
>> +	/* Prepare rproc for firmware loading if needed */
>> +	ret = rproc_prepare_device(rproc);
>> +	if (ret) {
>> +		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
>> +		goto disable_iommu;
>> +	}
>> +
>>  	ret = rproc_get_loaded_rsc_table(rproc);
>>  	if (ret) {
>>  		dev_err(dev, "can't load resource table: %d\n", ret);
>> @@ -1990,6 +1997,13 @@ int rproc_detach(struct rproc *rproc)
>>  	/* clean up all acquired resources */
>>  	rproc_resource_cleanup(rproc);
>>
>> +	/* Release HW resources if needed */
>> +	ret = rproc_unprepare_device(rproc);
>> +	if (ret) {
>> +		atomic_inc(&rproc->power);
>> +		goto out;
>> +	}
>> +
>>  	rproc_disable_iommu(rproc);
>>
>>  	/*
>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
>> index b325d28f627c..bf50d79b1f09 100644
>> --- a/drivers/remoteproc/stm32_rproc.c
>> +++ b/drivers/remoteproc/stm32_rproc.c
>> @@ -413,9 +413,6 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const
>> struct firmware *fw)
>>  	struct stm32_rproc *ddata = rproc->priv;
>>  	int ret;
>>
>> -	ret  = stm32_rproc_parse_memory_regions(rproc);
>> -	if (ret)
>> -		return ret;
>>
>>  	if (ddata->trproc)
>>  		ret = rproc_tee_get_rsc_table(ddata->trproc);
>> @@ -580,6 +577,12 @@ static int stm32_rproc_start(struct rproc *rproc)
>>
>>  	return reset_control_assert(ddata->hold_boot);
>>  }
>> +static int stm32_rproc_prepare(struct rproc *rproc)
>> +{
>> +	dev_err(&rproc->dev, "%s: %d\n", __func__, __LINE__);
>> +
>> +	return stm32_rproc_parse_memory_regions(rproc);
>> +}
>>
>>  static int stm32_rproc_attach(struct rproc *rproc)
>>  {
>> @@ -717,6 +720,7 @@ static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
>>  static struct rproc_ops st_rproc_ops = {
>>  	.start		= stm32_rproc_start,
>>  	.stop		= stm32_rproc_stop,
>> +	.prepare	= stm32_rproc_prepare,
>>  	.attach		= stm32_rproc_attach,
>>  	.detach		= stm32_rproc_detach,
>>  	.kick		= stm32_rproc_kick,
>> @@ -921,10 +925,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>>
>>  	if (state == M4_STATE_CRUN) {
>>  		rproc->state = RPROC_DETACHED;
>> -
>> -		ret = stm32_rproc_parse_memory_regions(rproc);
>> -		if (ret)
>> -			goto free_resources;
>>  	}
>>
>>  	rproc->has_iommu = false;
>> -- 
>> 2.17.1
>>
>>
>> ------------------------------------------------------------------------
>>
>> Subject: [PATCH 3/3] remoteproc: virtio: set to vring address to
>>  FW_RSC_ADDR_ANY on free
>>
>> The resource table vring structure is cleaned on free. But value is set
>> to 0. This value is considered as a valid address. Set the value
>> to  FW_RSC_ADDR_ANY instead.
>> This is needed to allow to reattach to an autonomous firmware.
>> An alternative would be to save the resource table before updating it.
>> On free the value would be reset to initial value.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>> ---
>>  drivers/remoteproc/remoteproc_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c
>> b/drivers/remoteproc/remoteproc_core.c
>> index f177561b8863..5b5de4db3981 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
>>  	 */
>>  	if (rproc->table_ptr) {
>>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
>> -		rsc->vring[idx].da = 0;
>> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
>>  		rsc->vring[idx].notifyid = -1;
>>  	}
>>  }
>> -- 
>> 2.17.1
>>
>>
>>
>>
>>
>>

Re: [PATCH v4 00/17] remoteproc: Add support for detaching a rproc

[Mathieu Poirier]

On Wed, Feb 03, 2021 at 08:58:15AM +0100, Arnaud POULIQUEN wrote:
> 
> 
> On 2/2/21 11:42 PM, Mathieu Poirier wrote:
> > On Tue, Feb 02, 2021 at 09:54:13AM +0100, Arnaud POULIQUEN wrote:
> >>
> >>
> >> On 2/2/21 1:49 AM, Mathieu Poirier wrote:
> >>> On Wed, Jan 27, 2021 at 10:21:24AM +0100, Arnaud POULIQUEN wrote:
> >>>> Hi Mathieu
> >>>>
> >>>> On 12/18/20 6:32 PM, Mathieu Poirier wrote:
> >>>>> 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.
> >>>>>
> >>>>> Of special interest in this series are patches 5 and 6 where getting the
> >>>>> address of the resource table installed by an eternal entity if moved to
> >>>>> the core.  This is to support scenarios where a remote process has been
> >>>>> booted by the core but is being detached.  To re-attach the remote
> >>>>> processor, the address of the resource table needs to be known at a later
> >>>>> time than the platform driver's probe() function.
> >>>>>
> >>>>> Applies cleanly on v5.10
> >>>>>
> >>>>> Thanks,
> >>>>> Mathieu
> >>>>>
> >>>>> [1]. https://lkml.org/lkml/2020/7/14/1600
> >>>>>
> >>>>> ----
> >>>>> New for v4:
> >>>>> - Made binding description OS agnostic (Rob)
> >>>>> - Added functionality to set the external resource table in the core
> >>>>> - Fixed a crash when detaching (Arnaud)
> >>>>> - Fixed error code propagation in rproc_cdev_relase() and rproc_del() (Arnaud)
> >>>>> - Added RB tags
> >>>>
> >>>>
> >>>> I tested you series, attach and  detach is working well.
> >>>>
> >>>> Then I faced issue when tried to re-attach after a detach.
> >>>>
> >>>
> >>> Right, in this case don't expect the re-attach to work properly because function
> >>> stm32_rproc_detach() does not exist.  As such the M4 doesn't put itself back
> >>> in "wait-for-attach" mode as it does when booted by the boot loader.  If I
> >>> remember correctly we talked about that during an earlier conversation and we
> >>> agreed FW support would be needed to properly test the re-attach.
> >>
> >> Yes you are right the remote firmware needs to be inform about the detach, and
> >> this is the purpose of the detach ops.
> >> But also some actions are missing on local side as some resources have also to
> >> be reinitialized as described in my previous mail.
> >> For instance the resource table is handled by the remoteproc framework. The
> >> remote firmware should only have a read access to this table.
> >>
> >>>  
> >>>> But I don't know if this feature has to be supported in this step.
> >>>>
> >>>> The 2 issues I found are:
> >>>>
> >>>> 1) memory carveouts are released on detach so need to be reinitialized.
> >>>> The use of prepare/unprepare for the attach and detach would solve the issue but
> >>>> probably need to add parameter to differentiate a start/stop from a attach/detach.
> > 
> > I am in agreement with you assesment and the patch you have proposed to fix it.
> > Right now carveouts are properly handled between start and stop operations
> > because their parsing and allocation is done as part for ops:parse_fw(), which
> > is called for each iteration.  Moving the parsing and allocation to
> > rproc_prepare_device() ends up doing the same thing.
> > 
> > I am not sure we absolutely need an extra parameter to differentiate between
> > start/stop and attach/detach.  Typically the rproc_prepare_device() is used to
> > do some kind of setup like providing power to a memory bank.  I suppose that if
> > a memory bank is already powered by the boot loader, asking to power it up again
> > won't do anything.
> > 
> > As such I suggest we keep the parameters as they are now.  We can revisit if a
> > use case comes up at a later time. 
> > 
> 
> Your suggestion sound good to me.
> 
> >>>>
> >>>> 2) The vrings in the loaded resource table (so no cached) has to be properly
> >>>> reinitialized. In rproc_free_vring  the vring da is set to 0 that is then
> >>>> considered as a fixed address.
> >>>>
> >>>> Here is a fix which works on the stm32 platform
> >>>>
> >>>> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
> >>>>  	 */
> >>>>  	if (rproc->table_ptr) {
> >>>>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> >>>> -		rsc->vring[idx].da = 0;
> >>>> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
> >>>>  		rsc->vring[idx].notifyid = -1;
> >>>>  	}
> >>>>  }
> > 
> > I see why this could be needed.  I initially assumed the remote processor would
> > install a new resource table in memory upon receiving notification the core is
> > going away.  But upon reflection the remote processor may not even have access
> > to the image it is running.
> 
> The risk here is that both processors try to access this table at the same time.
> 
> > 
> > Let me think about this further - I want to make sure we don't introduce a
> > regression for current implementations.
> 
> Just a precision: the vring DA can be fixed by the coprocessor firmware to
> impose the address, my patch is not sufficent in such case for the reattachment.
> That's why i suggested a copy of the resource table before updating it.
> 
> Thanks,
> Arnaud
> 
> > 
> >>>
> >>> In light of the above let me know if these two issues are still relevant.  If
> >>> so I'll investigate further.
> >>
> >> To highlight the issue just test attach/detach/attch  with a firmware that
> >> implements a RPMsg communication. On the second attach the virtio framework is
> >> not properly restarted.
> >>
> >> Then please find at the end of the mail 3 patches for test I added on top of
> >> your series,that allow me to reattach. Of course the RPMsg channels are not
> >> re-created as i don't implement the remote FW part, but the Linux virtio and
> >> RPmsg frameworks are restarted.
> >>
> >> - [PATCH 1/3] remoteproc: stm32: add capability to detach from the remoteproc
> >>   => Add a dummy function in stm32_rproc for test.
> >> - [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
> >>   => Add prepare/unprepare on attach/detach + implement attach in stm32mp1 to
> >>      reinitialize the memory region that as been cleaned on detach.
> >> - [PATCH 3/3] remoteproc: virtio: set to vring address to FW_RSC_ADDR_ANY on free
> >>   => Reinitialize the vring addresses on detach. For this one a better
> >>      implementation would be to use a cached resource table to fully
> >>      reinitialize it on re-attach.
> >>
> >> Thanks,
> >> Arnaud
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>
> >>>> Here, perhaps a better alternative would be to make a cached copy on attach
> >>>> before updating it. On the next attach, the cached copy would be copied as it is
> >>>> done in rproc_start.

I've been staring long and hard at this code and I don't see another way than
keeping a clean copy of the resource table as you mentionned above.  And
managing that table isn't trivial either.  I am working on a solution, something
that I should be able to publish by the end of the week.

Thanks,
Mathieu

> >>>>
> >>>> Thanks,
> >>>> Arnaud
> >>>>
> >>>>
> >>>>>
> >>>>> Mathieu Poirier (17):
> >>>>>   dt-bindings: remoteproc: Add bindind to support autonomous processors
> >>>>>   remoteproc: Re-check state in rproc_shutdown()
> >>>>>   remoteproc: Remove useless check in rproc_del()
> >>>>>   remoteproc: Rename function rproc_actuate()
> >>>>>   remoteproc: Add new get_loaded_rsc_table() remoteproc operation
> >>>>>   remoteproc: stm32: Move resource table setup to rproc_ops
> >>>>>   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: 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  |  27 +++
> >>>>>  drivers/remoteproc/remoteproc_cdev.c          |  32 ++-
> >>>>>  drivers/remoteproc/remoteproc_core.c          | 211 +++++++++++++++---
> >>>>>  drivers/remoteproc/remoteproc_internal.h      |   8 +
> >>>>>  drivers/remoteproc/remoteproc_sysfs.c         |  20 +-
> >>>>>  drivers/remoteproc/stm32_rproc.c              | 147 ++++++------
> >>>>>  include/linux/remoteproc.h                    |  24 +-
> >>>>>  7 files changed, 344 insertions(+), 125 deletions(-)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/remoteproc/remoteproc-core.yaml
> >>>>>
> >>
> >> Subject: [PATCH 1/3] remoteproc: stm32: add capability to detach from the
> >>  remoteproc
> >>
> >> Add a dummy function to allow to detach. No specific action is needed
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss-st.com>
> >> ---
> >>  drivers/remoteproc/stm32_rproc.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >> index 2c949725b91e..b325d28f627c 100644
> >> --- a/drivers/remoteproc/stm32_rproc.c
> >> +++ b/drivers/remoteproc/stm32_rproc.c
> >> @@ -590,6 +590,12 @@ static int stm32_rproc_attach(struct rproc *rproc)
> >>  	return reset_control_assert(ddata->hold_boot);
> >>  }
> >>
> >> +static int stm32_rproc_detach(struct rproc *rproc)
> >> +{
> >> +	/* Nothing to do but ops mandatory to support the detach feature */
> >> +	return 0;
> >> +}
> >> +
> >>  static int stm32_rproc_stop(struct rproc *rproc)
> >>  {
> >>  	struct stm32_rproc *ddata = rproc->priv;
> >> @@ -712,6 +718,7 @@ static struct rproc_ops st_rproc_ops = {
> >>  	.start		= stm32_rproc_start,
> >>  	.stop		= stm32_rproc_stop,
> >>  	.attach		= stm32_rproc_attach,
> >> +	.detach		= stm32_rproc_detach,
> >>  	.kick		= stm32_rproc_kick,
> >>  	.load		= rproc_elf_load_segments,
> >>  	.parse_fw	= stm32_rproc_parse_fw,
> >> -- 
> >> 2.17.1
> >>
> >>
> >> ------------------------------------------------------------------------
> >>
> >>
> >> Subject: [PATCH 2/3] remoteproc: Add prepare/unprepare for attach detach
> >>
> >> Some actions such as memory resources reallocation are needed when try
> >> to reattach. Use the prepare ops for these actions.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c | 14 ++++++++++++++
> >>  drivers/remoteproc/stm32_rproc.c     | 14 +++++++-------
> >>  2 files changed, 21 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index f1f51ad1a1d6..f177561b8863 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1557,6 +1557,13 @@ static int rproc_attach(struct rproc *rproc)
> >>  		return ret;
> >>  	}
> >>
> >> +	/* Prepare rproc for firmware loading if needed */
> >> +	ret = rproc_prepare_device(rproc);
> >> +	if (ret) {
> >> +		dev_err(dev, "can't prepare rproc %s: %d\n", rproc->name, ret);
> >> +		goto disable_iommu;
> >> +	}
> >> +
> >>  	ret = rproc_get_loaded_rsc_table(rproc);
> >>  	if (ret) {
> >>  		dev_err(dev, "can't load resource table: %d\n", ret);
> >> @@ -1990,6 +1997,13 @@ int rproc_detach(struct rproc *rproc)
> >>  	/* clean up all acquired resources */
> >>  	rproc_resource_cleanup(rproc);
> >>
> >> +	/* Release HW resources if needed */
> >> +	ret = rproc_unprepare_device(rproc);
> >> +	if (ret) {
> >> +		atomic_inc(&rproc->power);
> >> +		goto out;
> >> +	}
> >> +
> >>  	rproc_disable_iommu(rproc);
> >>
> >>  	/*
> >> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >> index b325d28f627c..bf50d79b1f09 100644
> >> --- a/drivers/remoteproc/stm32_rproc.c
> >> +++ b/drivers/remoteproc/stm32_rproc.c
> >> @@ -413,9 +413,6 @@ static int stm32_rproc_parse_fw(struct rproc *rproc, const
> >> struct firmware *fw)
> >>  	struct stm32_rproc *ddata = rproc->priv;
> >>  	int ret;
> >>
> >> -	ret  = stm32_rproc_parse_memory_regions(rproc);
> >> -	if (ret)
> >> -		return ret;
> >>
> >>  	if (ddata->trproc)
> >>  		ret = rproc_tee_get_rsc_table(ddata->trproc);
> >> @@ -580,6 +577,12 @@ static int stm32_rproc_start(struct rproc *rproc)
> >>
> >>  	return reset_control_assert(ddata->hold_boot);
> >>  }
> >> +static int stm32_rproc_prepare(struct rproc *rproc)
> >> +{
> >> +	dev_err(&rproc->dev, "%s: %d\n", __func__, __LINE__);
> >> +
> >> +	return stm32_rproc_parse_memory_regions(rproc);
> >> +}
> >>
> >>  static int stm32_rproc_attach(struct rproc *rproc)
> >>  {
> >> @@ -717,6 +720,7 @@ static int stm32_rproc_get_loaded_rsc_table(struct rproc *rproc)
> >>  static struct rproc_ops st_rproc_ops = {
> >>  	.start		= stm32_rproc_start,
> >>  	.stop		= stm32_rproc_stop,
> >> +	.prepare	= stm32_rproc_prepare,
> >>  	.attach		= stm32_rproc_attach,
> >>  	.detach		= stm32_rproc_detach,
> >>  	.kick		= stm32_rproc_kick,
> >> @@ -921,10 +925,6 @@ static int stm32_rproc_probe(struct platform_device *pdev)
> >>
> >>  	if (state == M4_STATE_CRUN) {
> >>  		rproc->state = RPROC_DETACHED;
> >> -
> >> -		ret = stm32_rproc_parse_memory_regions(rproc);
> >> -		if (ret)
> >> -			goto free_resources;
> >>  	}
> >>
> >>  	rproc->has_iommu = false;
> >> -- 
> >> 2.17.1
> >>
> >>
> >> ------------------------------------------------------------------------
> >>
> >> Subject: [PATCH 3/3] remoteproc: virtio: set to vring address to
> >>  FW_RSC_ADDR_ANY on free
> >>
> >> The resource table vring structure is cleaned on free. But value is set
> >> to 0. This value is considered as a valid address. Set the value
> >> to  FW_RSC_ADDR_ANY instead.
> >> This is needed to allow to reattach to an autonomous firmware.
> >> An alternative would be to save the resource table before updating it.
> >> On free the value would be reset to initial value.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> b/drivers/remoteproc/remoteproc_core.c
> >> index f177561b8863..5b5de4db3981 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -425,7 +425,7 @@ void rproc_free_vring(struct rproc_vring *rvring)
> >>  	 */
> >>  	if (rproc->table_ptr) {
> >>  		rsc = (void *)rproc->table_ptr + rvring->rvdev->rsc_offset;
> >> -		rsc->vring[idx].da = 0;
> >> +		rsc->vring[idx].da = FW_RSC_ADDR_ANY;
> >>  		rsc->vring[idx].notifyid = -1;
> >>  	}
> >>  }
> >> -- 
> >> 2.17.1
> >>
> >>
> >>
> >>
> >>
> >>