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

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

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
>

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

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();
>

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