| Message ID | 20221012151620.1725215-3-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make 'mlock' really private | expand |
On Wed, Oct 12, 2022 at 6:15 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. > > Note that we are shadowing the error code returned by > iio_device_claim_buffer_mode() so that we keep the original one > (-EAGAIN). The reason is that some userspace stack might already be > relying on this particular code so that we are not taking chances and > leave it alone. The above line widths seem a bit arbitrary to me. But I think it's due to function names in them. Perhaps you can make them less deviated by shuffling a bit, like moving "but to" to the next line. > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/health/max30100.c | 9 +++------ > 1 file changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c > index 2cca5e0519f8..6ac49901c9da 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)) { I think a summary of replacing error code is good to have here, like /* * Replacing -EBUSY or other error code * returned by iio_device_claim_buffer_mode() * because user space may rely on the current * one. */ > 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 */
On Wed, 2022-10-12 at 20:46 +0300, Andy Shevchenko wrote: > On Wed, Oct 12, 2022 at 6:15 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. > > > > Note that we are shadowing the error code returned by > > iio_device_claim_buffer_mode() so that we keep the original one > > (-EAGAIN). The reason is that some userspace stack might already be > > relying on this particular code so that we are not taking chances > > and > > leave it alone. > > The above line widths seem a bit arbitrary to me. But I think it's > due > to function names in them. > Perhaps you can make them less deviated by shuffling a bit, like > moving "but to" to the next line. > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/health/max30100.c | 9 +++------ > > 1 file changed, 3 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/health/max30100.c > > b/drivers/iio/health/max30100.c > > index 2cca5e0519f8..6ac49901c9da 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)) { > > I think a summary of replacing error code is good to have here, like > > /* > * Replacing -EBUSY or other error code > * returned by iio_device_claim_buffer_mode() > * because user space may rely on the current > * one. > */ > This might make sense... I'll wait for Jonathan's review to see how strong he feels about this. Maybe he can also add it when applying. - Nuno Sá >
diff --git a/drivers/iio/health/max30100.c b/drivers/iio/health/max30100.c index 2cca5e0519f8..6ac49901c9da 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. Note that we are shadowing the error code returned by iio_device_claim_buffer_mode() so that we keep the original one (-EAGAIN). The reason is that some userspace stack might already be relying on this particular code so that we are not taking chances and leave it alone. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/health/max30100.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)