[PATCH v2 0/7] remoteproc: core: misc update

[PATCH v2 0/7] remoteproc: core: misc update

[Peng Fan]

This patchset is a misc update of remoteproc_core.c.
Patch 1: Drop a pointless initialization to variable ret
Patch 2-3: Cleanup the included headers
Patch 4: Use cleanup.h to simplify code
Patch 5: Remove export of rproc_va_to_pa
Patch 6: Update stm32_rproc auto_boot assignment.
Patch 7: Use 1-bit bitfields for bool

I am also reviewing the rproc->lock usage and thinking whether we
need to add a lockdep_assert_held for some functions that should have
lock held. But not sure.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Add patch 6 "remoteproc: stm32: Avoid directly taking address of auto_boot"
  to address stm32_rproc.c build issue
- Link to v1: https://lore.kernel.org/r/20251005-remoteproc-cleanup-v1-0-09a9fdea0063@nxp.com

---
Peng Fan (7):
      remoteproc: core: Drop redundant initialization of 'ret' in rproc_shutdown()
      remoteproc: core: Sort header includes
      remoteproc: core: Removed unused headers
      remoteproc: core: Use cleanup.h macros to simplify lock handling
      remoteproc: core: Remove unused export of rproc_va_to_pa
      remoteproc: stm32: Avoid directly taking address of auto_boot
      remoteproc: core: Consolidate bool flags into 1-bit bitfields

 drivers/remoteproc/remoteproc_core.c | 144 ++++++++++++++---------------------
 drivers/remoteproc/stm32_rproc.c     |   5 +-
 include/linux/remoteproc.h           |  18 ++---
 3 files changed, 71 insertions(+), 96 deletions(-)
---
base-commit: 3b9b1f8df454caa453c7fb07689064edb2eda90a
change-id: 20251003-remoteproc-cleanup-345cd50fe138

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v2 1/7] remoteproc: core: Drop redundant initialization of 'ret' in rproc_shutdown()

[Peng Fan]

The variable ret is immediately assigned the return value of
mutex_lock_interruptible(), making its prior initialization to zero
unnecessary. Remove the redundant assignment

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 82567210052893a501e7591204af1feb07befb22..29bbaa349e340eedd122fb553004f7e6a5c46e55 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -1989,7 +1989,7 @@ EXPORT_SYMBOL(rproc_boot);
 int rproc_shutdown(struct rproc *rproc)
 {
 	struct device *dev = &rproc->dev;
-	int ret = 0;
+	int ret;
 
 	ret = mutex_lock_interruptible(&rproc->lock);
 	if (ret) {

-- 
2.37.1

[PATCH v2 2/7] remoteproc: core: Sort header includes

[Peng Fan]

Reordered the header includes in drivers/remoteproc/remoteproc_core.c
to follow alphabetical order to simplify future maintenance.

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 29bbaa349e340eedd122fb553004f7e6a5c46e55..f7d21e99d171667d925de769db003c4e13fe8fe8 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -16,29 +16,29 @@
 
 #define pr_fmt(fmt)    "%s: " fmt, __func__
 
+#include <asm/byteorder.h>
+#include <linux/crc32.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
 #include <linux/device.h>
-#include <linux/panic_notifier.h>
-#include <linux/slab.h>
-#include <linux/mutex.h>
 #include <linux/dma-mapping.h>
+#include <linux/elf.h>
 #include <linux/firmware.h>
-#include <linux/string.h>
-#include <linux/debugfs.h>
-#include <linux/rculist.h>
-#include <linux/remoteproc.h>
-#include <linux/iommu.h>
 #include <linux/idr.h>
-#include <linux/elf.h>
-#include <linux/crc32.h>
+#include <linux/iommu.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_platform.h>
 #include <linux/of_reserved_mem.h>
+#include <linux/panic_notifier.h>
+#include <linux/platform_device.h>
+#include <linux/rculist.h>
+#include <linux/remoteproc.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
-#include <asm/byteorder.h>
-#include <linux/platform_device.h>
 
 #include "remoteproc_internal.h"
 

-- 
2.37.1

[PATCH v2 3/7] remoteproc: core: Removed unused headers

[Peng Fan]

There is no user of crc32.h, debugfs.h, of_reserved_mem.h, virtio_ids.h,
so remove from the included headers.

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index f7d21e99d171667d925de769db003c4e13fe8fe8..8004a480348378abef78ad5641a8c8b5766c20a6 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -17,8 +17,6 @@
 #define pr_fmt(fmt)    "%s: " fmt, __func__
 
 #include <asm/byteorder.h>
-#include <linux/crc32.h>
-#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -30,14 +28,12 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of_platform.h>
-#include <linux/of_reserved_mem.h>
 #include <linux/panic_notifier.h>
 #include <linux/platform_device.h>
 #include <linux/rculist.h>
 #include <linux/remoteproc.h>
 #include <linux/slab.h>
 #include <linux/string.h>
-#include <linux/virtio_ids.h>
 #include <linux/virtio_ring.h>
 
 #include "remoteproc_internal.h"

-- 
2.37.1

[PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Peng Fan]

Replace manual mutex_lock/unlock and error-handling patterns with cleanup.h
macros (ACQUIRE, ACQUIRE_ERR, and scoped_guard) to streamline lock
management. As a result, several goto labels and redundant error paths are
eliminated.

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 113 ++++++++++++++---------------------
 1 file changed, 45 insertions(+), 68 deletions(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index 8004a480348378abef78ad5641a8c8b5766c20a6..dd859378f6ff6dec2728980cc82d31687aa7a3dc 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -17,6 +17,7 @@
 #define pr_fmt(fmt)    "%s: " fmt, __func__
 
 #include <asm/byteorder.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -1830,13 +1831,14 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	struct device *dev = &rproc->dev;
 	int ret;
 
-	ret = mutex_lock_interruptible(&rproc->lock);
+	ACQUIRE(mutex_intr, lock)(&rproc->lock);
+	ret = ACQUIRE_ERR(mutex_intr, &lock);
 	if (ret)
 		return ret;
 
 	/* State could have changed before we got the mutex */
 	if (rproc->state != RPROC_CRASHED)
-		goto unlock_mutex;
+		return ret;
 
 	dev_err(dev, "recovering %s\n", rproc->name);
 
@@ -1845,8 +1847,6 @@ int rproc_trigger_recovery(struct rproc *rproc)
 	else
 		ret = rproc_boot_recovery(rproc);
 
-unlock_mutex:
-	mutex_unlock(&rproc->lock);
 	return ret;
 }
 
@@ -1864,25 +1864,19 @@ static void rproc_crash_handler_work(struct work_struct *work)
 
 	dev_dbg(dev, "enter %s\n", __func__);
 
-	mutex_lock(&rproc->lock);
-
-	if (rproc->state == RPROC_CRASHED) {
+	scoped_guard(mutex, &rproc->lock) {
 		/* handle only the first crash detected */
-		mutex_unlock(&rproc->lock);
-		return;
-	}
+		if (rproc->state == RPROC_CRASHED)
+			return;
 
-	if (rproc->state == RPROC_OFFLINE) {
 		/* Don't recover if the remote processor was stopped */
-		mutex_unlock(&rproc->lock);
-		goto out;
-	}
-
-	rproc->state = RPROC_CRASHED;
-	dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
-		rproc->name);
+		if (rproc->state == RPROC_OFFLINE)
+			goto out;
 
-	mutex_unlock(&rproc->lock);
+		rproc->state = RPROC_CRASHED;
+		dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
+			rproc->name);
+	}
 
 	if (!rproc->recovery_disabled)
 		rproc_trigger_recovery(rproc);
