| Message ID | 20210818111139.330636-11-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Bring software triggers support to MAX1027-like ADCs | expand |
> -----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
> -----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
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
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
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:
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(+)