* [PATCH v2 0/2] ti_am335x_adc: Fix bugs related to oneshot read @ 2016-08-17 12:12 Vignesh R 2016-08-17 12:13 ` [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access Vignesh R 2016-08-17 12:13 ` [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample Vignesh R 0 siblings, 2 replies; 5+ messages in thread From: Vignesh R @ 2016-08-17 12:12 UTC (permalink / raw) To: Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones, Vignesh R, Andrew F . Davis, linux-iio, linux-omap, linux-kernel This series fixes a couple of bugs related to simultaneous read of multiple ADC channels in oneshot mode. Since, all channels share FIFO1, it has to be protected using a mutex, implemented by patch 1. Patch 2 increases the timeout waiting for a ADC sample. Vignesh R (2): iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample drivers/iio/adc/ti_am335x_adc.c | 16 ++++++++++++---- include/linux/mfd/ti_am335x_tscadc.h | 8 ++++---- 2 files changed, 16 insertions(+), 8 deletions(-) -- 2.9.2 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access 2016-08-17 12:12 [PATCH v2 0/2] ti_am335x_adc: Fix bugs related to oneshot read Vignesh R @ 2016-08-17 12:13 ` Vignesh R 2016-08-21 18:49 ` Jonathan Cameron 2016-08-17 12:13 ` [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample Vignesh R 1 sibling, 1 reply; 5+ messages in thread From: Vignesh R @ 2016-08-17 12:13 UTC (permalink / raw) To: Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones, Vignesh R, Andrew F . Davis, linux-iio, linux-omap, linux-kernel 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 <vigneshr@ti.com> --- 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) -- 2.9.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access 2016-08-17 12:13 ` [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access Vignesh R @ 2016-08-21 18:49 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2016-08-21 18:49 UTC (permalink / raw) To: Vignesh R Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones, Andrew F . Davis, linux-iio, linux-omap, linux-kernel 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 <vigneshr@ti.com> 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) > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample 2016-08-17 12:12 [PATCH v2 0/2] ti_am335x_adc: Fix bugs related to oneshot read Vignesh R 2016-08-17 12:13 ` [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access Vignesh R @ 2016-08-17 12:13 ` Vignesh R 2016-08-21 18:49 ` Jonathan Cameron 1 sibling, 1 reply; 5+ messages in thread From: Vignesh R @ 2016-08-17 12:13 UTC (permalink / raw) To: Jonathan Cameron Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones, Vignesh R, Andrew F . Davis, linux-iio, linux-omap, linux-kernel Now that open delay and sample delay for each channel is configurable via DT, the default IDLE_TIMEOUT value is not enough as this is calculated based on hardcoded macros. This results in driver returning EBUSY sometimes. Fix this by increasing the timeout value based on maximum value possible to open delay and sample delays for each channel. Fixes: 5dc11e810676e ("iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters") Signed-off-by: Vignesh R <vigneshr@ti.com> Acked-by: Lee Jones <lee.jones@linaro.org> --- v2: Add fixes tag. drivers/iio/adc/ti_am335x_adc.c | 2 +- include/linux/mfd/ti_am335x_tscadc.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c index bed9977a1863..c3cfacca2541 100644 --- a/drivers/iio/adc/ti_am335x_adc.c +++ b/drivers/iio/adc/ti_am335x_adc.c @@ -381,7 +381,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev, am335x_tsc_se_set_once(adc_dev->mfd_tscadc, step_en); - timeout = jiffies + usecs_to_jiffies + timeout = jiffies + msecs_to_jiffies (IDLE_TIMEOUT * adc_dev->channels); /* Wait for Fifo threshold interrupt */ while (1) { diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h index 2567a87872b0..7f55b8b41032 100644 --- a/include/linux/mfd/ti_am335x_tscadc.h +++ b/include/linux/mfd/ti_am335x_tscadc.h @@ -138,16 +138,16 @@ /* * time in us for processing a single channel, calculated as follows: * - * num cycles = open delay + (sample delay + conv time) * averaging + * max num cycles = open delay + (sample delay + conv time) * averaging * - * num cycles: 152 + (1 + 13) * 16 = 376 + * max num cycles: 262143 + (255 + 13) * 16 = 266431 * * clock frequency: 26MHz / 8 = 3.25MHz * clock period: 1 / 3.25MHz = 308ns * - * processing time: 376 * 308ns = 116us + * max processing time: 266431 * 308ns = 83ms(approx) */ -#define IDLE_TIMEOUT 116 /* microsec */ +#define IDLE_TIMEOUT 83 /* milliseconds */ #define TSCADC_CELLS 2 -- 2.9.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample 2016-08-17 12:13 ` [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample Vignesh R @ 2016-08-21 18:49 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2016-08-21 18:49 UTC (permalink / raw) To: Vignesh R Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler, Lee Jones, Andrew F . Davis, linux-iio, linux-omap, linux-kernel On 17/08/16 13:13, Vignesh R wrote: > Now that open delay and sample delay for each channel is configurable > via DT, the default IDLE_TIMEOUT value is not enough as this is > calculated based on hardcoded macros. This results in driver returning > EBUSY sometimes. Fix this by increasing the timeout > value based on maximum value possible to open delay and sample delays > for each channel. > > Fixes: 5dc11e810676e ("iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters") > Signed-off-by: Vignesh R <vigneshr@ti.com> > Acked-by: Lee Jones <lee.jones@linaro.org> Applied to the fixes-togreg branch of iio.git and marked for stable. Thanks, Jonathan > --- > > v2: Add fixes tag. > > drivers/iio/adc/ti_am335x_adc.c | 2 +- > include/linux/mfd/ti_am335x_tscadc.h | 8 ++++---- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c > index bed9977a1863..c3cfacca2541 100644 > --- a/drivers/iio/adc/ti_am335x_adc.c > +++ b/drivers/iio/adc/ti_am335x_adc.c > @@ -381,7 +381,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev, > > am335x_tsc_se_set_once(adc_dev->mfd_tscadc, step_en); > > - timeout = jiffies + usecs_to_jiffies > + timeout = jiffies + msecs_to_jiffies > (IDLE_TIMEOUT * adc_dev->channels); > /* Wait for Fifo threshold interrupt */ > while (1) { > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h > index 2567a87872b0..7f55b8b41032 100644 > --- a/include/linux/mfd/ti_am335x_tscadc.h > +++ b/include/linux/mfd/ti_am335x_tscadc.h > @@ -138,16 +138,16 @@ > /* > * time in us for processing a single channel, calculated as follows: > * > - * num cycles = open delay + (sample delay + conv time) * averaging > + * max num cycles = open delay + (sample delay + conv time) * averaging > * > - * num cycles: 152 + (1 + 13) * 16 = 376 > + * max num cycles: 262143 + (255 + 13) * 16 = 266431 > * > * clock frequency: 26MHz / 8 = 3.25MHz > * clock period: 1 / 3.25MHz = 308ns > * > - * processing time: 376 * 308ns = 116us > + * max processing time: 266431 * 308ns = 83ms(approx) > */ > -#define IDLE_TIMEOUT 116 /* microsec */ > +#define IDLE_TIMEOUT 83 /* milliseconds */ > > #define TSCADC_CELLS 2 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-08-21 19:15 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-08-17 12:12 [PATCH v2 0/2] ti_am335x_adc: Fix bugs related to oneshot read Vignesh R 2016-08-17 12:13 ` [PATCH v2 1/2] iio: adc: ti_am335x_adc: Protect FIFO1 from concurrent access Vignesh R 2016-08-21 18:49 ` Jonathan Cameron 2016-08-17 12:13 ` [PATCH v2 2/2] iio: adc: ti_am335x_adc: Increase timeout value waiting for ADC sample Vignesh R 2016-08-21 18:49 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).