[PATCH V7 0/2] remoteproc: support self recovery

[PATCH V7 0/2] remoteproc: support self recovery

[Peng Fan (OSS)]

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

V7:
 Per comments from Arnaud Pouliquen:
   Typo fixes
   Added A-b tag

V6:
 Rename rproc_firmware_recovery to rproc_boot_recovery
 Drop the unlock/lock when do reproc_attach_recovery

V5:
 Rename RPROC_FEAT_ATTACH_RECOVERY to RPROC_FEAT_ATTACH_ON_RECOVERY
 Add kerneldoc for rproc features
 Change rproc_set_feature to return int type and add a max feature check
 Use __rproc_detach and __rproc_attach when do attach recovery
 https://patchwork.kernel.org/project/linux-remoteproc/cover/20220615032048.465486-1-peng.fan@oss.nxp.com/

V4:
  Based on Bjorn's comments on V2-2
  Move the rproc_has_feature/rproc_set_feature to remoteproc_internal.h and
 Keep rproc_features still in remoteproc.h, because we use
 RPROC_MAX_FEATURES to declare bitmap.
  Update commit log for patch 2/2, and add comments

  https://patchwork.kernel.org/project/linux-remoteproc/cover/20220323034405.976643-1-peng.fan@oss.nxp.com/
 
V3:
 Resend the wrong labeled patchset
 https://patchwork.kernel.org/project/linux-remoteproc/list/?series=621311

 Write a cover-letter
 To i.MX8QM/QXP, they have a M4 core self-recovery capability without
 Linux loading firmware. The self recovery is done by
 SCU(System Control Unit). Current remoteproc framework only support Linux
 help recovery remote processor(stop, loading firmware, start). This
 patchset is support remote processor self recovery(attach recovery).

 In order to avoid introducing a new variable(bool support_self_recovery),
 patch 1 introduce a new function, rproc_has_feature to make code easy to
 extend, cleaner, such as we could move "bool has_iommu" to
 rproc_has_feature(rproc, RPROC_FEAT_IOMMU).

 Patch 2 is introduce a new function rproc_attach_recovery for
 self recovery, the original logic move to rproc_firmware_recovery meaning
 needs linux to help recovery.

 V2-version 2:
 https://patchwork.kernel.org/project/linux-remoteproc/list/?series=621311
 Introduce rproc_has_feature

 V2-version 1:
 https://patchwork.kernel.org/project/linux-remoteproc/patch/20220126085120.3397450-1-peng.fan@oss.nxp.com/
 Nothing change in V2.
 Only move this patch out from
 https://patchwork.kernel.org/project/linux-remoteproc/list/?series=604364


Peng Fan (2):
  remoteproc: introduce rproc features
  remoteproc: support attach recovery after rproc crash

 drivers/remoteproc/remoteproc_core.c     | 62 ++++++++++++++++--------
 drivers/remoteproc/remoteproc_internal.h | 15 ++++++
 include/linux/remoteproc.h               | 16 ++++++
 3 files changed, 74 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH V7 1/2] remoteproc: introduce rproc features

[Peng Fan (OSS)]

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

remote processor may support:
 - boot recovery with help from main processor
 - self recovery without help from main processor
 - iommu
 - etc

Introduce rproc features could simplify code to avoid adding more bool
flags

Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_internal.h | 15 +++++++++++++++
 include/linux/remoteproc.h               | 16 ++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 72d4d3d7d94d..82247cb5d0c2 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,21 @@ struct rproc_debug_trace {
 	struct rproc_mem_entry trace_mem;
 };
 
+static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature)
+{
+	return test_bit(feature, rproc->features);
+}
+
+static inline int rproc_set_feature(struct rproc *rproc, unsigned int feature)
+{
+	if (feature >= RPROC_MAX_FEATURES)
+		return -EINVAL;
+
+	set_bit(feature, rproc->features);
+
+	return 0;
+}
+
 /* from remoteproc_core.c */
 void rproc_release(struct kref *kref);
 irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index aea79c77db0f..18869924e299 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -489,6 +489,20 @@ struct rproc_dump_segment {
 	loff_t offset;
 };
 
+/**
+ * enum rproc_features - features supported
+ *
+ * @RPROC_FEAT_ATTACH_ON_RECOVERY: The remote processor does not need help
+ *				   from Linux to recover, such as firmware
+ *				   loading. Linux just needs to attach after
+ *				   recovery.
+ */
+
+enum rproc_features {
+	RPROC_FEAT_ATTACH_ON_RECOVERY,
+	RPROC_MAX_FEATURES,
+};
+
 /**
  * struct rproc - represents a physical remote processor device
  * @node: list node of this rproc object
@@ -530,6 +544,7 @@ struct rproc_dump_segment {
  * @elf_machine: firmware ELF machine
  * @cdev: character device of the rproc
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
+ * @features: indicate remoteproc features
  */
 struct rproc {
 	struct list_head node;
@@ -570,6 +585,7 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
 };
 
 /**
-- 
2.25.1

[PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Peng Fan (OSS)]

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

Current logic only support main processor to stop/start the remote
processor after crash. However to SoC, such as i.MX8QM/QXP, the
remote processor could do attach recovery after crash and trigger watchdog
to reboot itself. It does not need main processor to load image, or
stop/start remote processor.

Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
for the two cases. Boot recovery is as before, let main processor to
help recovery, while attach recovery is to recover itself without help.
To attach recovery, we only do detach and attach.

Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 62 +++++++++++++++++++---------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index ed374c8bf14a..ef5b9310bc83 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
 	return 0;
 }
 
+static int rproc_attach_recovery(struct rproc *rproc)
+{
+	int ret;
+
+	ret = __rproc_detach(rproc);
+	if (ret)
+		return ret;
+
+	return __rproc_attach(rproc);
+}
+
+static int rproc_boot_recovery(struct rproc *rproc)
+{
+	const struct firmware *firmware_p;
+	struct device *dev = &rproc->dev;
+	int ret;
+
+	ret = rproc_stop(rproc, true);
+	if (ret)
+		return ret;
+
+	/* generate coredump */
+	rproc->ops->coredump(rproc);
+
+	/* load firmware */
+	ret = request_firmware(&firmware_p, rproc->firmware, dev);
+	if (ret < 0) {
+		dev_err(dev, "request_firmware failed: %d\n", ret);
+		return ret;
+	}
+
+	/* boot the remote processor up again */
+	ret = rproc_start(rproc, firmware_p);
+
+	release_firmware(firmware_p);
+
+	return ret;
+}
+
 /**
  * rproc_trigger_recovery() - recover a remoteproc
  * @rproc: the remote processor
@@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
  */
 int rproc_trigger_recovery(struct rproc *rproc)
 {
-	const struct firmware *firmware_p;
 	struct device *dev = &rproc->dev;
 	int ret;
 
@@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc *rproc)
 
 	dev_err(dev, "recovering %s\n", rproc->name);
 
