[PATCH] media: staging/ipu7: use guard(mutex) and simplify retry loop in buttress

[PATCH] media: staging/ipu7: use guard(mutex) and simplify retry loop in buttress

[Laurian Iacob]

Use scope-based guard(mutex) from linux/cleanup.h in
ipu_buttress_ipc_send_msg() and ipu_buttress_authenticate() to simplify
error handling and remove redundant goto unlock paths.

Additionally, replace the backward goto retry label in
ipu_buttress_ipc_send_msg() with a standard for (;;) loop to improve
control flow readability.

Signed-off-by: Laurian Iacob <laurian.iacob@yahoo.com>
---
 drivers/staging/media/ipu7/ipu7-buttress.c | 56 ++++++++++------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/media/ipu7/ipu7-buttress.c b/drivers/staging/media/ipu7/ipu7-buttress.c
index 40c6c8473357..7ec8290c4432 100644
--- a/drivers/staging/media/ipu7/ipu7-buttress.c
+++ b/drivers/staging/media/ipu7/ipu7-buttress.c
@@ -6,6 +6,7 @@
 #include <asm/cpu_device_id.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -229,39 +230,39 @@ static int ipu_buttress_ipc_send_msg(struct ipu7_device *isp,
 	u32 val;
 	int ret;
 
-	mutex_lock(&b->ipc_mutex);
+	guard(mutex)(&b->ipc_mutex);
 
 	ret = ipu_buttress_ipc_validity_open(isp, ipc);
 	if (ret) {
 		dev_err(dev, "IPC validity open failed\n");
-		goto out;
+		return ret;
 	}
 
 	tx_timeout_jiffies = msecs_to_jiffies(BUTTRESS_IPC_TX_TIMEOUT_MS);
 	rx_timeout_jiffies = msecs_to_jiffies(BUTTRESS_IPC_RX_TIMEOUT_MS);
 
