linux-kernel - Re: [PATCH 5/7] iio: amplifiers: ad8366: use cleanup.h mutex guard

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20260123083629.0117c6aa@jic23-huawei>
Date: Fri, 23 Jan 2026 08:36:29 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: rodrigo.alencar@...log.com, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org, Michael Hennerich
 <Michael.Hennerich@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, David
 Lechner <dlechner@...libre.com>, Andy Shevchenko <andy@...nel.org>, Rob
 Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 5/7] iio: amplifiers: ad8366: use cleanup.h mutex guard

On Mon, 19 Jan 2026 15:43:07 +0100
Krzysztof Kozlowski <krzk@...nel.org> wrote:

> On 19/01/2026 15:36, Rodrigo Alencar via B4 Relay wrote:
> > From: Rodrigo Alencar <rodrigo.alencar@...log.com>
> > 
> > Changes related to mutex handling:
> > - use guard() from cleanup for mutex locking
> > - replace mutex_init() for devm_mutex_init()  
> 
> Why? We see all this from the diff but I do not see benefits.

Also, 2 patches as two different things.
That should make it easier to say what benefits of each is.

A little more comment on comments inline.


> 
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@...log.com>
> > ---
> >  drivers/iio/amplifiers/ad8366.c | 14 +++++++++-----
> >  1 file changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/amplifiers/ad8366.c b/drivers/iio/amplifiers/ad8366.c
> > index 160a8ab0c2ee..26856cb4216e 100644
> > --- a/drivers/iio/amplifiers/ad8366.c
> > +++ b/drivers/iio/amplifiers/ad8366.c
> > @@ -17,6 +17,7 @@
> >   * Copyright 2012-2026 Analog Devices Inc.
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/device.h>
> >  #include <linux/kernel.h>
> >  #include <linux/slab.h>
> > @@ -164,7 +165,8 @@ static int ad8366_read_raw(struct iio_dev *indio_dev,
> >  	int ret;
> >  	int code, gain = 0;
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);
> > +
> >  	switch (m) {
> >  	case IIO_CHAN_INFO_HARDWAREGAIN:
> >  		code = st->ch[chan->channel];
> > @@ -210,7 +212,6 @@ static int ad8366_read_raw(struct iio_dev *indio_dev,
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > -	mutex_unlock(&st->lock);  
> 
> This does not simplify code.
Agreed. Without moving to early returns there is little point in this change.
With early returns the argument of easier code flow can be made
though it isn't particularly strong in this case as only two exit
paths and they are right next to each other.

> 
> >  
> >  	return ret;
> >  };
> > @@ -267,7 +268,8 @@ static int ad8366_write_raw(struct iio_dev *indio_dev,
> >  		break;
> >  	}
> >  
> > -	mutex_lock(&st->lock);
> > +	guard(mutex)(&st->lock);  
> 
> Neither this.
With early returns this one is more obviously useful. Then can argue
above is for consistency.
> 
> > +
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_HARDWAREGAIN:
> >  		st->ch[chan->channel] = code;
> > @@ -276,7 +278,6 @@ static int ad8366_write_raw(struct iio_dev *indio_dev,
> >  	default:
> >  		ret = -EINVAL;
> >  	}
> > -	mutex_unlock(&st->lock);
> >  
> >  	return ret;
> >  }
> > @@ -336,10 +337,13 @@ static int ad8366_probe(struct spi_device *spi)
> >  	}
> >  
> >  	spi_set_drvdata(spi, indio_dev);
> > -	mutex_init(&st->lock);
> >  	st->spi = spi;
> >  	st->type = spi_get_device_id(spi)->driver_data;
> >  
> > +	ret = devm_mutex_init(&spi->dev, &st->lock);  
> 
> And this one actually has impact - missing mutex_destroy...
> 
> > +	if (ret)
> > +		return ret;
> > +
> >  	switch (st->type) {
> >  	case ID_AD8366:
> >  		indio_dev->channels = ad8366_channels;
> >   
> 
> 
> Best regards,
> Krzysztof