* [PATCH V5 0/2] remoteproc: support self recovery
@ 2022-06-15 3:20 Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 1/2] remoteproc: introduce rproc features Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash Peng Fan (OSS)
0 siblings, 2 replies; 5+ messages in thread
From: Peng Fan (OSS) @ 2022-06-15 3:20 UTC (permalink / raw)
To: bjorn.andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
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
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 | 64 +++++++++++++++++-------
drivers/remoteproc/remoteproc_internal.h | 15 ++++++
include/linux/remoteproc.h | 15 ++++++
3 files changed, 75 insertions(+), 19 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH V5 1/2] remoteproc: introduce rproc features
2022-06-15 3:20 [PATCH V5 0/2] remoteproc: support self recovery Peng Fan (OSS)
@ 2022-06-15 3:20 ` Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash Peng Fan (OSS)
1 sibling, 0 replies; 5+ messages in thread
From: Peng Fan (OSS) @ 2022-06-15 3:20 UTC (permalink / raw)
To: bjorn.andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
remote processor may support:
- firmware 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
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/remoteproc/remoteproc_internal.h | 15 +++++++++++++++
include/linux/remoteproc.h | 15 +++++++++++++++
2 files changed, 30 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 7c943f0a2fc4..96b65e7affda 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -489,6 +489,19 @@ struct rproc_dump_segment {
loff_t offset;
};
+/**
+ * enum rproc_features - features supported
+ *
+ * @RPROC_FEAT_ATTACH_ON_RECOVERY: remote processor no need help from Linux
+ * to recover, such as firmware loading.
+ * Linux just need 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 +543,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 +584,7 @@ struct rproc {
u16 elf_machine;
struct cdev cdev;
bool cdev_put_on_release;
+ DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
};
/**
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash
2022-06-15 3:20 [PATCH V5 0/2] remoteproc: support self recovery Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 1/2] remoteproc: introduce rproc features Peng Fan (OSS)
@ 2022-06-15 3:20 ` Peng Fan (OSS)
2022-06-22 17:25 ` Mathieu Poirier
1 sibling, 1 reply; 5+ messages in thread
From: Peng Fan (OSS) @ 2022-06-15 3:20 UTC (permalink / raw)
To: bjorn.andersson, mathieu.poirier; +Cc: linux-remoteproc, linux-kernel, Peng Fan
From: Peng Fan <peng.fan@nxp.com>
Current logic only support main processor to stop/start the remote
processor after rproc crash. However to SoC, such as i.MX8QM/QXP, the
remote processor could do attach recovery after crash and trigger watchdog
reboot. It does not need main processor to load image, or stop/start M4
core.
Introduce two functions: rproc_attach_recovery, rproc_firmware_recovery
for the two cases. Firmware recovery is as before, let main processor to
help recovery, while attach recovery is recover itself withou help.
To attach recovery, we only do detach and attach.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++---------
1 file changed, 45 insertions(+), 19 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 02a04ab34a23..1c1c90176aff 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1883,6 +1883,47 @@ static int __rproc_detach(struct rproc *rproc)
return 0;
}
+static int rproc_attach_recovery(struct rproc *rproc)
+{
+ int ret;
+
+ mutex_unlock(&rproc->lock);
+ ret = __rproc_detach(rproc);
+ mutex_lock(&rproc->lock);
+ if (ret)
+ return ret;
+
+ return __rproc_attach(rproc);
+}
+
+static int rproc_firmware_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
@@ -1897,7 +1938,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;
@@ -1911,24 +1951,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_firmware_recovery(rproc);
unlock_mutex:
mutex_unlock(&rproc->lock);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash
2022-06-15 3:20 ` [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash Peng Fan (OSS)
@ 2022-06-22 17:25 ` Mathieu Poirier
2022-06-22 19:44 ` Peng Fan
0 siblings, 1 reply; 5+ messages in thread
From: Mathieu Poirier @ 2022-06-22 17:25 UTC (permalink / raw)
To: Peng Fan (OSS); +Cc: bjorn.andersson, linux-remoteproc, linux-kernel, Peng Fan
On Wed, Jun 15, 2022 at 11:20:48AM +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 rproc crash. However to SoC, such as i.MX8QM/QXP, the
> remote processor could do attach recovery after crash and trigger watchdog
> reboot. It does not need main processor to load image, or stop/start M4
> core.
>
> Introduce two functions: rproc_attach_recovery, rproc_firmware_recovery
> for the two cases. Firmware recovery is as before, let main processor to
> help recovery, while attach recovery is recover itself withou help.
> To attach recovery, we only do detach and attach.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/remoteproc/remoteproc_core.c | 64 +++++++++++++++++++---------
> 1 file changed, 45 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 02a04ab34a23..1c1c90176aff 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1883,6 +1883,47 @@ static int __rproc_detach(struct rproc *rproc)
> return 0;
> }
>
> +static int rproc_attach_recovery(struct rproc *rproc)
> +{
> + int ret;
> +
> + mutex_unlock(&rproc->lock);
> + ret = __rproc_detach(rproc);
> + mutex_lock(&rproc->lock);
> + if (ret)
> + return ret;
> +
> + return __rproc_attach(rproc);
> +}
> +
> +static int rproc_firmware_recovery(struct rproc *rproc)
s/rproc_firmware_recovery/rproc_boot_recovery
> +{
> + 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
> @@ -1897,7 +1938,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;
>
> @@ -1911,24 +1951,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_firmware_recovery(rproc);
This patch contains a serious flaw related to locking that should have been
obvious when it was put together. Please go back and carefully review the code you are submitting.
I will not consider another revision of this set until July 15th.
Thanks,
Mathieu
>
> unlock_mutex:
> mutex_unlock(&rproc->lock);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash
2022-06-22 17:25 ` Mathieu Poirier
@ 2022-06-22 19:44 ` Peng Fan
0 siblings, 0 replies; 5+ messages in thread
From: Peng Fan @ 2022-06-22 19:44 UTC (permalink / raw)
To: Mathieu Poirier, Peng Fan; +Cc: bjorn.andersson, linux-remoteproc, linux-kernel
Hi Mathieu,
> Subject: Re: [PATCH V5 2/2] remoteproc: support attach recovery after rproc
> crash
>
> On Wed, Jun 15, 2022 at 11:20:48AM +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 rproc crash. However to SoC, such as i.MX8QM/QXP, the
> > remote processor could do attach recovery after crash and trigger
> > watchdog reboot. It does not need main processor to load image, or
> > stop/start M4 core.
> >
> > Introduce two functions: rproc_attach_recovery,
> > rproc_firmware_recovery for the two cases. Firmware recovery is as
> > before, let main processor to help recovery, while attach recovery is recover
> itself withou help.
> > To attach recovery, we only do detach and attach.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > drivers/remoteproc/remoteproc_core.c | 64
> > +++++++++++++++++++---------
> > 1 file changed, 45 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c
> > b/drivers/remoteproc/remoteproc_core.c
> > index 02a04ab34a23..1c1c90176aff 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1883,6 +1883,47 @@ static int __rproc_detach(struct rproc *rproc)
> > return 0;
> > }
> >
> > +static int rproc_attach_recovery(struct rproc *rproc) {
> > + int ret;
> > +
> > + mutex_unlock(&rproc->lock);
> > + ret = __rproc_detach(rproc);
> > + mutex_lock(&rproc->lock);
> > + if (ret)
> > + return ret;
> > +
> > + return __rproc_attach(rproc);
> > +}
> > +
> > +static int rproc_firmware_recovery(struct rproc *rproc)
>
> s/rproc_firmware_recovery/rproc_boot_recovery
>
> > +{
> > + 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
> > @@ -1897,7 +1938,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;
> >
> > @@ -1911,24 +1951,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_firmware_recovery(rproc);
>
> This patch contains a serious flaw related to locking that should have been
> obvious when it was put together. Please go back and carefully review the
> code you are submitting.
I think you mean the following change? In v4, I use rproc_detach, but
I missed to drop the unlock and lock when changing to use __rproc_detach
based on your comments in V4. My bad.
+static int rproc_attach_recovery(struct rproc *rproc)
+{
+ int ret;
+
+ mutex_unlock(&rproc->lock);
+ ret = __rproc_detach(rproc);
+ mutex_lock(&rproc->lock);
+ if (ret)
+ return ret;
+
+ return __rproc_attach(rproc);
+}
I will drop the unlock and lock as below.
static int rproc_attach_recovery(struct rproc *rproc)
{
int ret;
ret = __rproc_detach(rproc);
if (ret)
return ret;
return __rproc_attach(rproc);
}
>
> I will not consider another revision of this set until July 15th.
No problem. Hope until then, my v6 patch would not be just
enter into your queue and be the end one :)
Thanks,
Peng.
>
> Thanks,
> Mathieu
>
> >
> > unlock_mutex:
> > mutex_unlock(&rproc->lock);
> > --
> > 2.25.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-22 19:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-15 3:20 [PATCH V5 0/2] remoteproc: support self recovery Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 1/2] remoteproc: introduce rproc features Peng Fan (OSS)
2022-06-15 3:20 ` [PATCH V5 2/2] remoteproc: support attach recovery after rproc crash Peng Fan (OSS)
2022-06-22 17:25 ` Mathieu Poirier
2022-06-22 19:44 ` Peng Fan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).