-	ret = rproc_stop(rproc, true);
-	if (ret)
-		goto unlock_mutex;
-
-	/* generate coredump */
-	rproc->ops->coredump(rproc);
-
-	/* load firmware */
-	ret = request_firmware(&firmware_p, rproc->firmware, dev);
-	if (ret < 0) {
-		dev_err(dev, "request_firmware failed: %d\n", ret);
-		goto unlock_mutex;
-	}
-
-	/* boot the remote processor up again */
-	ret = rproc_start(rproc, firmware_p);
-
-	release_firmware(firmware_p);
+	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
+		ret = rproc_attach_recovery(rproc);
+	else
+		ret = rproc_boot_recovery(rproc);
 
 unlock_mutex:
 	mutex_unlock(&rproc->lock);
-- 
2.25.1

RE: [PATCH V7 0/2] remoteproc: support self recovery

[Peng Fan]

Hi Bjorn, Mathieu

It almost two and a half month until now, I not got any response.
You accept or not accept or suggestion? 

In V6, I got a typo  comment from Arnaud, then Mathieu replied
dropped.

In V7, I add A-b from Arnaud and no more response.

I not understand why ignoring me.

Regards,
Peng.

> Subject: [PATCH V7 0/2] remoteproc: support self recovery
> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> V7:
>  Per comments from Arnaud Pouliquen:
>    Typo fixes
>    Added A-b tag
> 
> V6:
>  Rename rproc_firmware_recovery to rproc_boot_recovery  Drop the
> unlock/lock when do reproc_attach_recovery
> 
> V5:
>  Rename RPROC_FEAT_ATTACH_RECOVERY to
> RPROC_FEAT_ATTACH_ON_RECOVERY  Add kerneldoc for rproc features
> Change rproc_set_feature to return int type and add a max feature check
> Use __rproc_detach and __rproc_attach when do attach recovery
> https://patchwork.kernel.org/project/linux-
> remoteproc/cover/20220615032048.465486-1-peng.fan@oss.nxp.com/
> 
> V4:
>   Based on Bjorn's comments on V2-2
>   Move the rproc_has_feature/rproc_set_feature to remoteproc_internal.h
> and  Keep rproc_features still in remoteproc.h, because we use
> RPROC_MAX_FEATURES to declare bitmap.
>   Update commit log for patch 2/2, and add comments
> 
>   https://patchwork.kernel.org/project/linux-
> remoteproc/cover/20220323034405.976643-1-peng.fan@oss.nxp.com/
> 
> V3:
>  Resend the wrong labeled patchset
>  https://patchwork.kernel.org/project/linux-
> remoteproc/list/?series=621311
> 
>  Write a cover-letter
>  To i.MX8QM/QXP, they have a M4 core self-recovery capability without
> Linux loading firmware. The self recovery is done by  SCU(System Control
> Unit). Current remoteproc framework only support Linux  help recovery
> remote processor(stop, loading firmware, start). This  patchset is support
> remote processor self recovery(attach recovery).
> 
>  In order to avoid introducing a new variable(bool support_self_recovery),
> patch 1 introduce a new function, rproc_has_feature to make code easy to
> extend, cleaner, such as we could move "bool has_iommu" to
> rproc_has_feature(rproc, RPROC_FEAT_IOMMU).
> 
>  Patch 2 is introduce a new function rproc_attach_recovery for  self recovery,
> the original logic move to rproc_firmware_recovery meaning  needs linux to
> help recovery.
> 
>  V2-version 2:
>  https://patchwork.kernel.org/project/linux-
> remoteproc/list/?series=621311
>  Introduce rproc_has_feature
> 
>  V2-version 1:
>  https://patchwork.kernel.org/project/linux-
> remoteproc/patch/20220126085120.3397450-1-peng.fan@oss.nxp.com/
>  Nothing change in V2.
>  Only move this patch out from
>  https://patchwork.kernel.org/project/linux-
> remoteproc/list/?series=604364
> 
> 
> Peng Fan (2):
>   remoteproc: introduce rproc features
>   remoteproc: support attach recovery after rproc crash
> 
>  drivers/remoteproc/remoteproc_core.c     | 62 ++++++++++++++++--------
>  drivers/remoteproc/remoteproc_internal.h | 15 ++++++
>  include/linux/remoteproc.h               | 16 ++++++
>  3 files changed, 74 insertions(+), 19 deletions(-)
> 
> --
> 2.25.1

RE: [PATCH V7 0/2] remoteproc: support self recovery

[Peng Fan]

Correct Bjorn's mail address.

> Subject: RE: [PATCH V7 0/2] remoteproc: support self recovery
> 
> Hi Bjorn, Mathieu
> 
> It almost two and a half month until now, I not got any response.
> You accept or not accept or suggestion?
> 
> In V6, I got a typo  comment from Arnaud, then Mathieu replied dropped.
> 
> In V7, I add A-b from Arnaud and no more response.
> 
> I not understand why ignoring me.
> 

