diff mbox series

[v3,3/4] iio: buffer: iio: core: move to the cleanup.h magic

Message ID 20240229-iio-use-cleanup-magic-v3-3-c3d34889ae3c@analog.com (mailing list archive)
State Accepted
Headers show
Series iio: move IIO to the cleanup.h magic | expand

Commit Message

Nuno Sa Feb. 29, 2024, 3:10 p.m. UTC 
Use the new cleanup magic for handling mutexes in IIO. This allows us to
greatly simplify some code paths.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/iio/industrialio-buffer.c | 122 +++++++++++++++-----------------------
 1 file changed, 48 insertions(+), 74 deletions(-)

Comments

Andy Shevchenko March 16, 2024, 7:38 p.m. UTC | #1 
Thu, Feb 29, 2024 at 04:10:27PM +0100, Nuno Sa kirjoitti:
> Use the new cleanup magic for handling mutexes in IIO. This allows us to
> greatly simplify some code paths.

...

>  	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
>  	if (ret < 0)
> -		goto error_ret;
> -	if (!state && ret) {
> -		ret = iio_scan_mask_clear(buffer, this_attr->address);
> -		if (ret)
> -			goto error_ret;
> -	} else if (state && !ret) {
> +		return ret;
> +
> +	if (state && ret)
> +		return len;

I would leave the original checks. It's natural pattern

	if (foo && !bar)
	if (!foo && bar) // or 'else if' depending on the context

> +	if (state)
>  		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> -		if (ret)
> -			goto error_ret;
> -	}
> +	else
> +		ret = iio_scan_mask_clear(buffer, this_attr->address);
> +	if (ret)
> +		return ret;
>  
> -error_ret:
> -	mutex_unlock(&iio_dev_opaque->mlock);
> -
> -	return ret < 0 ? ret : len;
> +	return len;

...

>  	/* Already in desired state */
>  	if (inlist == requested_state)
> -		goto done;
> +		return len;

Returning error code immediately is fine, but splitting return success seems to
me a bit too much. It is harder to follow (you really need to understand how many
"success" variants can be).

>  	if (requested_state)
>  		ret = __iio_update_buffers(indio_dev, buffer, NULL);
>  	else
>  		ret = __iio_update_buffers(indio_dev, NULL, buffer);
> +	if (ret)
> +		return ret;
>  
> -done:
> -	mutex_unlock(&iio_dev_opaque->mlock);
> -	return (ret < 0) ? ret : len;
> +	return len;
>  }
Andy Shevchenko March 16, 2024, 7:49 p.m. UTC | #2 
Thu, Feb 29, 2024 at 04:10:27PM +0100, Nuno Sa kirjoitti:
> Use the new cleanup magic for handling mutexes in IIO. This allows us to
> greatly simplify some code paths.

