[PATCH v3 0/2] remoteproc: xlnx: remote crash recovery

[PATCH v3 0/2] remoteproc: xlnx: remote crash recovery

[Tanmay Shah]

Remote processor can crash or hang during normal execution. Linux
remoteproc framework supports different mechanisms to recover the
remote processor and re-establish the RPMsg communication in such case.

Crash reporting on AMD-Xilinx platform:

1) Using debugfs node

User can report the crash to the core framework via debugfs node using
following command:

echo 1 > /sys/kernel/debug/remoteproc/remoteproc0/crash

2) Remoteproc notify to the host about crash state and crash reason
via the resource table

This is a platform specific method where the remote firmware contains
vendor specific resource to update the crash state and the crash
reason. Then the remote notifies the crash to the host via mailbox
notification. The host then will check this resource on every mbox
notification and reports the crash to the core framework if needed.

Crash recovery mechanism on AMD-Xilnx platform:

There are two mechanisms available to recover the remote processor from
the crash. 1) boot recovery, 2) attach on recovery

Remoteproc core framework will choose proper mechanism based on the
rproc features set by the platform driver.

1) Boot recovery

This is the default mechanism to recover the remote processor.
In this method core framework will first stop the remote processor,
load the firmware again and then starts the remote processor. On
AMD-Xilinx platforms this method is supported. The default coredump
method is supported.

2) Attach on recovery

If RPROC_ATTACH_ON_RECOVERY feature is enabled by the platform driver,
then the core framework will choose this method for recovery.

On versal and later platforms following is the sequence of events expected
during remoteproc crash and attach on recovery:

a) Remoteproc attach/detach flow is working, and RPMsg comm is established
b) Remote processor (RPU) crashed (crash not reported yet)
c) Platform management controller is instructed to stop and reload elf
   on inactive remote processor before reboot (platform specific method)
d) Platform management controller reboots the remote processor
e) Remote processor boots again, and detects previous crash (platform
   specific mechanism to detect the crash)
f) Remote processor Reports crash to the Linux (Host) and wait for
   the recovery.
g) Linux performs full detach and reattach to remote processor.
h) Normal RPMsg communication is established.

It is required to destroy all RPMsg related resources and recreate them
during recovery to establish successful RPMsg communication. To achieve
this complete rproc_detach followed by rproc_boot calls are needed. That
is what this patch-series is fixing along with adding rproc recovery
methods for AMD-Xilinx platforms.

Change log:

Changes in 3: 
  - both rproc_attach_recovery() and
    rproc_boot_recovery() are called the same way.
  - remove unrelated changes
  - %s/kick/mailbox notification/
  - %s/core framework/rproc core framework/
  - fold simple function within zynqmp_r5_handle_rsc().
  - remove spurious change
  - reset crash state after reporting the crash
  - document set and reset of ATTACH_ON_RECOVERY flag
  - set recovery_disabled flag to false
  - check condition rproc->crash_reason != NULL

Changes in v2:
  - use rproc_boot instead of rproc_attach
  - move debug message early in the function
  - clear attach recovery boot flag during detach and stop ops


Tanmay Shah (2):
  remoteproc: core: full attach detach during recovery
  remoteproc: xlnx: add crash detection mechanism

 drivers/remoteproc/remoteproc_core.c    | 33 ++++++--------
 drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
 2 files changed, 72 insertions(+), 21 deletions(-)


base-commit: 85ab651885e1b542ee0bb9ec4642ef0b11716997
-- 
2.34.1

[PATCH v3 1/2] remoteproc: core: full attach detach during recovery

[Tanmay Shah]

Current attach on recovery mechanism loads the clean resource table
during recovery, but doesn't re-allocate the resources. RPMsg
communication will fail after recovery due to this. Fix this
incorrect behavior by doing the full detach and attach of remote
processor during the recovery. This will load the clean resource table
and re-allocate all the resources, which will set up correct vring
information in the resource table.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---

Changes in v3:
 - both rproc_attach_recovery() and
   rproc_boot_recovery() are called the same way.
 - remove unrelated changes

Changes in v2:
 - use rproc_boot instead of rproc_attach
 - move debug message early in the function

 drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index aada2780b343..790ad7c6d12e 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
 {
 	int ret;
 
-	ret = __rproc_detach(rproc);
+	ret = rproc_detach(rproc);
 	if (ret)
 		return ret;
 
-	return __rproc_attach(rproc);
+	return rproc_boot(rproc);
 }
 
 static int rproc_boot_recovery(struct rproc *rproc)
@@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
 	struct device *dev = &rproc->dev;
 	int ret;
 
-	ret = rproc_stop(rproc, true);
+	ret = mutex_lock_interruptible(&rproc->lock);
 	if (ret)
 		return ret;
 
+	ret = rproc_stop(rproc, true);
+	if (ret)
+		goto unlock_mutex;
+
 	/* generate coredump */
 	rproc->ops->coredump(rproc);
 
@@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
 	ret = request_firmware(&firmware_p, rproc->firmware, dev);
 	if (ret < 0) {
 		dev_err(dev, "request_firmware failed: %d\n", ret);
-		return ret;
+		goto unlock_mutex;
 	}
 
 	/* boot the remote processor up again */
@@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
 
 	release_firmware(firmware_p);
 
+unlock_mutex:
+	mutex_unlock(&rproc->lock);
 	return ret;
 }
 
@@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
 int rproc_trigger_recovery(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
-	int ret;
-
-	ret = mutex_lock_interruptible(&rproc->lock);
-	if (ret)
-		return ret;
-
-	/* State could have changed before we got the mutex */
-	if (rproc->state != RPROC_CRASHED)
-		goto unlock_mutex;
 
 	dev_err(dev, "recovering %s\n", rproc->name);
 
 	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
-		ret = rproc_attach_recovery(rproc);
+		return rproc_attach_recovery(rproc);
 	else
-		ret = rproc_boot_recovery(rproc);
-
-unlock_mutex:
-	mutex_unlock(&rproc->lock);
-	return ret;
+		return rproc_boot_recovery(rproc);
 }
 
 /**
@@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
 		return ret;
 	}
 
-	if (rproc->state != RPROC_ATTACHED) {
+	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
 		ret = -EINVAL;
 		goto out;
 	}
-- 
2.34.1

[PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Tanmay Shah]

Remote processor will report the crash reason via the resource table
and notify the host via mailbox notification. The host checks this
crash reason on every mailbox notification from the remote and report
to the rproc core framework. Then the rproc core framework will start
the recovery process.

Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
---

Changes in v3:
  - %s/kick/mailbox notification/
  - %s/core framework/rproc core framework/
  - fold simple function within zynqmp_r5_handle_rsc().
  - remove spurious change
  - reset crash state after reporting the crash
  - document set and reset of ATTACH_ON_RECOVERY flag
  - set recovery_disabled flag to false
  - check condition rproc->crash_reason != NULL

Changes in v2:
  - clear attach recovery boot flag during detach and stop ops

 drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
 1 file changed, 59 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
index bd619a6c42aa..0d831330ea90 100644
--- a/drivers/remoteproc/xlnx_r5_remoteproc.c
+++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
@@ -108,6 +108,10 @@ struct rsc_tbl_data {
 	const uintptr_t rsc_tbl;
 } __packed;
 
+enum fw_vendor_rsc {
+	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
+};
+
 /*
  * Hardcoded TCM bank values. This will stay in driver to maintain backward
  * compatibility with device-tree that does not have TCM information.
@@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
 	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
 };
 
+/**
+ * struct xlnx_rproc_crash_report - resource to know crash status and reason
+ *
+ * @crash_state: if true, the rproc is notifying crash, time to recover
+ * @crash_reason: reason of crash
+ */
+struct xlnx_rproc_crash_report {
+	u32 crash_state;
+	u32 crash_reason;
+} __packed;
+
 /**
  * struct zynqmp_r5_core - remoteproc core's internal data
  *
+ * @crash_report: rproc crash state and reason
  * @rsc_tbl_va: resource table virtual address
  * @sram: Array of sram memories assigned to this core
  * @num_sram: number of sram for this core
@@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
  * @ipi: pointer to mailbox information
  */
 struct zynqmp_r5_core {
+	struct xlnx_rproc_crash_report *crash_report;
 	void __iomem *rsc_tbl_va;
 	struct zynqmp_sram_bank *sram;
 	int num_sram;
@@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
 static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
 {
 	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
+	struct zynqmp_r5_core *r5_core;
+	struct rproc *rproc;
 	struct mbox_info *ipi;
 	size_t len;
 
 	ipi = container_of(cl, struct mbox_info, mbox_cl);
+	r5_core = ipi->r5_core;
+	rproc = r5_core->rproc;
 
 	/* copy data from ipi buffer to r5_core */
 	ipi_msg = (struct zynqmp_ipi_message *)msg;
@@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
 	buf_msg->len = len;
 	memcpy(buf_msg->data, ipi_msg->data, len);
 
+	/* Check for crash only if rproc crash is expected */
+	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
+		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
+			rproc_report_crash(rproc,
+					   r5_core->crash_report->crash_reason);
+			r5_core->crash_report->crash_state = 0;
+			r5_core->crash_report->crash_reason = 0;
+		}
+	}
+
 	/* received and processed interrupt ack */
 	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
 		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
@@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(r5_core->dev, "core force power down failed\n");
 
+	/*
+	 * Clear attach on recovery flag during stop operation. The next state
+	 * of the remote processor is expected to be "Running" state. In this
+	 * state boot recovery method must take place over attach on recovery.
+	 */
+	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
+
 	return ret;
 }
 
@@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
 
 static int zynqmp_r5_attach(struct rproc *rproc)
 {
+	/* Enable attach on recovery method. Clear it during rproc stop. */
+	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
+
 	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
 
 	return 0;
@@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
 	 */
 	zynqmp_r5_rproc_kick(rproc, 0);
 
+	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
+
 	return 0;
 }
 
+static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
+				int offset, int avail)
+{
+	struct zynqmp_r5_core *r5_core = rproc->priv;
+	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
+
+	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
+		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
+	else
+		return RSC_IGNORED;
+
+	return RSC_HANDLED;
+}
+
 static const struct rproc_ops zynqmp_r5_rproc_ops = {
 	.prepare	= zynqmp_r5_rproc_prepare,
 	.unprepare	= zynqmp_r5_rproc_unprepare,
@@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
 	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
 	.attach		= zynqmp_r5_attach,
 	.detach		= zynqmp_r5_detach,
+	.handle_rsc	= zynqmp_r5_handle_rsc,
 };
 
 /**
@@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
 
 	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
 
-	r5_rproc->recovery_disabled = true;
+	r5_rproc->recovery_disabled = false;
 	r5_rproc->has_iommu = false;
 	r5_rproc->auto_boot = false;
 	r5_core = r5_rproc->priv;
-- 
2.34.1

Re: [PATCH v3 1/2] remoteproc: core: full attach detach during recovery

[Bjorn Andersson]

On Mon, Feb 23, 2026 at 10:50:05AM -0800, Tanmay Shah wrote:
> Current attach on recovery mechanism loads the clean resource table
> during recovery, but doesn't re-allocate the resources. RPMsg
> communication will fail after recovery due to this. Fix this
> incorrect behavior by doing the full detach and attach of remote
> processor during the recovery. This will load the clean resource table
> and re-allocate all the resources, which will set up correct vring
> information in the resource table.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> 
> Changes in v3:
>  - both rproc_attach_recovery() and
>    rproc_boot_recovery() are called the same way.
>  - remove unrelated changes
> 
> Changes in v2:
>  - use rproc_boot instead of rproc_attach
>  - move debug message early in the function
> 
>  drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index aada2780b343..790ad7c6d12e 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
>  {
>  	int ret;
>  
> -	ret = __rproc_detach(rproc);
> +	ret = rproc_detach(rproc);
>  	if (ret)
>  		return ret;
>  
> -	return __rproc_attach(rproc);
> +	return rproc_boot(rproc);
>  }
>  
>  static int rproc_boot_recovery(struct rproc *rproc)
> @@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  	struct device *dev = &rproc->dev;
>  	int ret;
>  
> -	ret = rproc_stop(rproc, true);
> +	ret = mutex_lock_interruptible(&rproc->lock);
>  	if (ret)
>  		return ret;
>  
> +	ret = rproc_stop(rproc, true);
> +	if (ret)
> +		goto unlock_mutex;
> +
>  	/* generate coredump */
>  	rproc->ops->coredump(rproc);
>  
> @@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>  	if (ret < 0) {
>  		dev_err(dev, "request_firmware failed: %d\n", ret);
> -		return ret;
> +		goto unlock_mutex;
>  	}
>  
>  	/* boot the remote processor up again */
> @@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  
>  	release_firmware(firmware_p);
>  
> +unlock_mutex:
> +	mutex_unlock(&rproc->lock);
>  	return ret;
>  }
>  
> @@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
>  int rproc_trigger_recovery(struct rproc *rproc)
>  {
>  	struct device *dev = &rproc->dev;
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&rproc->lock);
> -	if (ret)
> -		return ret;
> -
> -	/* State could have changed before we got the mutex */
> -	if (rproc->state != RPROC_CRASHED)
> -		goto unlock_mutex;
>  
>  	dev_err(dev, "recovering %s\n", rproc->name);
>  
>  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> -		ret = rproc_attach_recovery(rproc);
> +		return rproc_attach_recovery(rproc);

