From: Peng Fan <[email protected]>
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
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/[email protected]/
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 | 67 +++++++++++++++++-------
drivers/remoteproc/remoteproc_internal.h | 10 ++++
include/linux/remoteproc.h | 7 +++
3 files changed, 65 insertions(+), 19 deletions(-)
--
2.25.1
From: Peng Fan <[email protected]>
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 <[email protected]>
---
drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++--------
1 file changed, 48 insertions(+), 19 deletions(-)
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index c510125769b9..ad04515750ef 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1887,6 +1887,50 @@ 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;
+
+ if (atomic_inc_return(&rproc->power) > 1)
+ return 0;
+
+ 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
@@ -1901,7 +1945,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;
@@ -1915,24 +1958,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_RECOVERY))
+ ret = rproc_attach_recovery(rproc);
+ else
+ ret = rproc_firmware_recovery(rproc);
unlock_mutex:
mutex_unlock(&rproc->lock);
--
2.25.1
From: Peng Fan <[email protected]>
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 <[email protected]>
---
drivers/remoteproc/remoteproc_internal.h | 10 ++++++++++
include/linux/remoteproc.h | 7 +++++++
2 files changed, 17 insertions(+)
diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 72d4d3d7d94d..e9ae743c5587 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -24,6 +24,16 @@ 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 void rproc_set_feature(struct rproc *rproc, unsigned int feature)
+{
+ set_bit(feature, rproc->features);
+}
+
/* 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..7847c6b10a69 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -489,6 +489,11 @@ struct rproc_dump_segment {
loff_t offset;
};
+enum rproc_features {
+ RPROC_FEAT_ATTACH_RECOVERY,
+ RPROC_MAX_FEATURES,
+};
+
/**
* struct rproc - represents a physical remote processor device
* @node: list node of this rproc object
@@ -530,6 +535,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 +576,7 @@ struct rproc {
u16 elf_machine;
struct cdev cdev;
bool cdev_put_on_release;
+ DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
};
/**
--
2.25.1
> Subject: [PATCH V4 0/2] remoteproc: support self recovery
Gentle ping..
Thanks
Peng.
>
> From: Peng Fan <[email protected]>
>
> 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
>
> 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/20220126085
> [email protected]/
> 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 | 67
> +++++++++++++++++-------
> drivers/remoteproc/remoteproc_internal.h | 10 ++++
> include/linux/remoteproc.h | 7 +++
> 3 files changed, 65 insertions(+), 19 deletions(-)
>
> --
> 2.25.1
Mathieu, Bjorn
> Subject: RE: [PATCH V4 0/2] remoteproc: support self recovery
Do you have time to give a look? i.MX8QM/QXP remoteproc
support depends on this feature.
Thanks,
Peng.
>
> > Subject: [PATCH V4 0/2] remoteproc: support self recovery
>
> Gentle ping..
>
> Thanks
> Peng.
>
> >
> > From: Peng Fan <[email protected]>
> >
> > 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
> >
> > V3:
> > Resend the wrong labeled patchset
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> remoteproc%2Flist%2F%3Fseries%3D621
> >
> 311&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> 08da1b
> >
> 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> 263603
> > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> JBTiI6I
> >
> k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5dxwnc716qj%2FQc6TzayLIn
> %2BiMJ8
> > oSwksVUIfmw6atVk%3D&reserved=0
> >
> > 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-
> remoteproc%2Flist%2F%3Fseries%3D621
> >
> 311&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> 08da1b
> >
> 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> 263603
> > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> JBTiI6I
> >
> k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5dxwnc716qj%2FQc6TzayLIn
> %2BiMJ8
> > oSwksVUIfmw6atVk%3D&reserved=0
> > Introduce rproc_has_feature
> >
> > V2-version 1:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > hwork.kernel.org%2Fproject%2Flinux-
> remoteproc%2Fpatch%2F20220126085&am
> >
> p;data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c91312908da1b7
> ad5a8
> > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378525116302636
> 03%7CUnk
> >
> nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> aWw
> >
> iLCJXVCI6Mn0%3D%7C3000&sdata=eMRjdCSvLJrnvtS9QGUQYnr%2BtUPJkS
> RSryJ
> > p6%2BvvfNo%3D&reserved=0
> > [email protected]/
> > 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-
> remoteproc%2Flist%2F%3Fseries%3D604
> >
> 364&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> 08da1b
> >
> 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> 263603
> > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> JBTiI6I
> >
> k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=eJSTzHgK7eMYgLoENZ1dfZds
> uj9O6kj
> > aPyBm61WDXpM%3D&reserved=0
> >
> >
> >
> > Peng Fan (2):
> > remoteproc: introduce rproc features
> > remoteproc: support attach recovery after rproc crash
> >
> > drivers/remoteproc/remoteproc_core.c | 67
> > +++++++++++++++++-------
> > drivers/remoteproc/remoteproc_internal.h | 10 ++++
> > include/linux/remoteproc.h | 7 +++
> > 3 files changed, 65 insertions(+), 19 deletions(-)
> >
> > --
> > 2.25.1
On Wed, 27 Apr 2022 at 02:37, Peng Fan <[email protected]> wrote:
>
> Mathieu, Bjorn
>
> > Subject: RE: [PATCH V4 0/2] remoteproc: support self recovery
>
> Do you have time to give a look? i.MX8QM/QXP remoteproc
> support depends on this feature.
As I have stated before, this feature changes the heart of the
remoteproc core state machine and I want to make sure we get it right.
That in turn will require time that is currently spent reviewing other
patchset on the mailing list. I suggest you start reviewing patches
if you want things to go faster. That will help with the backlog,
which will allow me to look at more complex work such as yours.
>
> Thanks,
> Peng.
>
> >
> > > Subject: [PATCH V4 0/2] remoteproc: support self recovery
> >
> > Gentle ping..
> >
> > Thanks
> > Peng.
> >
> > >
> > > From: Peng Fan <[email protected]>
> > >
> > > 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
> > >
> > > V3:
> > > Resend the wrong labeled patchset
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > remoteproc%2Flist%2F%3Fseries%3D621
> > >
> > 311&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> > 08da1b
> > >
> > 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> > 263603
> > > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> > JBTiI6I
> > >
> > k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5dxwnc716qj%2FQc6TzayLIn
> > %2BiMJ8
> > > oSwksVUIfmw6atVk%3D&reserved=0
> > >
> > > 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-
> > remoteproc%2Flist%2F%3Fseries%3D621
> > >
> > 311&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> > 08da1b
> > >
> > 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> > 263603
> > > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> > JBTiI6I
> > >
> > k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=5dxwnc716qj%2FQc6TzayLIn
> > %2BiMJ8
> > > oSwksVUIfmw6atVk%3D&reserved=0
> > > Introduce rproc_has_feature
> > >
> > > V2-version 1:
> > >
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > > hwork.kernel.org%2Fproject%2Flinux-
> > remoteproc%2Fpatch%2F20220126085&am
> > >
> > p;data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c91312908da1b7
> > ad5a8
> > > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6378525116302636
> > 03%7CUnk
> > >
> > nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1h
> > aWw
> > >
> > iLCJXVCI6Mn0%3D%7C3000&sdata=eMRjdCSvLJrnvtS9QGUQYnr%2BtUPJkS
> > RSryJ
> > > p6%2BvvfNo%3D&reserved=0
> > > [email protected]/
> > > 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-
> > remoteproc%2Flist%2F%3Fseries%3D604
> > >
> > 364&data=04%7C01%7Cpeng.fan%40nxp.com%7C79b37b5586cc4c913129
> > 08da1b
> > >
> > 7ad5a8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637852511630
> > 263603
> > > %7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLC
> > JBTiI6I
> > >
> > k1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=eJSTzHgK7eMYgLoENZ1dfZds
> > uj9O6kj
> > > aPyBm61WDXpM%3D&reserved=0
> > >
> > >
> > >
> > > Peng Fan (2):
> > > remoteproc: introduce rproc features
> > > remoteproc: support attach recovery after rproc crash
> > >
> > > drivers/remoteproc/remoteproc_core.c | 67
> > > +++++++++++++++++-------
> > > drivers/remoteproc/remoteproc_internal.h | 10 ++++
> > > include/linux/remoteproc.h | 7 +++
> > > 3 files changed, 65 insertions(+), 19 deletions(-)
> > >
> > > --
> > > 2.25.1
>
On Wed, 23 Mar 2022 at 03:42, Peng Fan (OSS) <[email protected]> wrote:
>
> From: Peng Fan <[email protected]>
>
> 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
I have received your patches but there is a significant backlog to go
through before I can take a look at them.
Thanks,
Mathieu
>
> 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/[email protected]/
> 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 | 67 +++++++++++++++++-------
> drivers/remoteproc/remoteproc_internal.h | 10 ++++
> include/linux/remoteproc.h | 7 +++
> 3 files changed, 65 insertions(+), 19 deletions(-)
>
> --
> 2.25.1
>
Hi Peng,
On Wed, Mar 23, 2022 at 11:44:04AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/remoteproc/remoteproc_internal.h | 10 ++++++++++
> include/linux/remoteproc.h | 7 +++++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 72d4d3d7d94d..e9ae743c5587 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -24,6 +24,16 @@ 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 void rproc_set_feature(struct rproc *rproc, unsigned int feature)
> +{
I would expect this function to return -EINVAL if feature is >
RPROC_MAX_FEATURES.
> + set_bit(feature, rproc->features);
> +}
> +
> /* 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..7847c6b10a69 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -489,6 +489,11 @@ struct rproc_dump_segment {
> loff_t offset;
> };
>
Please add proper kernel documentation.
> +enum rproc_features {
> + RPROC_FEAT_ATTACH_RECOVERY,
s/RPROC_FEAT_ATTACH_RECOVERY/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 +535,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 +576,7 @@ struct rproc {
> u16 elf_machine;
> struct cdev cdev;
> bool cdev_put_on_release;
> + DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> };
>
> /**
> --
> 2.25.1
>
On Wed, Mar 23, 2022 at 11:44:05AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++--------
> 1 file changed, 48 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index c510125769b9..ad04515750ef 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1887,6 +1887,50 @@ 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;
> +
> + if (atomic_inc_return(&rproc->power) > 1)
> + return 0;
> +
> + return rproc_attach(rproc);
The correct functions to call here are __rproc_detach() and __rproc_attach() -
they are similar to rproc_stop() and rproc_start() without the parts related to
the firmware image.
And you should not be dealing with rproc->power at this stage, which leaves the
remote processor's state machine in the same way as the start/stop scenario.
That being said, now that we actually count how many instances are requesting
the remote processor to be powered, we might have a problem with rproc->power when
the system is recovered after a crash. But we can address that when we get
there.
Thanks,
Mathieu
> +}
> +
> +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
> @@ -1901,7 +1945,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;
>
> @@ -1915,24 +1958,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_RECOVERY))
> + ret = rproc_attach_recovery(rproc);
> + else
> + ret = rproc_firmware_recovery(rproc);
>
> unlock_mutex:
> mutex_unlock(&rproc->lock);
> --
> 2.25.1
>