@@ -1915,23 +1909,21 @@ int rproc_boot(struct rproc *rproc)
 
 	dev = &rproc->dev;
 
-	ret = mutex_lock_interruptible(&rproc->lock);
+	ACQUIRE(mutex_intr, lock)(&rproc->lock);
+	ret = ACQUIRE_ERR(mutex_intr, &lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
 		return ret;
 	}
 
 	if (rproc->state == RPROC_DELETED) {
-		ret = -ENODEV;
 		dev_err(dev, "can't boot deleted rproc %s\n", rproc->name);
-		goto unlock_mutex;
+		return -ENODEV;
 	}
 
 	/* skip the boot or attach process if rproc is already powered up */
-	if (atomic_inc_return(&rproc->power) > 1) {
-		ret = 0;
-		goto unlock_mutex;
-	}
+	if (atomic_inc_return(&rproc->power) > 1)
+		return 0;
 
 	if (rproc->state == RPROC_DETACHED) {
 		dev_info(dev, "attaching to %s\n", rproc->name);
@@ -1955,8 +1947,7 @@ int rproc_boot(struct rproc *rproc)
 downref_rproc:
 	if (ret)
 		atomic_dec(&rproc->power);
-unlock_mutex:
-	mutex_unlock(&rproc->lock);
+
 	return ret;
 }
 EXPORT_SYMBOL(rproc_boot);
@@ -1987,26 +1978,24 @@ int rproc_shutdown(struct rproc *rproc)
 	struct device *dev = &rproc->dev;
 	int ret;
 
-	ret = mutex_lock_interruptible(&rproc->lock);
+	ACQUIRE(mutex_intr, lock)(&rproc->lock);
+	ret = ACQUIRE_ERR(mutex_intr, &lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
 		return ret;
 	}
 
-	if (rproc->state != RPROC_RUNNING &&
-	    rproc->state != RPROC_ATTACHED) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED)
+		return -EINVAL;
 
 	/* if the remote proc is still needed, bail out */
 	if (!atomic_dec_and_test(&rproc->power))
-		goto out;
+		return ret;
 
 	ret = rproc_stop(rproc, false);
 	if (ret) {
 		atomic_inc(&rproc->power);
-		goto out;
+		return ret;
 	}
 
 	/* clean up all acquired resources */
@@ -2021,8 +2010,7 @@ int rproc_shutdown(struct rproc *rproc)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
-out:
-	mutex_unlock(&rproc->lock);
+
 	return ret;
 }
 EXPORT_SYMBOL(rproc_shutdown);
@@ -2052,27 +2040,25 @@ int rproc_detach(struct rproc *rproc)
 	struct device *dev = &rproc->dev;
 	int ret;
 
-	ret = mutex_lock_interruptible(&rproc->lock);
+	ACQUIRE(mutex_intr, lock)(&rproc->lock);
+	ret = ACQUIRE_ERR(mutex_intr, &lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
 		return ret;
 	}
 
 	if (rproc->state != RPROC_ATTACHED) {
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	/* if the remote proc is still needed, bail out */
-	if (!atomic_dec_and_test(&rproc->power)) {
-		ret = 0;
-		goto out;
-	}
+	if (!atomic_dec_and_test(&rproc->power))
+		return 0;
 
 	ret = __rproc_detach(rproc);
 	if (ret) {
 		atomic_inc(&rproc->power);
-		goto out;
+		return ret;
 	}
 
 	/* clean up all acquired resources */
@@ -2087,8 +2073,7 @@ int rproc_detach(struct rproc *rproc)
 	kfree(rproc->cached_table);
 	rproc->cached_table = NULL;
 	rproc->table_ptr = NULL;
-out:
-	mutex_unlock(&rproc->lock);
+
 	return ret;
 }
 EXPORT_SYMBOL(rproc_detach);
@@ -2192,7 +2177,8 @@ int rproc_set_firmware(struct rproc *rproc, const char *fw_name)
 
 	dev = rproc->dev.parent;
 