rproc_trigger_recovery() can be called either from scheduled work or
directly from the debugfs/sysfs interface, it doesn't seem safe to me to
call rproc_attach_recovery() without ensuring mutual exclusion between
multiple parallel callers.

In fact, I can see the relationship between the commit message and the
changes in rproc_attach_recovery() and rproc_detach(), but I'm not sure
why you need to change rproc_boot_recovery() and
rproc_trigger_recovery(). Perhaps you're just missing some explanation
in the commit message?

Regards,
Bjorn

>  	else
> -		ret = rproc_boot_recovery(rproc);
> -
> -unlock_mutex:
> -	mutex_unlock(&rproc->lock);
> -	return ret;
> +		return rproc_boot_recovery(rproc);
>  }
>  
>  /**
> @@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
>  		return ret;
>  	}
>  
> -	if (rproc->state != RPROC_ATTACHED) {
> +	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
>  		ret = -EINVAL;
>  		goto out;
>  	}
> -- 
> 2.34.1
> 

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Bjorn Andersson]

On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
> Remote processor will report the crash reason via the resource table
> and notify the host via mailbox notification. The host checks this
> crash reason on every mailbox notification from the remote and report
> to the rproc core framework. Then the rproc core framework will start
> the recovery process.
> 
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> ---
> 
> Changes in v3:
>   - %s/kick/mailbox notification/
>   - %s/core framework/rproc core framework/
>   - fold simple function within zynqmp_r5_handle_rsc().
>   - remove spurious change
>   - reset crash state after reporting the crash
>   - document set and reset of ATTACH_ON_RECOVERY flag
>   - set recovery_disabled flag to false
>   - check condition rproc->crash_reason != NULL
> 
> Changes in v2:
>   - clear attach recovery boot flag during detach and stop ops
> 
>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> index bd619a6c42aa..0d831330ea90 100644
> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
>  	const uintptr_t rsc_tbl;
>  } __packed;
>  
> +enum fw_vendor_rsc {
> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,

Given that this is a vendor-specific resource, wouldn't it be nice to
find e.g. XLNX somewhere in the name? Same thing with the enum itself.

> +};
> +
>  /*
>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
>   * compatibility with device-tree that does not have TCM information.
> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
>  };
>  
> +/**
> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
> + *
> + * @crash_state: if true, the rproc is notifying crash, time to recover
> + * @crash_reason: reason of crash
> + */
> +struct xlnx_rproc_crash_report {
> +	u32 crash_state;
> +	u32 crash_reason;
> +} __packed;
> +
>  /**
>   * struct zynqmp_r5_core - remoteproc core's internal data
>   *
> + * @crash_report: rproc crash state and reason
>   * @rsc_tbl_va: resource table virtual address
>   * @sram: Array of sram memories assigned to this core
>   * @num_sram: number of sram for this core
> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>   * @ipi: pointer to mailbox information
>   */
>  struct zynqmp_r5_core {
> +	struct xlnx_rproc_crash_report *crash_report;
>  	void __iomem *rsc_tbl_va;
>  	struct zynqmp_sram_bank *sram;
>  	int num_sram;
> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>  {
>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
> +	struct zynqmp_r5_core *r5_core;
> +	struct rproc *rproc;
>  	struct mbox_info *ipi;
>  	size_t len;
>  
>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
> +	r5_core = ipi->r5_core;
> +	rproc = r5_core->rproc;
>  
>  	/* copy data from ipi buffer to r5_core */
>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>  	buf_msg->len = len;
>  	memcpy(buf_msg->data, ipi_msg->data, len);
>  
> +	/* Check for crash only if rproc crash is expected */
> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {

Nit. I'd prefer the order of these to be swapped...

Compare:

"Check if we have crashed, and if so check that we're in a state where
that makes sense."

vs the way you're ordering this:

"Check if we're in a state, and if in that state we have crashed"


The "have we crashed" question is the most-significant-bit of this
chunk, making that the outermost conditional makes it faster for the
next reader to orient themselves in the code.

> +			rproc_report_crash(rproc,
> +					   r5_core->crash_report->crash_reason);

Are these two value spaces synchronized? crash_reason seems to be a
generic 32-bit number without particular definition, and you pass it
into a enum rproc_crash_type.

I presume the outcome is that you get the string
"crash detected in <name>: type: unknown" in your log for most cases?


In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
For the watchdog bite there isn't much information, but for the fatal
error we have a error string which we print, then we call
rproc_report_crash(FATAL) which results in another "useless" print.

Perhaps we could expand rproc_report_crash() to allow drivers to provide
some information about the crash beyond the enum.

Something like:
	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);

Would that be useful to you? Would it be valuable to turn your
"crash_reason" into a human readable string?

> +			r5_core->crash_report->crash_state = 0;
> +			r5_core->crash_report->crash_reason = 0;
> +		}
> +	}
> +
>  	/* received and processed interrupt ack */
>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
>  	if (ret)
>  		dev_err(r5_core->dev, "core force power down failed\n");
>  
> +	/*
> +	 * Clear attach on recovery flag during stop operation. The next state
> +	 * of the remote processor is expected to be "Running" state. In this
> +	 * state boot recovery method must take place over attach on recovery.
> +	 */
> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> +
>  	return ret;
>  }
>  
> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>  
>  static int zynqmp_r5_attach(struct rproc *rproc)
>  {
> +	/* Enable attach on recovery method. Clear it during rproc stop. */
> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
> +
>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>  
>  	return 0;
> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
>  	 */
>  	zynqmp_r5_rproc_kick(rproc, 0);
>  
> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> +
>  	return 0;
>  }
>  
> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
> +				int offset, int avail)
> +{
> +	struct zynqmp_r5_core *r5_core = rproc->priv;
> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
> +
> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);

I don't think you need the cast.

Regards,
Bjorn

> +	else
> +		return RSC_IGNORED;
> +
> +	return RSC_HANDLED;
> +}
> +
>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
>  	.prepare	= zynqmp_r5_rproc_prepare,
>  	.unprepare	= zynqmp_r5_rproc_unprepare,
> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>  	.attach		= zynqmp_r5_attach,
>  	.detach		= zynqmp_r5_detach,
> +	.handle_rsc	= zynqmp_r5_handle_rsc,
>  };
>  
>  /**
> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>  
>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>  
> -	r5_rproc->recovery_disabled = true;
> +	r5_rproc->recovery_disabled = false;
>  	r5_rproc->has_iommu = false;
>  	r5_rproc->auto_boot = false;
>  	r5_core = r5_rproc->priv;
> -- 
> 2.34.1
> 

Re: [PATCH v3 1/2] remoteproc: core: full attach detach during recovery

[Shah, Tanmay]

Hello,

Thank you for the reviews. My response below:

On 2/23/2026 1:27 PM, Bjorn Andersson wrote:
> On Mon, Feb 23, 2026 at 10:50:05AM -0800, Tanmay Shah wrote:
>> Current attach on recovery mechanism loads the clean resource table
>> during recovery, but doesn't re-allocate the resources. RPMsg
>> communication will fail after recovery due to this. Fix this
>> incorrect behavior by doing the full detach and attach of remote
>> processor during the recovery. This will load the clean resource table
>> and re-allocate all the resources, which will set up correct vring
>> information in the resource table.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> Changes in v3:
>>  - both rproc_attach_recovery() and
>>    rproc_boot_recovery() are called the same way.
>>  - remove unrelated changes
>>
>> Changes in v2:
>>  - use rproc_boot instead of rproc_attach
>>  - move debug message early in the function
>>
>>  drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>> index aada2780b343..790ad7c6d12e 100644
>> --- a/drivers/remoteproc/remoteproc_core.c
>> +++ b/drivers/remoteproc/remoteproc_core.c
>> @@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
>>  {
>>  	int ret;
>>  
>> -	ret = __rproc_detach(rproc);
>> +	ret = rproc_detach(rproc);
>>  	if (ret)
>>  		return ret;
>>  
>> -	return __rproc_attach(rproc);
>> +	return rproc_boot(rproc);
>>  }
>>  
>>  static int rproc_boot_recovery(struct rproc *rproc)
>> @@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>  	struct device *dev = &rproc->dev;
>>  	int ret;
>>  
>> -	ret = rproc_stop(rproc, true);
>> +	ret = mutex_lock_interruptible(&rproc->lock);
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = rproc_stop(rproc, true);
>> +	if (ret)
>> +		goto unlock_mutex;
>> +
>>  	/* generate coredump */
>>  	rproc->ops->coredump(rproc);
>>  
>> @@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>  	if (ret < 0) {
>>  		dev_err(dev, "request_firmware failed: %d\n", ret);
>> -		return ret;
>> +		goto unlock_mutex;
>>  	}
>>  
>>  	/* boot the remote processor up again */
>> @@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>  
>>  	release_firmware(firmware_p);
>>  
>> +unlock_mutex:
>> +	mutex_unlock(&rproc->lock);
>>  	return ret;
>>  }
>>  
>> @@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>  int rproc_trigger_recovery(struct rproc *rproc)
>>  {
>>  	struct device *dev = &rproc->dev;
>> -	int ret;
>> -
>> -	ret = mutex_lock_interruptible(&rproc->lock);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* State could have changed before we got the mutex */
>> -	if (rproc->state != RPROC_CRASHED)
>> -		goto unlock_mutex;
>>  
>>  	dev_err(dev, "recovering %s\n", rproc->name);
>>  
>>  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
>> -		ret = rproc_attach_recovery(rproc);
>> +		return rproc_attach_recovery(rproc);
> 
> rproc_trigger_recovery() can be called either from scheduled work or
> directly from the debugfs/sysfs interface, it doesn't seem safe to me to
> call rproc_attach_recovery() without ensuring mutual exclusion between
> multiple parallel callers.
> 

I think mutual exclusion is still maintained.

> In fact, I can see the relationship between the commit message and the
> changes in rproc_attach_recovery() and rproc_detach(), but I'm not sure
> why you need to change rproc_boot_recovery() and
> rproc_trigger_recovery(). Perhaps you're just missing some explanation
> in the commit message?
> 

Here, I am refactoring how lock is used and that is why I have to modify
rproc_trigger_recovery() and rproc_boot_recovery().

Before:

rproc_trigger_recovery() -> lock() -> __rproc_detach() /
rproc_boot_recovery() -> unlock()

Now, __rproc_detach is replaced with rproc_detach(), which already has
mutual exclusion implemented within the call.

After:

1) for attach recovery
rproc_trigger_recovery() -> rproc_attach_recovery() -> rproc_detach() ->
lock() -> ... -> unlock() -> rproc_boot() -> lock() ... -> unlock()

2) To call rproc_attach_recovery() and rproc_boot_recovery() in the same
manner, I modified rproc_boot_recovery() and introduced mutual exclusion
around it.

If you prefer, I can add commit message explaining this change. This is
only refactoring of the code and no new feature though.
Let me know if something is still missing in the implementation or in
the above explanation.

Thank You,
Tanmay

