On 17/08/16 13:13, Vignesh R wrote:
> It is possible that two or more ADC channels can be simultaneously
> requested for raw samples, in which case there can be race in access to
> FIFO data resulting in loss of samples.
> If am335x_tsc_se_set_once() is called again from tiadc_read_raw(), when
> ADC is still acquired to sample one of the channels, the second process
> might be put into uninterruptible sleep state. Fix these issues, by
> protecting FIFO access and channel configurations with a mutex. Since
> tiadc_read_raw() might take anywhere between few microseconds to few
> milliseconds to finish execution (depending on averaging and delay
> values supplied via DT), its better to use mutex instead of spinlock.
>
> Fixes: 7ca6740cd1cd4 ("mfd: input: iio: ti_amm335x: Rework TSC/ADC
> synchronization")
> Signed-off-by: Vignesh R <[email protected]>
Thanks for tracking this down and for the patch in the
first place!
Applied to the fixes-togreg branch of iio.git and marked for
stable. Should head upstream sometime this week.
Jonathan
> ---
>
> v2: Add fixes tag.
>
> drivers/iio/adc/ti_am335x_adc.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 8a368756881b..bed9977a1863 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -32,6 +32,7 @@
>
> struct tiadc_device {
> struct ti_tscadc_dev *mfd_tscadc;
> + struct mutex fifo1_lock; /* to protect fifo access */
> int channels;
> u8 channel_line[8];
> u8 channel_step[8];
> @@ -359,6 +360,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> + int ret = IIO_VAL_INT;
> int i, map_val;
> unsigned int fifo1count, read, stepid;
> bool found = false;
> @@ -372,6 +374,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> if (!step_en)
> return -EINVAL;
>
> + mutex_lock(&adc_dev->fifo1_lock);
> fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
> while (fifo1count--)
> tiadc_readl(adc_dev, REG_FIFO1);
> @@ -388,7 +391,8 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
>
> if (time_after(jiffies, timeout)) {
> am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
> - return -EAGAIN;
> + ret = -EAGAIN;
> + goto err_unlock;
> }
> }
> map_val = adc_dev->channel_step[chan->scan_index];
> @@ -414,8 +418,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
> am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
>
> if (found == false)
> - return -EBUSY;
> - return IIO_VAL_INT;
> + ret = -EBUSY;
> +
> +err_unlock:
> + mutex_unlock(&adc_dev->fifo1_lock);
> + return ret;
> }
>
> static const struct iio_info tiadc_info = {
> @@ -483,6 +490,7 @@ static int tiadc_probe(struct platform_device *pdev)
>
> tiadc_step_config(indio_dev);
> tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
> + mutex_init(&adc_dev->fifo1_lock);
>
> err = tiadc_channel_init(indio_dev, adc_dev->channels);
> if (err < 0)
>