-	ret = mutex_lock_interruptible(&rproc->lock);
+	ACQUIRE(mutex_intr, lock)(&rproc->lock);
+	ret = ACQUIRE_ERR(mutex_intr, &lock);
 	if (ret) {
 		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
 		return -EINVAL;
@@ -2200,28 +2186,22 @@ int rproc_set_firmware(struct rproc *rproc, const char *fw_name)
 
 	if (rproc->state != RPROC_OFFLINE) {
 		dev_err(dev, "can't change firmware while running\n");
-		ret = -EBUSY;
-		goto out;
+		return -EBUSY;
 	}
 
 	len = strcspn(fw_name, "\n");
 	if (!len) {
 		dev_err(dev, "can't provide empty string for firmware name\n");
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
 	p = kstrndup(fw_name, len, GFP_KERNEL);
-	if (!p) {
-		ret = -ENOMEM;
-		goto out;
-	}
+	if (!p)
+		return -ENOMEM;
 
 	kfree_const(rproc->firmware);
 	rproc->firmware = p;
 
-out:
-	mutex_unlock(&rproc->lock);
 	return ret;
 }
 EXPORT_SYMBOL(rproc_set_firmware);
@@ -2316,9 +2296,8 @@ int rproc_add(struct rproc *rproc)
 	}
 
 	/* expose to rproc_get_by_phandle users */
-	mutex_lock(&rproc_list_mutex);
-	list_add_rcu(&rproc->node, &rproc_list);
-	mutex_unlock(&rproc_list_mutex);
+	scoped_guard(mutex, &rproc_list_mutex)
+		list_add_rcu(&rproc->node, &rproc_list);
 
 	return 0;
 
@@ -2582,16 +2561,14 @@ int rproc_del(struct rproc *rproc)
 	/* TODO: make sure this works with rproc->power > 1 */
 	rproc_shutdown(rproc);
 
-	mutex_lock(&rproc->lock);
-	rproc->state = RPROC_DELETED;
-	mutex_unlock(&rproc->lock);
+	scoped_guard(mutex, &rproc->lock)
+		rproc->state = RPROC_DELETED;
 
 	rproc_delete_debug_dir(rproc);
 
 	/* the rproc is downref'ed as soon as it's removed from the klist */
-	mutex_lock(&rproc_list_mutex);
-	list_del_rcu(&rproc->node);
-	mutex_unlock(&rproc_list_mutex);
+	scoped_guard(mutex, &rproc_list_mutex)
+		list_del_rcu(&rproc->node);
 
 	/* Ensure that no readers of rproc_list are still active */
 	synchronize_rcu();

-- 
2.37.1

[PATCH v2 5/7] remoteproc: core: Remove unused export of rproc_va_to_pa

[Peng Fan]

commit 086d08725d34 ("remoteproc: create vdev subdevice with specific dma
memory pool") added an export for rproc_va_to_pa. However, since its
introduction, this symbol has not been used by any loadable modules. It
remains only referenced within remoteproc_virtio.c, which is always built
together with remoteproc_core.c.

As such, exporting rproc_va_to_pa is unnecessary, so remove the export.

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/remoteproc_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
index dd859378f6ff6dec2728980cc82d31687aa7a3dc..383479d624c89da1c481adc956a169c03b793bcc 100644
--- a/drivers/remoteproc/remoteproc_core.c
+++ b/drivers/remoteproc/remoteproc_core.c
@@ -156,7 +156,6 @@ phys_addr_t rproc_va_to_pa(void *cpu_addr)
 	WARN_ON(!virt_addr_valid(cpu_addr));
 	return virt_to_phys(cpu_addr);
 }
-EXPORT_SYMBOL(rproc_va_to_pa);
 
 /**
  * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address

-- 
2.37.1

[PATCH v2 6/7] remoteproc: stm32: Avoid directly taking address of auto_boot

[Peng Fan]

The rproc->auto_boot field is going to be defined as a bit-field, which
makes it illegal to take its address in C.

To avoid build issue, a temporary boolean variable is introduced in
stm32_rproc_probe() to hold the parsed value from the device tree.
The value is then assigned to rproc->auto_boot after parsing.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/remoteproc/stm32_rproc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 431648607d53ae58a9a556d53f17b1bf924bcd80..b28907c240125cdcf73867e2704eaa974d5e1401 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -838,6 +838,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
 	const char *fw_name;
 	struct rproc *rproc;
 	unsigned int state;
+	bool auto_boot;
 	int ret;
 
 	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
@@ -857,10 +858,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
 
 	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
 
-	ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
+	ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot);
 	if (ret)
 		goto free_rproc;
 
+	rproc->auto_boot = auto_boot;
+
 	ret = stm32_rproc_of_memory_translations(pdev, ddata);
 	if (ret)
 		goto free_rproc;

-- 
2.37.1

[PATCH v2 7/7] remoteproc: core: Consolidate bool flags into 1-bit bitfields

[Peng Fan]

Per Documentation/process/coding-style.rst rule 17 regarding the use of
bool types:
If a structure has many true/false values, consider consolidating them into
a bitfield with 1-bit members, or using an appropriate fixed-width type
such as u8.

This commit replaces multiple bool members in struct rproc with 1-bit
bitfields and groups them together. This change reduces the overall size of
struct rproc from 0x4d8 to 0x4c8 on ARM64.

No functional changes.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 include/linux/remoteproc.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index b4795698d8c2a4e80ccafbe632436c4dfb636a1e..d8468a96edfbd82f4011881c10f59bf7c12e9c1a 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -528,21 +528,21 @@ enum rproc_features {
  * @index: index of this rproc device
  * @crash_handler: workqueue for handling a crash
  * @crash_cnt: crash counter
- * @recovery_disabled: flag that state if recovery was disabled
  * @max_notifyid: largest allocated notify id.
  * @table_ptr: pointer to the resource table in effect
  * @clean_table: copy of the resource table without modifications.  Used
  *		 when a remote processor is attached or detached from the core
  * @cached_table: copy of the resource table
  * @table_sz: size of @cached_table
- * @has_iommu: flag to indicate if remote processor is behind an MMU
- * @auto_boot: flag to indicate if remote processor should be auto-started
- * @sysfs_read_only: flag to make remoteproc sysfs files read only
  * @dump_segments: list of segments in the firmware
  * @nb_vdev: number of vdev currently handled by rproc
  * @elf_class: firmware ELF class
  * @elf_machine: firmware ELF machine
  * @cdev: character device of the rproc
+ * @recovery_disabled: flag that state if recovery was disabled
+ * @has_iommu: flag to indicate if remote processor is behind an MMU
+ * @auto_boot: flag to indicate if remote processor should be auto-started
+ * @sysfs_read_only: flag to make remoteproc sysfs files read only
  * @cdev_put_on_release: flag to indicate if remoteproc should be shutdown on @char_dev release
  * @features: indicate remoteproc features
  */