Regards,
Peng.
> Regards,
> Peng.
> 
> > Subject: [PATCH V7 0/2] remoteproc: support self recovery
> >
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > V7:
> >  Per comments from Arnaud Pouliquen:
> >    Typo fixes
> >    Added A-b tag
> >
> > V6:
> >  Rename rproc_firmware_recovery to rproc_boot_recovery  Drop the
> > unlock/lock when do reproc_attach_recovery
> >
> > V5:
> >  Rename RPROC_FEAT_ATTACH_RECOVERY to
> > RPROC_FEAT_ATTACH_ON_RECOVERY  Add kerneldoc for rproc features
> Change
> > rproc_set_feature to return int type and add a max feature check Use
> > __rproc_detach and __rproc_attach when do attach recovery
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0 remoteproc/cover/20220615032048.465486-1-
> peng.fan@oss.nxp.com/
> >
> > V4:
> >   Based on Bjorn's comments on V2-2
> >   Move the rproc_has_feature/rproc_set_feature to
> > remoteproc_internal.h and  Keep rproc_features still in remoteproc.h,
> > because we use RPROC_MAX_FEATURES to declare bitmap.
> >   Update commit log for patch 2/2, and add comments
> >
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0 remoteproc/cover/20220323034405.976643-1-
> peng.fan@oss.nxp.com/
> >
> > V3:
> >  Resend the wrong labeled patchset
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0
> > remoteproc/list/?series=621311
> >
> >  Write a cover-letter
> >  To i.MX8QM/QXP, they have a M4 core self-recovery capability without
> > Linux loading firmware. The self recovery is done by  SCU(System
> > Control Unit). Current remoteproc framework only support Linux  help
> > recovery remote processor(stop, loading firmware, start). This
> > patchset is support remote processor self recovery(attach recovery).
> >
> >  In order to avoid introducing a new variable(bool
> > support_self_recovery), patch 1 introduce a new function,
> > rproc_has_feature to make code easy to extend, cleaner, such as we
> > could move "bool has_iommu" to rproc_has_feature(rproc,
> RPROC_FEAT_IOMMU).
> >
> >  Patch 2 is introduce a new function rproc_attach_recovery for  self
> > recovery, the original logic move to rproc_firmware_recovery meaning
> > needs linux to help recovery.
> >
> >  V2-version 2:
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0
> > remoteproc/list/?series=621311
> >  Introduce rproc_has_feature
> >
> >  V2-version 1:
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0 remoteproc/patch/20220126085120.3397450-1-
> peng.fan@oss.nxp.com/
> >  Nothing change in V2.
> >  Only move this patch out from
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> &amp;data=05%7C01%7Cpeng.fan%40nxp.
> >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> d99c5c301
> >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wL
> >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> %7C%7C&
> >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> mp;reserve
> > d=0
> > remoteproc/list/?series=604364
> >
> >
> > Peng Fan (2):
> >   remoteproc: introduce rproc features
> >   remoteproc: support attach recovery after rproc crash
> >
> >  drivers/remoteproc/remoteproc_core.c     | 62 ++++++++++++++++--------
> >  drivers/remoteproc/remoteproc_internal.h | 15 ++++++
> >  include/linux/remoteproc.h               | 16 ++++++
> >  3 files changed, 74 insertions(+), 19 deletions(-)
> >
> > --
> > 2.25.1

Re: [PATCH V7 0/2] remoteproc: support self recovery

[Mathieu Poirier]

On Tue, Sep 20, 2022 at 06:34:18AM +0000, Peng Fan wrote:
> Correct Bjorn's mail address.
> 
> > Subject: RE: [PATCH V7 0/2] remoteproc: support self recovery
> > 
> > Hi Bjorn, Mathieu
> > 
> > It almost two and a half month until now, I not got any response.
> > You accept or not accept or suggestion?
> > 
> > In V6, I got a typo  comment from Arnaud, then Mathieu replied dropped.
> > 
> > In V7, I add A-b from Arnaud and no more response.
> > 
> > I not understand why ignoring me.

If you were subscribed to the remoteproc mailing list you would see that every
two weeks I send an email that lists the patchsets in my queue and the order in
which I intend to review them.

That helps people know what kind of traffic maintainers currently deal with. If
a patchset is not listed on that email it is likely because it slipped through
the cracks, as it is the case with this set.  You did not notify me and as such
it was impossible for me to know about it.

Last but not least I do not recall you reviewing a single patchset on this list
since the beginning of the year, which automatically negate your right to
complain about how long it takes for patches to be reviewed.

> > 
> 
> Regards,
> Peng.
> > Regards,
> > Peng.
> > 
> > > Subject: [PATCH V7 0/2] remoteproc: support self recovery
> > >
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > V7:
> > >  Per comments from Arnaud Pouliquen:
> > >    Typo fixes
> > >    Added A-b tag
> > >
> > > V6:
> > >  Rename rproc_firmware_recovery to rproc_boot_recovery  Drop the
> > > unlock/lock when do reproc_attach_recovery
> > >
> > > V5:
> > >  Rename RPROC_FEAT_ATTACH_RECOVERY to
> > > RPROC_FEAT_ATTACH_ON_RECOVERY  Add kerneldoc for rproc features
> > Change
> > > rproc_set_feature to return int type and add a max feature check Use
> > > __rproc_detach and __rproc_attach when do attach recovery
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0 remoteproc/cover/20220615032048.465486-1-
> > peng.fan@oss.nxp.com/
> > >
> > > V4:
> > >   Based on Bjorn's comments on V2-2
> > >   Move the rproc_has_feature/rproc_set_feature to
> > > remoteproc_internal.h and  Keep rproc_features still in remoteproc.h,
> > > because we use RPROC_MAX_FEATURES to declare bitmap.
> > >   Update commit log for patch 2/2, and add comments
> > >
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0 remoteproc/cover/20220323034405.976643-1-
> > peng.fan@oss.nxp.com/
> > >
> > > V3:
> > >  Resend the wrong labeled patchset
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0
> > > remoteproc/list/?series=621311
> > >
> > >  Write a cover-letter
> > >  To i.MX8QM/QXP, they have a M4 core self-recovery capability without
> > > Linux loading firmware. The self recovery is done by  SCU(System
> > > Control Unit). Current remoteproc framework only support Linux  help
> > > recovery remote processor(stop, loading firmware, start). This
> > > patchset is support remote processor self recovery(attach recovery).
> > >
> > >  In order to avoid introducing a new variable(bool
> > > support_self_recovery), patch 1 introduce a new function,
> > > rproc_has_feature to make code easy to extend, cleaner, such as we
> > > could move "bool has_iommu" to rproc_has_feature(rproc,
> > RPROC_FEAT_IOMMU).
> > >
> > >  Patch 2 is introduce a new function rproc_attach_recovery for  self
> > > recovery, the original logic move to rproc_firmware_recovery meaning
> > > needs linux to help recovery.
> > >
> > >  V2-version 2:
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0
> > > remoteproc/list/?series=621311
> > >  Introduce rproc_has_feature
> > >
> > >  V2-version 1:
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0 remoteproc/patch/20220126085120.3397450-1-
> > peng.fan@oss.nxp.com/
> > >  Nothing change in V2.
> > >  Only move this patch out from
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > >
> > com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > d99c5c301
> > >
> > 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > yJWIjoiMC4wL
> > >
> > jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > %7C%7C&
> > >
> > amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > mp;reserve
> > > d=0
> > > remoteproc/list/?series=604364
> > >
> > >
> > > Peng Fan (2):
> > >   remoteproc: introduce rproc features
> > >   remoteproc: support attach recovery after rproc crash
> > >
> > >  drivers/remoteproc/remoteproc_core.c     | 62 ++++++++++++++++--------
> > >  drivers/remoteproc/remoteproc_internal.h | 15 ++++++
> > >  include/linux/remoteproc.h               | 16 ++++++
> > >  3 files changed, 74 insertions(+), 19 deletions(-)
> > >
> > > --
> > > 2.25.1
> 