Forgot, the Subject has a leftover 'iio: core:'.
Nuno Sá March 18, 2024, 9:23 a.m. UTC | #3 
On Sat, 2024-03-16 at 21:38 +0200, Andy Shevchenko wrote:
> Thu, Feb 29, 2024 at 04:10:27PM +0100, Nuno Sa kirjoitti:
> > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > greatly simplify some code paths.
> 
> ...
> 
> >  	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
> >  	if (ret < 0)
> > -		goto error_ret;
> > -	if (!state && ret) {
> > -		ret = iio_scan_mask_clear(buffer, this_attr->address);
> > -		if (ret)
> > -			goto error_ret;
> > -	} else if (state && !ret) {
> > +		return ret;
> > +
> > +	if (state && ret)
> > +		return len;
> 
> I would leave the original checks. It's natural pattern
> 

It was suggested by Jonathan to have it like it is now (and no strong feelings
from my side)...

- Nuno Sá
Jonathan Cameron March 18, 2024, 12:35 p.m. UTC | #4 
On Sat, 16 Mar 2024 21:38:04 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Thu, Feb 29, 2024 at 04:10:27PM +0100, Nuno Sa kirjoitti:
> > Use the new cleanup magic for handling mutexes in IIO. This allows us to
> > greatly simplify some code paths.  
> 
> ...
> 
> >  	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
> >  	if (ret < 0)
> > -		goto error_ret;
> > -	if (!state && ret) {
> > -		ret = iio_scan_mask_clear(buffer, this_attr->address);
> > -		if (ret)
> > -			goto error_ret;
> > -	} else if (state && !ret) {
> > +		return ret;
> > +
> > +	if (state && ret)
> > +		return len;  
> 
> I would leave the original checks. It's natural pattern
> 
> 	if (foo && !bar)
> 	if (!foo && bar) // or 'else if' depending on the context
> 
> > +	if (state)
> >  		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> > -		if (ret)
> > -			goto error_ret;
> > -	}
> > +	else
> > +		ret = iio_scan_mask_clear(buffer, this_attr->address);
> > +	if (ret)
> > +		return ret;
> >  
> > -error_ret:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> > -
> > -	return ret < 0 ? ret : len;
> > +	return len;  
> 
> ...
> 
> >  	/* Already in desired state */
> >  	if (inlist == requested_state)
> > -		goto done;
> > +		return len;  
> 
> Returning error code immediately is fine, but splitting return success seems to
> me a bit too much. It is harder to follow (you really need to understand how many
> "success" variants can be).

The pattern of detecting 'nothing to do' and existing early is pretty common.
I agree that it gets dubious if there are lots of early 'success' exits, but
this one case seems reasonable to me.

Jonathan


> 
> >  	if (requested_state)
> >  		ret = __iio_update_buffers(indio_dev, buffer, NULL);
> >  	else
> >  		ret = __iio_update_buffers(indio_dev, NULL, buffer);
> > +	if (ret)
> > +		return ret;
> >  
> > -done:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> > -	return (ret < 0) ? ret : len;
> > +	return len;
> >  }  
>
diff mbox series

Patch

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index b581a7e80566..1d950a3e153b 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -10,6 +10,7 @@ 
  * - Alternative access techniques?
  */
 #include <linux/anon_inodes.h>
+#include <linux/cleanup.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/device.h>
@@ -533,28 +534,26 @@  static ssize_t iio_scan_el_store(struct device *dev,
 	ret = kstrtobool(buf, &state);
 	if (ret < 0)
 		return ret;
-	mutex_lock(&iio_dev_opaque->mlock);
-	if (iio_buffer_is_active(buffer)) {
-		ret = -EBUSY;
-		goto error_ret;
-	}
+
+	guard(mutex)(&iio_dev_opaque->mlock);
+	if (iio_buffer_is_active(buffer))
+		return -EBUSY;
+
 	ret = iio_scan_mask_query(indio_dev, buffer, this_attr->address);
 	if (ret < 0)
-		goto error_ret;
-	if (!state && ret) {
-		ret = iio_scan_mask_clear(buffer, this_attr->address);
-		if (ret)
-			goto error_ret;
-	} else if (state && !ret) {
+		return ret;
+
+	if (state && ret)
+		return len;
+
+	if (state)
 		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
-		if (ret)
-			goto error_ret;
-	}
+	else
+		ret = iio_scan_mask_clear(buffer, this_attr->address);
+	if (ret)
+		return ret;
 
-error_ret:
-	mutex_unlock(&iio_dev_opaque->mlock);
-
-	return ret < 0 ? ret : len;
+	return len;
 }
 
 static ssize_t iio_scan_el_ts_show(struct device *dev,
@@ -581,16 +580,13 @@  static ssize_t iio_scan_el_ts_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&iio_dev_opaque->mlock);
-	if (iio_buffer_is_active(buffer)) {
-		ret = -EBUSY;
-		goto error_ret;
-	}
-	buffer->scan_timestamp = state;
-error_ret:
-	mutex_unlock(&iio_dev_opaque->mlock);
+	guard(mutex)(&iio_dev_opaque->mlock);
+	if (iio_buffer_is_active(buffer))
+		return -EBUSY;
 