@@ -570,21 +570,21 @@ struct rproc {
 	int index;
 	struct work_struct crash_handler;
 	unsigned int crash_cnt;
-	bool recovery_disabled;
 	int max_notifyid;
 	struct resource_table *table_ptr;
 	struct resource_table *clean_table;
 	struct resource_table *cached_table;
 	size_t table_sz;
-	bool has_iommu;
-	bool auto_boot;
-	bool sysfs_read_only;
 	struct list_head dump_segments;
 	int nb_vdev;
 	u8 elf_class;
 	u16 elf_machine;
 	struct cdev cdev;
-	bool cdev_put_on_release;
+	bool recovery_disabled :1;
+	bool has_iommu :1;
+	bool auto_boot :1;
+	bool sysfs_read_only :1;
+	bool cdev_put_on_release :1;
 	DECLARE_BITMAP(features, RPROC_MAX_FEATURES);
 };
 

-- 
2.37.1

Re: [PATCH v2 1/7] remoteproc: core: Drop redundant initialization of 'ret' in rproc_shutdown()

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> The variable ret is immediately assigned the return value of
> mutex_lock_interruptible(), making its prior initialization to zero
> unnecessary. Remove the redundant assignment
> 
> No functional changes.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Acked-by: Andrew Davis <afd@ti.com>