-try:
-	reinit_completion(&ipc->send_complete);
-	if (msg->require_resp)
-		reinit_completion(&ipc->recv_complete);
+	for (;;) {
+		reinit_completion(&ipc->send_complete);
+		if (msg->require_resp)
+			reinit_completion(&ipc->recv_complete);
 
-	dev_dbg(dev, "IPC command: 0x%x\n", msg->cmd);
-	writel(msg->cmd, isp->base + ipc->data0_out);
-	val = BUTTRESS_IU2CSEDB0_BUSY | msg->cmd_size;
-	writel(val, isp->base + ipc->db0_out);
+		dev_dbg(dev, "IPC command: 0x%x\n", msg->cmd);
+		writel(msg->cmd, isp->base + ipc->data0_out);
+		val = BUTTRESS_IU2CSEDB0_BUSY | msg->cmd_size;
+		writel(val, isp->base + ipc->db0_out);
 
-	tout = wait_for_completion_timeout(&ipc->send_complete,
-					   tx_timeout_jiffies);
-	if (!tout) {
+		tout = wait_for_completion_timeout(&ipc->send_complete,
+						   tx_timeout_jiffies);
+		if (tout)
+			break;
 		dev_err(dev, "send IPC response timeout\n");
-		if (!retry--) {
+		if (retry-- == 0) {
 			ret = -ETIMEDOUT;
 			goto out;
 		}
 
 		/* Try again if CSE is not responding on first try */
 		writel(0, isp->base + ipc->db0_out);
-		goto try;
 	}
 
 	if (!msg->require_resp) {
@@ -852,11 +853,11 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 		return 0;
 	}
 
-	mutex_lock(&b->auth_mutex);
+	guard(mutex)(&b->auth_mutex);
 
 	if (ipu_buttress_auth_done(isp)) {
 		ret = 0;
-		goto out_unlock;
+		return 0;
 	}
 
 	/*
@@ -879,7 +880,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				    BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
 	if (ret) {
 		dev_err(dev, "CSE boot_load failed\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	mask = BUTTRESS_SECURITY_CTL_FW_SETUP_MASK;
@@ -891,13 +892,12 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_BOOTLOAD_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "CSE boot_load timeout\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	if ((data & mask) == fail) {
 		dev_err(dev, "CSE auth failed\n");
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	ret = readl_poll_timeout(isp->base + BOOTLOADER_STATUS_OFFSET,
@@ -905,7 +905,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_BOOTLOAD_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "Unexpected magic number 0x%x\n", data);
-		goto out_unlock;
+		return ret;
 	}
 
 	/*
@@ -919,7 +919,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				    BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
 	if (ret) {
 		dev_err(dev, "CSE authenticate_run failed\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	done = BUTTRESS_SECURITY_CTL_AUTH_DONE;
@@ -929,21 +929,17 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_AUTHENTICATE_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "CSE authenticate timeout\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	if ((data & mask) == fail) {
 		dev_err(dev, "CSE boot_load failed\n");
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	dev_info(dev, "CSE authenticate_run done\n");
 
-out_unlock:
-	mutex_unlock(&b->auth_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int ipu_buttress_send_tsc_request(struct ipu7_device *isp)
-- 
2.43.0

Re: [PATCH] media: staging/ipu7: use guard(mutex) and simplify retry loop in buttress

[Dan Carpenter]

On Thu, Nov 05, 2026 at 12:28:32AM +0200, Laurian Iacob wrote:
> Use scope-based guard(mutex) from linux/cleanup.h in
> ipu_buttress_ipc_send_msg() and ipu_buttress_authenticate() to simplify
> error handling and remove redundant goto unlock paths.

Unfortunately, you didn't remove them all.

> 
> Additionally, replace the backward goto retry label in
> ipu_buttress_ipc_send_msg() with a standard for (;;) loop to improve
> control flow readability.

I don't like that you got rid of the try: label.  How is that related
to locking.

regards,
dan carpenter

Re: [PATCH] media: staging/ipu7: use guard(mutex) and simplify retry loop in buttress

[Laurian Iacob]

Thanks for the catch, Dan! I missed the remaining mutex_unlock in 
ipu_buttress_ipc_send_msg() and the other functions in this file. 
I will restore the try: label and convert all mutexes in ipu7-buttress.c 
consistently in v2.

Best regards,
Laurian

[PATCH v2] media: staging/ipu7: use guard(mutex) in ipu7-buttress.c

[Laurian Iacob]

Use scope-based guard(mutex) from linux/cleanup.h across
ipu7-buttress.c to simplify error handling, remove redundant goto
unlock labels, and ensure consistent mutex locking across all helper
functions.

Signed-off-by: Laurian Iacob <laurian.iacob@yahoo.com>
---
Changes in v2:
- Restore the 'try:' label in ipu_buttress_ipc_send_msg() (Dan Carpenter)
- Remove leftover mutex_unlock() in ipu_buttress_ipc_send_msg() and dead
  'ret = 0' in ipu_buttress_authenticate() (Dan Carpenter)
- Convert remaining mutexes in ipu7-buttress.c to guard(mutex)

 drivers/staging/media/ipu7/ipu7-buttress.c | 75 +++++++++-------------
 1 file changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/staging/media/ipu7/ipu7-buttress.c b/drivers/staging/media/ipu7/ipu7-buttress.c
index 40c6c8473357..e79bdb742210 100644
--- a/drivers/staging/media/ipu7/ipu7-buttress.c
+++ b/drivers/staging/media/ipu7/ipu7-buttress.c
@@ -6,6 +6,7 @@
 #include <asm/cpu_device_id.h>
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
@@ -76,7 +77,7 @@ int ipu_buttress_ipc_reset(struct ipu7_device *isp,
 		return 0;
 	}
 
-	mutex_lock(&b->ipc_mutex);
+	guard(mutex)(&b->ipc_mutex);
 
 	/* Clear-by-1 CSR (all bits), corresponding internal states. */
 	val = readl(isp->base + ipc->csr_in);
@@ -155,7 +156,6 @@ int ipu_buttress_ipc_reset(struct ipu7_device *isp,
 					"RST_PHASE2 retry csr_in = %x\n", val);
 				break;
 			}
-			mutex_unlock(&b->ipc_mutex);
 			return 0;
 		case QUERY:
 			/*
@@ -173,7 +173,6 @@ int ipu_buttress_ipc_reset(struct ipu7_device *isp,
 		}
 	} while (retries--);
 
-	mutex_unlock(&b->ipc_mutex);
 	dev_err(dev, "Timed out while waiting for CSE\n");
 
 	return -ETIMEDOUT;
@@ -229,12 +228,12 @@ static int ipu_buttress_ipc_send_msg(struct ipu7_device *isp,
 	u32 val;
 	int ret;
 
-	mutex_lock(&b->ipc_mutex);
+	guard(mutex)(&b->ipc_mutex);
 
 	ret = ipu_buttress_ipc_validity_open(isp, ipc);
 	if (ret) {
 		dev_err(dev, "IPC validity open failed\n");
-		goto out;
+		return ret;
 	}
 
 	tx_timeout_jiffies = msecs_to_jiffies(BUTTRESS_IPC_TX_TIMEOUT_MS);
@@ -296,7 +295,6 @@ static int ipu_buttress_ipc_send_msg(struct ipu7_device *isp,
 
 out:
 	ipu_buttress_ipc_validity_close(isp, ipc);
-	mutex_unlock(&b->ipc_mutex);
 
 	return ret;
 }
@@ -527,13 +525,13 @@ static int ipu7_buttress_powerup(struct device *dev,
 	if (!ctrl)
 		return 0;
 
-	mutex_lock(&isp->buttress.power_mutex);
+	guard(mutex)(&isp->buttress.power_mutex);
 
 	exp_sts = ctrl->pwr_sts_on << ctrl->pwr_sts_shift;
 	if (ctrl->subsys_id == IPU_IS) {
 		ret = isys_d2d_power(dev, true);
 		if (ret)
-			goto out_power;
+			return ret;
 		isys_nde_control(dev, true);
 	}
 
@@ -560,7 +558,7 @@ static int ipu7_buttress_powerup(struct device *dev,
 	if (ret) {
 		dev_err(dev, "%s power up timeout with status: 0x%x\n",
 			ctrl->subsys_id == IPU_IS ? "IS" : "PS", val);
-		goto out_power;
+		return ret;
 	}
 
 	dev_dbg(dev, "%s power up successfully. status: 0x%x\n",
@@ -571,10 +569,7 @@ static int ipu7_buttress_powerup(struct device *dev,
 	val &= ~ctrl->ovrd_clk;
 	writel(val, isp->base + BUTTRESS_REG_SLEEP_LEVEL_CFG);
 
-out_power:
-	mutex_unlock(&isp->buttress.power_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int ipu7_buttress_powerdown(struct device *dev,
@@ -587,7 +582,7 @@ static int ipu7_buttress_powerdown(struct device *dev,
 	if (!ctrl)
 		return 0;
 
-	mutex_lock(&isp->buttress.power_mutex);
+	guard(mutex)(&isp->buttress.power_mutex);
 
 	exp_sts = ctrl->pwr_sts_off << ctrl->pwr_sts_shift;
 	val = 0x8U << ctrl->ratio_shift;
@@ -601,20 +596,18 @@ static int ipu7_buttress_powerdown(struct device *dev,
 	if (ret) {
 		dev_err(dev, "%s power down timeout with status: 0x%x\n",
 			ctrl->subsys_id == IPU_IS ? "IS" : "PS", val);
-		goto out_power;
+		return ret;
 	}
 
 	dev_dbg(dev, "%s power down successfully. status: 0x%x\n",
 		ctrl->subsys_id == IPU_IS ? "IS" : "PS", val);
-out_power:
-	if (ctrl->subsys_id == IPU_IS && !ret) {
+
+	if (ctrl->subsys_id == IPU_IS) {
 		isys_d2d_power(dev, false);
 		isys_nde_control(dev, false);
 	}
 
-	mutex_unlock(&isp->buttress.power_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int ipu8_buttress_powerup(struct device *dev,
@@ -628,12 +621,12 @@ static int ipu8_buttress_powerup(struct device *dev,
 	if (!ctrl)
 		return 0;
 
-	mutex_lock(&isp->buttress.power_mutex);
+	guard(mutex)(&isp->buttress.power_mutex);
 	exp_sts = ctrl->pwr_sts_on << ctrl->pwr_sts_shift;
 	if (ctrl->subsys_id == IPU_IS) {
 		ret = isys_d2d_power(dev, true);
 		if (ret)
-			goto out_power;
+			return ret;
 		isys_nde_control(dev, true);
 	}
 
@@ -658,15 +651,13 @@ static int ipu8_buttress_powerup(struct device *dev,
 	if (ret) {
 		dev_err(dev, "%s power up timeout with status: 0x%x\n",
 			ctrl->subsys_id == IPU_IS ? "IS" : "PS", val);
-		goto out_power;
+		return ret;
 	}
 
 	dev_dbg(dev, "%s power up successfully. status: 0x%x\n",
 		ctrl->subsys_id == IPU_IS ? "IS" : "PS", val);
-out_power:
-	mutex_unlock(&isp->buttress.power_mutex);
 
-	return ret;
+	return 0;
 }
 
 static int ipu8_buttress_powerdown(struct device *dev,
@@ -679,7 +670,7 @@ static int ipu8_buttress_powerdown(struct device *dev,
 	if (!ctrl)
 		return 0;
 
-	mutex_lock(&isp->buttress.power_mutex);
+	guard(mutex)(&isp->buttress.power_mutex);
 	exp_sts = ctrl->pwr_sts_off << ctrl->pwr_sts_shift;
 
 	if (ctrl->subsys_id == IPU_PS)
@@ -712,7 +703,6 @@ static int ipu8_buttress_powerdown(struct device *dev,
 		if (val & ctrl->own_clk_ack)
 			writel(0, isp->base + BUTTRESS_REG_PS_PLL_ENABLE);
 	}
-	mutex_unlock(&isp->buttress.power_mutex);
 
 	return ret;
 }
@@ -852,12 +842,10 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 		return 0;
 	}
 
-	mutex_lock(&b->auth_mutex);
+	guard(mutex)(&b->auth_mutex);
 
-	if (ipu_buttress_auth_done(isp)) {
-		ret = 0;
-		goto out_unlock;
-	}
+	if (ipu_buttress_auth_done(isp))
+		return 0;
 
 	/*
 	 * BUTTRESS_REG_FW_SOURCE_BASE needs to be set with FW CPD
@@ -879,7 +867,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				    BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
 	if (ret) {
 		dev_err(dev, "CSE boot_load failed\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	mask = BUTTRESS_SECURITY_CTL_FW_SETUP_MASK;
@@ -891,13 +879,12 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_BOOTLOAD_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "CSE boot_load timeout\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	if ((data & mask) == fail) {
 		dev_err(dev, "CSE auth failed\n");
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	ret = readl_poll_timeout(isp->base + BOOTLOADER_STATUS_OFFSET,
@@ -905,7 +892,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_BOOTLOAD_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "Unexpected magic number 0x%x\n", data);
-		goto out_unlock;
+		return ret;
 	}
 
 	/*
@@ -919,7 +906,7 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				    BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
 	if (ret) {
 		dev_err(dev, "CSE authenticate_run failed\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	done = BUTTRESS_SECURITY_CTL_AUTH_DONE;
@@ -929,21 +916,17 @@ int ipu_buttress_authenticate(struct ipu7_device *isp)
 				 BUTTRESS_CSE_AUTHENTICATE_TIMEOUT_US);
 	if (ret) {
 		dev_err(dev, "CSE authenticate timeout\n");
-		goto out_unlock;
+		return ret;
 	}
 
 	if ((data & mask) == fail) {
 		dev_err(dev, "CSE boot_load failed\n");
-		ret = -EINVAL;
-		goto out_unlock;
+		return -EINVAL;
 	}
 
 	dev_info(dev, "CSE authenticate_run done\n");
 
-out_unlock:
-	mutex_unlock(&b->auth_mutex);
-
-	return ret;
+	return 0;
 }
 
 static int ipu_buttress_send_tsc_request(struct ipu7_device *isp)
-- 
2.43.0

Re: [PATCH] media: staging/ipu7: use guard(mutex) and simplify retry loop in buttress

[Sakari Ailus]

Hi Laurian,

On Thu, Nov 05, 2026 at 12:28:32AM +0200, Laurian Iacob wrote:
> Use scope-based guard(mutex) from linux/cleanup.h in
> ipu_buttress_ipc_send_msg() and ipu_buttress_authenticate() to simplify
> error handling and remove redundant goto unlock paths.
> 
> Additionally, replace the backward goto retry label in
> ipu_buttress_ipc_send_msg() with a standard for (;;) loop to improve
> control flow readability.
> 
> Signed-off-by: Laurian Iacob <laurian.iacob@yahoo.com>

Thanks for the patch.

Unfortunately the ipu7 staging driver will be removed soon, therefore it's
a target of fixes only.

-- 
Kind regards,

Sakari Ailus