diff mbox series

[v2,14/16] iio: health: max30100: do not use internal iio_dev lock

Message ID 20221004134909.1692021-15-nuno.sa@analog.com (mailing list archive)
State Changes Requested
Headers show
Series Make 'mlock' really private | expand

Commit Message

Nuno Sa Oct. 4, 2022, 1:49 p.m. UTC
The pattern used in this device does not quite fit in the
iio_device_claim_direct_mode() typical usage. In this case,
iio_buffer_enabled() was being used not to prevent the raw access but to
allow it. Hence, let's make use of the new
iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
during the complete read.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/health/max30100.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko Oct. 4, 2022, 2:13 p.m. UTC | #1
On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The pattern used in this device does not quite fit in the
> iio_device_claim_direct_mode() typical usage. In this case,
> iio_buffer_enabled() was being used not to prevent the raw access but to
> allow it. Hence, let's make use of the new
> iio_device_claim_buffer_mode() API to make sure we stay in buffered mode
> during the complete read.

...

> +               if (iio_device_claim_buffer_mode(indio_dev)) {
>                         ret = -EAGAIN;

Why is the error code shadowed? Isn't it better to return exactly the
one you resend to the upper caller(s)? Each unclear error code
shadowing should be at least explained.

> +               } else {

>                 }
Nuno Sá Oct. 5, 2022, 8:09 a.m. UTC | #2
On Tue, 2022-10-04 at 17:13 +0300, Andy Shevchenko wrote:
> On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > The pattern used in this device does not quite fit in the
> > iio_device_claim_direct_mode() typical usage. In this case,
> > iio_buffer_enabled() was being used not to prevent the raw access
> > but to
> > allow it. Hence, let's make use of the new
> > iio_device_claim_buffer_mode() API to make sure we stay in buffered
> > mode
> > during the complete read.
> 
> ...
> 
> > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> >                         ret = -EAGAIN;
> 
> Why is the error code shadowed? Isn't it better to return exactly the
> one you resend to the upper caller(s)? Each unclear error code
> shadowing should be at least explained.
> >           }

I'm keeping the same error that was returned before. Changing the error
code returned to userspace can break some apps relying on it. But if
everyone is ok with assuming the risk and changing it, fine by me.


- Nuno Sá
Jonathan Cameron Oct. 9, 2022, 12:14 p.m. UTC | #3
On Wed, 05 Oct 2022 10:09:29 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Tue, 2022-10-04 at 17:13 +0300, Andy Shevchenko wrote:
> > On Tue, Oct 4, 2022 at 4:49 PM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > The pattern used in this device does not quite fit in the
> > > iio_device_claim_direct_mode() typical usage. In this case,
> > > iio_buffer_enabled() was being used not to prevent the raw access
> > > but to
> > > allow it. Hence, let's make use of the new
> > > iio_device_claim_buffer_mode() API to make sure we stay in buffered
> > > mode
> > > during the complete read.  
> > 
> > ...
> >   
> > > +               if (iio_device_claim_buffer_mode(indio_dev)) {
> > >                         ret = -EAGAIN;  
> > 
> > Why is the error code shadowed? Isn't it better to return exactly the
> > one you resend to the upper caller(s)? Each unclear error code
> > shadowing should be at least explained.  
> > >           }  
> 
> I'm keeping the same error that was returned before. Changing the error
> code returned to userspace can break some apps relying on it. But if
> everyone is ok with assuming the risk and changing it, fine by me.

Hmm. For most drivers I'd say change it, but these weird health parts
use a highly custom userspace so it's just possible we'd break it
by changing the return code.  Unfortunately I don't know of anyone with current
access to the code of that software stack to check this for us as
there have been a lot of changes at TI in recent years.

So probably best to leave it alone, but add a comment to the patch description
to give reasoning.

Thanks,

Jonathan

> 
> 
> - Nuno Sá
diff mbox series

Patch

diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c
index ad5717965223..465ce8996d35 100644
--- a/drivers/iio/health/max30100.c
+++ b/drivers/iio/health/max30100.c
@@ -387,18 +387,15 @@  static int max30100_read_raw(struct iio_dev *indio_dev,
 		 * Temperature reading can only be acquired while engine
 		 * is running
 		 */
-		mutex_lock(&indio_dev->mlock);
-
-		if (!iio_buffer_enabled(indio_dev))
+		if (iio_device_claim_buffer_mode(indio_dev)) {
 			ret = -EAGAIN;
-		else {
+		} else {
 			ret = max30100_get_temp(data, val);
 			if (!ret)
 				ret = IIO_VAL_INT;
 
+			iio_device_release_buffer_mode(indio_dev);
 		}
-
-		mutex_unlock(&indio_dev->mlock);
 		break;
 	case IIO_CHAN_INFO_SCALE:
 		*val = 1;  /* 0.0625 */