Re: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

Re: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4547 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20210818111139.330636-11-miquel.raynal@bootlin.com>
References: <20210818111139.330636-11-miquel.raynal@bootlin.com>
TO: Miquel Raynal <miquel.raynal@bootlin.com>
TO: Jonathan Cameron <jic23@kernel.org>
TO: "Lars-Peter Clausen" <lars@metafoo.de>
CC: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
CC: linux-iio(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org
CC: Miquel Raynal <miquel.raynal@bootlin.com>

Hi Miquel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iio/togreg]
[also build test WARNING on linux/master linus/master v5.14-rc6 next-20210818]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Miquel-Raynal/Bring-software-triggers-support-to-MAX1027-like-ADCs/20210818-191619
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
:::::: branch date: 10 hours ago
:::::: commit date: 10 hours ago
config: i386-randconfig-m021-20210818 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/iio/adc/max1027.c:366 max1027_read_raw() warn: inconsistent returns '&st->lock'.

vim +366 drivers/iio/adc/max1027.c

fc167f62483325 Philippe Reynes 2014-06-14  326  
fc167f62483325 Philippe Reynes 2014-06-14  327  static int max1027_read_raw(struct iio_dev *indio_dev,
fc167f62483325 Philippe Reynes 2014-06-14  328  			    struct iio_chan_spec const *chan,
fc167f62483325 Philippe Reynes 2014-06-14  329  			    int *val, int *val2, long mask)
fc167f62483325 Philippe Reynes 2014-06-14  330  {
fc167f62483325 Philippe Reynes 2014-06-14  331  	int ret = 0;
fc167f62483325 Philippe Reynes 2014-06-14  332  	struct max1027_state *st = iio_priv(indio_dev);
fc167f62483325 Philippe Reynes 2014-06-14  333  
fc167f62483325 Philippe Reynes 2014-06-14  334  	mutex_lock(&st->lock);
fc167f62483325 Philippe Reynes 2014-06-14  335  
fc167f62483325 Philippe Reynes 2014-06-14  336  	switch (mask) {
fc167f62483325 Philippe Reynes 2014-06-14  337  	case IIO_CHAN_INFO_RAW:
69515d07652c09 Miquel Raynal   2021-08-18  338  		if (iio_buffer_enabled(indio_dev))
69515d07652c09 Miquel Raynal   2021-08-18  339  			return -EBUSY;
fc167f62483325 Philippe Reynes 2014-06-14  340  		ret = max1027_read_single_value(indio_dev, chan, val);
fc167f62483325 Philippe Reynes 2014-06-14  341  		break;
fc167f62483325 Philippe Reynes 2014-06-14  342  	case IIO_CHAN_INFO_SCALE:
fc167f62483325 Philippe Reynes 2014-06-14  343  		switch (chan->type) {
fc167f62483325 Philippe Reynes 2014-06-14  344  		case IIO_TEMP:
fc167f62483325 Philippe Reynes 2014-06-14  345  			*val = 1;
fc167f62483325 Philippe Reynes 2014-06-14  346  			*val2 = 8;
fc167f62483325 Philippe Reynes 2014-06-14  347  			ret = IIO_VAL_FRACTIONAL;
fc167f62483325 Philippe Reynes 2014-06-14  348  			break;
fc167f62483325 Philippe Reynes 2014-06-14  349  		case IIO_VOLTAGE:
fc167f62483325 Philippe Reynes 2014-06-14  350  			*val = 2500;
7af5257d84275c Miquel Raynal   2019-10-11  351  			*val2 = chan->scan_type.realbits;
fc167f62483325 Philippe Reynes 2014-06-14  352  			ret = IIO_VAL_FRACTIONAL_LOG2;
fc167f62483325 Philippe Reynes 2014-06-14  353  			break;
fc167f62483325 Philippe Reynes 2014-06-14  354  		default:
fc167f62483325 Philippe Reynes 2014-06-14  355  			ret = -EINVAL;
fc167f62483325 Philippe Reynes 2014-06-14  356  			break;
fc167f62483325 Philippe Reynes 2014-06-14  357  		}
fc167f62483325 Philippe Reynes 2014-06-14  358  		break;
fc167f62483325 Philippe Reynes 2014-06-14  359  	default:
fc167f62483325 Philippe Reynes 2014-06-14  360  		ret = -EINVAL;
fc167f62483325 Philippe Reynes 2014-06-14  361  		break;
fc167f62483325 Philippe Reynes 2014-06-14  362  	}
fc167f62483325 Philippe Reynes 2014-06-14  363  
fc167f62483325 Philippe Reynes 2014-06-14  364  	mutex_unlock(&st->lock);
fc167f62483325 Philippe Reynes 2014-06-14  365  
fc167f62483325 Philippe Reynes 2014-06-14 @366  	return ret;
fc167f62483325 Philippe Reynes 2014-06-14  367  }
fc167f62483325 Philippe Reynes 2014-06-14  368  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38766 bytes --]