>   drivers/remoteproc/remoteproc_core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 82567210052893a501e7591204af1feb07befb22..29bbaa349e340eedd122fb553004f7e6a5c46e55 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1989,7 +1989,7 @@ EXPORT_SYMBOL(rproc_boot);
>   int rproc_shutdown(struct rproc *rproc)
>   {
>   	struct device *dev = &rproc->dev;
> -	int ret = 0;
> +	int ret;
>   
>   	ret = mutex_lock_interruptible(&rproc->lock);
>   	if (ret) {
> 

Re: [PATCH v2 2/7] remoteproc: core: Sort header includes

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> Reordered the header includes in drivers/remoteproc/remoteproc_core.c
> to follow alphabetical order to simplify future maintenance.
> 
> No functional changes.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Acked-by: Andrew Davis <afd@ti.com>

>   drivers/remoteproc/remoteproc_core.c | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 29bbaa349e340eedd122fb553004f7e6a5c46e55..f7d21e99d171667d925de769db003c4e13fe8fe8 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -16,29 +16,29 @@
>   
>   #define pr_fmt(fmt)    "%s: " fmt, __func__
>   
> +#include <asm/byteorder.h>
> +#include <linux/crc32.h>
> +#include <linux/debugfs.h>
>   #include <linux/delay.h>
> -#include <linux/kernel.h>
> -#include <linux/module.h>
>   #include <linux/device.h>
> -#include <linux/panic_notifier.h>
> -#include <linux/slab.h>
> -#include <linux/mutex.h>
>   #include <linux/dma-mapping.h>
> +#include <linux/elf.h>
>   #include <linux/firmware.h>
> -#include <linux/string.h>
> -#include <linux/debugfs.h>
> -#include <linux/rculist.h>
> -#include <linux/remoteproc.h>
> -#include <linux/iommu.h>
>   #include <linux/idr.h>
> -#include <linux/elf.h>
> -#include <linux/crc32.h>
> +#include <linux/iommu.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
>   #include <linux/of_platform.h>
>   #include <linux/of_reserved_mem.h>
> +#include <linux/panic_notifier.h>
> +#include <linux/platform_device.h>
> +#include <linux/rculist.h>
> +#include <linux/remoteproc.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
>   #include <linux/virtio_ids.h>
>   #include <linux/virtio_ring.h>
> -#include <asm/byteorder.h>
> -#include <linux/platform_device.h>
>   
>   #include "remoteproc_internal.h"
>   
> 

Re: [PATCH v2 3/7] remoteproc: core: Removed unused headers

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> There is no user of crc32.h, debugfs.h, of_reserved_mem.h, virtio_ids.h,
> so remove from the included headers.
> 
> No functional changes.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Acked-by: Andrew Davis <afd@ti.com>

>   drivers/remoteproc/remoteproc_core.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index f7d21e99d171667d925de769db003c4e13fe8fe8..8004a480348378abef78ad5641a8c8b5766c20a6 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -17,8 +17,6 @@
>   #define pr_fmt(fmt)    "%s: " fmt, __func__
>   
>   #include <asm/byteorder.h>
> -#include <linux/crc32.h>
> -#include <linux/debugfs.h>
>   #include <linux/delay.h>
>   #include <linux/device.h>
>   #include <linux/dma-mapping.h>
> @@ -30,14 +28,12 @@
>   #include <linux/module.h>
>   #include <linux/mutex.h>
>   #include <linux/of_platform.h>
> -#include <linux/of_reserved_mem.h>
>   #include <linux/panic_notifier.h>
>   #include <linux/platform_device.h>
>   #include <linux/rculist.h>
>   #include <linux/remoteproc.h>
>   #include <linux/slab.h>
>   #include <linux/string.h>
> -#include <linux/virtio_ids.h>
>   #include <linux/virtio_ring.h>
>   
>   #include "remoteproc_internal.h"
> 

Re: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> Replace manual mutex_lock/unlock and error-handling patterns with cleanup.h
> macros (ACQUIRE, ACQUIRE_ERR, and scoped_guard) to streamline lock
> management. As a result, several goto labels and redundant error paths are
> eliminated.
> 
> No functional changes.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/remoteproc/remoteproc_core.c | 113 ++++++++++++++---------------------
>   1 file changed, 45 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index 8004a480348378abef78ad5641a8c8b5766c20a6..dd859378f6ff6dec2728980cc82d31687aa7a3dc 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -17,6 +17,7 @@
>   #define pr_fmt(fmt)    "%s: " fmt, __func__
>   
>   #include <asm/byteorder.h>
> +#include <linux/cleanup.h>
>   #include <linux/delay.h>
>   #include <linux/device.h>
>   #include <linux/dma-mapping.h>
> @@ -1830,13 +1831,14 @@ int rproc_trigger_recovery(struct rproc *rproc)
>   	struct device *dev = &rproc->dev;
>   	int ret;
>   
> -	ret = mutex_lock_interruptible(&rproc->lock);
> +	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
>   	if (ret)
>   		return ret;
>   
>   	/* State could have changed before we got the mutex */
>   	if (rproc->state != RPROC_CRASHED)
> -		goto unlock_mutex;
> +		return ret;
>   
>   	dev_err(dev, "recovering %s\n", rproc->name);
>   
> @@ -1845,8 +1847,6 @@ int rproc_trigger_recovery(struct rproc *rproc)
>   	else
>   		ret = rproc_boot_recovery(rproc);
>   
> -unlock_mutex:
> -	mutex_unlock(&rproc->lock);
>   	return ret;
>   }
>   
> @@ -1864,25 +1864,19 @@ static void rproc_crash_handler_work(struct work_struct *work)
>   
>   	dev_dbg(dev, "enter %s\n", __func__);
>   
> -	mutex_lock(&rproc->lock);
> -
> -	if (rproc->state == RPROC_CRASHED) {
> +	scoped_guard(mutex, &rproc->lock) {

Not sure this one is worth switching to scoped_guard as is doesn't save
us needing the goto out label. Plus it adds indent to a bunch of lines.

>   		/* handle only the first crash detected */
> -		mutex_unlock(&rproc->lock);
> -		return;
> -	}
> +		if (rproc->state == RPROC_CRASHED)
> +			return;
>   
> -	if (rproc->state == RPROC_OFFLINE) {
>   		/* Don't recover if the remote processor was stopped */
> -		mutex_unlock(&rproc->lock);
> -		goto out;
> -	}
> -
> -	rproc->state = RPROC_CRASHED;
> -	dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
> -		rproc->name);
> +		if (rproc->state == RPROC_OFFLINE)
> +			goto out;
>   
> -	mutex_unlock(&rproc->lock);
> +		rproc->state = RPROC_CRASHED;
> +		dev_err(dev, "handling crash #%u in %s\n", ++rproc->crash_cnt,
> +			rproc->name);
> +	}
>   
>   	if (!rproc->recovery_disabled)
>   		rproc_trigger_recovery(rproc);
> @@ -1915,23 +1909,21 @@ int rproc_boot(struct rproc *rproc)
>   
>   	dev = &rproc->dev;
>   
> -	ret = mutex_lock_interruptible(&rproc->lock);
> +	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
>   	if (ret) {
>   		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>   		return ret;
>   	}
>   
>   	if (rproc->state == RPROC_DELETED) {
> -		ret = -ENODEV;
>   		dev_err(dev, "can't boot deleted rproc %s\n", rproc->name);
> -		goto unlock_mutex;
> +		return -ENODEV;
>   	}
>   
>   	/* skip the boot or attach process if rproc is already powered up */
> -	if (atomic_inc_return(&rproc->power) > 1) {
> -		ret = 0;
> -		goto unlock_mutex;
> -	}
> +	if (atomic_inc_return(&rproc->power) > 1)
> +		return 0;
>   
>   	if (rproc->state == RPROC_DETACHED) {
>   		dev_info(dev, "attaching to %s\n", rproc->name);
> @@ -1955,8 +1947,7 @@ int rproc_boot(struct rproc *rproc)
>   downref_rproc:
>   	if (ret)
>   		atomic_dec(&rproc->power);
> -unlock_mutex:
> -	mutex_unlock(&rproc->lock);
> +
>   	return ret;
>   }
>   EXPORT_SYMBOL(rproc_boot);
> @@ -1987,26 +1978,24 @@ int rproc_shutdown(struct rproc *rproc)
>   	struct device *dev = &rproc->dev;
>   	int ret;
>   
> -	ret = mutex_lock_interruptible(&rproc->lock);
> +	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
>   	if (ret) {
>   		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>   		return ret;
>   	}
>   
> -	if (rproc->state != RPROC_RUNNING &&
> -	    rproc->state != RPROC_ATTACHED) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED)

I liked this better as two lines

if (rproc->state != RPROC_RUNNING &&
     rproc->state != RPROC_ATTACHED) {

> +		return -EINVAL;
>   
>   	/* if the remote proc is still needed, bail out */
>   	if (!atomic_dec_and_test(&rproc->power))
> -		goto out;
> +		return ret;
>   
>   	ret = rproc_stop(rproc, false);
>   	if (ret) {
>   		atomic_inc(&rproc->power);
> -		goto out;
> +		return ret;
>   	}
>   
>   	/* clean up all acquired resources */
> @@ -2021,8 +2010,7 @@ int rproc_shutdown(struct rproc *rproc)
>   	kfree(rproc->cached_table);
>   	rproc->cached_table = NULL;
>   	rproc->table_ptr = NULL;
> -out:
> -	mutex_unlock(&rproc->lock);
> +
>   	return ret;
>   }
>   EXPORT_SYMBOL(rproc_shutdown);
> @@ -2052,27 +2040,25 @@ int rproc_detach(struct rproc *rproc)
>   	struct device *dev = &rproc->dev;
>   	int ret;
>   
> -	ret = mutex_lock_interruptible(&rproc->lock);
> +	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
>   	if (ret) {
>   		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>   		return ret;
>   	}
>   
>   	if (rproc->state != RPROC_ATTACHED) {
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}

The above becomes one line, so you can drop the { }

Andrew