-	return ret ? ret : len;
+	buffer->scan_timestamp = state;
+
+	return len;
 }
 
 static int iio_buffer_add_channel_sysfs(struct iio_dev *indio_dev,
@@ -674,21 +670,16 @@  static ssize_t length_store(struct device *dev, struct device_attribute *attr,
 	if (val == buffer->length)
 		return len;
 
-	mutex_lock(&iio_dev_opaque->mlock);
-	if (iio_buffer_is_active(buffer)) {
-		ret = -EBUSY;
-	} else {
-		buffer->access->set_length(buffer, val);
-		ret = 0;
-	}
-	if (ret)
-		goto out;
+	guard(mutex)(&iio_dev_opaque->mlock);
+	if (iio_buffer_is_active(buffer))
+		return -EBUSY;
+
+	buffer->access->set_length(buffer, val);
+
 	if (buffer->length && buffer->length < buffer->watermark)
 		buffer->watermark = buffer->length;
-out:
-	mutex_unlock(&iio_dev_opaque->mlock);
 
-	return ret ? ret : len;
+	return len;
 }
 
 static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
@@ -1268,7 +1259,6 @@  int iio_update_buffers(struct iio_dev *indio_dev,
 		       struct iio_buffer *remove_buffer)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
-	int ret;
 
 	if (insert_buffer == remove_buffer)
 		return 0;
@@ -1277,8 +1267,8 @@  int iio_update_buffers(struct iio_dev *indio_dev,
 	    insert_buffer->direction == IIO_BUFFER_DIRECTION_OUT)
 		return -EINVAL;
 
-	mutex_lock(&iio_dev_opaque->info_exist_lock);
-	mutex_lock(&iio_dev_opaque->mlock);
+	guard(mutex)(&iio_dev_opaque->info_exist_lock);
+	guard(mutex)(&iio_dev_opaque->mlock);
 
 	if (insert_buffer && iio_buffer_is_active(insert_buffer))
 		insert_buffer = NULL;
@@ -1286,23 +1276,13 @@  int iio_update_buffers(struct iio_dev *indio_dev,
 	if (remove_buffer && !iio_buffer_is_active(remove_buffer))
 		remove_buffer = NULL;
 
-	if (!insert_buffer && !remove_buffer) {
-		ret = 0;
-		goto out_unlock;
-	}
+	if (!insert_buffer && !remove_buffer)
+		return 0;
 
-	if (!indio_dev->info) {
-		ret = -ENODEV;
-		goto out_unlock;
-	}
+	if (!indio_dev->info)
+		return -ENODEV;
 
-	ret = __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
-
-out_unlock:
-	mutex_unlock(&iio_dev_opaque->mlock);
-	mutex_unlock(&iio_dev_opaque->info_exist_lock);
-
-	return ret;
+	return __iio_update_buffers(indio_dev, insert_buffer, remove_buffer);
 }
 EXPORT_SYMBOL_GPL(iio_update_buffers);
 
@@ -1326,22 +1306,22 @@  static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
 	if (ret < 0)
 		return ret;
 
-	mutex_lock(&iio_dev_opaque->mlock);
+	guard(mutex)(&iio_dev_opaque->mlock);
 
 	/* Find out if it is in the list */
 	inlist = iio_buffer_is_active(buffer);
 	/* Already in desired state */
 	if (inlist == requested_state)
-		goto done;
+		return len;
 
 	if (requested_state)
 		ret = __iio_update_buffers(indio_dev, buffer, NULL);
 	else
 		ret = __iio_update_buffers(indio_dev, NULL, buffer);
+	if (ret)
+		return ret;
 
-done:
-	mutex_unlock(&iio_dev_opaque->mlock);
-	return (ret < 0) ? ret : len;
+	return len;
 }
 
 static ssize_t watermark_show(struct device *dev, struct device_attribute *attr,
@@ -1368,23 +1348,17 @@  static ssize_t watermark_store(struct device *dev,
 	if (!val)
 		return -EINVAL;
 
-	mutex_lock(&iio_dev_opaque->mlock);
+	guard(mutex)(&iio_dev_opaque->mlock);
 
-	if (val > buffer->length) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (val > buffer->length)
+		return -EINVAL;
 
-	if (iio_buffer_is_active(buffer)) {
-		ret = -EBUSY;
-		goto out;
-	}
+	if (iio_buffer_is_active(buffer))
+		return -EBUSY;
 
 	buffer->watermark = val;
-out:
-	mutex_unlock(&iio_dev_opaque->mlock);
 
-	return ret ? ret : len;
+	return len;
 }
 
 static ssize_t data_available_show(struct device *dev,