| Message ID | 20220920112821.975359-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make 'mlock' really private | expand |
Hi Nuno, nuno.sa@analog.com wrote on Tue, 20 Sep 2022 13:28:07 +0200: > Drop 'mlock' usage by making use of iio_device_claim_direct_mode(). > This change actually makes sure we cannot do a single conversion while > buffering is enable. Note there was a potential race in the previous > code since we were only acquiring the lock after checking if the bus is > enabled. > > Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices") To answer your question in the cover letter, I feel like this Fixes: tag is deserved. Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/adc/ad_sigma_delta.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > index 261a9a6b45e1..d8570f620785 100644 > --- a/drivers/iio/adc/ad_sigma_delta.c > +++ b/drivers/iio/adc/ad_sigma_delta.c > @@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > unsigned int data_reg; > int ret = 0; > > - if (iio_buffer_enabled(indio_dev)) > - return -EBUSY; > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > > - mutex_lock(&indio_dev->mlock); > ad_sigma_delta_set_channel(sigma_delta, chan->address); > > spi_bus_lock(sigma_delta->spi->master); > @@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > sigma_delta->bus_locked = false; > spi_bus_unlock(sigma_delta->spi->master); > - mutex_unlock(&indio_dev->mlock); > + iio_device_release_direct_mode(indio_dev); > > if (ret) > return ret; Thanks, Miquèl
On Tue, 20 Sep 2022 13:54:10 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Nuno, > > nuno.sa@analog.com wrote on Tue, 20 Sep 2022 13:28:07 +0200: > > > Drop 'mlock' usage by making use of iio_device_claim_direct_mode(). > > This change actually makes sure we cannot do a single conversion while > > buffering is enable. Note there was a potential race in the previous > > code since we were only acquiring the lock after checking if the bus is > > enabled. > > > > Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices") > > To answer your question in the cover letter, I feel like this Fixes: > tag is deserved. > > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com> Agreed to the fixes tag being valid. There is the slightly fun aspect that the issue predates the introduction of iio_device_claim_direct_mode() so when this gets backported we could get failures. However, I think the oldest kernel anyone is bothering with in the stable series is 4.9 and that function was introduced earlier the same year as that kernel so we are probably fine :) I'm going to pick the easy ones up in this series, though note that these are 'probably' now being queued up for next cycle. Linus made some noises about a possible rc8 though so I might do an extra pull request if that looks likely - then these might make it in rather sooner. Jonathan > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/adc/ad_sigma_delta.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c > > index 261a9a6b45e1..d8570f620785 100644 > > --- a/drivers/iio/adc/ad_sigma_delta.c > > +++ b/drivers/iio/adc/ad_sigma_delta.c > > @@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > > unsigned int data_reg; > > int ret = 0; > > > > - if (iio_buffer_enabled(indio_dev)) > > - return -EBUSY; > > + ret = iio_device_claim_direct_mode(indio_dev); > > + if (ret) > > + return ret; > > > > - mutex_lock(&indio_dev->mlock); > > ad_sigma_delta_set_channel(sigma_delta, chan->address); > > > > spi_bus_lock(sigma_delta->spi->master); > > @@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, > > ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); > > sigma_delta->bus_locked = false; > > spi_bus_unlock(sigma_delta->spi->master); > > - mutex_unlock(&indio_dev->mlock); > > + iio_device_release_direct_mode(indio_dev); > > > > if (ret) > > return ret; > > > Thanks, > Miquèl
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c index 261a9a6b45e1..d8570f620785 100644 --- a/drivers/iio/adc/ad_sigma_delta.c +++ b/drivers/iio/adc/ad_sigma_delta.c @@ -281,10 +281,10 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, unsigned int data_reg; int ret = 0; - if (iio_buffer_enabled(indio_dev)) - return -EBUSY; + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; - mutex_lock(&indio_dev->mlock); ad_sigma_delta_set_channel(sigma_delta, chan->address); spi_bus_lock(sigma_delta->spi->master); @@ -323,7 +323,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev, ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE); sigma_delta->bus_locked = false; spi_bus_unlock(sigma_delta->spi->master); - mutex_unlock(&indio_dev->mlock); + iio_device_release_direct_mode(indio_dev); if (ret) return ret;
Drop 'mlock' usage by making use of iio_device_claim_direct_mode(). This change actually makes sure we cannot do a single conversion while buffering is enable. Note there was a potential race in the previous code since we were only acquiring the lock after checking if the bus is enabled. Fixes: af3008485ea0 ("iio:adc: Add common code for ADI Sigma Deltadevices") Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/adc/ad_sigma_delta.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)