> Regards,
> Bjorn
> 
>>  	else
>> -		ret = rproc_boot_recovery(rproc);
>> -
>> -unlock_mutex:
>> -	mutex_unlock(&rproc->lock);
>> -	return ret;
>> +		return rproc_boot_recovery(rproc);
>>  }
>>  
>>  /**
>> @@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
>>  		return ret;
>>  	}
>>  
>> -	if (rproc->state != RPROC_ATTACHED) {
>> +	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
>>  		ret = -EINVAL;
>>  		goto out;
>>  	}
>> -- 
>> 2.34.1
>>

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Shah, Tanmay]

On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
>> Remote processor will report the crash reason via the resource table
>> and notify the host via mailbox notification. The host checks this
>> crash reason on every mailbox notification from the remote and report
>> to the rproc core framework. Then the rproc core framework will start
>> the recovery process.
>>
>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>> ---
>>
>> Changes in v3:
>>   - %s/kick/mailbox notification/
>>   - %s/core framework/rproc core framework/
>>   - fold simple function within zynqmp_r5_handle_rsc().
>>   - remove spurious change
>>   - reset crash state after reporting the crash
>>   - document set and reset of ATTACH_ON_RECOVERY flag
>>   - set recovery_disabled flag to false
>>   - check condition rproc->crash_reason != NULL
>>
>> Changes in v2:
>>   - clear attach recovery boot flag during detach and stop ops
>>
>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
>>  1 file changed, 59 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> index bd619a6c42aa..0d831330ea90 100644
>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
>>  	const uintptr_t rsc_tbl;
>>  } __packed;
>>  
>> +enum fw_vendor_rsc {
>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
> 
> Given that this is a vendor-specific resource, wouldn't it be nice to
> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
> 

Ack. I will change name for enum and resource both.

>> +};
>> +
>>  /*
>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
>>   * compatibility with device-tree that does not have TCM information.
>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
>>  };
>>  
>> +/**
>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
>> + *
>> + * @crash_state: if true, the rproc is notifying crash, time to recover
>> + * @crash_reason: reason of crash
>> + */
>> +struct xlnx_rproc_crash_report {
>> +	u32 crash_state;
>> +	u32 crash_reason;
>> +} __packed;
>> +
>>  /**
>>   * struct zynqmp_r5_core - remoteproc core's internal data
>>   *
>> + * @crash_report: rproc crash state and reason
>>   * @rsc_tbl_va: resource table virtual address
>>   * @sram: Array of sram memories assigned to this core
>>   * @num_sram: number of sram for this core
>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>   * @ipi: pointer to mailbox information
>>   */
>>  struct zynqmp_r5_core {
>> +	struct xlnx_rproc_crash_report *crash_report;
>>  	void __iomem *rsc_tbl_va;
>>  	struct zynqmp_sram_bank *sram;
>>  	int num_sram;
>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>  {
>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
>> +	struct zynqmp_r5_core *r5_core;
>> +	struct rproc *rproc;
>>  	struct mbox_info *ipi;
>>  	size_t len;
>>  
>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
>> +	r5_core = ipi->r5_core;
>> +	rproc = r5_core->rproc;
>>  
>>  	/* copy data from ipi buffer to r5_core */
>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>  	buf_msg->len = len;
>>  	memcpy(buf_msg->data, ipi_msg->data, len);
>>  
>> +	/* Check for crash only if rproc crash is expected */
>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
> 
> Nit. I'd prefer the order of these to be swapped...
> 
> Compare:
> 
> "Check if we have crashed, and if so check that we're in a state where
> that makes sense."
> 
> vs the way you're ordering this:
> 
> "Check if we're in a state, and if in that state we have crashed"
> 
> 
> The "have we crashed" question is the most-significant-bit of this
> chunk, making that the outermost conditional makes it faster for the
> next reader to orient themselves in the code.

Ack, that makes sense.

> 
>> +			rproc_report_crash(rproc,
>> +					   r5_core->crash_report->crash_reason);
> 
> Are these two value spaces synchronized? crash_reason seems to be a
> generic 32-bit number without particular definition, and you pass it
> into a enum rproc_crash_type.
> 

Yes, crash_reason is supposed to be enum rproc_crash_type.

> I presume the outcome is that you get the string
> "crash detected in <name>: type: unknown" in your log for most cases?
> 

So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
more reasons would have to go in the "unknown" case.

> 
> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
> For the watchdog bite there isn't much information, but for the fatal
> error we have a error string which we print, then we call
> rproc_report_crash(FATAL) which results in another "useless" print.
> 
> Perhaps we could expand rproc_report_crash() to allow drivers to provide
> some information about the crash beyond the enum.
> 
> Something like:
> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
> 
> Would that be useful to you? Would it be valuable to turn your
> "crash_reason" into a human readable string?
> 

Yes, it is valuable to turn "crash_reason" to human readable string.
Should we leave that part to each driver and not have it in the common
framework?

If we are to refactor rproc_report_crash, then I think following is more
flexible:

rproc_report_crash(rproc, const char *crash_reason_str);

Then each platform driver can print crash reason however they see fit.
We can also avoid printing crash reason two times this way.

If we do this, then crash_reason can be defined for each driver
individually. That's more appropriate as each vendor can have different
enum for crash.

Let me know your thoughts.

>> +			r5_core->crash_report->crash_state = 0;
>> +			r5_core->crash_report->crash_reason = 0;
>> +		}
>> +	}
>> +
>>  	/* received and processed interrupt ack */
>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
>>  	if (ret)
>>  		dev_err(r5_core->dev, "core force power down failed\n");
>>  
>> +	/*
>> +	 * Clear attach on recovery flag during stop operation. The next state
>> +	 * of the remote processor is expected to be "Running" state. In this
>> +	 * state boot recovery method must take place over attach on recovery.
>> +	 */
>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>> +
>>  	return ret;
>>  }
>>  
>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>>  
>>  static int zynqmp_r5_attach(struct rproc *rproc)
>>  {
>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
>> +
>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>>  
>>  	return 0;
>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
>>  	 */
>>  	zynqmp_r5_rproc_kick(rproc, 0);
>>  
>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>> +
>>  	return 0;
>>  }
>>  
>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
>> +				int offset, int avail)
>> +{
>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
>> +
>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
> 
> I don't think you need the cast.
> 
> Regards,
> Bjorn
> 
>> +	else
>> +		return RSC_IGNORED;
>> +
>> +	return RSC_HANDLED;
>> +}
>> +
>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>  	.prepare	= zynqmp_r5_rproc_prepare,
>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>>  	.attach		= zynqmp_r5_attach,
>>  	.detach		= zynqmp_r5_detach,
>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
>>  };
>>  
>>  /**
>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>  
>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>  
>> -	r5_rproc->recovery_disabled = true;
>> +	r5_rproc->recovery_disabled = false;
>>  	r5_rproc->has_iommu = false;
>>  	r5_rproc->auto_boot = false;
>>  	r5_core = r5_rproc->priv;
>> -- 
>> 2.34.1
>>

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Shah, Tanmay]

On 2/23/2026 4:40 PM, Shah, Tanmay wrote:
> 
> 
> On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
>> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
>>> Remote processor will report the crash reason via the resource table
>>> and notify the host via mailbox notification. The host checks this
>>> crash reason on every mailbox notification from the remote and report
>>> to the rproc core framework. Then the rproc core framework will start
>>> the recovery process.
>>>
>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>> ---
>>>
>>> Changes in v3:
>>>   - %s/kick/mailbox notification/
>>>   - %s/core framework/rproc core framework/
>>>   - fold simple function within zynqmp_r5_handle_rsc().
>>>   - remove spurious change
>>>   - reset crash state after reporting the crash
>>>   - document set and reset of ATTACH_ON_RECOVERY flag
>>>   - set recovery_disabled flag to false
>>>   - check condition rproc->crash_reason != NULL
>>>
>>> Changes in v2:
>>>   - clear attach recovery boot flag during detach and stop ops
>>>
>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
>>>  1 file changed, 59 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> index bd619a6c42aa..0d831330ea90 100644
>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
>>>  	const uintptr_t rsc_tbl;
>>>  } __packed;
>>>  
>>> +enum fw_vendor_rsc {
>>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
>>
>> Given that this is a vendor-specific resource, wouldn't it be nice to
>> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
>>
> 
> Ack. I will change name for enum and resource both.
> 
>>> +};
>>> +
>>>  /*
>>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
>>>   * compatibility with device-tree that does not have TCM information.
>>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
>>>  };
>>>  
>>> +/**
>>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
>>> + *
>>> + * @crash_state: if true, the rproc is notifying crash, time to recover
>>> + * @crash_reason: reason of crash
>>> + */
>>> +struct xlnx_rproc_crash_report {
>>> +	u32 crash_state;
>>> +	u32 crash_reason;
>>> +} __packed;
>>> +
>>>  /**
>>>   * struct zynqmp_r5_core - remoteproc core's internal data
>>>   *
>>> + * @crash_report: rproc crash state and reason
>>>   * @rsc_tbl_va: resource table virtual address
>>>   * @sram: Array of sram memories assigned to this core
>>>   * @num_sram: number of sram for this core
>>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>   * @ipi: pointer to mailbox information
>>>   */
>>>  struct zynqmp_r5_core {
>>> +	struct xlnx_rproc_crash_report *crash_report;
>>>  	void __iomem *rsc_tbl_va;
>>>  	struct zynqmp_sram_bank *sram;
>>>  	int num_sram;
>>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
>>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>  {
>>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
>>> +	struct zynqmp_r5_core *r5_core;
>>> +	struct rproc *rproc;
>>>  	struct mbox_info *ipi;
>>>  	size_t len;
>>>  
>>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
>>> +	r5_core = ipi->r5_core;
>>> +	rproc = r5_core->rproc;
>>>  
>>>  	/* copy data from ipi buffer to r5_core */
>>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
>>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>  	buf_msg->len = len;
>>>  	memcpy(buf_msg->data, ipi_msg->data, len);
>>>  
>>> +	/* Check for crash only if rproc crash is expected */
>>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
>>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
>>
>> Nit. I'd prefer the order of these to be swapped...
>>
>> Compare:
>>
>> "Check if we have crashed, and if so check that we're in a state where
>> that makes sense."
>>
>> vs the way you're ordering this:
>>
>> "Check if we're in a state, and if in that state we have crashed"
>>
>>
>> The "have we crashed" question is the most-significant-bit of this
>> chunk, making that the outermost conditional makes it faster for the
>> next reader to orient themselves in the code.
> 
> Ack, that makes sense.
> 
>>
>>> +			rproc_report_crash(rproc,
>>> +					   r5_core->crash_report->crash_reason);
>>
>> Are these two value spaces synchronized? crash_reason seems to be a
>> generic 32-bit number without particular definition, and you pass it
>> into a enum rproc_crash_type.
>>
> 
> Yes, crash_reason is supposed to be enum rproc_crash_type.
> 
>> I presume the outcome is that you get the string
>> "crash detected in <name>: type: unknown" in your log for most cases?
>>
> 
> So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
> more reasons would have to go in the "unknown" case.
> 
>>
>> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
>> For the watchdog bite there isn't much information, but for the fatal
>> error we have a error string which we print, then we call
>> rproc_report_crash(FATAL) which results in another "useless" print.
>>
>> Perhaps we could expand rproc_report_crash() to allow drivers to provide
>> some information about the crash beyond the enum.
>>
>> Something like:
>> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
>>
>> Would that be useful to you? Would it be valuable to turn your
>> "crash_reason" into a human readable string?
>>
> 
> Yes, it is valuable to turn "crash_reason" to human readable string.
> Should we leave that part to each driver and not have it in the common
> framework?
> 
> If we are to refactor rproc_report_crash, then I think following is more
> flexible:
> 
> rproc_report_crash(rproc, const char *crash_reason_str);
> 
> Then each platform driver can print crash reason however they see fit.
> We can also avoid printing crash reason two times this way.
> 

Hi Bjorn,

I take this back. I think crash_reason can be defined differently for
each firmware project. I would like to provide that flexibility to the
firmware developer. Hence, I prefer not to convert crash_reason integer
to human readable string, as can be different for different fw projects.

Instead, the xlnx platform driver will simply print the crash_reason
integer as given by the firmware, and notify the crash to the core
framework as following:

rproc_report_crash(rproc, RPROC_FATAL_ERROR);

This way, we don't have to modify the rproc_report_crash() API.
I hope this makes sense.

I will wait for your response before sending the new version. Rest of
the comments I will address as asked.

Thanks,
Tanmay

> If we do this, then crash_reason can be defined for each driver
> individually. That's more appropriate as each vendor can have different
> enum for crash.
> 
> Let me know your thoughts.
> 
>>> +			r5_core->crash_report->crash_state = 0;
>>> +			r5_core->crash_report->crash_reason = 0;
>>> +		}
>>> +	}
>>> +
>>>  	/* received and processed interrupt ack */
>>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
>>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
>>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
>>>  	if (ret)
>>>  		dev_err(r5_core->dev, "core force power down failed\n");
>>>  
>>> +	/*
>>> +	 * Clear attach on recovery flag during stop operation. The next state
>>> +	 * of the remote processor is expected to be "Running" state. In this
>>> +	 * state boot recovery method must take place over attach on recovery.
>>> +	 */
>>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>> +
>>>  	return ret;
>>>  }
>>>  
>>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>>>  
>>>  static int zynqmp_r5_attach(struct rproc *rproc)
>>>  {
>>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
>>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
>>> +
>>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>>>  
>>>  	return 0;
>>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
>>>  	 */
>>>  	zynqmp_r5_rproc_kick(rproc, 0);
>>>  
>>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
>>> +				int offset, int avail)
>>> +{
>>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
>>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
>>> +
>>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
>>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
>>
>> I don't think you need the cast.
>>
>> Regards,
>> Bjorn
>>
>>> +	else
>>> +		return RSC_IGNORED;
>>> +
>>> +	return RSC_HANDLED;
>>> +}
>>> +
>>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>  	.prepare	= zynqmp_r5_rproc_prepare,
>>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
>>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>>>  	.attach		= zynqmp_r5_attach,
>>>  	.detach		= zynqmp_r5_detach,
>>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
>>>  };
>>>  
>>>  /**
>>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>>  
>>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>>  
>>> -	r5_rproc->recovery_disabled = true;
>>> +	r5_rproc->recovery_disabled = false;
>>>  	r5_rproc->has_iommu = false;
>>>  	r5_rproc->auto_boot = false;
>>>  	r5_core = r5_rproc->priv;
>>> -- 
>>> 2.34.1
>>>
> 

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Bjorn Andersson]

On Wed, Feb 25, 2026 at 11:22:05AM -0600, Shah, Tanmay wrote:
> 
> 
> On 2/23/2026 4:40 PM, Shah, Tanmay wrote:
> > 
> > 
> > On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
> >> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
> >>> Remote processor will report the crash reason via the resource table
> >>> and notify the host via mailbox notification. The host checks this
> >>> crash reason on every mailbox notification from the remote and report
> >>> to the rproc core framework. Then the rproc core framework will start
> >>> the recovery process.
> >>>
> >>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >>> ---
> >>>
> >>> Changes in v3:
> >>>   - %s/kick/mailbox notification/
> >>>   - %s/core framework/rproc core framework/
> >>>   - fold simple function within zynqmp_r5_handle_rsc().
> >>>   - remove spurious change
> >>>   - reset crash state after reporting the crash
> >>>   - document set and reset of ATTACH_ON_RECOVERY flag
> >>>   - set recovery_disabled flag to false
> >>>   - check condition rproc->crash_reason != NULL
> >>>
> >>> Changes in v2:
> >>>   - clear attach recovery boot flag during detach and stop ops
> >>>
> >>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
> >>>  1 file changed, 59 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>> index bd619a6c42aa..0d831330ea90 100644
> >>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
> >>>  	const uintptr_t rsc_tbl;
> >>>  } __packed;
> >>>  
> >>> +enum fw_vendor_rsc {
> >>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
> >>
> >> Given that this is a vendor-specific resource, wouldn't it be nice to
> >> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
> >>
> > 
> > Ack. I will change name for enum and resource both.
> > 
> >>> +};
> >>> +
> >>>  /*
> >>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
> >>>   * compatibility with device-tree that does not have TCM information.
> >>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
> >>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
> >>>  };
> >>>  
> >>> +/**
> >>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
> >>> + *
> >>> + * @crash_state: if true, the rproc is notifying crash, time to recover
> >>> + * @crash_reason: reason of crash
> >>> + */
> >>> +struct xlnx_rproc_crash_report {
> >>> +	u32 crash_state;
> >>> +	u32 crash_reason;
> >>> +} __packed;
> >>> +
> >>>  /**
> >>>   * struct zynqmp_r5_core - remoteproc core's internal data
> >>>   *
> >>> + * @crash_report: rproc crash state and reason
> >>>   * @rsc_tbl_va: resource table virtual address
> >>>   * @sram: Array of sram memories assigned to this core
> >>>   * @num_sram: number of sram for this core
> >>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
> >>>   * @ipi: pointer to mailbox information
> >>>   */
> >>>  struct zynqmp_r5_core {
> >>> +	struct xlnx_rproc_crash_report *crash_report;
> >>>  	void __iomem *rsc_tbl_va;
> >>>  	struct zynqmp_sram_bank *sram;
> >>>  	int num_sram;
> >>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
> >>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
> >>>  {
> >>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
> >>> +	struct zynqmp_r5_core *r5_core;
> >>> +	struct rproc *rproc;
> >>>  	struct mbox_info *ipi;
> >>>  	size_t len;
> >>>  
> >>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
> >>> +	r5_core = ipi->r5_core;
> >>> +	rproc = r5_core->rproc;
> >>>  
> >>>  	/* copy data from ipi buffer to r5_core */
> >>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
> >>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
> >>>  	buf_msg->len = len;
> >>>  	memcpy(buf_msg->data, ipi_msg->data, len);
> >>>  
> >>> +	/* Check for crash only if rproc crash is expected */
> >>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
> >>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
> >>
> >> Nit. I'd prefer the order of these to be swapped...
> >>
> >> Compare:
> >>
> >> "Check if we have crashed, and if so check that we're in a state where
> >> that makes sense."
> >>
> >> vs the way you're ordering this:
> >>
> >> "Check if we're in a state, and if in that state we have crashed"
> >>
> >>
> >> The "have we crashed" question is the most-significant-bit of this
> >> chunk, making that the outermost conditional makes it faster for the
> >> next reader to orient themselves in the code.
> > 
> > Ack, that makes sense.
> > 
> >>
> >>> +			rproc_report_crash(rproc,
> >>> +					   r5_core->crash_report->crash_reason);
> >>
> >> Are these two value spaces synchronized? crash_reason seems to be a
> >> generic 32-bit number without particular definition, and you pass it
> >> into a enum rproc_crash_type.
> >>
> > 
> > Yes, crash_reason is supposed to be enum rproc_crash_type.
> > 
> >> I presume the outcome is that you get the string
> >> "crash detected in <name>: type: unknown" in your log for most cases?
> >>
> > 
> > So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
> > more reasons would have to go in the "unknown" case.
> > 
> >>
> >> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
> >> For the watchdog bite there isn't much information, but for the fatal
> >> error we have a error string which we print, then we call
> >> rproc_report_crash(FATAL) which results in another "useless" print.
> >>
> >> Perhaps we could expand rproc_report_crash() to allow drivers to provide
> >> some information about the crash beyond the enum.
> >>
> >> Something like:
> >> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
> >>
> >> Would that be useful to you? Would it be valuable to turn your
> >> "crash_reason" into a human readable string?
> >>
> > 
> > Yes, it is valuable to turn "crash_reason" to human readable string.
> > Should we leave that part to each driver and not have it in the common
> > framework?
> > 
> > If we are to refactor rproc_report_crash, then I think following is more
> > flexible:
> > 
> > rproc_report_crash(rproc, const char *crash_reason_str);
> > 
> > Then each platform driver can print crash reason however they see fit.
> > We can also avoid printing crash reason two times this way.
> > 
> 
> Hi Bjorn,
> 
> I take this back. I think crash_reason can be defined differently for
> each firmware project. I would like to provide that flexibility to the
> firmware developer. Hence, I prefer not to convert crash_reason integer
> to human readable string, as can be different for different fw projects.
> 

Then we certainly shouldn't pass it as the second argument of
rproc_report_crash().

> Instead, the xlnx platform driver will simply print the crash_reason
> integer as given by the firmware, and notify the crash to the core
> framework as following:
> 
> rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> 
> This way, we don't have to modify the rproc_report_crash() API.
> I hope this makes sense.
> 

Yes, that makes sense.

I think I'd like to make the proposed modification regardless, but that
is then a completely separate change.

> I will wait for your response before sending the new version. Rest of
> the comments I will address as asked.
> 

Is your struct xlnx_rproc_crash_report already defined and in use by the
firmware? If not, I'd recommend that you spend a little bit extra time
thinking about the content of it. E.g. the human readable char [] found
in Qualcomm's crash reports is quite useful...

Regards,
Bjorn

> Thanks,
> Tanmay
> 
> > If we do this, then crash_reason can be defined for each driver
> > individually. That's more appropriate as each vendor can have different
> > enum for crash.
> > 
> > Let me know your thoughts.
> > 
> >>> +			r5_core->crash_report->crash_state = 0;
> >>> +			r5_core->crash_report->crash_reason = 0;
> >>> +		}
> >>> +	}
> >>> +
> >>>  	/* received and processed interrupt ack */
> >>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
> >>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
> >>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> >>>  	if (ret)
> >>>  		dev_err(r5_core->dev, "core force power down failed\n");
> >>>  
> >>> +	/*
> >>> +	 * Clear attach on recovery flag during stop operation. The next state
> >>> +	 * of the remote processor is expected to be "Running" state. In this
> >>> +	 * state boot recovery method must take place over attach on recovery.
> >>> +	 */
> >>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> >>> +
> >>>  	return ret;
> >>>  }
> >>>  
> >>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
> >>>  
> >>>  static int zynqmp_r5_attach(struct rproc *rproc)
> >>>  {
> >>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
> >>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
> >>> +
> >>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
> >>>  
> >>>  	return 0;
> >>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
> >>>  	 */
> >>>  	zynqmp_r5_rproc_kick(rproc, 0);
> >>>  
> >>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> >>> +
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
> >>> +				int offset, int avail)
> >>> +{
> >>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
> >>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
> >>> +
> >>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
> >>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
> >>
> >> I don't think you need the cast.
> >>
> >> Regards,
> >> Bjorn
> >>
> >>> +	else
> >>> +		return RSC_IGNORED;
> >>> +
> >>> +	return RSC_HANDLED;
> >>> +}
> >>> +
> >>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >>>  	.prepare	= zynqmp_r5_rproc_prepare,
> >>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
> >>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
> >>>  	.attach		= zynqmp_r5_attach,
> >>>  	.detach		= zynqmp_r5_detach,
> >>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
> >>>  };
> >>>  
> >>>  /**
> >>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> >>>  
> >>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
> >>>  
> >>> -	r5_rproc->recovery_disabled = true;
> >>> +	r5_rproc->recovery_disabled = false;
> >>>  	r5_rproc->has_iommu = false;
> >>>  	r5_rproc->auto_boot = false;
> >>>  	r5_core = r5_rproc->priv;
> >>> -- 
> >>> 2.34.1
> >>>
> > 
> 

Re: [PATCH v3 1/2] remoteproc: core: full attach detach during recovery

[Bjorn Andersson]

On Mon, Feb 23, 2026 at 03:43:56PM -0600, Shah, Tanmay wrote:
> Hello,
> 
> Thank you for the reviews. My response below:
> 
> On 2/23/2026 1:27 PM, Bjorn Andersson wrote:
> > On Mon, Feb 23, 2026 at 10:50:05AM -0800, Tanmay Shah wrote:
> >> Current attach on recovery mechanism loads the clean resource table
> >> during recovery, but doesn't re-allocate the resources. RPMsg
> >> communication will fail after recovery due to this. Fix this
> >> incorrect behavior by doing the full detach and attach of remote
> >> processor during the recovery. This will load the clean resource table
> >> and re-allocate all the resources, which will set up correct vring
> >> information in the resource table.
> >>
> >> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >> ---
> >>
> >> Changes in v3:
> >>  - both rproc_attach_recovery() and
> >>    rproc_boot_recovery() are called the same way.
> >>  - remove unrelated changes
> >>
> >> Changes in v2:
> >>  - use rproc_boot instead of rproc_attach
> >>  - move debug message early in the function
> >>
> >>  drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
> >>  1 file changed, 13 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> >> index aada2780b343..790ad7c6d12e 100644
> >> --- a/drivers/remoteproc/remoteproc_core.c
> >> +++ b/drivers/remoteproc/remoteproc_core.c
> >> @@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
> >>  {
> >>  	int ret;
> >>  
> >> -	ret = __rproc_detach(rproc);
> >> +	ret = rproc_detach(rproc);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	return __rproc_attach(rproc);
> >> +	return rproc_boot(rproc);
> >>  }
> >>  
> >>  static int rproc_boot_recovery(struct rproc *rproc)
> >> @@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
> >>  	struct device *dev = &rproc->dev;
> >>  	int ret;
> >>  
> >> -	ret = rproc_stop(rproc, true);
> >> +	ret = mutex_lock_interruptible(&rproc->lock);
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> +	ret = rproc_stop(rproc, true);
> >> +	if (ret)
> >> +		goto unlock_mutex;
> >> +
> >>  	/* generate coredump */
> >>  	rproc->ops->coredump(rproc);
> >>  
> >> @@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
> >>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
> >>  	if (ret < 0) {
> >>  		dev_err(dev, "request_firmware failed: %d\n", ret);
> >> -		return ret;
> >> +		goto unlock_mutex;
> >>  	}
> >>  
> >>  	/* boot the remote processor up again */
> >> @@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
> >>  
> >>  	release_firmware(firmware_p);
> >>  
> >> +unlock_mutex:
> >> +	mutex_unlock(&rproc->lock);
> >>  	return ret;
> >>  }
> >>  
> >> @@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
> >>  int rproc_trigger_recovery(struct rproc *rproc)
> >>  {
> >>  	struct device *dev = &rproc->dev;
> >> -	int ret;
> >> -
> >> -	ret = mutex_lock_interruptible(&rproc->lock);
> >> -	if (ret)
> >> -		return ret;
> >> -
> >> -	/* State could have changed before we got the mutex */
> >> -	if (rproc->state != RPROC_CRASHED)
> >> -		goto unlock_mutex;
> >>  
> >>  	dev_err(dev, "recovering %s\n", rproc->name);
> >>  
> >>  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
> >> -		ret = rproc_attach_recovery(rproc);
> >> +		return rproc_attach_recovery(rproc);
> > 
> > rproc_trigger_recovery() can be called either from scheduled work or
> > directly from the debugfs/sysfs interface, it doesn't seem safe to me to
> > call rproc_attach_recovery() without ensuring mutual exclusion between
> > multiple parallel callers.
> > 
> 
> I think mutual exclusion is still maintained.
> 
> > In fact, I can see the relationship between the commit message and the
> > changes in rproc_attach_recovery() and rproc_detach(), but I'm not sure
> > why you need to change rproc_boot_recovery() and
> > rproc_trigger_recovery(). Perhaps you're just missing some explanation
> > in the commit message?
> > 
> 
> Here, I am refactoring how lock is used and that is why I have to modify
> rproc_trigger_recovery() and rproc_boot_recovery().
> 
> Before:
> 
> rproc_trigger_recovery() -> lock() -> __rproc_detach() /
> rproc_boot_recovery() -> unlock()
> 
> Now, __rproc_detach is replaced with rproc_detach(), which already has
> mutual exclusion implemented within the call.
> 
> After:
> 
> 1) for attach recovery
> rproc_trigger_recovery() -> rproc_attach_recovery() -> rproc_detach() ->
> lock() -> ... -> unlock() -> rproc_boot() -> lock() ... -> unlock()
> 

