diff mbox series

[v1,2/3] platform/x86: intel_scu_ipc: Simplify code with cleanup helpers

Message ID 20241016115033.858574-3-andriy.shevchenko@linux.intel.com (mailing list archive)
State Changes Requested, archived
Headers show
Series platform/x86: intel_scu_ipc: Avoid working around IO and cleanups | expand

Commit Message

Andy Shevchenko Oct. 16, 2024, 11:48 a.m. UTC 
Use macros defined in linux/cleanup.h to automate resource lifetime
control in the driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel_scu_ipc.c | 102 ++++++++++++---------------
 1 file changed, 44 insertions(+), 58 deletions(-)

Comments

Mika Westerberg Oct. 21, 2024, 7 a.m. UTC | #1 
On Wed, Oct 16, 2024 at 02:48:25PM +0300, Andy Shevchenko wrote:
> Use macros defined in linux/cleanup.h to automate resource lifetime
> control in the driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel_scu_ipc.c | 102 ++++++++++++---------------
>  1 file changed, 44 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
> index 290b38627542..ffb0a2524388 100644
> --- a/drivers/platform/x86/intel_scu_ipc.c
> +++ b/drivers/platform/x86/intel_scu_ipc.c
> @@ -13,6 +13,7 @@
>   * along with other APIs.
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/errno.h>
> @@ -99,23 +100,22 @@ static struct class intel_scu_ipc_class = {
>   */
>  struct intel_scu_ipc_dev *intel_scu_ipc_dev_get(void)
>  {
> -	struct intel_scu_ipc_dev *scu = NULL;
> +	guard(mutex)(&ipclock);
>  
> -	mutex_lock(&ipclock);
>  	if (ipcdev) {
>  		get_device(&ipcdev->dev);
> +

unintended whitespace change?

>  		/*
>  		 * Prevent the IPC provider from being unloaded while it
>  		 * is being used.
>  		 */
> -		if (!try_module_get(ipcdev->owner))
> -			put_device(&ipcdev->dev);
> -		else
> -			scu = ipcdev;
> +		if (try_module_get(ipcdev->owner))
> +			return ipcdev;
> +
> +		put_device(&ipcdev->dev);
>  	}
>  
> -	mutex_unlock(&ipclock);
> -	return scu;
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(intel_scu_ipc_dev_get);
>  
> @@ -289,12 +289,11 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
>  
>  	memset(cbuf, 0, sizeof(cbuf));
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	for (nc = 0; nc < count; nc++, offset += 2) {
>  		cbuf[offset] = addr[nc];
> @@ -319,13 +318,14 @@ static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
>  	}
>  
>  	err = intel_scu_ipc_check_status(scu);
> -	if (!err) { /* Read rbuf */
> -		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> -			wbuf[nc] = ipc_data_readl(scu, offset);
> -		memcpy(data, wbuf, count);
> -	}

Here you changed also the check to be for if (err) instead of (!err) so
at least mention why you did these at the same time you did the cleanup
change. It is fine by me but good to mention in the commit message.

> -	mutex_unlock(&ipclock);
> -	return err;
> +	if (err)
> +		return err;
> +
> +	for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> +		wbuf[nc] = ipc_data_readl(scu, offset);
> +	memcpy(data, wbuf, count);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -446,17 +446,15 @@ int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
>  	u32 cmdval;
>  	int err;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	cmdval = sub << 12 | cmd;
>  	ipc_command(scu, cmdval);
>  	err = intel_scu_ipc_check_status(scu);
> -	mutex_unlock(&ipclock);
>  	if (err)
>  		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
>  	return err;
> @@ -485,18 +483,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
>  {
>  	size_t outbuflen = DIV_ROUND_UP(outlen, sizeof(u32));
>  	size_t inbuflen = DIV_ROUND_UP(inlen, sizeof(u32));
> -	u32 cmdval, inbuf[4] = {};
> +	u32 cmdval, inbuf[4] = {}, outbuf[4] = {};
>  	int i, err;
>  
>  	if (inbuflen > 4 || outbuflen > 4)
>  		return -EINVAL;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	scu = intel_scu_ipc_get(scu);
> -	if (IS_ERR(scu)) {
> -		mutex_unlock(&ipclock);
> +	if (IS_ERR(scu))
>  		return PTR_ERR(scu);
> -	}
>  
>  	memcpy(inbuf, in, inlen);
>  	for (i = 0; i < inbuflen; i++)
> @@ -505,20 +502,17 @@ int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
>  	cmdval = (size << 16) | (sub << 12) | cmd;
>  	ipc_command(scu, cmdval);
>  	err = intel_scu_ipc_check_status(scu);
> -
> -	if (!err) {
> -		u32 outbuf[4] = {};
> -
> -		for (i = 0; i < outbuflen; i++)
> -			outbuf[i] = ipc_data_readl(scu, 4 * i);
> -
> -		memcpy(out, outbuf, outlen);

Same here.

> +	if (err) {
> +		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
> +		return err;
>  	}
>  
> -	mutex_unlock(&ipclock);
> -	if (err)
> -		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
> -	return err;
> +	for (i = 0; i < outbuflen; i++)
> +		outbuf[i] = ipc_data_readl(scu, 4 * i);
> +
> +	memcpy(out, outbuf, outlen);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(intel_scu_ipc_dev_command_with_size);
>  
> @@ -572,18 +566,15 @@ __intel_scu_ipc_register(struct device *parent,
>  	struct intel_scu_ipc_dev *scu;
>  	void __iomem *ipc_base;
>  
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	/* We support only one IPC */
> -	if (ipcdev) {
> -		err = -EBUSY;
> -		goto err_unlock;
> -	}
> +	if (ipcdev)
> +		return ERR_PTR(-EBUSY);
>  
>  	scu = kzalloc(sizeof(*scu), GFP_KERNEL);
> -	if (!scu) {
> -		err = -ENOMEM;
> -		goto err_unlock;
> -	}
> +	if (!scu)
> +		return ERR_PTR(-ENOMEM);
>  
>  	scu->owner = owner;
>  	scu->dev.parent = parent;
> @@ -621,13 +612,11 @@ __intel_scu_ipc_register(struct device *parent,
>  	err = device_register(&scu->dev);
>  	if (err) {
>  		put_device(&scu->dev);
> -		goto err_unlock;
> +		return ERR_PTR(err);
>  	}
>  
>  	/* Assign device at last */
>  	ipcdev = scu;
> -	mutex_unlock(&ipclock);
> -
>  	return scu;
>  
>  err_unmap:
> @@ -636,9 +625,6 @@ __intel_scu_ipc_register(struct device *parent,
>  	release_mem_region(scu_data->mem.start, resource_size(&scu_data->mem));
>  err_free:
>  	kfree(scu);
> -err_unlock:
> -	mutex_unlock(&ipclock);
> -
>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
> @@ -652,12 +638,12 @@ EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
>   */
>  void intel_scu_ipc_unregister(struct intel_scu_ipc_dev *scu)
>  {
> -	mutex_lock(&ipclock);
> +	guard(mutex)(&ipclock);
> +
>  	if (!WARN_ON(!ipcdev)) {
>  		ipcdev = NULL;
>  		device_unregister(&scu->dev);
>  	}
> -	mutex_unlock(&ipclock);
>  }
>  EXPORT_SYMBOL_GPL(intel_scu_ipc_unregister);
>  
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
Andy Shevchenko Oct. 21, 2024, 8:34 a.m. UTC | #2 
On Mon, Oct 21, 2024 at 10:00:05AM +0300, Mika Westerberg wrote:
> On Wed, Oct 16, 2024 at 02:48:25PM +0300, Andy Shevchenko wrote:
> > Use macros defined in linux/cleanup.h to automate resource lifetime
> > control in the driver.

...

> >  		get_device(&ipcdev->dev);
> > +
> 
> unintended whitespace change?

Hmm... I don't remember, probably.
I will remove this part in v2.

...

> >  	err = intel_scu_ipc_check_status(scu);
> > -	if (!err) { /* Read rbuf */
> > -		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
> > -			wbuf[nc] = ipc_data_readl(scu, offset);
> > -		memcpy(data, wbuf, count);
> > -	}
> 
> Here you changed also the check to be for if (err) instead of (!err) so
> at least mention why you did these at the same time you did the cleanup
> change. It is fine by me but good to mention in the commit message.

Yes, this is the part of the cleanup change. It's a byproduct of it.
I will try to mention this in the commit message, however I consider
the text redundant.

...

> > -	if (!err) {
> > -		u32 outbuf[4] = {};
> > -
> > -		for (i = 0; i < outbuflen; i++)
> > -			outbuf[i] = ipc_data_readl(scu, 4 * i);
> > -
> > -		memcpy(out, outbuf, outlen);
> 
> Same here.

Same answer.

> > +	if (err) {
> > +		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
> > +		return err;
> >  	}

...

Thank you for the review!
diff mbox series

Patch

diff --git a/drivers/platform/x86/intel_scu_ipc.c b/drivers/platform/x86/intel_scu_ipc.c
index 290b38627542..ffb0a2524388 100644
--- a/drivers/platform/x86/intel_scu_ipc.c
+++ b/drivers/platform/x86/intel_scu_ipc.c
@@ -13,6 +13,7 @@ 
  * along with other APIs.
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/errno.h>
@@ -99,23 +100,22 @@  static struct class intel_scu_ipc_class = {
  */
 struct intel_scu_ipc_dev *intel_scu_ipc_dev_get(void)
 {
-	struct intel_scu_ipc_dev *scu = NULL;
+	guard(mutex)(&ipclock);
 
-	mutex_lock(&ipclock);
 	if (ipcdev) {
 		get_device(&ipcdev->dev);
+
 		/*
 		 * Prevent the IPC provider from being unloaded while it
 		 * is being used.
 		 */
-		if (!try_module_get(ipcdev->owner))
-			put_device(&ipcdev->dev);
-		else
-			scu = ipcdev;
+		if (try_module_get(ipcdev->owner))
+			return ipcdev;
+
+		put_device(&ipcdev->dev);
 	}
 
-	mutex_unlock(&ipclock);
-	return scu;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(intel_scu_ipc_dev_get);
 
@@ -289,12 +289,11 @@  static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 
 	memset(cbuf, 0, sizeof(cbuf));
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	for (nc = 0; nc < count; nc++, offset += 2) {
 		cbuf[offset] = addr[nc];
@@ -319,13 +318,14 @@  static int pwr_reg_rdwr(struct intel_scu_ipc_dev *scu, u16 *addr, u8 *data,
 	}
 
 	err = intel_scu_ipc_check_status(scu);
-	if (!err) { /* Read rbuf */
-		for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
-			wbuf[nc] = ipc_data_readl(scu, offset);
-		memcpy(data, wbuf, count);
-	}
-	mutex_unlock(&ipclock);
-	return err;
+	if (err)
+		return err;
+
+	for (nc = 0, offset = 0; nc < 4; nc++, offset += 4)
+		wbuf[nc] = ipc_data_readl(scu, offset);
+	memcpy(data, wbuf, count);
+
+	return 0;
 }
 
 /**
@@ -446,17 +446,15 @@  int intel_scu_ipc_dev_simple_command(struct intel_scu_ipc_dev *scu, int cmd,
 	u32 cmdval;
 	int err;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	cmdval = sub << 12 | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);
-	mutex_unlock(&ipclock);
 	if (err)
 		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
 	return err;
@@ -485,18 +483,17 @@  int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
 {
 	size_t outbuflen = DIV_ROUND_UP(outlen, sizeof(u32));
 	size_t inbuflen = DIV_ROUND_UP(inlen, sizeof(u32));
-	u32 cmdval, inbuf[4] = {};
+	u32 cmdval, inbuf[4] = {}, outbuf[4] = {};
 	int i, err;
 
 	if (inbuflen > 4 || outbuflen > 4)
 		return -EINVAL;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	scu = intel_scu_ipc_get(scu);
-	if (IS_ERR(scu)) {
-		mutex_unlock(&ipclock);
+	if (IS_ERR(scu))
 		return PTR_ERR(scu);
-	}
 
 	memcpy(inbuf, in, inlen);
 	for (i = 0; i < inbuflen; i++)
@@ -505,20 +502,17 @@  int intel_scu_ipc_dev_command_with_size(struct intel_scu_ipc_dev *scu, int cmd,
 	cmdval = (size << 16) | (sub << 12) | cmd;
 	ipc_command(scu, cmdval);
 	err = intel_scu_ipc_check_status(scu);
-
-	if (!err) {
-		u32 outbuf[4] = {};
-
-		for (i = 0; i < outbuflen; i++)
-			outbuf[i] = ipc_data_readl(scu, 4 * i);
-
-		memcpy(out, outbuf, outlen);
+	if (err) {
+		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
+		return err;
 	}
 
-	mutex_unlock(&ipclock);
-	if (err)
-		dev_err(&scu->dev, "IPC command %#x failed with %d\n", cmdval, err);
-	return err;
+	for (i = 0; i < outbuflen; i++)
+		outbuf[i] = ipc_data_readl(scu, 4 * i);
+
+	memcpy(out, outbuf, outlen);
+
+	return 0;
 }
 EXPORT_SYMBOL(intel_scu_ipc_dev_command_with_size);
 
@@ -572,18 +566,15 @@  __intel_scu_ipc_register(struct device *parent,
 	struct intel_scu_ipc_dev *scu;
 	void __iomem *ipc_base;
 
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	/* We support only one IPC */
-	if (ipcdev) {
-		err = -EBUSY;
-		goto err_unlock;
-	}
+	if (ipcdev)
+		return ERR_PTR(-EBUSY);
 
 	scu = kzalloc(sizeof(*scu), GFP_KERNEL);
-	if (!scu) {
-		err = -ENOMEM;
-		goto err_unlock;
-	}
+	if (!scu)
+		return ERR_PTR(-ENOMEM);
 
 	scu->owner = owner;
 	scu->dev.parent = parent;
@@ -621,13 +612,11 @@  __intel_scu_ipc_register(struct device *parent,
 	err = device_register(&scu->dev);
 	if (err) {
 		put_device(&scu->dev);
-		goto err_unlock;
+		return ERR_PTR(err);
 	}
 
 	/* Assign device at last */
 	ipcdev = scu;
-	mutex_unlock(&ipclock);
-
 	return scu;
 
 err_unmap:
@@ -636,9 +625,6 @@  __intel_scu_ipc_register(struct device *parent,
 	release_mem_region(scu_data->mem.start, resource_size(&scu_data->mem));
 err_free:
 	kfree(scu);
-err_unlock:
-	mutex_unlock(&ipclock);
-
 	return ERR_PTR(err);
 }
 EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
@@ -652,12 +638,12 @@  EXPORT_SYMBOL_GPL(__intel_scu_ipc_register);
  */
 void intel_scu_ipc_unregister(struct intel_scu_ipc_dev *scu)
 {
-	mutex_lock(&ipclock);
+	guard(mutex)(&ipclock);
+
 	if (!WARN_ON(!ipcdev)) {
 		ipcdev = NULL;
 		device_unregister(&scu->dev);
 	}
-	mutex_unlock(&ipclock);
 }
 EXPORT_SYMBOL_GPL(intel_scu_ipc_unregister);