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

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

[Peng Fan (OSS)]

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 and let us optimize current code.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 New

 include/linux/remoteproc.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 93a1d0050fbc..51edaf80692c 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -417,6 +417,7 @@ struct rproc_ops {
  *			has attached to it
  * @RPROC_DETACHED:	device has been booted by another entity and waiting
  *			for the core to attach to it
+ * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self recovery
  * @RPROC_LAST:		just keep this one at the end
  *
  * Please note that the values of these states are used as indices
@@ -489,6 +490,11 @@ struct rproc_dump_segment {
 	loff_t offset;
 };
 
+enum rproc_features {
+	RPROC_FEAT_ATTACH_RECOVERY = 0,
+	RPROC_MAX_FEATURES = 32,
+};
+
 /**
  * struct rproc - represents a physical remote processor device
  * @node: list node of this rproc object
@@ -530,6 +536,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,8 +577,19 @@ struct rproc {
 	u16 elf_machine;
 	struct cdev cdev;
 	bool cdev_put_on_release;
+	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
 };
 
+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);
+}
+
 /**
  * struct rproc_subdev - subdevice tied to a remoteproc
  * @node: list node related to the rproc subdevs list
-- 
2.30.0

[PATCH V2 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 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>
---

V2:
 use rproc_has_feature in patch 1/2

 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 69f51acf235e..366fad475898 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.30.0

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

[Mathieu Poirier]

On Mon, 7 Mar 2022 at 23:08, Peng Fan (OSS) <peng.fan@oss.nxp.com> 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>
> ---
>
> V2:
>  use rproc_has_feature in patch 1/2
>
>  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 69f51acf235e..366fad475898 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);

Should I assume this set, which is labeled V2, replaces this other
patch [1] that is also labeled V2, sent out on January 26th?  If so,
why are they both labeled with the same tag and why isn't there a
cover letter to clearly state your intent?  More importantly, why am I
having this conversation with an experienced kernel developer that
should know better?

Any reason I should not move this work to the very bottom of my patch
queue or better yet, simply drop it?

[1]. https://lore.kernel.org/lkml/20220207173456.GA3355405@p14s/t/

>
>  unlock_mutex:
>         mutex_unlock(&rproc->lock);
> --
> 2.30.0
>

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

[Peng Fan]

> Subject: Re: [PATCH V2 2/2] remoteproc: support attach recovery after rproc
> crash
> 
> On Mon, 7 Mar 2022 at 23:08, Peng Fan (OSS) <peng.fan@oss.nxp.com>
> 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>
> > ---
> >
> > V2:
> >  use rproc_has_feature in patch 1/2
> >
> >  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 69f51acf235e..366fad475898 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);
> 
> Should I assume this set, which is labeled V2, replaces this other patch [1]
> that is also labeled V2, sent out on January 26th?  If so, why are they both
> labeled with the same tag and why isn't there a cover letter to clearly state
> your intent?  More importantly, why am I having this conversation with an
> experienced kernel developer that should know better?
> 
> Any reason I should not move this work to the very bottom of my patch queue
> or better yet, simply drop it?

My bad. This patchset should labeled V3. I'll resend the patchset with a cover-letter
and label V3. Thanks for your patience.

Thanks,
Peng.

> 
> [1].
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.ke
> rnel.org%2Flkml%2F20220207173456.GA3355405%40p14s%2Ft%2F&amp;da
> ta=04%7C01%7Cpeng.fan%40nxp.com%7Ccb80ecb9fb3348d1222a08da01fc2
> 6cf%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637824479736
> 071420%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=FIdhBvAUP
> NZDPxzEW6wW%2B0GPzoQ7MUm8IbXc7yq%2BP6w%3D&amp;reserved=0
> 
> >
> >  unlock_mutex:
> >         mutex_unlock(&rproc->lock);
> > --
> > 2.30.0
> >

Re: [PATCH V2 1/2] remoteproc: introduce rproc features

[Bjorn Andersson]

On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote:

> 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 and let us optimize current code.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  New
> 
>  include/linux/remoteproc.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h

This is the API that other parts of the kernel use to interact with a
struct rproc, what feature flags do you have a need for other parts of
the kernel to be able to query?

> index 93a1d0050fbc..51edaf80692c 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -417,6 +417,7 @@ struct rproc_ops {
>   *			has attached to it
>   * @RPROC_DETACHED:	device has been booted by another entity and waiting
>   *			for the core to attach to it
> + * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self recovery

This seems to belong in the other patch...

>   * @RPROC_LAST:		just keep this one at the end
>   *
>   * Please note that the values of these states are used as indices
> @@ -489,6 +490,11 @@ struct rproc_dump_segment {
>  	loff_t offset;
>  };
>  
> +enum rproc_features {
> +	RPROC_FEAT_ATTACH_RECOVERY = 0,

No need to specify that this is bit 0, and the enum will do that for you.

> +	RPROC_MAX_FEATURES = 32,

You're using DECLARE_BITMAP() so why 32?

Regards,
Bjorn

> +};
> +
>  /**
>   * struct rproc - represents a physical remote processor device
>   * @node: list node of this rproc object
> @@ -530,6 +536,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,8 +577,19 @@ struct rproc {
>  	u16 elf_machine;
>  	struct cdev cdev;
>  	bool cdev_put_on_release;
> +	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
>  };
>  
> +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);
> +}
> +
>  /**
>   * struct rproc_subdev - subdevice tied to a remoteproc
>   * @node: list node related to the rproc subdevs list
> -- 
> 2.30.0
> 

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

[Bjorn Andersson]

On Tue 08 Mar 00:48 CST 2022, 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

Does it really do something called "attach recovery and trigger watchdog
reboot"? Doesn't it just reboot itself and Linux needs to detach and
reattach to get something (what?) reset?

> 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>
> ---
> 
> V2:
>  use rproc_has_feature in patch 1/2
> 
>  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 69f51acf235e..366fad475898 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)

In the stop/coredump/start path the code _will_ attempt to recover the
remote processor. With rproc_detach() and rproc_attach() fiddling with
the rproc->power refcount this might do something, or it might not do
something. And with the mutex_unlock() it's likely that you're opening
of up for various race conditions inbetween.


PS. Does anyone actually use this refcount, or are we just all holding
our breath for it never going beyond 1?

Regards,
Bjorn

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

RE: [PATCH V2 1/2] remoteproc: introduce rproc features

[Peng Fan]

> Subject: Re: [PATCH V2 1/2] remoteproc: introduce rproc features
> 
> On Tue 08 Mar 00:48 CST 2022, Peng Fan (OSS) wrote:
> 
> > 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 and let us optimize current code.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V2:
> >  New
> >
> >  include/linux/remoteproc.h | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> 
> This is the API that other parts of the kernel use to interact with a struct rproc,
> what feature flags do you have a need for other parts of the kernel to be able
> to query?

I no need other parts of kernel to query rproc features.
In my V1 patch to support i.MX8QM/QXP self recovery, I introduced a new bool
in "struct rproc", but it is not preferred. So I think to use a enum flag, after
looking into struct rproc, I see the possibility to use one API rproc_has_feature
to simplify code, such as move has_iommu into 
rproc_has_feature(rproc, RPROC_FEAT_IOMMU), and in future, when need to
add new flag/capability of rproc, we could reuse the rproc_has_feature API.

> 
> > index 93a1d0050fbc..51edaf80692c 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -417,6 +417,7 @@ struct rproc_ops {
> >   *			has attached to it
> >   * @RPROC_DETACHED:	device has been booted by another entity and
> waiting
> >   *			for the core to attach to it
> > + * @RPROC_CRASHED_ATTACH_RECOVERY: device has crashed and self
> > + recovery
> 
> This seems to belong in the other patch...

I'll move this to patch 2/2

> 
> >   * @RPROC_LAST:		just keep this one at the end
> >   *
> >   * Please note that the values of these states are used as indices @@
> > -489,6 +490,11 @@ struct rproc_dump_segment {
> >  	loff_t offset;
> >  };
> >
> > +enum rproc_features {
> > +	RPROC_FEAT_ATTACH_RECOVERY = 0,
> 
> No need to specify that this is bit 0, and the enum will do that for you.

sure

> 
> > +	RPROC_MAX_FEATURES = 32,
> 
> You're using DECLARE_BITMAP() so why 32?

Just wanna to limit max features to one int, I may mis use this,
will fix in new version.

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +};
> > +
> >  /**
> >   * struct rproc - represents a physical remote processor device
> >   * @node: list node of this rproc object @@ -530,6 +536,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,8 +577,19 @@ struct rproc {
> >  	u16 elf_machine;
> >  	struct cdev cdev;
> >  	bool cdev_put_on_release;
> > +	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
> >  };
> >
> > +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);
> > +}
> > +
> >  /**
> >   * struct rproc_subdev - subdevice tied to a remoteproc
> >   * @node: list node related to the rproc subdevs list
> > --
> > 2.30.0
> >

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

[Peng Fan]

> Subject: Re: [PATCH V2 2/2] remoteproc: support attach recovery after rproc
> crash
> 
> On Tue 08 Mar 00:48 CST 2022, 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
> 
> Does it really do something called "attach recovery and trigger watchdog
> reboot"? Doesn't it just reboot itself and Linux needs to detach and reattach
> to get something (what?) reset?

I mean the remote processor could re-run without linux to load firmware/stop/
start. Linux side needs to detach/attach to communicate with remote processor.

> 
> > 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>
> > ---
> >
> > V2:
> >  use rproc_has_feature in patch 1/2
> >
> >  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 69f51acf235e..366fad475898 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)
> 
> In the stop/coredump/start path the code _will_ attempt to recover the
> remote processor. With rproc_detach() and rproc_attach() fiddling with the
> rproc->power refcount this might do something, or it might not do something.
> And with the mutex_unlock() it's likely that you're opening of up for various
> race conditions inbetween.

Rproc_boot will inc rproc->power.
Rproc_detach will decrease rproc->power
Rproc_attach not touch rproc->power.

When do attach recovery, the logic is detach->attach. So I add one
inc rproc->power check to avoid count mis-usage.

> 
> 
> PS. Does anyone actually use this refcount, or are we just all holding our
> breath for it never going beyond 1?

I think latter usage.

Thanks,
Peng.

> 
> Regards,
> Bjorn
> 
> > +		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.30.0
> >