The concern I had was that we're letting others execute inbetween
rproc_detach() and rproc_boot(). But perhaps that's okay?

> 2) To call rproc_attach_recovery() and rproc_boot_recovery() in the same
> manner, I modified rproc_boot_recovery() and introduced mutual exclusion
> around it.
> 

No, in rproc_boot_recovery() you're locking around stop and start. In
rproc_attach_recovery() you're locking within each part.

Looking more at this, I dislike the asymmetry between rproc_detach() vs
rproc_boot(), and how I presume this then relies on rproc_boot() mostly
just calling rproc_attach().

This seems to come from the fact that rproc_attach() and rproc_start()
aren't symmetrical and rproc_detach() and rproc_stop() aren't.

It would be good if we could get this lined up, so that it's not so hard
to reason about the different code paths through the core.


Regardless of this though, the removal of the check for state !=
RPROC_CRASHED (under a lock) means that inbetween
rproc_crash_handler_work() and rproc_trigger_recovery() someone can
write "recover" into the "recovery" debugfs entry and we will recover
twice.

Regards,
Bjorn

> If you prefer, I can add commit message explaining this change. This is
> only refactoring of the code and no new feature though.
> Let me know if something is still missing in the implementation or in
> the above explanation.
> 
> Thank You,
> Tanmay
> 
> > Regards,
> > Bjorn
> > 
> >>  	else
> >> -		ret = rproc_boot_recovery(rproc);
> >> -
> >> -unlock_mutex:
> >> -	mutex_unlock(&rproc->lock);
> >> -	return ret;
> >> +		return rproc_boot_recovery(rproc);
> >>  }
> >>  
> >>  /**
> >> @@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
> >>  		return ret;
> >>  	}
> >>  
> >> -	if (rproc->state != RPROC_ATTACHED) {
> >> +	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
> >>  		ret = -EINVAL;
> >>  		goto out;
> >>  	}
> >> -- 
> >> 2.34.1
> >>
> 

Re: [PATCH v3 1/2] remoteproc: core: full attach detach during recovery

[Shah, Tanmay]

On 2/25/2026 5:42 PM, Bjorn Andersson wrote:
> On Mon, Feb 23, 2026 at 03:43:56PM -0600, Shah, Tanmay wrote:
>> Hello,
>>
>> Thank you for the reviews. My response below:
>>
>> On 2/23/2026 1:27 PM, Bjorn Andersson wrote:
>>> On Mon, Feb 23, 2026 at 10:50:05AM -0800, Tanmay Shah wrote:
>>>> Current attach on recovery mechanism loads the clean resource table
>>>> during recovery, but doesn't re-allocate the resources. RPMsg
>>>> communication will fail after recovery due to this. Fix this
>>>> incorrect behavior by doing the full detach and attach of remote
>>>> processor during the recovery. This will load the clean resource table
>>>> and re-allocate all the resources, which will set up correct vring
>>>> information in the resource table.
>>>>
>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>> ---
>>>>
>>>> Changes in v3:
>>>>  - both rproc_attach_recovery() and
>>>>    rproc_boot_recovery() are called the same way.
>>>>  - remove unrelated changes
>>>>
>>>> Changes in v2:
>>>>  - use rproc_boot instead of rproc_attach
>>>>  - move debug message early in the function
>>>>
>>>>  drivers/remoteproc/remoteproc_core.c | 33 +++++++++++-----------------
>>>>  1 file changed, 13 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>> index aada2780b343..790ad7c6d12e 100644
>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>> @@ -1777,11 +1777,11 @@ static int rproc_attach_recovery(struct rproc *rproc)
>>>>  {
>>>>  	int ret;
>>>>  
>>>> -	ret = __rproc_detach(rproc);
>>>> +	ret = rproc_detach(rproc);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	return __rproc_attach(rproc);
>>>> +	return rproc_boot(rproc);
>>>>  }
>>>>  
>>>>  static int rproc_boot_recovery(struct rproc *rproc)
>>>> @@ -1790,10 +1790,14 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  	struct device *dev = &rproc->dev;
>>>>  	int ret;
>>>>  
>>>> -	ret = rproc_stop(rproc, true);
>>>> +	ret = mutex_lock_interruptible(&rproc->lock);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> +	ret = rproc_stop(rproc, true);
>>>> +	if (ret)
>>>> +		goto unlock_mutex;
>>>> +
>>>>  	/* generate coredump */
>>>>  	rproc->ops->coredump(rproc);
>>>>  
>>>> @@ -1801,7 +1805,7 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  	ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>  	if (ret < 0) {
>>>>  		dev_err(dev, "request_firmware failed: %d\n", ret);
>>>> -		return ret;
>>>> +		goto unlock_mutex;
>>>>  	}
>>>>  
>>>>  	/* boot the remote processor up again */
>>>> @@ -1809,6 +1813,8 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  
>>>>  	release_firmware(firmware_p);
>>>>  
>>>> +unlock_mutex:
>>>> +	mutex_unlock(&rproc->lock);
>>>>  	return ret;
>>>>  }
>>>>  
>>>> @@ -1827,26 +1833,13 @@ static int rproc_boot_recovery(struct rproc *rproc)
>>>>  int rproc_trigger_recovery(struct rproc *rproc)
>>>>  {
>>>>  	struct device *dev = &rproc->dev;
>>>> -	int ret;
>>>> -
>>>> -	ret = mutex_lock_interruptible(&rproc->lock);
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>> -	/* State could have changed before we got the mutex */
>>>> -	if (rproc->state != RPROC_CRASHED)
>>>> -		goto unlock_mutex;
>>>>  
>>>>  	dev_err(dev, "recovering %s\n", rproc->name);
>>>>  
>>>>  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
>>>> -		ret = rproc_attach_recovery(rproc);
>>>> +		return rproc_attach_recovery(rproc);
>>>
>>> rproc_trigger_recovery() can be called either from scheduled work or
>>> directly from the debugfs/sysfs interface, it doesn't seem safe to me to
>>> call rproc_attach_recovery() without ensuring mutual exclusion between
>>> multiple parallel callers.
>>>
>>
>> I think mutual exclusion is still maintained.
>>
>>> In fact, I can see the relationship between the commit message and the
>>> changes in rproc_attach_recovery() and rproc_detach(), but I'm not sure
>>> why you need to change rproc_boot_recovery() and
>>> rproc_trigger_recovery(). Perhaps you're just missing some explanation
>>> in the commit message?
>>>
>>
>> Here, I am refactoring how lock is used and that is why I have to modify
>> rproc_trigger_recovery() and rproc_boot_recovery().
>>
>> Before:
>>
>> rproc_trigger_recovery() -> lock() -> __rproc_detach() /
>> rproc_boot_recovery() -> unlock()
>>
>> Now, __rproc_detach is replaced with rproc_detach(), which already has
>> mutual exclusion implemented within the call.
>>
>> After:
>>
>> 1) for attach recovery
>> rproc_trigger_recovery() -> rproc_attach_recovery() -> rproc_detach() ->
>> lock() -> ... -> unlock() -> rproc_boot() -> lock() ... -> unlock()
>>
> 
> The concern I had was that we're letting others execute inbetween
> rproc_detach() and rproc_boot(). But perhaps that's okay?
> 
>> 2) To call rproc_attach_recovery() and rproc_boot_recovery() in the same
>> manner, I modified rproc_boot_recovery() and introduced mutual exclusion
>> around it.
>>
> 
> No, in rproc_boot_recovery() you're locking around stop and start. In
> rproc_attach_recovery() you're locking within each part.
> 
> Looking more at this, I dislike the asymmetry between rproc_detach() vs
> rproc_boot(), and how I presume this then relies on rproc_boot() mostly
> just calling rproc_attach().
> 
> This seems to come from the fact that rproc_attach() and rproc_start()
> aren't symmetrical and rproc_detach() and rproc_stop() aren't.
> 
> It would be good if we could get this lined up, so that it's not so hard
> to reason about the different code paths through the core.
> 
> 
> Regardless of this though, the removal of the check for state !=
> RPROC_CRASHED (under a lock) means that inbetween
> rproc_crash_handler_work() and rproc_trigger_recovery() someone can
> write "recover" into the "recovery" debugfs entry and we will recover
> twice.
> 

