Re: [PATCH v2 4/5] iio: buffer: iio: core: move to the cleanup.h magic

["Nuno Sá" <noname.nuno@gmail.com>]

On Sun, 2024-02-25 at 12:53 +0000, Jonathan Cameron wrote:
> On Fri, 23 Feb 2024 13:43:47 +0100
> Nuno Sa <nuno.sa@analog.com> wrote:
> 
> > 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>
> 
> I think we can do more in here as a result of early returns being available.
> 
> > ---
> >  drivers/iio/industrialio-buffer.c | 105 ++++++++++++++------------------------
> >  1 file changed, 38 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-
> > buffer.c
> > index b581a7e80566..ec6bc881cf13 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,25 @@ 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;
> > +		return ret;
> 
> We could short cut this I think and end up with a simpler flow.
> The early returns allow something like
> 
> 	if (state && ret)  /* Nothing to do */
> 		return len;
> 
> 	if (state)
>   		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> 	else
> 		ret = iio_scan_mask_clear(buffer, this_attr->address);
> 	if (ret)
> 		return ret;
> 
> 	return len;

Nice...

> 
> >  	if (!state && ret) {
> >  		ret = iio_scan_mask_clear(buffer, this_attr->address);
> >  		if (ret)
> > -			goto error_ret;
> > +			return ret;
> >  	} else if (state && !ret) {
> >  		ret = iio_scan_mask_set(indio_dev, buffer, this_attr->address);
> >  		if (ret)
> > -			goto error_ret;
> > +			return ret;
> >  	}
> >  
> > -error_ret:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> > -
> > -	return ret < 0 ? ret : len;
> > +	return len;
> >  }
> >  
> 
> 
> 
> ...
> 
> >  
> > @@ -1326,21 +1305,19 @@ 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);
> >  
> > -done:
> > -	mutex_unlock(&iio_dev_opaque->mlock);
> >  	return (ret < 0) ? ret : len;
> Maybe just switch this for
> 
> 	if (ret < 0)
> 		return ret;
> 
> 	return len;
> 
> So it looks more like the new return len above?
> 

Ok, I typically prefer that form anyways :)

- Nuno Sá