RE: [PATCH V7 0/2] remoteproc: support self recovery

[Peng Fan]

> Subject: Re: [PATCH V7 0/2] remoteproc: support self recovery
> 
> On Tue, Sep 20, 2022 at 06:34:18AM +0000, Peng Fan wrote:
> > Correct Bjorn's mail address.
> >
> > > Subject: RE: [PATCH V7 0/2] remoteproc: support self recovery
> > >
> > > Hi Bjorn, Mathieu
> > >
> > > It almost two and a half month until now, I not got any response.
> > > You accept or not accept or suggestion?
> > >
> > > In V6, I got a typo  comment from Arnaud, then Mathieu replied
> dropped.
> > >
> > > In V7, I add A-b from Arnaud and no more response.
> > >
> > > I not understand why ignoring me.
> 
> If you were subscribed to the remoteproc mailing list you would see that
> every two weeks I send an email that lists the patchsets in my queue and
> the order in which I intend to review them.
> 
> That helps people know what kind of traffic maintainers currently deal with.
> If a patchset is not listed on that email it is likely because it slipped through
> the cracks, as it is the case with this set.  You did not notify me and as such it
> was impossible for me to know about it.

Well.  In future, I will ping you if patch is not on your list.

> 
> Last but not least I do not recall you reviewing a single patchset on this list
> since the beginning of the year, which automatically negate your right to
> complain about how long it takes for patches to be reviewed.

ok,  I get your point. I could help review imx specific part and
remoterpoc common part, for other soc specific support, I may not
able to help.

Thanks,
Peng.

> 
> > >
> >
> > Regards,
> > Peng.
> > > Regards,
> > > Peng.
> > >
> > > > Subject: [PATCH V7 0/2] remoteproc: support self recovery
> > > >
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > >
> > > > V7:
> > > >  Per comments from Arnaud Pouliquen:
> > > >    Typo fixes
> > > >    Added A-b tag
> > > >
> > > > V6:
> > > >  Rename rproc_firmware_recovery to rproc_boot_recovery  Drop the
> > > > unlock/lock when do reproc_attach_recovery
> > > >
> > > > V5:
> > > >  Rename RPROC_FEAT_ATTACH_RECOVERY to
> > > > RPROC_FEAT_ATTACH_ON_RECOVERY  Add kerneldoc for rproc
> features
> > > Change
> > > > rproc_set_feature to return int type and add a max feature check
> > > > Use __rproc_detach and __rproc_attach when do attach recovery
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0 remoteproc/cover/20220615032048.465486-1-
> > > peng.fan@oss.nxp.com/
> > > >
> > > > V4:
> > > >   Based on Bjorn's comments on V2-2
> > > >   Move the rproc_has_feature/rproc_set_feature to
> > > > remoteproc_internal.h and  Keep rproc_features still in
> > > > remoteproc.h, because we use RPROC_MAX_FEATURES to declare
> bitmap.
> > > >   Update commit log for patch 2/2, and add comments
> > > >
> > > >
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0 remoteproc/cover/20220323034405.976643-1-
> > > peng.fan@oss.nxp.com/
> > > >
> > > > V3:
> > > >  Resend the wrong labeled patchset
> > > >
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0
> > > > remoteproc/list/?series=621311
> > > >
> > > >  Write a cover-letter
> > > >  To i.MX8QM/QXP, they have a M4 core self-recovery capability
> > > > without Linux loading firmware. The self recovery is done by
> > > > SCU(System Control Unit). Current remoteproc framework only
> > > > support Linux  help recovery remote processor(stop, loading
> > > > firmware, start). This patchset is support remote processor self
> recovery(attach recovery).
> > > >
> > > >  In order to avoid introducing a new variable(bool
> > > > support_self_recovery), patch 1 introduce a new function,
> > > > rproc_has_feature to make code easy to extend, cleaner, such as we
> > > > could move "bool has_iommu" to rproc_has_feature(rproc,
> > > RPROC_FEAT_IOMMU).
> > > >
> > > >  Patch 2 is introduce a new function rproc_attach_recovery for
> > > > self recovery, the original logic move to rproc_firmware_recovery
> > > > meaning needs linux to help recovery.
> > > >
> > > >  V2-version 2:
> > > >
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0
> > > > remoteproc/list/?series=621311
> > > >  Introduce rproc_has_feature
> > > >
> > > >  V2-version 1:
> > > >
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0 remoteproc/patch/20220126085120.3397450-1-
> > > peng.fan@oss.nxp.com/
> > > >  Nothing change in V2.
> > > >  Only move this patch out from
> > > >
> > > >
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa
> > > tc
> > > > hwork.kernel.org%2Fproject%2Flinux-
> > > &amp;data=05%7C01%7Cpeng.fan%40nxp.
> > > >
> > >
> com%7C9dfde1d27090471a892208da9ab7bede%7C686ea1d3bc2b4c6fa92c
> > > d99c5c301
> > > >
> > >
> 635%7C0%7C0%7C637992411213950582%7CUnknown%7CTWFpbGZsb3d8e
> > > yJWIjoiMC4wL
> > > >
> > >
> jAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C
> > > %7C%7C&
> > > >
> > >
> amp;sdata=LHIDdh2f%2BKYD2dJmiEqBGV0D5p4qgiay5KhvuTdDjHQ%3D&a
> > > mp;reserve
> > > > d=0
> > > > remoteproc/list/?series=604364
> > > >
> > > >
> > > > Peng Fan (2):
> > > >   remoteproc: introduce rproc features
> > > >   remoteproc: support attach recovery after rproc crash
> > > >
> > > >  drivers/remoteproc/remoteproc_core.c     | 62 ++++++++++++++++-----
> ---
> > > >  drivers/remoteproc/remoteproc_internal.h | 15 ++++++
> > > >  include/linux/remoteproc.h               | 16 ++++++
> > > >  3 files changed, 74 insertions(+), 19 deletions(-)
> > > >
> > > > --
> > > > 2.25.1
> >

Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Mathieu Poirier]