>   
>   	/* if the remote proc is still needed, bail out */
> -	if (!atomic_dec_and_test(&rproc->power)) {
> -		ret = 0;
> -		goto out;
> -	}
> +	if (!atomic_dec_and_test(&rproc->power))
> +		return 0;
>   
>   	ret = __rproc_detach(rproc);
>   	if (ret) {
>   		atomic_inc(&rproc->power);
> -		goto out;
> +		return ret;
>   	}
>   
>   	/* clean up all acquired resources */
> @@ -2087,8 +2073,7 @@ int rproc_detach(struct rproc *rproc)
>   	kfree(rproc->cached_table);
>   	rproc->cached_table = NULL;
>   	rproc->table_ptr = NULL;
> -out:
> -	mutex_unlock(&rproc->lock);
> +
>   	return ret;
>   }
>   EXPORT_SYMBOL(rproc_detach);
> @@ -2192,7 +2177,8 @@ int rproc_set_firmware(struct rproc *rproc, const char *fw_name)
>   
>   	dev = rproc->dev.parent;
>   
> -	ret = mutex_lock_interruptible(&rproc->lock);
> +	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> +	ret = ACQUIRE_ERR(mutex_intr, &lock);
>   	if (ret) {
>   		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>   		return -EINVAL;
> @@ -2200,28 +2186,22 @@ int rproc_set_firmware(struct rproc *rproc, const char *fw_name)
>   
>   	if (rproc->state != RPROC_OFFLINE) {
>   		dev_err(dev, "can't change firmware while running\n");
> -		ret = -EBUSY;
> -		goto out;
> +		return -EBUSY;
>   	}
>   
>   	len = strcspn(fw_name, "\n");
>   	if (!len) {
>   		dev_err(dev, "can't provide empty string for firmware name\n");
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
>   	}
>   
>   	p = kstrndup(fw_name, len, GFP_KERNEL);
> -	if (!p) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> +	if (!p)
> +		return -ENOMEM;
>   
>   	kfree_const(rproc->firmware);
>   	rproc->firmware = p;
>   
> -out:
> -	mutex_unlock(&rproc->lock);
>   	return ret;
>   }
>   EXPORT_SYMBOL(rproc_set_firmware);
> @@ -2316,9 +2296,8 @@ int rproc_add(struct rproc *rproc)
>   	}
>   
>   	/* expose to rproc_get_by_phandle users */
> -	mutex_lock(&rproc_list_mutex);
> -	list_add_rcu(&rproc->node, &rproc_list);
> -	mutex_unlock(&rproc_list_mutex);
> +	scoped_guard(mutex, &rproc_list_mutex)
> +		list_add_rcu(&rproc->node, &rproc_list);
>   
>   	return 0;
>   
> @@ -2582,16 +2561,14 @@ int rproc_del(struct rproc *rproc)
>   	/* TODO: make sure this works with rproc->power > 1 */
>   	rproc_shutdown(rproc);
>   
> -	mutex_lock(&rproc->lock);
> -	rproc->state = RPROC_DELETED;
> -	mutex_unlock(&rproc->lock);
> +	scoped_guard(mutex, &rproc->lock)
> +		rproc->state = RPROC_DELETED;
>   
>   	rproc_delete_debug_dir(rproc);
>   
>   	/* the rproc is downref'ed as soon as it's removed from the klist */
> -	mutex_lock(&rproc_list_mutex);
> -	list_del_rcu(&rproc->node);
> -	mutex_unlock(&rproc_list_mutex);
> +	scoped_guard(mutex, &rproc_list_mutex)
> +		list_del_rcu(&rproc->node);
>   
>   	/* Ensure that no readers of rproc_list are still active */
>   	synchronize_rcu();
> 

Re: [PATCH v2 5/7] remoteproc: core: Remove unused export of rproc_va_to_pa

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> commit 086d08725d34 ("remoteproc: create vdev subdevice with specific dma
> memory pool") added an export for rproc_va_to_pa. However, since its
> introduction, this symbol has not been used by any loadable modules. It
> remains only referenced within remoteproc_virtio.c, which is always built
> together with remoteproc_core.c.
> 
> As such, exporting rproc_va_to_pa is unnecessary, so remove the export.
> 
> No functional changes.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

Acked-by: Andrew Davis <afd@ti.com>

>   drivers/remoteproc/remoteproc_core.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> index dd859378f6ff6dec2728980cc82d31687aa7a3dc..383479d624c89da1c481adc956a169c03b793bcc 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -156,7 +156,6 @@ phys_addr_t rproc_va_to_pa(void *cpu_addr)
>   	WARN_ON(!virt_addr_valid(cpu_addr));
>   	return virt_to_phys(cpu_addr);
>   }
> -EXPORT_SYMBOL(rproc_va_to_pa);
>   
>   /**
>    * rproc_da_to_va() - lookup the kernel virtual address for a remoteproc address
> 

Re: [PATCH v2 6/7] remoteproc: stm32: Avoid directly taking address of auto_boot

[Andrew Davis]

On 10/10/25 7:24 AM, Peng Fan wrote:
> The rproc->auto_boot field is going to be defined as a bit-field, which

One of the pitfalls of bit-fields :)

I'm assuming if you drop the next patch you will drop this patch too.

Andrew

> makes it illegal to take its address in C.
> 
> To avoid build issue, a temporary boolean variable is introduced in
> stm32_rproc_probe() to hold the parsed value from the device tree.
> The value is then assigned to rproc->auto_boot after parsing.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>   drivers/remoteproc/stm32_rproc.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> index 431648607d53ae58a9a556d53f17b1bf924bcd80..b28907c240125cdcf73867e2704eaa974d5e1401 100644
> --- a/drivers/remoteproc/stm32_rproc.c
> +++ b/drivers/remoteproc/stm32_rproc.c
> @@ -838,6 +838,7 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>   	const char *fw_name;
>   	struct rproc *rproc;
>   	unsigned int state;
> +	bool auto_boot;
>   	int ret;
>   
>   	ret = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> @@ -857,10 +858,12 @@ static int stm32_rproc_probe(struct platform_device *pdev)
>   
>   	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
>   
> -	ret = stm32_rproc_parse_dt(pdev, ddata, &rproc->auto_boot);
> +	ret = stm32_rproc_parse_dt(pdev, ddata, &auto_boot);
>   	if (ret)
>   		goto free_rproc;
>   
> +	rproc->auto_boot = auto_boot;
> +
>   	ret = stm32_rproc_of_memory_translations(pdev, ddata);
>   	if (ret)
>   		goto free_rproc;
> 

Re: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Dan Carpenter]

Hi Peng,

kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Peng-Fan/remoteproc-core-Drop-redundant-initialization-of-ret-in-rproc_shutdown/20251010-202737
base:   3b9b1f8df454caa453c7fb07689064edb2eda90a
patch link:    https://lore.kernel.org/r/20251010-remoteproc-cleanup-v2-4-7cecf1bfd81c%40nxp.com
patch subject: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling
config: i386-randconfig-141-20251012 (https://download.01.org/0day-ci/archive/20251012/202510121908.7aduLIkw-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202510121908.7aduLIkw-lkp@intel.com/

smatch warnings:
drivers/remoteproc/remoteproc_core.c:1841 rproc_trigger_recovery() warn: missing error code? 'ret'
drivers/remoteproc/remoteproc_core.c:1993 rproc_shutdown() warn: missing error code? 'ret'

vim +/ret +1841 drivers/remoteproc/remoteproc_core.c

70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1829  int rproc_trigger_recovery(struct rproc *rproc)
70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1830  {
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1831  	struct device *dev = &rproc->dev;
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1832  	int ret;
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1833  
c42baf6f84c7694 Peng Fan             2025-10-10  1834  	ACQUIRE(mutex_intr, lock)(&rproc->lock);
c42baf6f84c7694 Peng Fan             2025-10-10  1835  	ret = ACQUIRE_ERR(mutex_intr, &lock);
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1836  	if (ret)
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1837  		return ret;
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1838  
0b145574b6cd2b3 Alex Elder           2020-02-28  1839  	/* State could have changed before we got the mutex */
0b145574b6cd2b3 Alex Elder           2020-02-28  1840  	if (rproc->state != RPROC_CRASHED)
c42baf6f84c7694 Peng Fan             2025-10-10 @1841  		return ret;

