| Message ID | 20221004134909.1692021-15-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Make 'mlock' really private | expand |
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 { > }
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á
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 --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 */
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(-)