On Tue, Jul 05, 2022 at 09:15:27AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Current logic only support main processor to stop/start the remote
> processor after crash. However to SoC, such as i.MX8QM/QXP, the
> remote processor could do attach recovery after crash and trigger watchdog
> to reboot itself. It does not need main processor to load image, or
> stop/start remote processor.
> 
> Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
> for the two cases. Boot recovery is as before, let main processor to
> help recovery, while attach recovery is to recover itself without help.
> To attach recovery, we only do detach and attach.
> 
> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/remoteproc/remoteproc_core.c | 62 +++++++++++++++++++---------
>  1 file changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index ed374c8bf14a..ef5b9310bc83 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
>  	return 0;
>  }
>  
> +static int rproc_attach_recovery(struct rproc *rproc)
> +{
> +	int ret;
> +
> +	ret = __rproc_detach(rproc);
> +	if (ret)
> +		return ret;

I thought there was a specific reason to _not_ call rproc->ops->coredump() for
processors that have been attached to but looking at the STM32 and IMX_DSP now, it
would seem logical to do so.  Am I missing something?

And this set will need a rebase.

Thanks,
Mathieu

> +
> +	return __rproc_attach(rproc);
> +}
> +
> +static int rproc_boot_recovery(struct rproc *rproc)
> +{
> +	const struct firmware *firmware_p;
> +	struct device *dev = &rproc->dev;
> +	int ret;
> +
> +	ret = rproc_stop(rproc, true);
> +	if (ret)
> +		return ret;
> +
> +	/* generate coredump */
> +	rproc->ops->coredump(rproc);
> +
> +	/* load firmware */
> +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> +	if (ret < 0) {
> +		dev_err(dev, "request_firmware failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* boot the remote processor up again */
> +	ret = rproc_start(rproc, firmware_p);
> +
> +	release_firmware(firmware_p);
> +
> +	return ret;
> +}
> +
>  /**
>   * rproc_trigger_recovery() - recover a remoteproc
>   * @rproc: the remote processor
> @@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
>   */
>  int rproc_trigger_recovery(struct rproc *rproc)
>  {
> -	const struct firmware *firmware_p;
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> @@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc *rproc)
>  
>  	dev_err(dev, "recovering %s\n", rproc->name);
>  
> -	ret = rproc_stop(rproc, true);
> -	if (ret)
> -		goto unlock_mutex;
> -
> -	/* generate coredump */
> -	rproc->ops->coredump(rproc);
> -
> -	/* load firmware */
> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> -	if (ret < 0) {
> -		dev_err(dev, "request_firmware failed: %d\n", ret);
> -		goto unlock_mutex;
> -	}
> -
> -	/* boot the remote processor up again */
> -	ret = rproc_start(rproc, firmware_p);
> -
> -	release_firmware(firmware_p);
> +	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> +		ret = rproc_attach_recovery(rproc);
> +	else
> +		ret = rproc_boot_recovery(rproc);
>  
>  unlock_mutex:
>  	mutex_unlock(&rproc->lock);
> -- 
> 2.25.1
> 

RE: [PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Peng Fan]

Hi Mathieu,

> Subject: Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc
> crash
> 
> On Tue, Jul 05, 2022 at 09:15:27AM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Current logic only support main processor to stop/start the remote
> > processor after crash. However to SoC, such as i.MX8QM/QXP, the remote
> > processor could do attach recovery after crash and trigger watchdog to
> > reboot itself. It does not need main processor to load image, or
> > stop/start remote processor.
> >
> > Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
> > for the two cases. Boot recovery is as before, let main processor to
> > help recovery, while attach recovery is to recover itself without help.
> > To attach recovery, we only do detach and attach.
> >
> > Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/remoteproc/remoteproc_core.c | 62
> > +++++++++++++++++++---------
> >  1 file changed, 43 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > index ed374c8bf14a..ef5b9310bc83 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
> >  	return 0;
> >  }
> >
> > +static int rproc_attach_recovery(struct rproc *rproc) {
> > +	int ret;
> > +
> > +	ret = __rproc_detach(rproc);
> > +	if (ret)
> > +		return ret;
> 
> I thought there was a specific reason to _not_ call rproc->ops->coredump()
> for processors that have been attached to but looking at the STM32 and
> IMX_DSP now, it would seem logical to do so.  Am I missing something?

ATTACH RECOVERY is to support recovery without help from Linux,

STM32 and IMX_DSP, both require linux to load image and start remote
core. So the two cases should not enable feature:
RPROC_FEAT_ATTACH_ON_RECOVERY

Also considering the recovery is out of linux control, actually when linux
start dump, remote core may already recovered. 

> 
> And this set will need a rebase.

I'll do the rebase and send V8 if the upper explanation could eliminate
your concern.

Thanks,
Peng.

> 
> Thanks,
> Mathieu
> 
> > +
> > +	return __rproc_attach(rproc);
> > +}
> > +
> > +static int rproc_boot_recovery(struct rproc *rproc) {
> > +	const struct firmware *firmware_p;
> > +	struct device *dev = &rproc->dev;
> > +	int ret;
> > +
> > +	ret = rproc_stop(rproc, true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* generate coredump */
> > +	rproc->ops->coredump(rproc);
> > +
> > +	/* load firmware */
> > +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "request_firmware failed: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* boot the remote processor up again */
> > +	ret = rproc_start(rproc, firmware_p);
> > +
> > +	release_firmware(firmware_p);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * rproc_trigger_recovery() - recover a remoteproc
> >   * @rproc: the remote processor
> > @@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
> >   */
> >  int rproc_trigger_recovery(struct rproc *rproc)  {
> > -	const struct firmware *firmware_p;
> >  	struct device *dev = &rproc->dev;
> >  	int ret;
> >
> > @@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc
> > *rproc)
> >
> >  	dev_err(dev, "recovering %s\n", rproc->name);
> >
> > -	ret = rproc_stop(rproc, true);
> > -	if (ret)
> > -		goto unlock_mutex;
> > -
> > -	/* generate coredump */
> > -	rproc->ops->coredump(rproc);
> > -
> > -	/* load firmware */
> > -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> > -	if (ret < 0) {
> > -		dev_err(dev, "request_firmware failed: %d\n", ret);
> > -		goto unlock_mutex;
> > -	}
> > -
> > -	/* boot the remote processor up again */
> > -	ret = rproc_start(rproc, firmware_p);
> > -
> > -	release_firmware(firmware_p);
> > +	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> > +		ret = rproc_attach_recovery(rproc);
> > +	else
> > +		ret = rproc_boot_recovery(rproc);
> >
> >  unlock_mutex:
> >  	mutex_unlock(&rproc->lock);
> > --
> > 2.25.1
> >

Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Arnaud POULIQUEN]

Hi,

On 9/27/22 05:03, Peng Fan wrote:
> Hi Mathieu,
> 
>> Subject: Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc
>> crash
>>
>> On Tue, Jul 05, 2022 at 09:15:27AM +0800, Peng Fan (OSS) wrote:
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Current logic only support main processor to stop/start the remote
>>> processor after crash. However to SoC, such as i.MX8QM/QXP, the remote
>>> processor could do attach recovery after crash and trigger watchdog to
>>> reboot itself. It does not need main processor to load image, or
>>> stop/start remote processor.
>>>
>>> Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
>>> for the two cases. Boot recovery is as before, let main processor to
>>> help recovery, while attach recovery is to recover itself without help.
>>> To attach recovery, we only do detach and attach.
>>>
>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>  drivers/remoteproc/remoteproc_core.c | 62
>>> +++++++++++++++++++---------
>>>  1 file changed, 43 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>> b/drivers/remoteproc/remoteproc_core.c
>>> index ed374c8bf14a..ef5b9310bc83 100644
>>> --- a/drivers/remoteproc/remoteproc_core.c
>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>> @@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
>>>  	return 0;
>>>  }
>>>
>>> +static int rproc_attach_recovery(struct rproc *rproc) {
>>> +	int ret;
>>> +
>>> +	ret = __rproc_detach(rproc);
>>> +	if (ret)
>>> +		return ret;
>>
>> I thought there was a specific reason to _not_ call rproc->ops->coredump()
>> for processors that have been attached to but looking at the STM32 and
>> IMX_DSP now, it would seem logical to do so.  Am I missing something?
> 
> ATTACH RECOVERY is to support recovery without help from Linux,
> 
> STM32 and IMX_DSP, both require linux to load image and start remote
> core. So the two cases should not enable feature:
> RPROC_FEAT_ATTACH_ON_RECOVERY
> 
> Also considering the recovery is out of linux control, actually when linux
> start dump, remote core may already recovered. 

I asked myself the same question. Indeed how to manage a core dump if the
remote processor restarts autonomously.
The answer doesn't seem obvious because it seems to be platform specific.

For time being on STM32 we consider that the remoteproc memory can be corrupted
so we don't plan to enable the feature by default even if the hardware allows it.

If we implement it, I would see 2 use cases:
- no core dump, the remote processor restart autonomously (need to manage the
VIRTIO_CONFIG_S_NEEDS_RESET in resource table vdev for resynchronization)
- core dump and the Linux stm32 driver handle the reset of the remote
processor core to be able to perform the core dump (no firmware loading)

What about dealing with the coredump in a separate thread, based on a concrete
use case/need?

Regards,
Arnaud
 
> 
>>
>> And this set will need a rebase.
> 
> I'll do the rebase and send V8 if the upper explanation could eliminate
> your concern.
> 
> Thanks,
> Peng.
> 
>>
>> Thanks,
>> Mathieu
>>
>>> +
>>> +	return __rproc_attach(rproc);
>>> +}
>>> +
>>> +static int rproc_boot_recovery(struct rproc *rproc) {
>>> +	const struct firmware *firmware_p;
>>> +	struct device *dev = &rproc->dev;
>>> +	int ret;
>>> +
>>> +	ret = rproc_stop(rproc, true);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* generate coredump */
>>> +	rproc->ops->coredump(rproc);
>>> +
>>> +	/* load firmware */
>>> +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "request_firmware failed: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	/* boot the remote processor up again */
>>> +	ret = rproc_start(rproc, firmware_p);
>>> +
>>> +	release_firmware(firmware_p);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>>  /**
>>>   * rproc_trigger_recovery() - recover a remoteproc
>>>   * @rproc: the remote processor
>>> @@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
>>>   */
>>>  int rproc_trigger_recovery(struct rproc *rproc)  {
>>> -	const struct firmware *firmware_p;
>>>  	struct device *dev = &rproc->dev;
>>>  	int ret;
>>>
>>> @@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc
>>> *rproc)
>>>
>>>  	dev_err(dev, "recovering %s\n", rproc->name);
>>>
>>> -	ret = rproc_stop(rproc, true);
>>> -	if (ret)
>>> -		goto unlock_mutex;
>>> -
>>> -	/* generate coredump */
>>> -	rproc->ops->coredump(rproc);
>>> -
>>> -	/* load firmware */
>>> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "request_firmware failed: %d\n", ret);
>>> -		goto unlock_mutex;
>>> -	}
>>> -
>>> -	/* boot the remote processor up again */
>>> -	ret = rproc_start(rproc, firmware_p);
>>> -
>>> -	release_firmware(firmware_p);
>>> +	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
>>> +		ret = rproc_attach_recovery(rproc);
>>> +	else
>>> +		ret = rproc_boot_recovery(rproc);
>>>
>>>  unlock_mutex:
>>>  	mutex_unlock(&rproc->lock);
>>> --
>>> 2.25.1
>>>

Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Mathieu Poirier]