Okay, to address this concern I will maintain original code in the
rproc_trigger_reocvery().

In, rproc_attach_recovery() I won't be calling rproc_detach() and
rproc_boot(). Instead I will programming sequence needed from
rproc_detach(), and then call rproc_attach() directly. This will help
maintain mutual exclusion same as before during the recovery process.

This will duplicate some code, but that can be refactored later, when
asymmetry of rproc_detach() vs rproc_stop() will be fixed.

I hope that's fine. I am open to any other proposal as well.

If you prefer to see v4, before further reviews let me know.

Thanks,
Tanmay


> Regards,
> Bjorn
> 
>> If you prefer, I can add commit message explaining this change. This is
>> only refactoring of the code and no new feature though.
>> Let me know if something is still missing in the implementation or in
>> the above explanation.
>>
>> Thank You,
>> Tanmay
>>
>>> Regards,
>>> Bjorn
>>>
>>>>  	else
>>>> -		ret = rproc_boot_recovery(rproc);
>>>> -
>>>> -unlock_mutex:
>>>> -	mutex_unlock(&rproc->lock);
>>>> -	return ret;
>>>> +		return rproc_boot_recovery(rproc);
>>>>  }
>>>>  
>>>>  /**
>>>> @@ -2057,7 +2050,7 @@ int rproc_detach(struct rproc *rproc)
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> -	if (rproc->state != RPROC_ATTACHED) {
>>>> +	if (rproc->state != RPROC_ATTACHED && rproc->state != RPROC_CRASHED) {
>>>>  		ret = -EINVAL;
>>>>  		goto out;
>>>>  	}
>>>> -- 
>>>> 2.34.1
>>>>
>>

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Shah, Tanmay]

On 2/25/2026 5:30 PM, Bjorn Andersson wrote:
> On Wed, Feb 25, 2026 at 11:22:05AM -0600, Shah, Tanmay wrote:
>>
>>
>> On 2/23/2026 4:40 PM, Shah, Tanmay wrote:
>>>
>>>
>>> On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
>>>> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
>>>>> Remote processor will report the crash reason via the resource table
>>>>> and notify the host via mailbox notification. The host checks this
>>>>> crash reason on every mailbox notification from the remote and report
>>>>> to the rproc core framework. Then the rproc core framework will start
>>>>> the recovery process.
>>>>>
>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>>   - %s/kick/mailbox notification/
>>>>>   - %s/core framework/rproc core framework/
>>>>>   - fold simple function within zynqmp_r5_handle_rsc().
>>>>>   - remove spurious change
>>>>>   - reset crash state after reporting the crash
>>>>>   - document set and reset of ATTACH_ON_RECOVERY flag
>>>>>   - set recovery_disabled flag to false
>>>>>   - check condition rproc->crash_reason != NULL
>>>>>
>>>>> Changes in v2:
>>>>>   - clear attach recovery boot flag during detach and stop ops
>>>>>
>>>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
>>>>>  1 file changed, 59 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>> index bd619a6c42aa..0d831330ea90 100644
>>>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
>>>>>  	const uintptr_t rsc_tbl;
>>>>>  } __packed;
>>>>>  
>>>>> +enum fw_vendor_rsc {
>>>>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
>>>>
>>>> Given that this is a vendor-specific resource, wouldn't it be nice to
>>>> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
>>>>
>>>
>>> Ack. I will change name for enum and resource both.
>>>
>>>>> +};
>>>>> +
>>>>>  /*
>>>>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
>>>>>   * compatibility with device-tree that does not have TCM information.
>>>>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
>>>>>  };
>>>>>  
>>>>> +/**
>>>>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
>>>>> + *
>>>>> + * @crash_state: if true, the rproc is notifying crash, time to recover
>>>>> + * @crash_reason: reason of crash
>>>>> + */
>>>>> +struct xlnx_rproc_crash_report {
>>>>> +	u32 crash_state;
>>>>> +	u32 crash_reason;
>>>>> +} __packed;
>>>>> +
>>>>>  /**
>>>>>   * struct zynqmp_r5_core - remoteproc core's internal data
>>>>>   *
>>>>> + * @crash_report: rproc crash state and reason
>>>>>   * @rsc_tbl_va: resource table virtual address
>>>>>   * @sram: Array of sram memories assigned to this core
>>>>>   * @num_sram: number of sram for this core
>>>>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>>>   * @ipi: pointer to mailbox information
>>>>>   */
>>>>>  struct zynqmp_r5_core {
>>>>> +	struct xlnx_rproc_crash_report *crash_report;
>>>>>  	void __iomem *rsc_tbl_va;
>>>>>  	struct zynqmp_sram_bank *sram;
>>>>>  	int num_sram;
>>>>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
>>>>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>>>  {
>>>>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
>>>>> +	struct zynqmp_r5_core *r5_core;
>>>>> +	struct rproc *rproc;
>>>>>  	struct mbox_info *ipi;
>>>>>  	size_t len;
>>>>>  
>>>>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
>>>>> +	r5_core = ipi->r5_core;
>>>>> +	rproc = r5_core->rproc;
>>>>>  
>>>>>  	/* copy data from ipi buffer to r5_core */
>>>>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
>>>>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>>>  	buf_msg->len = len;
>>>>>  	memcpy(buf_msg->data, ipi_msg->data, len);
>>>>>  
>>>>> +	/* Check for crash only if rproc crash is expected */
>>>>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
>>>>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
>>>>
>>>> Nit. I'd prefer the order of these to be swapped...
>>>>
>>>> Compare:
>>>>
>>>> "Check if we have crashed, and if so check that we're in a state where
>>>> that makes sense."
>>>>
>>>> vs the way you're ordering this:
>>>>
>>>> "Check if we're in a state, and if in that state we have crashed"
>>>>
>>>>
>>>> The "have we crashed" question is the most-significant-bit of this
>>>> chunk, making that the outermost conditional makes it faster for the
>>>> next reader to orient themselves in the code.
>>>
>>> Ack, that makes sense.
>>>
>>>>
>>>>> +			rproc_report_crash(rproc,
>>>>> +					   r5_core->crash_report->crash_reason);
>>>>
>>>> Are these two value spaces synchronized? crash_reason seems to be a
>>>> generic 32-bit number without particular definition, and you pass it
>>>> into a enum rproc_crash_type.
>>>>
>>>
>>> Yes, crash_reason is supposed to be enum rproc_crash_type.
>>>
>>>> I presume the outcome is that you get the string
>>>> "crash detected in <name>: type: unknown" in your log for most cases?
>>>>
>>>
>>> So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
>>> more reasons would have to go in the "unknown" case.
>>>
>>>>
>>>> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
>>>> For the watchdog bite there isn't much information, but for the fatal
>>>> error we have a error string which we print, then we call
>>>> rproc_report_crash(FATAL) which results in another "useless" print.
>>>>
>>>> Perhaps we could expand rproc_report_crash() to allow drivers to provide
>>>> some information about the crash beyond the enum.
>>>>
>>>> Something like:
>>>> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
>>>>
>>>> Would that be useful to you? Would it be valuable to turn your
>>>> "crash_reason" into a human readable string?
>>>>
>>>
>>> Yes, it is valuable to turn "crash_reason" to human readable string.
>>> Should we leave that part to each driver and not have it in the common
>>> framework?
>>>
>>> If we are to refactor rproc_report_crash, then I think following is more
>>> flexible:
>>>
>>> rproc_report_crash(rproc, const char *crash_reason_str);
>>>
>>> Then each platform driver can print crash reason however they see fit.
>>> We can also avoid printing crash reason two times this way.
>>>
>>
>> Hi Bjorn,
>>
>> I take this back. I think crash_reason can be defined differently for
>> each firmware project. I would like to provide that flexibility to the
>> firmware developer. Hence, I prefer not to convert crash_reason integer
>> to human readable string, as can be different for different fw projects.
>>
> 
> Then we certainly shouldn't pass it as the second argument of
> rproc_report_crash().
> 
>> Instead, the xlnx platform driver will simply print the crash_reason
>> integer as given by the firmware, and notify the crash to the core
>> framework as following:
>>
>> rproc_report_crash(rproc, RPROC_FATAL_ERROR);
>>
>> This way, we don't have to modify the rproc_report_crash() API.
>> I hope this makes sense.
>>
> 
> Yes, that makes sense.
> 
> I think I'd like to make the proposed modification regardless, but that
> is then a completely separate change.
> 
>> I will wait for your response before sending the new version. Rest of
>> the comments I will address as asked.
>>
> 
> Is your struct xlnx_rproc_crash_report already defined and in use by the
> firmware? If not, I'd recommend that you spend a little bit extra time
> thinking about the content of it. E.g. the human readable char [] found
> in Qualcomm's crash reports is quite useful...
> 

We can modify the resource structure as needed. I looked at the qcom
rproc crash report. I don't know much the qcom smem infrastructure, but
per my understanding qcom rproc uses crash_reason integer to retrieve
the string format of the crash reason stored in the smem, via smem
driver. That's too complex for my use case. Also, I prefer not to map
crash reason number with the fixed string. Instead would like to provide
flexibility to the user to insert human readable string as needed.

How does following resource definition looks like?

struct fw_rsc_xlnx_crash_report {
        uint32_t type;
        uint32_t crash_state;
        uint32_t crash_reason;
        char crash_reason_str[16];
        uint32_t reserved;
} __packed;

So, if the user prefer to provide human readable string along with the
integer, then 16 characters should be enough, and they can choose any
string to insert when reporting the crash.

Linux side will simply print those 16 characters as string. We don't
need to verify the content of it. It is users responsibility to make
sure the characters are valid, if not crash_reason_str[0] should be '\0'.

Thanks,
Tanmay

> Regards,
> Bjorn
> 
>> Thanks,
>> Tanmay
>>
>>> If we do this, then crash_reason can be defined for each driver
>>> individually. That's more appropriate as each vendor can have different
>>> enum for crash.
>>>
>>> Let me know your thoughts.
>>>
>>>>> +			r5_core->crash_report->crash_state = 0;
>>>>> +			r5_core->crash_report->crash_reason = 0;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>>  	/* received and processed interrupt ack */
>>>>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
>>>>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
>>>>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
>>>>>  	if (ret)
>>>>>  		dev_err(r5_core->dev, "core force power down failed\n");
>>>>>  
>>>>> +	/*
>>>>> +	 * Clear attach on recovery flag during stop operation. The next state
>>>>> +	 * of the remote processor is expected to be "Running" state. In this
>>>>> +	 * state boot recovery method must take place over attach on recovery.
>>>>> +	 */
>>>>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>>>> +
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>>>>>  
>>>>>  static int zynqmp_r5_attach(struct rproc *rproc)
>>>>>  {
>>>>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
>>>>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
>>>>> +
>>>>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>>>>>  
>>>>>  	return 0;
>>>>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
>>>>>  	 */
>>>>>  	zynqmp_r5_rproc_kick(rproc, 0);
>>>>>  
>>>>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
>>>>> +				int offset, int avail)
>>>>> +{
>>>>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
>>>>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
>>>>> +
>>>>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
>>>>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
>>>>
>>>> I don't think you need the cast.
>>>>
>>>> Regards,
>>>> Bjorn
>>>>
>>>>> +	else
>>>>> +		return RSC_IGNORED;
>>>>> +
>>>>> +	return RSC_HANDLED;
>>>>> +}
>>>>> +
>>>>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>>>  	.prepare	= zynqmp_r5_rproc_prepare,
>>>>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
>>>>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>>>>>  	.attach		= zynqmp_r5_attach,
>>>>>  	.detach		= zynqmp_r5_detach,
>>>>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
>>>>>  };
>>>>>  
>>>>>  /**
>>>>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>>>>  
>>>>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>>>>  
>>>>> -	r5_rproc->recovery_disabled = true;
>>>>> +	r5_rproc->recovery_disabled = false;
>>>>>  	r5_rproc->has_iommu = false;
>>>>>  	r5_rproc->auto_boot = false;
>>>>>  	r5_core = r5_rproc->priv;
>>>>> -- 
>>>>> 2.34.1
>>>>>
>>>
>>

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Bjorn Andersson]

On Thu, Feb 26, 2026 at 04:57:06PM -0600, Shah, Tanmay wrote:
> 
> 
> On 2/25/2026 5:30 PM, Bjorn Andersson wrote:
> > On Wed, Feb 25, 2026 at 11:22:05AM -0600, Shah, Tanmay wrote:
> >>
> >>
> >> On 2/23/2026 4:40 PM, Shah, Tanmay wrote:
> >>>
> >>>
> >>> On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
> >>>> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
> >>>>> Remote processor will report the crash reason via the resource table
> >>>>> and notify the host via mailbox notification. The host checks this
> >>>>> crash reason on every mailbox notification from the remote and report
> >>>>> to the rproc core framework. Then the rproc core framework will start
> >>>>> the recovery process.
> >>>>>
> >>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
> >>>>> ---
> >>>>>
> >>>>> Changes in v3:
> >>>>>   - %s/kick/mailbox notification/
> >>>>>   - %s/core framework/rproc core framework/
> >>>>>   - fold simple function within zynqmp_r5_handle_rsc().
> >>>>>   - remove spurious change
> >>>>>   - reset crash state after reporting the crash
> >>>>>   - document set and reset of ATTACH_ON_RECOVERY flag
> >>>>>   - set recovery_disabled flag to false
> >>>>>   - check condition rproc->crash_reason != NULL
> >>>>>
> >>>>> Changes in v2:
> >>>>>   - clear attach recovery boot flag during detach and stop ops
> >>>>>
> >>>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
> >>>>>  1 file changed, 59 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>>>> index bd619a6c42aa..0d831330ea90 100644
> >>>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
> >>>>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
> >>>>>  	const uintptr_t rsc_tbl;
> >>>>>  } __packed;
> >>>>>  
> >>>>> +enum fw_vendor_rsc {
> >>>>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
> >>>>
> >>>> Given that this is a vendor-specific resource, wouldn't it be nice to
> >>>> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
> >>>>
> >>>
> >>> Ack. I will change name for enum and resource both.
> >>>
> >>>>> +};
> >>>>> +
> >>>>>  /*
> >>>>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
> >>>>>   * compatibility with device-tree that does not have TCM information.
> >>>>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
> >>>>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
> >>>>>  };
> >>>>>  
> >>>>> +/**
> >>>>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
> >>>>> + *
> >>>>> + * @crash_state: if true, the rproc is notifying crash, time to recover
> >>>>> + * @crash_reason: reason of crash
> >>>>> + */
> >>>>> +struct xlnx_rproc_crash_report {
> >>>>> +	u32 crash_state;
> >>>>> +	u32 crash_reason;
> >>>>> +} __packed;
> >>>>> +
> >>>>>  /**
> >>>>>   * struct zynqmp_r5_core - remoteproc core's internal data
> >>>>>   *
> >>>>> + * @crash_report: rproc crash state and reason
> >>>>>   * @rsc_tbl_va: resource table virtual address
> >>>>>   * @sram: Array of sram memories assigned to this core
> >>>>>   * @num_sram: number of sram for this core
> >>>>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
> >>>>>   * @ipi: pointer to mailbox information
> >>>>>   */
> >>>>>  struct zynqmp_r5_core {
> >>>>> +	struct xlnx_rproc_crash_report *crash_report;
> >>>>>  	void __iomem *rsc_tbl_va;
> >>>>>  	struct zynqmp_sram_bank *sram;
> >>>>>  	int num_sram;
> >>>>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
> >>>>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
> >>>>>  {
> >>>>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
> >>>>> +	struct zynqmp_r5_core *r5_core;
> >>>>> +	struct rproc *rproc;
> >>>>>  	struct mbox_info *ipi;
> >>>>>  	size_t len;
> >>>>>  
> >>>>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
> >>>>> +	r5_core = ipi->r5_core;
> >>>>> +	rproc = r5_core->rproc;
> >>>>>  
> >>>>>  	/* copy data from ipi buffer to r5_core */
> >>>>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
> >>>>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
> >>>>>  	buf_msg->len = len;
> >>>>>  	memcpy(buf_msg->data, ipi_msg->data, len);
> >>>>>  
> >>>>> +	/* Check for crash only if rproc crash is expected */
> >>>>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
> >>>>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
> >>>>
> >>>> Nit. I'd prefer the order of these to be swapped...
> >>>>
> >>>> Compare:
> >>>>
> >>>> "Check if we have crashed, and if so check that we're in a state where
> >>>> that makes sense."
> >>>>
> >>>> vs the way you're ordering this:
> >>>>
> >>>> "Check if we're in a state, and if in that state we have crashed"
> >>>>
> >>>>
> >>>> The "have we crashed" question is the most-significant-bit of this
> >>>> chunk, making that the outermost conditional makes it faster for the
> >>>> next reader to orient themselves in the code.
> >>>
> >>> Ack, that makes sense.
> >>>
> >>>>
> >>>>> +			rproc_report_crash(rproc,
> >>>>> +					   r5_core->crash_report->crash_reason);
> >>>>
> >>>> Are these two value spaces synchronized? crash_reason seems to be a
> >>>> generic 32-bit number without particular definition, and you pass it
> >>>> into a enum rproc_crash_type.
> >>>>
> >>>
> >>> Yes, crash_reason is supposed to be enum rproc_crash_type.
> >>>
> >>>> I presume the outcome is that you get the string
> >>>> "crash detected in <name>: type: unknown" in your log for most cases?
> >>>>
> >>>
> >>> So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
> >>> more reasons would have to go in the "unknown" case.
> >>>
> >>>>
> >>>> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
> >>>> For the watchdog bite there isn't much information, but for the fatal
> >>>> error we have a error string which we print, then we call
> >>>> rproc_report_crash(FATAL) which results in another "useless" print.
> >>>>
> >>>> Perhaps we could expand rproc_report_crash() to allow drivers to provide
> >>>> some information about the crash beyond the enum.
> >>>>
> >>>> Something like:
> >>>> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
> >>>>
> >>>> Would that be useful to you? Would it be valuable to turn your
> >>>> "crash_reason" into a human readable string?
> >>>>
> >>>
> >>> Yes, it is valuable to turn "crash_reason" to human readable string.
> >>> Should we leave that part to each driver and not have it in the common
> >>> framework?
> >>>
> >>> If we are to refactor rproc_report_crash, then I think following is more
> >>> flexible:
> >>>
> >>> rproc_report_crash(rproc, const char *crash_reason_str);
> >>>
> >>> Then each platform driver can print crash reason however they see fit.
> >>> We can also avoid printing crash reason two times this way.
> >>>
> >>
> >> Hi Bjorn,
> >>
> >> I take this back. I think crash_reason can be defined differently for
> >> each firmware project. I would like to provide that flexibility to the
> >> firmware developer. Hence, I prefer not to convert crash_reason integer
> >> to human readable string, as can be different for different fw projects.
> >>
> > 
> > Then we certainly shouldn't pass it as the second argument of
> > rproc_report_crash().
> > 
> >> Instead, the xlnx platform driver will simply print the crash_reason
> >> integer as given by the firmware, and notify the crash to the core
> >> framework as following:
> >>
> >> rproc_report_crash(rproc, RPROC_FATAL_ERROR);
> >>
> >> This way, we don't have to modify the rproc_report_crash() API.
> >> I hope this makes sense.
> >>
> > 
> > Yes, that makes sense.
> > 
> > I think I'd like to make the proposed modification regardless, but that
> > is then a completely separate change.
> > 
> >> I will wait for your response before sending the new version. Rest of
> >> the comments I will address as asked.
> >>
> > 
> > Is your struct xlnx_rproc_crash_report already defined and in use by the
> > firmware? If not, I'd recommend that you spend a little bit extra time
> > thinking about the content of it. E.g. the human readable char [] found
> > in Qualcomm's crash reports is quite useful...
> > 
> 
> We can modify the resource structure as needed.

Just to make sure it's clear, this is merely a suggestion based on my
experience, not a review request. But if the format of the structure
isn't already defined, I would suggest packing some additional
information in there.

> I looked at the qcom
> rproc crash report. I don't know much the qcom smem infrastructure, but
> per my understanding qcom rproc uses crash_reason integer to retrieve
> the string format of the crash reason stored in the smem, via smem
> driver. That's too complex for my use case. Also, I prefer not to map
> crash reason number with the fixed string. Instead would like to provide
> flexibility to the user to insert human readable string as needed.
> 

Qualcomm has SMEM which is an allocate-only "heap" which is shared
between Linux and the different remoteprocs, so that's used to share
such information between the different systems.

It sounds correct to me that you don't want to introduce something like
that to solve this problem. In fact I think your solution is quite
elegant.

> How does following resource definition looks like?
> 
> struct fw_rsc_xlnx_crash_report {
>         uint32_t type;

It sounds like a good idea to have a "type" or "version" here, to allow
changing the struct at a later point if necessary.

But you're probably only going to be using a few bits here, and iiuc you
only use 1 bit from the "crash_state". So you should be able to reduce
the size of these types.

>         uint32_t crash_state;
>         uint32_t crash_reason;
>         char crash_reason_str[16];

Would that be sufficient to convey a good error message?

As an example I triggered an error on one of the Qualcomm remoteprocs
earlier today, the error message is 17 characters, but then it includes
the process, file and line number of the error and the caller. So in
total it's 73 characters - but it gave me a pretty good idea of what
went wrong.

>         uint32_t reserved;

Why pad here? If you want an even size, make the string 20 characters
(or adjust the u32 pair at the beginning). If you want room for future
improvements, use the "type" field for that.

> } __packed;
> 
> So, if the user prefer to provide human readable string along with the
> integer, then 16 characters should be enough, and they can choose any
> string to insert when reporting the crash.
> 

Give it some extra thought, and weigh it against memory budget etc and
pick something that you think suits you.

> Linux side will simply print those 16 characters as string. We don't
> need to verify the content of it. It is users responsibility to make
> sure the characters are valid, if not crash_reason_str[0] should be '\0'.
> 

I'd still suggest that you \0-terminate the string explicitly on the
Linux side, just to be safe. You don't want a firmware bug to crash your
Linux driver.

Regards,
Bjorn

> Thanks,
> Tanmay
> 
> > Regards,
> > Bjorn
> > 
> >> Thanks,
> >> Tanmay
> >>
> >>> If we do this, then crash_reason can be defined for each driver
> >>> individually. That's more appropriate as each vendor can have different
> >>> enum for crash.
> >>>
> >>> Let me know your thoughts.
> >>>
> >>>>> +			r5_core->crash_report->crash_state = 0;
> >>>>> +			r5_core->crash_report->crash_reason = 0;
> >>>>> +		}
> >>>>> +	}
> >>>>> +
> >>>>>  	/* received and processed interrupt ack */
> >>>>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
> >>>>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
> >>>>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
> >>>>>  	if (ret)
> >>>>>  		dev_err(r5_core->dev, "core force power down failed\n");
> >>>>>  
> >>>>> +	/*
> >>>>> +	 * Clear attach on recovery flag during stop operation. The next state
> >>>>> +	 * of the remote processor is expected to be "Running" state. In this
> >>>>> +	 * state boot recovery method must take place over attach on recovery.
> >>>>> +	 */
> >>>>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> >>>>> +
> >>>>>  	return ret;
> >>>>>  }
> >>>>>  
> >>>>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
> >>>>>  
> >>>>>  static int zynqmp_r5_attach(struct rproc *rproc)
> >>>>>  {
> >>>>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
> >>>>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
> >>>>> +
> >>>>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
> >>>>>  
> >>>>>  	return 0;
> >>>>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
> >>>>>  	 */
> >>>>>  	zynqmp_r5_rproc_kick(rproc, 0);
> >>>>>  
> >>>>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
> >>>>> +
> >>>>>  	return 0;
> >>>>>  }
> >>>>>  
> >>>>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
> >>>>> +				int offset, int avail)
> >>>>> +{
> >>>>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
> >>>>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
> >>>>> +
> >>>>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
> >>>>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
> >>>>
> >>>> I don't think you need the cast.
> >>>>
> >>>> Regards,
> >>>> Bjorn
> >>>>
> >>>>> +	else
> >>>>> +		return RSC_IGNORED;
> >>>>> +
> >>>>> +	return RSC_HANDLED;
> >>>>> +}
> >>>>> +
> >>>>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >>>>>  	.prepare	= zynqmp_r5_rproc_prepare,
> >>>>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
> >>>>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
> >>>>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
> >>>>>  	.attach		= zynqmp_r5_attach,
> >>>>>  	.detach		= zynqmp_r5_detach,
> >>>>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
> >>>>>  };
> >>>>>  
> >>>>>  /**
> >>>>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
> >>>>>  
> >>>>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
> >>>>>  
> >>>>> -	r5_rproc->recovery_disabled = true;
> >>>>> +	r5_rproc->recovery_disabled = false;
> >>>>>  	r5_rproc->has_iommu = false;
> >>>>>  	r5_rproc->auto_boot = false;
> >>>>>  	r5_core = r5_rproc->priv;
> >>>>> -- 
> >>>>> 2.34.1
> >>>>>
> >>>
> >>
> 

Re: [PATCH v3 2/2] remoteproc: xlnx: add crash detection mechanism

[Shah, Tanmay]

On 2/26/2026 10:19 PM, Bjorn Andersson wrote:
> On Thu, Feb 26, 2026 at 04:57:06PM -0600, Shah, Tanmay wrote:
>>
>>
>> On 2/25/2026 5:30 PM, Bjorn Andersson wrote:
>>> On Wed, Feb 25, 2026 at 11:22:05AM -0600, Shah, Tanmay wrote:
>>>>
>>>>
>>>> On 2/23/2026 4:40 PM, Shah, Tanmay wrote:
>>>>>
>>>>>
>>>>> On 2/23/2026 1:55 PM, Bjorn Andersson wrote:
>>>>>> On Mon, Feb 23, 2026 at 10:50:06AM -0800, Tanmay Shah wrote:
>>>>>>> Remote processor will report the crash reason via the resource table
>>>>>>> and notify the host via mailbox notification. The host checks this
>>>>>>> crash reason on every mailbox notification from the remote and report
>>>>>>> to the rproc core framework. Then the rproc core framework will start
>>>>>>> the recovery process.
>>>>>>>
>>>>>>> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v3:
>>>>>>>   - %s/kick/mailbox notification/
>>>>>>>   - %s/core framework/rproc core framework/
>>>>>>>   - fold simple function within zynqmp_r5_handle_rsc().
>>>>>>>   - remove spurious change
>>>>>>>   - reset crash state after reporting the crash
>>>>>>>   - document set and reset of ATTACH_ON_RECOVERY flag
>>>>>>>   - set recovery_disabled flag to false
>>>>>>>   - check condition rproc->crash_reason != NULL
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>>   - clear attach recovery boot flag during detach and stop ops
>>>>>>>
>>>>>>>  drivers/remoteproc/xlnx_r5_remoteproc.c | 60 ++++++++++++++++++++++++-
>>>>>>>  1 file changed, 59 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/remoteproc/xlnx_r5_remoteproc.c b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>>>> index bd619a6c42aa..0d831330ea90 100644
>>>>>>> --- a/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>>>> +++ b/drivers/remoteproc/xlnx_r5_remoteproc.c
>>>>>>> @@ -108,6 +108,10 @@ struct rsc_tbl_data {
>>>>>>>  	const uintptr_t rsc_tbl;
>>>>>>>  } __packed;
>>>>>>>  
>>>>>>> +enum fw_vendor_rsc {
>>>>>>> +	FW_RSC_VENDOR_CRASH_REASON = RSC_VENDOR_START,
>>>>>>
>>>>>> Given that this is a vendor-specific resource, wouldn't it be nice to
>>>>>> find e.g. XLNX somewhere in the name? Same thing with the enum itself.
>>>>>>
>>>>>
>>>>> Ack. I will change name for enum and resource both.
>>>>>
>>>>>>> +};
>>>>>>> +
>>>>>>>  /*
>>>>>>>   * Hardcoded TCM bank values. This will stay in driver to maintain backward
>>>>>>>   * compatibility with device-tree that does not have TCM information.
>>>>>>> @@ -127,9 +131,21 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>>>>>  	{0xffe30000UL, 0x30000, 0x10000UL, PD_R5_1_BTCM, "btcm1"},
>>>>>>>  };
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * struct xlnx_rproc_crash_report - resource to know crash status and reason
>>>>>>> + *
>>>>>>> + * @crash_state: if true, the rproc is notifying crash, time to recover
>>>>>>> + * @crash_reason: reason of crash
>>>>>>> + */
>>>>>>> +struct xlnx_rproc_crash_report {
>>>>>>> +	u32 crash_state;
>>>>>>> +	u32 crash_reason;
>>>>>>> +} __packed;
>>>>>>> +
>>>>>>>  /**
>>>>>>>   * struct zynqmp_r5_core - remoteproc core's internal data
>>>>>>>   *
>>>>>>> + * @crash_report: rproc crash state and reason
>>>>>>>   * @rsc_tbl_va: resource table virtual address
>>>>>>>   * @sram: Array of sram memories assigned to this core
>>>>>>>   * @num_sram: number of sram for this core
>>>>>>> @@ -143,6 +159,7 @@ static const struct mem_bank_data zynqmp_tcm_banks_lockstep[] = {
>>>>>>>   * @ipi: pointer to mailbox information
>>>>>>>   */
>>>>>>>  struct zynqmp_r5_core {
>>>>>>> +	struct xlnx_rproc_crash_report *crash_report;
>>>>>>>  	void __iomem *rsc_tbl_va;
>>>>>>>  	struct zynqmp_sram_bank *sram;
>>>>>>>  	int num_sram;
>>>>>>> @@ -227,10 +244,14 @@ static void handle_event_notified(struct work_struct *work)
>>>>>>>  static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>>>>>  {
>>>>>>>  	struct zynqmp_ipi_message *ipi_msg, *buf_msg;
>>>>>>> +	struct zynqmp_r5_core *r5_core;
>>>>>>> +	struct rproc *rproc;
>>>>>>>  	struct mbox_info *ipi;
>>>>>>>  	size_t len;
>>>>>>>  
>>>>>>>  	ipi = container_of(cl, struct mbox_info, mbox_cl);
>>>>>>> +	r5_core = ipi->r5_core;
>>>>>>> +	rproc = r5_core->rproc;
>>>>>>>  
>>>>>>>  	/* copy data from ipi buffer to r5_core */
>>>>>>>  	ipi_msg = (struct zynqmp_ipi_message *)msg;
>>>>>>> @@ -244,6 +265,16 @@ static void zynqmp_r5_mb_rx_cb(struct mbox_client *cl, void *msg)
>>>>>>>  	buf_msg->len = len;
>>>>>>>  	memcpy(buf_msg->data, ipi_msg->data, len);
>>>>>>>  
>>>>>>> +	/* Check for crash only if rproc crash is expected */
>>>>>>> +	if (rproc->state == RPROC_ATTACHED || rproc->state == RPROC_RUNNING) {
>>>>>>> +		if (r5_core->crash_report && r5_core->crash_report->crash_state) {
>>>>>>
>>>>>> Nit. I'd prefer the order of these to be swapped...
>>>>>>
>>>>>> Compare:
>>>>>>
>>>>>> "Check if we have crashed, and if so check that we're in a state where
>>>>>> that makes sense."
>>>>>>
>>>>>> vs the way you're ordering this:
>>>>>>
>>>>>> "Check if we're in a state, and if in that state we have crashed"
>>>>>>
>>>>>>
>>>>>> The "have we crashed" question is the most-significant-bit of this
>>>>>> chunk, making that the outermost conditional makes it faster for the
>>>>>> next reader to orient themselves in the code.
>>>>>
>>>>> Ack, that makes sense.
>>>>>
>>>>>>
>>>>>>> +			rproc_report_crash(rproc,
>>>>>>> +					   r5_core->crash_report->crash_reason);
>>>>>>
>>>>>> Are these two value spaces synchronized? crash_reason seems to be a
>>>>>> generic 32-bit number without particular definition, and you pass it
>>>>>> into a enum rproc_crash_type.
>>>>>>
>>>>>
>>>>> Yes, crash_reason is supposed to be enum rproc_crash_type.
>>>>>
>>>>>> I presume the outcome is that you get the string
>>>>>> "crash detected in <name>: type: unknown" in your log for most cases?
>>>>>>
>>>>>
>>>>> So far, we have only "WATCHDOG" and "FATAL ERROR" cases. I guess any
>>>>> more reasons would have to go in the "unknown" case.
>>>>>
>>>>>>
>>>>>> In the Qualcomm drivers we can get RPROC_WATCHDOG or RPROC_FATAL_ERROR.
>>>>>> For the watchdog bite there isn't much information, but for the fatal
>>>>>> error we have a error string which we print, then we call
>>>>>> rproc_report_crash(FATAL) which results in another "useless" print.
>>>>>>
>>>>>> Perhaps we could expand rproc_report_crash() to allow drivers to provide
>>>>>> some information about the crash beyond the enum.
>>>>>>
>>>>>> Something like:
>>>>>> 	rproc_report_crash(rproc, RPROC_FATAL_ERROR, "%d", report->crash_reason);
>>>>>>
>>>>>> Would that be useful to you? Would it be valuable to turn your
>>>>>> "crash_reason" into a human readable string?
>>>>>>
>>>>>
>>>>> Yes, it is valuable to turn "crash_reason" to human readable string.
>>>>> Should we leave that part to each driver and not have it in the common
>>>>> framework?
>>>>>
>>>>> If we are to refactor rproc_report_crash, then I think following is more
>>>>> flexible:
>>>>>
>>>>> rproc_report_crash(rproc, const char *crash_reason_str);
>>>>>
>>>>> Then each platform driver can print crash reason however they see fit.
>>>>> We can also avoid printing crash reason two times this way.
>>>>>
>>>>
>>>> Hi Bjorn,
>>>>
>>>> I take this back. I think crash_reason can be defined differently for
>>>> each firmware project. I would like to provide that flexibility to the
>>>> firmware developer. Hence, I prefer not to convert crash_reason integer
>>>> to human readable string, as can be different for different fw projects.
>>>>
>>>
>>> Then we certainly shouldn't pass it as the second argument of
>>> rproc_report_crash().
>>>
>>>> Instead, the xlnx platform driver will simply print the crash_reason
>>>> integer as given by the firmware, and notify the crash to the core
>>>> framework as following:
>>>>
>>>> rproc_report_crash(rproc, RPROC_FATAL_ERROR);
>>>>
>>>> This way, we don't have to modify the rproc_report_crash() API.
>>>> I hope this makes sense.
>>>>
>>>
>>> Yes, that makes sense.
>>>
>>> I think I'd like to make the proposed modification regardless, but that
>>> is then a completely separate change.
>>>
>>>> I will wait for your response before sending the new version. Rest of
>>>> the comments I will address as asked.
>>>>
>>>
>>> Is your struct xlnx_rproc_crash_report already defined and in use by the
>>> firmware? If not, I'd recommend that you spend a little bit extra time
>>> thinking about the content of it. E.g. the human readable char [] found
>>> in Qualcomm's crash reports is quite useful...
>>>
>>
>> We can modify the resource structure as needed.
> 
> Just to make sure it's clear, this is merely a suggestion based on my
> experience, not a review request. But if the format of the structure
> isn't already defined, I would suggest packing some additional
> information in there.
> 

Thanks Bjorn, yes I understand this. In fact people's past experiences
help new code development robust and I appreciate every suggestion.

>> I looked at the qcom
>> rproc crash report. I don't know much the qcom smem infrastructure, but
>> per my understanding qcom rproc uses crash_reason integer to retrieve
>> the string format of the crash reason stored in the smem, via smem
>> driver. That's too complex for my use case. Also, I prefer not to map
>> crash reason number with the fixed string. Instead would like to provide
>> flexibility to the user to insert human readable string as needed.
>>
> 
> Qualcomm has SMEM which is an allocate-only "heap" which is shared
> between Linux and the different remoteprocs, so that's used to share
> such information between the different systems.
> 
> It sounds correct to me that you don't want to introduce something like
> that to solve this problem. In fact I think your solution is quite
> elegant.
> 
>> How does following resource definition looks like?
>>
>> struct fw_rsc_xlnx_crash_report {
>>         uint32_t type;
> 
> It sounds like a good idea to have a "type" or "version" here, to allow
> changing the struct at a later point if necessary.
> 

Thanks, I will add version field.

> But you're probably only going to be using a few bits here, and iiuc you
> only use 1 bit from the "crash_state". So you should be able to reduce
> the size of these types.
> 

Yes I think changing type to u8 makes sense.

>>         uint32_t crash_state;
>>         uint32_t crash_reason;
>>         char crash_reason_str[16];
> 
> Would that be sufficient to convey a good error message?
> 
> As an example I triggered an error on one of the Qualcomm remoteprocs
> earlier today, the error message is 17 characters, but then it includes
> the process, file and line number of the error and the caller. So in
> total it's 73 characters - but it gave me a pretty good idea of what
> went wrong.

This crash reason string is not meant for long debug/error messages.
Example of crash reasons I am expecting as following:

"mem fault"
"hard fault"
"thermal fault"
"io fault"
"watchdog rst"
"power on rst" etc..

Just some short description of the reason to report the crash.

We have other features in the remoteproc subsystem for long messages
such as "trace buffers" and "coredump". Users can use those features to
print long error/debug messages when needed.

> 
>>         uint32_t reserved;
> 
> Why pad here? If you want an even size, make the string 20 characters
> (or adjust the u32 pair at the beginning). If you want room for future
> improvements, use the "type" field for that.
> 

Ack, I think this is not needed.

>> } __packed;
>>
>> So, if the user prefer to provide human readable string along with the
>> integer, then 16 characters should be enough, and they can choose any
>> string to insert when reporting the crash.
>>
> 
> Give it some extra thought, and weigh it against memory budget etc and
> pick something that you think suits you.
> 

Good idea, I will redesign & re-test this as discussed, and think about
future extension as well.

>> Linux side will simply print those 16 characters as string. We don't
>> need to verify the content of it. It is users responsibility to make
>> sure the characters are valid, if not crash_reason_str[0] should be '\0'.
>>
> 
> I'd still suggest that you \0-terminate the string explicitly on the
> Linux side, just to be safe. You don't want a firmware bug to crash your
> Linux driver.
> 

Yup, Ack. Also, while printing I will print only first 15 characters.

> Regards,
> Bjorn
> 
>> Thanks,
>> Tanmay
>>
>>> Regards,
>>> Bjorn
>>>
>>>> Thanks,
>>>> Tanmay
>>>>
>>>>> If we do this, then crash_reason can be defined for each driver
>>>>> individually. That's more appropriate as each vendor can have different
>>>>> enum for crash.
>>>>>
>>>>> Let me know your thoughts.
>>>>>
>>>>>>> +			r5_core->crash_report->crash_state = 0;
>>>>>>> +			r5_core->crash_report->crash_reason = 0;
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +
>>>>>>>  	/* received and processed interrupt ack */
>>>>>>>  	if (mbox_send_message(ipi->rx_chan, NULL) < 0)
>>>>>>>  		dev_err(cl->dev, "ack failed to mbox rx_chan\n");
>>>>>>> @@ -438,6 +469,13 @@ static int zynqmp_r5_rproc_stop(struct rproc *rproc)
>>>>>>>  	if (ret)
>>>>>>>  		dev_err(r5_core->dev, "core force power down failed\n");
>>>>>>>  
>>>>>>> +	/*
>>>>>>> +	 * Clear attach on recovery flag during stop operation. The next state
>>>>>>> +	 * of the remote processor is expected to be "Running" state. In this
>>>>>>> +	 * state boot recovery method must take place over attach on recovery.
>>>>>>> +	 */
>>>>>>> +	test_and_clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>>>>>> +
>>>>>>>  	return ret;
>>>>>>>  }
>>>>>>>  
>>>>>>> @@ -859,6 +897,9 @@ static int zynqmp_r5_get_rsc_table_va(struct zynqmp_r5_core *r5_core)
>>>>>>>  
>>>>>>>  static int zynqmp_r5_attach(struct rproc *rproc)
>>>>>>>  {
>>>>>>> +	/* Enable attach on recovery method. Clear it during rproc stop. */
>>>>>>> +	rproc_set_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY);
>>>>>>> +
>>>>>>>  	dev_dbg(&rproc->dev, "rproc %d attached\n", rproc->index);
>>>>>>>  
>>>>>>>  	return 0;
>>>>>>> @@ -873,9 +914,25 @@ static int zynqmp_r5_detach(struct rproc *rproc)
>>>>>>>  	 */
>>>>>>>  	zynqmp_r5_rproc_kick(rproc, 0);
>>>>>>>  
>>>>>>> +	clear_bit(RPROC_FEAT_ATTACH_ON_RECOVERY, rproc->features);
>>>>>>> +
>>>>>>>  	return 0;
>>>>>>>  }
>>>>>>>  
>>>>>>> +static int zynqmp_r5_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc,
>>>>>>> +				int offset, int avail)
>>>>>>> +{
>>>>>>> +	struct zynqmp_r5_core *r5_core = rproc->priv;
>>>>>>> +	void *rsc_offset = (r5_core->rsc_tbl_va + offset);
>>>>>>> +
>>>>>>> +	if (rsc_type == FW_RSC_VENDOR_CRASH_REASON)
>>>>>>> +		r5_core->crash_report = (struct xlnx_rproc_crash_report *)(rsc_offset);
>>>>>>
>>>>>> I don't think you need the cast.
>>>>>>
>>>>>> Regards,
>>>>>> Bjorn
>>>>>>
>>>>>>> +	else
>>>>>>> +		return RSC_IGNORED;
>>>>>>> +
>>>>>>> +	return RSC_HANDLED;
>>>>>>> +}
>>>>>>> +
>>>>>>>  static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>>>>>  	.prepare	= zynqmp_r5_rproc_prepare,
>>>>>>>  	.unprepare	= zynqmp_r5_rproc_unprepare,
>>>>>>> @@ -890,6 +947,7 @@ static const struct rproc_ops zynqmp_r5_rproc_ops = {
>>>>>>>  	.get_loaded_rsc_table = zynqmp_r5_get_loaded_rsc_table,
>>>>>>>  	.attach		= zynqmp_r5_attach,
>>>>>>>  	.detach		= zynqmp_r5_detach,
>>>>>>> +	.handle_rsc	= zynqmp_r5_handle_rsc,
>>>>>>>  };
>>>>>>>  
>>>>>>>  /**
>>>>>>> @@ -923,7 +981,7 @@ static struct zynqmp_r5_core *zynqmp_r5_add_rproc_core(struct device *cdev)
>>>>>>>  
>>>>>>>  	rproc_coredump_set_elf_info(r5_rproc, ELFCLASS32, EM_ARM);
>>>>>>>  
>>>>>>> -	r5_rproc->recovery_disabled = true;
>>>>>>> +	r5_rproc->recovery_disabled = false;
>>>>>>>  	r5_rproc->has_iommu = false;
>>>>>>>  	r5_rproc->auto_boot = false;
>>>>>>>  	r5_core = r5_rproc->priv;
>>>>>>> -- 
>>>>>>> 2.34.1
>>>>>>>
>>>>>
>>>>
>>