Please change this to either "return 0;" or "return -ERRORCODE;"

0b145574b6cd2b3 Alex Elder           2020-02-28  1842  
0b145574b6cd2b3 Alex Elder           2020-02-28  1843  	dev_err(dev, "recovering %s\n", rproc->name);
0b145574b6cd2b3 Alex Elder           2020-02-28  1844  
ba194232edc032b Peng Fan             2022-09-28  1845  	if (rproc_has_feature(rproc, RPROC_FEAT_ATTACH_ON_RECOVERY))
ba194232edc032b Peng Fan             2022-09-28  1846  		ret = rproc_attach_recovery(rproc);
ba194232edc032b Peng Fan             2022-09-28  1847  	else
ba194232edc032b Peng Fan             2022-09-28  1848  		ret = rproc_boot_recovery(rproc);
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1849  
7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1850  	return ret;
70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1851  }

[ snip ]

c13b780c4597e1e Suman Anna           2022-02-13  1976  int rproc_shutdown(struct rproc *rproc)
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1977  {
b5ab5e24e960b9f Ohad Ben-Cohen       2012-05-30  1978  	struct device *dev = &rproc->dev;
ee3d85da617a065 Peng Fan             2025-10-10  1979  	int ret;
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1980  
c42baf6f84c7694 Peng Fan             2025-10-10  1981  	ACQUIRE(mutex_intr, lock)(&rproc->lock);
c42baf6f84c7694 Peng Fan             2025-10-10  1982  	ret = ACQUIRE_ERR(mutex_intr, &lock);
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1983  	if (ret) {
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1984  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
c13b780c4597e1e Suman Anna           2022-02-13  1985  		return ret;
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1986  	}
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1987  
c42baf6f84c7694 Peng Fan             2025-10-10  1988  	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED)
c42baf6f84c7694 Peng Fan             2025-10-10  1989  		return -EINVAL;
5e6a0e05270e3a4 Shengjiu Wang        2022-03-28  1990  
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1991  	/* if the remote proc is still needed, bail out */
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1992  	if (!atomic_dec_and_test(&rproc->power))
c42baf6f84c7694 Peng Fan             2025-10-10 @1993  		return ret;

Same.

400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1994  
fcd58037f28bf70 Arnaud Pouliquen     2018-04-10  1995  	ret = rproc_stop(rproc, false);
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1996  	if (ret) {
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1997  		atomic_inc(&rproc->power);
c42baf6f84c7694 Peng Fan             2025-10-10  1998  		return ret;
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1999  	}
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2000  
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2001  	/* clean up all acquired resources */
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2002  	rproc_resource_cleanup(rproc);
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2003  
33467ac3c8dc805 Loic Pallardy        2020-04-16  2004  	/* release HW resources if needed */
33467ac3c8dc805 Loic Pallardy        2020-04-16  2005  	rproc_unprepare_device(rproc);
33467ac3c8dc805 Loic Pallardy        2020-04-16  2006  
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2007  	rproc_disable_iommu(rproc);
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2008  
988d204cdaf604c Bjorn Andersson      2016-08-11  2009  	/* Free the copy of the resource table */
a0c10687ec9506b Bjorn Andersson      2016-12-30  2010  	kfree(rproc->cached_table);
a0c10687ec9506b Bjorn Andersson      2016-12-30  2011  	rproc->cached_table = NULL;
988d204cdaf604c Bjorn Andersson      2016-08-11  2012  	rproc->table_ptr = NULL;
c42baf6f84c7694 Peng Fan             2025-10-10  2013  
c13b780c4597e1e Suman Anna           2022-02-13  2014  	return ret;
400e64df6b237eb Ohad Ben-Cohen       2011-10-20  2015  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Peng Fan]