Re: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Miquel Raynal]

Hi Jonathan,

Jonathan Cameron <jic23@kernel.org> wrote on Mon, 30 Aug 2021 11:20:24
+0100:

> On Fri, 20 Aug 2021 07:30:07 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Sa, Nuno <Nuno.Sa@analog.com>
> > > Sent: Friday, August 20, 2021 9:21 AM
> > > To: Miquel Raynal <miquel.raynal@bootlin.com>; Jonathan Cameron
> > > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > accesses during buffer reads
> > > 
> > > [External]
> > > 
> > > 
> > >     
> > > > -----Original Message-----
> > > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > > > <lars@metafoo.de>
> > > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> > > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal
> > > > <miquel.raynal@bootlin.com>
> > > > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > > accesses during buffer reads
> > > >
> > > > [External]
> > > >
> > > > When hardware buffers are enabled (the cnvst pin being the    
> > > trigger),    
> > > > one
> > > > should not mess with the device state by requesting a single channel
> > > > read. Prevent it with a iio_buffer_enabled() check.
> > > >
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  drivers/iio/adc/max1027.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > > index 223c9e4abd86..83526f3d7d3a 100644
> > > > --- a/drivers/iio/adc/max1027.c
> > > > +++ b/drivers/iio/adc/max1027.c
> > > > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > > > *indio_dev,
> > > >
> > > >  	switch (mask) {
> > > >  	case IIO_CHAN_INFO_RAW:
> > > > +		if (iio_buffer_enabled(indio_dev))
> > > > +			return -EBUSY;    
> > > 
> > > I guess 'iio_device_claim_direct_mode()' would be a better option
> > > here? There's nothing preventing this check to pass and then,
> > > concurrently
> > > someone enables the buffer...
> > >     
> > 
> > Taking a second look, it seems that this check is already done [1]? Am I missing
> > I missing something?

You're right, I missed that too.

> > Also, I think we are returning with the 'st->lock' held...
> > 
> > [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247  
> Absolutely agree this should be done with iio_device_claim_direct_mode() to close the
> possible races.

Didn't know this helper, nice.

> I wonder why this one has been missed in all the cleanups of that stuff? Looks like
> a simple case, but I guess it wasn't immediately visible in the read_raw() function
> so no one noticed.
> 
> Jonathan
> 

Thanks,
Miquèl

Re: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Jonathan Cameron]

On Fri, 20 Aug 2021 07:30:07 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Sa, Nuno <Nuno.Sa@analog.com>
> > Sent: Friday, August 20, 2021 9:21 AM
> > To: Miquel Raynal <miquel.raynal@bootlin.com>; Jonathan Cameron
> > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > accesses during buffer reads
> > 
> > [External]
> > 
> > 
> >   
> > > -----Original Message-----
> > > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Sent: Wednesday, August 18, 2021 1:12 PM
> > > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > > <lars@metafoo.de>
> > > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> > > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal
> > > <miquel.raynal@bootlin.com>
> > > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > > accesses during buffer reads
> > >
> > > [External]
> > >
> > > When hardware buffers are enabled (the cnvst pin being the  
> > trigger),  
> > > one
> > > should not mess with the device state by requesting a single channel
> > > read. Prevent it with a iio_buffer_enabled() check.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  drivers/iio/adc/max1027.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > > index 223c9e4abd86..83526f3d7d3a 100644
> > > --- a/drivers/iio/adc/max1027.c
> > > +++ b/drivers/iio/adc/max1027.c
> > > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > > *indio_dev,
> > >
> > >  	switch (mask) {
> > >  	case IIO_CHAN_INFO_RAW:
> > > +		if (iio_buffer_enabled(indio_dev))
> > > +			return -EBUSY;  
> > 
> > I guess 'iio_device_claim_direct_mode()' would be a better option
> > here? There's nothing preventing this check to pass and then,
> > concurrently
> > someone enables the buffer...
> >   
> 
> Taking a second look, it seems that this check is already done [1]? Am I missing
> I missing something?
> 
> Also, I think we are returning with the 'st->lock' held...
> 
> [1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247
Absolutely agree this should be done with iio_device_claim_direct_mode() to close the
possible races.

I wonder why this one has been missed in all the cleanups of that stuff? Looks like
a simple case, but I guess it wasn't immediately visible in the read_raw() function
so no one noticed.

Jonathan

RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Sa, Nuno]

> -----Original Message-----
> From: Sa, Nuno <Nuno.Sa@analog.com>
> Sent: Friday, August 20, 2021 9:21 AM
> To: Miquel Raynal <miquel.raynal@bootlin.com>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> accesses during buffer reads
> 
> [External]
> 
> 
> 
> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Wednesday, August 18, 2021 1:12 PM
> > To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> > <lars@metafoo.de>
> > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> > iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal
> > <miquel.raynal@bootlin.com>
> > Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> > accesses during buffer reads
> >
> > [External]
> >
> > When hardware buffers are enabled (the cnvst pin being the
> trigger),
> > one
> > should not mess with the device state by requesting a single channel
> > read. Prevent it with a iio_buffer_enabled() check.
> >
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/iio/adc/max1027.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> > index 223c9e4abd86..83526f3d7d3a 100644
> > --- a/drivers/iio/adc/max1027.c
> > +++ b/drivers/iio/adc/max1027.c
> > @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> > *indio_dev,
> >
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> > +		if (iio_buffer_enabled(indio_dev))
> > +			return -EBUSY;
> 
> I guess 'iio_device_claim_direct_mode()' would be a better option
> here? There's nothing preventing this check to pass and then,
> concurrently
> someone enables the buffer...
> 

Taking a second look, it seems that this check is already done [1]? Am I missing
I missing something?

Also, I think we are returning with the 'st->lock' held...

[1]: https://elixir.bootlin.com/linux/latest/source/drivers/iio/adc/max1027.c#L247

RE: [PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Sa, Nuno]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Wednesday, August 18, 2021 1:12 PM
> To: Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; Miquel Raynal
> <miquel.raynal@bootlin.com>
> Subject: [PATCH 10/16] iio: adc: max1027: Prevent single channel
> accesses during buffer reads
> 
> [External]
> 
> When hardware buffers are enabled (the cnvst pin being the trigger),
> one
> should not mess with the device state by requesting a single channel
> read. Prevent it with a iio_buffer_enabled() check.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/iio/adc/max1027.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
> index 223c9e4abd86..83526f3d7d3a 100644
> --- a/drivers/iio/adc/max1027.c
> +++ b/drivers/iio/adc/max1027.c
> @@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev
> *indio_dev,
> 
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> +		if (iio_buffer_enabled(indio_dev))
> +			return -EBUSY;

I guess 'iio_device_claim_direct_mode()' would be a better option
here? There's nothing preventing this check to pass and then, concurrently
someone enables the buffer...

- Nuno Sá
>  		ret = max1027_read_single_value(indio_dev, chan,
> val);
>  		break;
>  	case IIO_CHAN_INFO_SCALE:
> --
> 2.27.0

[PATCH 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads

[Miquel Raynal]

When hardware buffers are enabled (the cnvst pin being the trigger), one
should not mess with the device state by requesting a single channel
read. Prevent it with a iio_buffer_enabled() check.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/max1027.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/iio/adc/max1027.c b/drivers/iio/adc/max1027.c
index 223c9e4abd86..83526f3d7d3a 100644
--- a/drivers/iio/adc/max1027.c
+++ b/drivers/iio/adc/max1027.c
@@ -335,6 +335,8 @@ static int max1027_read_raw(struct iio_dev *indio_dev,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
+		if (iio_buffer_enabled(indio_dev))
+			return -EBUSY;
 		ret = max1027_read_single_value(indio_dev, chan, val);
 		break;
 	case IIO_CHAN_INFO_SCALE:
-- 
2.27.0