On Tue, Sep 27, 2022 at 10:10:31AM +0200, Arnaud POULIQUEN wrote:
> Hi,
> 
> On 9/27/22 05:03, Peng Fan wrote:
> > Hi Mathieu,
> > 
> >> Subject: Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc
> >> crash
> >>
> >> On Tue, Jul 05, 2022 at 09:15:27AM +0800, Peng Fan (OSS) wrote:
> >>> From: Peng Fan <peng.fan@nxp.com>
> >>>
> >>> Current logic only support main processor to stop/start the remote
> >>> processor after crash. However to SoC, such as i.MX8QM/QXP, the remote
> >>> processor could do attach recovery after crash and trigger watchdog to
> >>> reboot itself. It does not need main processor to load image, or
> >>> stop/start remote processor.
> >>>
> >>> Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
> >>> for the two cases. Boot recovery is as before, let main processor to
> >>> help recovery, while attach recovery is to recover itself without help.
> >>> To attach recovery, we only do detach and attach.
> >>>
> >>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>  drivers/remoteproc/remoteproc_core.c | 62
> >>> +++++++++++++++++++---------
> >>>  1 file changed, 43 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >>> b/drivers/remoteproc/remoteproc_core.c
> >>> index ed374c8bf14a..ef5b9310bc83 100644
> >>> --- a/drivers/remoteproc/remoteproc_core.c
> >>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>> @@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static int rproc_attach_recovery(struct rproc *rproc) {
> >>> +	int ret;
> >>> +
> >>> +	ret = __rproc_detach(rproc);
> >>> +	if (ret)
> >>> +		return ret;
> >>
> >> I thought there was a specific reason to _not_ call rproc->ops->coredump()
> >> for processors that have been attached to but looking at the STM32 and
> >> IMX_DSP now, it would seem logical to do so.  Am I missing something?
> > 
> > ATTACH RECOVERY is to support recovery without help from Linux,
> > 
> > STM32 and IMX_DSP, both require linux to load image and start remote
> > core. So the two cases should not enable feature:
> > RPROC_FEAT_ATTACH_ON_RECOVERY
> > 
> > Also considering the recovery is out of linux control, actually when linux
> > start dump, remote core may already recovered. 
> 
> I asked myself the same question. Indeed how to manage a core dump if the
> remote processor restarts autonomously.
> The answer doesn't seem obvious because it seems to be platform specific.
> 
> For time being on STM32 we consider that the remoteproc memory can be corrupted
> so we don't plan to enable the feature by default even if the hardware allows it.
> 
> If we implement it, I would see 2 use cases:
> - no core dump, the remote processor restart autonomously (need to manage the
> VIRTIO_CONFIG_S_NEEDS_RESET in resource table vdev for resynchronization)
> - core dump and the Linux stm32 driver handle the reset of the remote
> processor core to be able to perform the core dump (no firmware loading)
> 
> What about dealing with the coredump in a separate thread, based on a concrete
> use case/need?

Definitely, we can deal with that later.

Peng - please send me a rebase as quickly as possible.

> 
> Regards,
> Arnaud
>  
> > 
> >>
> >> And this set will need a rebase.
> > 
> > I'll do the rebase and send V8 if the upper explanation could eliminate
> > your concern.
> > 
> > Thanks,
> > Peng.
> > 
> >>
> >> Thanks,
> >> Mathieu
> >>
> >>> +
> >>> +	return __rproc_attach(rproc);
> >>> +}
> >>> +
> >>> +static int rproc_boot_recovery(struct rproc *rproc) {
> >>> +	const struct firmware *firmware_p;
> >>> +	struct device *dev = &rproc->dev;
> >>> +	int ret;
> >>> +
> >>> +	ret = rproc_stop(rproc, true);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	/* generate coredump */
> >>> +	rproc->ops->coredump(rproc);
> >>> +
> >>> +	/* load firmware */
> >>> +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> >>> +	if (ret < 0) {
> >>> +		dev_err(dev, "request_firmware failed: %d\n", ret);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	/* boot the remote processor up again */
> >>> +	ret = rproc_start(rproc, firmware_p);
> >>> +
> >>> +	release_firmware(firmware_p);
> >>> +
> >>> +	return ret;
> >>> +}
> >>> +
> >>>  /**
> >>>   * rproc_trigger_recovery() - recover a remoteproc
> >>>   * @rproc: the remote processor
> >>> @@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
> >>>   */
> >>>  int rproc_trigger_recovery(struct rproc *rproc)  {
> >>> -	const struct firmware *firmware_p;
> >>>  	struct device *dev = &rproc->dev;
> >>>  	int ret;
> >>>
> >>> @@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc
> >>> *rproc)
> >>>
> >>>  	dev_err(dev, "recovering %s\n", rproc->name);
> >>>
> >>> -	ret = rproc_stop(rproc, true);
> >>> -	if (ret)
> >>> -		goto unlock_mutex;
> >>> -
> >>> -	/* generate coredump */
> >>> -	rproc->ops->coredump(rproc);
> >>> -
> >>> -	/* load firmware */
> >>> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> >>> -	if (ret < 0) {
> >>> -		dev_err(dev, "request_firmware failed: %d\n", ret);
> >>> -		goto unlock_mutex;
> >>> -	}
> >>> -
> >>> -	/* boot the remote processor up again */
> >>> -	ret = rproc_start(rproc, firmware_p);
> >>> -
> >>> -	release_firmware(firmware_p);
> >>> +	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> >>> +		ret = rproc_attach_recovery(rproc);
> >>> +	else
> >>> +		ret = rproc_boot_recovery(rproc);
> >>>
> >>>  unlock_mutex:
> >>>  	mutex_unlock(&rproc->lock);
> >>> --
> >>> 2.25.1
> >>>

Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc crash

[Peng Fan]

On 9/28/2022 1:44 AM, Mathieu Poirier wrote:
> On Tue, Sep 27, 2022 at 10:10:31AM +0200, Arnaud POULIQUEN wrote:
>> Hi,
>>
>> On 9/27/22 05:03, Peng Fan wrote:
>>> Hi Mathieu,
>>>
>>>> Subject: Re: [PATCH V7 2/2] remoteproc: support attach recovery after rproc
>>>> crash
>>>>
>>>> On Tue, Jul 05, 2022 at 09:15:27AM +0800, Peng Fan (OSS) wrote:
>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>
>>>>> Current logic only support main processor to stop/start the remote
>>>>> processor after crash. However to SoC, such as i.MX8QM/QXP, the remote
>>>>> processor could do attach recovery after crash and trigger watchdog to
>>>>> reboot itself. It does not need main processor to load image, or
>>>>> stop/start remote processor.
>>>>>
>>>>> Introduce two functions: rproc_attach_recovery, rproc_boot_recovery
>>>>> for the two cases. Boot recovery is as before, let main processor to
>>>>> help recovery, while attach recovery is to recover itself without help.
>>>>> To attach recovery, we only do detach and attach.
>>>>>
>>>>> Acked-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
>>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>>> ---
>>>>>   drivers/remoteproc/remoteproc_core.c | 62
>>>>> +++++++++++++++++++---------
>>>>>   1 file changed, 43 insertions(+), 19 deletions(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
>>>>> b/drivers/remoteproc/remoteproc_core.c
>>>>> index ed374c8bf14a..ef5b9310bc83 100644
>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>> @@ -1884,6 +1884,45 @@ static int __rproc_detach(struct rproc *rproc)
>>>>>   	return 0;
>>>>>   }
>>>>>
>>>>> +static int rproc_attach_recovery(struct rproc *rproc) {
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = __rproc_detach(rproc);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>
>>>> I thought there was a specific reason to _not_ call rproc->ops->coredump()
>>>> for processors that have been attached to but looking at the STM32 and
>>>> IMX_DSP now, it would seem logical to do so.  Am I missing something?
>>>
>>> ATTACH RECOVERY is to support recovery without help from Linux,
>>>
>>> STM32 and IMX_DSP, both require linux to load image and start remote
>>> core. So the two cases should not enable feature:
>>> RPROC_FEAT_ATTACH_ON_RECOVERY
>>>
>>> Also considering the recovery is out of linux control, actually when linux
>>> start dump, remote core may already recovered.
>>
>> I asked myself the same question. Indeed how to manage a core dump if the
>> remote processor restarts autonomously.
>> The answer doesn't seem obvious because it seems to be platform specific.
>>
>> For time being on STM32 we consider that the remoteproc memory can be corrupted
>> so we don't plan to enable the feature by default even if the hardware allows it.
>>
>> If we implement it, I would see 2 use cases:
>> - no core dump, the remote processor restart autonomously (need to manage the
>> VIRTIO_CONFIG_S_NEEDS_RESET in resource table vdev for resynchronization)
>> - core dump and the Linux stm32 driver handle the reset of the remote
>> processor core to be able to perform the core dump (no firmware loading)
>>
>> What about dealing with the coredump in a separate thread, based on a concrete
>> use case/need?
> 
> Definitely, we can deal with that later.
> 
> Peng - please send me a rebase as quickly as possible.

Mathieu,

Just send out V8 rebased on linux-next/master tag: next-20220927

Thanks,
Peng.
> 
>>
>> Regards,
>> Arnaud
>>   
>>>
>>>>
>>>> And this set will need a rebase.
>>>
>>> I'll do the rebase and send V8 if the upper explanation could eliminate
>>> your concern.
>>>
>>> Thanks,
>>> Peng.
>>>
>>>>
>>>> Thanks,
>>>> Mathieu
>>>>
>>>>> +
>>>>> +	return __rproc_attach(rproc);
>>>>> +}
>>>>> +
>>>>> +static int rproc_boot_recovery(struct rproc *rproc) {
>>>>> +	const struct firmware *firmware_p;
>>>>> +	struct device *dev = &rproc->dev;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = rproc_stop(rproc, true);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	/* generate coredump */
>>>>> +	rproc->ops->coredump(rproc);
>>>>> +
>>>>> +	/* load firmware */
>>>>> +	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>> +	if (ret < 0) {
>>>>> +		dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>> +		return ret;
>>>>> +	}
>>>>> +
>>>>> +	/* boot the remote processor up again */
>>>>> +	ret = rproc_start(rproc, firmware_p);
>>>>> +
>>>>> +	release_firmware(firmware_p);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>> +
>>>>>   /**
>>>>>    * rproc_trigger_recovery() - recover a remoteproc
>>>>>    * @rproc: the remote processor
>>>>> @@ -1898,7 +1937,6 @@ static int __rproc_detach(struct rproc *rproc)
>>>>>    */
>>>>>   int rproc_trigger_recovery(struct rproc *rproc)  {
>>>>> -	const struct firmware *firmware_p;
>>>>>   	struct device *dev = &rproc->dev;
>>>>>   	int ret;
>>>>>
>>>>> @@ -1912,24 +1950,10 @@ int rproc_trigger_recovery(struct rproc
>>>>> *rproc)
>>>>>
>>>>>   	dev_err(dev, "recovering %s\n", rproc->name);
>>>>>
>>>>> -	ret = rproc_stop(rproc, true);
>>>>> -	if (ret)
>>>>> -		goto unlock_mutex;
>>>>> -
>>>>> -	/* generate coredump */
>>>>> -	rproc->ops->coredump(rproc);
>>>>> -
>>>>> -	/* load firmware */
>>>>> -	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>> -		goto unlock_mutex;
>>>>> -	}
>>>>> -
>>>>> -	/* boot the remote processor up again */
>>>>> -	ret = rproc_start(rproc, firmware_p);
>>>>> -
>>>>> -	release_firmware(firmware_p);
>>>>> +	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
>>>>> +		ret = rproc_attach_recovery(rproc);
>>>>> +	else
>>>>> +		ret = rproc_boot_recovery(rproc);
>>>>>
>>>>>   unlock_mutex:
>>>>>   	mutex_unlock(&rproc->lock);
>>>>> --
>>>>> 2.25.1
>>>>>