Hi Dan,

I am not sure, Is this false alarm?

On Tue, Oct 14, 2025 at 11:39:41AM +0300, Dan Carpenter wrote:
>Hi Peng,
>
>
>vim +/ret +1841 drivers/remoteproc/remoteproc_core.c
>
>70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1829  int rproc_trigger_recovery(struct rproc *rproc)
>70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1830  {
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1831  	struct device *dev = &rproc->dev;
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1832  	int ret;
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1833  
>c42baf6f84c7694 Peng Fan             2025-10-10  1834  	ACQUIRE(mutex_intr, lock)(&rproc->lock);
>c42baf6f84c7694 Peng Fan             2025-10-10  1835  	ret = ACQUIRE_ERR(mutex_intr, &lock);
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1836  	if (ret)
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1837  		return ret;
>7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1838  
>0b145574b6cd2b3 Alex Elder           2020-02-28  1839  	/* State could have changed before we got the mutex */
>0b145574b6cd2b3 Alex Elder           2020-02-28  1840  	if (rproc->state != RPROC_CRASHED)
>c42baf6f84c7694 Peng Fan             2025-10-10 @1841  		return ret;
>
>Please change this to either "return 0;" or "return -ERRORCODE;"

ACQUIRE_ERR should already returns 0. This change does not change the
assignment to ret as my understanding. Please help to see if this is false
alarm or I miss something?

>
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1980  
>c42baf6f84c7694 Peng Fan             2025-10-10  1981  	ACQUIRE(mutex_intr, lock)(&rproc->lock);
>c42baf6f84c7694 Peng Fan             2025-10-10  1982  	ret = ACQUIRE_ERR(mutex_intr, &lock);
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1983  	if (ret) {
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1984  		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, ret);
>c13b780c4597e1e Suman Anna           2022-02-13  1985  		return ret;
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1986  	}
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1987  
>c42baf6f84c7694 Peng Fan             2025-10-10  1988  	if (rproc->state != RPROC_RUNNING && rproc->state != RPROC_ATTACHED)
>c42baf6f84c7694 Peng Fan             2025-10-10  1989  		return -EINVAL;
>5e6a0e05270e3a4 Shengjiu Wang        2022-03-28  1990  
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1991  	/* if the remote proc is still needed, bail out */
>400e64df6b237eb Ohad Ben-Cohen       2011-10-20  1992  	if (!atomic_dec_and_test(&rproc->power))
>c42baf6f84c7694 Peng Fan             2025-10-10 @1993  		return ret;
>
>Same.

Same as above.

Thanks,
Peng
>

Re: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Peng Fan]

Hi Andrew,

Thanks for your reviewing.

On Fri, Oct 10, 2025 at 07:34:13PM -0500, Andrew Davis wrote:
>On 10/10/25 7:24 AM, Peng Fan wrote:
>> Replace manual mutex_lock/unlock and error-handling patterns with cleanup.h
>> macros (ACQUIRE, ACQUIRE_ERR, and scoped_guard) to streamline lock
>> management. As a result, several goto labels and redundant error paths are
>> eliminated.
>> 
>> -
>> -	if (rproc->state == RPROC_CRASHED) {
>> +	scoped_guard(mutex, &rproc->lock) {
>
>Not sure this one is worth switching to scoped_guard as is doesn't save
>us needing the goto out label. Plus it adds indent to a bunch of lines.

I was thinking to align the usage of cleanup API, avoiding mix the usage
the cleanup API and direct usage of mutex_lock/unlock API.

Considering the deadlock report [1], we may need to rethink the lock
scope in remoteproc_core.c. I gave a look on ->lock, but it is a bit
vague on which exact entries in rproc it is protecting. 
Before [1] is fixed, this patch might be dropped.

Thanks
Peng

[1]https://lore.kernel.org/linux-remoteproc/6460478.iFIW2sfyFC@nailgun/T/#u

Re: [PATCH v2 4/7] remoteproc: core: Use cleanup.h macros to simplify lock handling

[Dan Carpenter]

On Tue, Oct 14, 2025 at 06:45:11PM +0800, Peng Fan wrote:
> Hi Dan,
> 
> I am not sure, Is this false alarm?
> 
> On Tue, Oct 14, 2025 at 11:39:41AM +0300, Dan Carpenter wrote:
> >Hi Peng,
> >
> >
> >vim +/ret +1841 drivers/remoteproc/remoteproc_core.c
> >
> >70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1829  int rproc_trigger_recovery(struct rproc *rproc)
> >70b85ef83ce3523 Fernando Guzman Lugo 2012-08-30  1830  {
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1831  	struct device *dev = &rproc->dev;
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1832  	int ret;
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1833  
> >c42baf6f84c7694 Peng Fan             2025-10-10  1834  	ACQUIRE(mutex_intr, lock)(&rproc->lock);
> >c42baf6f84c7694 Peng Fan             2025-10-10  1835  	ret = ACQUIRE_ERR(mutex_intr, &lock);
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1836  	if (ret)
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1837  		return ret;
> >7e83cab824a8670 Sarangdhar Joshi     2017-05-26  1838  
> >0b145574b6cd2b3 Alex Elder           2020-02-28  1839  	/* State could have changed before we got the mutex */
> >0b145574b6cd2b3 Alex Elder           2020-02-28  1840  	if (rproc->state != RPROC_CRASHED)
> >c42baf6f84c7694 Peng Fan             2025-10-10 @1841  		return ret;
> >
> >Please change this to either "return 0;" or "return -ERRORCODE;"
> 
> ACQUIRE_ERR should already returns 0. This change does not change the
> assignment to ret as my understanding. Please help to see if this is false
> alarm or I miss something?
> 

I guess if this was already merged then it's fine.  But "return ret" looks
like an error path where "return 0;" is obvious.  This code will always
trigger a Smatch warning, and I always tell people that old code has been
reviewed so all the warnings are false positives, still someone will
eventually change this to "return -EINVAL;" because it looks so much like
a mistake.

regards,
dan carpenter