This patch does the following
1. Handle the return values of wait_for_completion_interruptible_timeout
2. Add spin locks to avoid race conditions during ISR.
Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
Cc: Doug Anderson <[email protected]>
Cc: Lars-Peter Clausen <[email protected]>
---
Discussion thread for this patch can be found at
http://www.gossamer-threads.com/lists/linux/kernel/1693284?page=last
I've not seen any reference to spin lock usage in IIO.
Kindly, suggest me if there is a better way to avoid the race.
Thanks,
Naveen
drivers/iio/adc/exynos_adc.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index ed6fdd7..4de28ae 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -91,6 +91,7 @@ struct exynos_adc {
struct completion completion;
+ spinlock_t reg_lock;
u32 value;
unsigned int version;
};
@@ -117,7 +118,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
long mask)
{
struct exynos_adc *info = iio_priv(indio_dev);
- unsigned long timeout;
+ long timeout;
u32 con1, con2;
if (mask != IIO_CHAN_INFO_RAW)
@@ -143,15 +144,19 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
ADC_V1_CON(info->regs));
}
+ INIT_COMPLETION(info->completion);
+
timeout = wait_for_completion_interruptible_timeout
(&info->completion, EXYNOS_ADC_TIMEOUT);
+
*val = info->value;
mutex_unlock(&indio_dev->mlock);
if (timeout == 0)
return -ETIMEDOUT;
-
+ else if (timeout < 0)
+ return timeout;
return IIO_VAL_INT;
}
@@ -159,6 +164,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
{
struct exynos_adc *info = (struct exynos_adc *)dev_id;
+ spin_lock(&info->reg_lock);
+
/* Read value */
info->value = readl(ADC_V1_DATX(info->regs)) &
ADC_DATX_MASK;
@@ -170,6 +177,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
complete(&info->completion);
+ spin_unlock(&info->reg_lock);
+
return IRQ_HANDLED;
}
@@ -327,6 +336,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
else
indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
+ spin_lock_init(&info->reg_lock);
ret = iio_device_register(indio_dev);
if (ret)
goto err_irq;
--
1.7.9.5
On 03/15/2013 05:26 PM, Naveen Krishna Chatradhi wrote:
> This patch does the following
> 1. Handle the return values of wait_for_completion_interruptible_timeout
> 2. Add spin locks to avoid race conditions during ISR.
>
> Signed-off-by: Naveen Krishna Chatradhi <[email protected]>
> Cc: Doug Anderson <[email protected]>
> Cc: Lars-Peter Clausen <[email protected]>
> ---
> Discussion thread for this patch can be found at
> http://www.gossamer-threads.com/lists/linux/kernel/1693284?page=last
>
> I've not seen any reference to spin lock usage in IIO.
> Kindly, suggest me if there is a better way to avoid the race.
>
> Thanks,
> Naveen
> drivers/iio/adc/exynos_adc.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index ed6fdd7..4de28ae 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -91,6 +91,7 @@ struct exynos_adc {
>
> struct completion completion;
>
> + spinlock_t reg_lock;
> u32 value;
> unsigned int version;
> };
> @@ -117,7 +118,7 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> long mask)
> {
> struct exynos_adc *info = iio_priv(indio_dev);
> - unsigned long timeout;
> + long timeout;
> u32 con1, con2;
>
> if (mask != IIO_CHAN_INFO_RAW)
> @@ -143,15 +144,19 @@ static int exynos_read_raw(struct iio_dev *indio_dev,
> ADC_V1_CON(info->regs));
> }
>
> + INIT_COMPLETION(info->completion);
> +
This needs to happen before you start the transfer.
> timeout = wait_for_completion_interruptible_timeout
> (&info->completion, EXYNOS_ADC_TIMEOUT);
> +
> *val = info->value;
>
> mutex_unlock(&indio_dev->mlock);
>
> if (timeout == 0)
> return -ETIMEDOUT;
> -
> + else if (timeout < 0)
> + return timeout;
> return IIO_VAL_INT;
> }
>
> @@ -159,6 +164,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
> {
> struct exynos_adc *info = (struct exynos_adc *)dev_id;
>
> + spin_lock(&info->reg_lock);
> +
> /* Read value */
> info->value = readl(ADC_V1_DATX(info->regs)) &
> ADC_DATX_MASK;
> @@ -170,6 +177,8 @@ static irqreturn_t exynos_adc_isr(int irq, void *dev_id)
>
> complete(&info->completion);
>
> + spin_unlock(&info->reg_lock);
> +
What exactly is the spinlock protecting against here? Concurrent runs of
exynos_adc_isr? This is probably not issue in the first place.
What you want to protect against is that completion is completed between the
call to INIT_COMPLETION() and the start of a new conversion. So the sections
that need to be under the spinlock are the complete call here and the point
from INIT_COMPLETION until the transfer is started in exynos_read_raw(). Make
sure to use spin_lock_irq there.
> return IRQ_HANDLED;
> }
>
> @@ -327,6 +336,7 @@ static int exynos_adc_probe(struct platform_device *pdev)
> else
> indio_dev->num_channels = MAX_ADC_V2_CHANNELS;
>
> + spin_lock_init(&info->reg_lock);
> ret = iio_device_register(indio_dev);
> if (ret)
> goto err_irq;
On Fri, Mar 15, 2013 at 2:53 PM, Lars-Peter Clausen <[email protected]> wrote:
> What exactly is the spinlock protecting against here? Concurrent runs of
> exynos_adc_isr? This is probably not issue in the first place.
>
> What you want to protect against is that completion is completed between the
> call to INIT_COMPLETION() and the start of a new conversion. So the sections
> that need to be under the spinlock are the complete call here and the point
> from INIT_COMPLETION until the transfer is started in exynos_read_raw(). Make
> sure to use spin_lock_irq there.
...and at that point I _think_ you won't also need the mutex.
A reasonable way to test to see if you've got this all correct would be to:
* Start two processes that are reading from different ADCs that will
report very different values (maybe add a device tree node for adc1 or
adc7 and use those since they're not really connected to
thermistors?).
* Have your two processes read as fast as they can. This could just
be "while true; do cat /sys/class/hwmon/hwmon0/device/temp1_input;
done"
* Decrease your timeout and maybe(?) sprinkle some random udelays in
the irq handler so that the timeouts happen sometimes but not others.
* Periodically cancel one of the readers with Ctrl-C
If all is working well then you should always get back the right value
from the right reader (and get no crashes).
-Doug
On 03/16/2013 01:37 AM, Doug Anderson wrote:
> On Fri, Mar 15, 2013 at 2:53 PM, Lars-Peter Clausen <[email protected]> wrote:
>> What exactly is the spinlock protecting against here? Concurrent runs of
>> exynos_adc_isr? This is probably not issue in the first place.
>>
>> What you want to protect against is that completion is completed between the
>> call to INIT_COMPLETION() and the start of a new conversion. So the sections
>> that need to be under the spinlock are the complete call here and the point
>> from INIT_COMPLETION until the transfer is started in exynos_read_raw(). Make
>> sure to use spin_lock_irq there.
>
> ...and at that point I _think_ you won't also need the mutex.
>
> A reasonable way to test to see if you've got this all correct would be to:
>
> * Start two processes that are reading from different ADCs that will
> report very different values (maybe add a device tree node for adc1 or
> adc7 and use those since they're not really connected to
> thermistors?).
>
> * Have your two processes read as fast as they can. This could just
> be "while true; do cat /sys/class/hwmon/hwmon0/device/temp1_input;
> done"
>
> * Decrease your timeout and maybe(?) sprinkle some random udelays in
> the irq handler so that the timeouts happen sometimes but not others.
>
> * Periodically cancel one of the readers with Ctrl-C
>
> If all is working well then you should always get back the right value
> from the right reader (and get no crashes).
>
I think you still need the mutex for serialization, otherwise the requests
would just cancel each other out. Btw. what happens if you start a conversion
while another is still in progress? Is it possible to abort a conversion?
- Lars
Lars,
On Sat, Mar 16, 2013 at 7:41 AM, Lars-Peter Clausen <[email protected]> wrote:
> I think you still need the mutex for serialization, otherwise the requests
> would just cancel each other out. Btw. what happens if you start a conversion
> while another is still in progress? Is it possible to abort a conversion?
I was thinking that the spinlock would just replace the mutex for the
purposes of serialization.
I stepped back a bit, though, and I'm wondering if we're over-thinking
things. The timeout case should certainly be handled properly (thanks
for pointing it out), but getting a timeout is really not expected and
adding a lot of extra overhead to handle it elegantly seems a bit
much?
Specifically, the mutex means that we have one user of the ADC at a
time, and ADC conversion has nothing variable about it. The user
manual that I have access to talks about 12-bit conversion happening
in 1 microsecond with a 5MHz input clock or 5 microseconds with a 1MHz
input clock. Even if someone has clocks configured very differently,
it would be hard to imagine a conversion actually taking a full
second.
...so that means that if the timeout actually fires then something
else fairly drastic has gone wrong. It's _very_ unlikely that the IRQ
will still go off for this conversion sometime in the future.
To me, total modifications to what's landed already ought to be:
* Change timeout to long (from unsigned long)
* Make sure we return errors (negative results) from
wait_for_completion_interruptible_timeout() properly.
* If we get back a value of 0 from
wait_for_completion_interruptible_timeout() then we should print a
warning and attempt machinations to reset the ADC. Without ever
seeing real-world situtations that would cause a real timeout these
machinations would be a bit of a guess (is resetting the adc useful
when it's more likely that someone accidentally messed with the clock
tree or power gated the ADC?)... ...or perhaps a warning and a TODO
in the code would be enough?
Thoughts?
-Doug
On 04/03/2013 07:06 PM, Doug Anderson wrote:
> Lars,
>
> On Sat, Mar 16, 2013 at 7:41 AM, Lars-Peter Clausen <[email protected]> wrote:
>> I think you still need the mutex for serialization, otherwise the requests
>> would just cancel each other out. Btw. what happens if you start a conversion
>> while another is still in progress? Is it possible to abort a conversion?
>
> I was thinking that the spinlock would just replace the mutex for the
> purposes of serialization.
>
Since we sleep inside the protected section we need to use a mutex.
> I stepped back a bit, though, and I'm wondering if we're over-thinking
> things. The timeout case should certainly be handled properly (thanks
> for pointing it out), but getting a timeout is really not expected and
> adding a lot of extra overhead to handle it elegantly seems a bit
> much?
>
> Specifically, the mutex means that we have one user of the ADC at a
> time, and ADC conversion has nothing variable about it. The user
> manual that I have access to talks about 12-bit conversion happening
> in 1 microsecond with a 5MHz input clock or 5 microseconds with a 1MHz
> input clock. Even if someone has clocks configured very differently,
> it would be hard to imagine a conversion actually taking a full
> second.
>
> ...so that means that if the timeout actually fires then something
> else fairly drastic has gone wrong. It's _very_ unlikely that the IRQ
> will still go off for this conversion sometime in the future.
>
It's not the timeout case I'm worried about, but the case where the transfer
is interrupted by the user. Even though it is rather unlikely for the
problem to occur we should still try to avoid it, this is one of these
annoying heisenbugs that happen once in a while and nobody is able to
reproduce them.
> To me, total modifications to what's landed already ought to be:
>
> * Change timeout to long (from unsigned long)
>
> * Make sure we return errors (negative results) from
> wait_for_completion_interruptible_timeout() properly.
>
> * If we get back a value of 0 from
> wait_for_completion_interruptible_timeout() then we should print a
> warning and attempt machinations to reset the ADC. Without ever
> seeing real-world situtations that would cause a real timeout these
> machinations would be a bit of a guess (is resetting the adc useful
> when it's more likely that someone accidentally messed with the clock
> tree or power gated the ADC?)... ...or perhaps a warning and a TODO
> in the code would be enough?
>
>
> Thoughts?
I think most of this is already implemented and Naveen sent a patch to reset
the controller in case of a timeout, which is a good idea and works fine,
but you still should handle the case where the user aborted the transfer.
Just resetting the core should work as well in that case.
- Lars
Lars,
On Fri, Apr 5, 2013 at 1:53 AM, Lars-Peter Clausen <[email protected]> wrote:
> Since we sleep inside the protected section we need to use a mutex.
Ah, good point.
> It's not the timeout case I'm worried about, but the case where the transfer
> is interrupted by the user. Even though it is rather unlikely for the
> problem to occur we should still try to avoid it, this is one of these
> annoying heisenbugs that happen once in a while and nobody is able to
> reproduce them.
Yes, of course. Then we can also get extra confidence that the reset
logic works well by stressing out this case... :)
This makes me think, though. Given how fast we expect the ADC
transaction to finish, would there be any benefit to making the wait
non-interruptible and then shortening the timeout a whole lot. If we
shortened to 1ms then we're really not "non-interruptible" for very
long and there's less chance of subtle bugs in the way that reset
works.
-Doug
On 04/05/2013 04:56 PM, Doug Anderson wrote:
> Lars,
>
> On Fri, Apr 5, 2013 at 1:53 AM, Lars-Peter Clausen <[email protected]> wrote:
>> Since we sleep inside the protected section we need to use a mutex.
>
> Ah, good point.
>
>> It's not the timeout case I'm worried about, but the case where the transfer
>> is interrupted by the user. Even though it is rather unlikely for the
>> problem to occur we should still try to avoid it, this is one of these
>> annoying heisenbugs that happen once in a while and nobody is able to
>> reproduce them.
>
> Yes, of course. Then we can also get extra confidence that the reset
> logic works well by stressing out this case... :)
>
> This makes me think, though. Given how fast we expect the ADC
> transaction to finish, would there be any benefit to making the wait
> non-interruptible and then shortening the timeout a whole lot. If we
> shortened to 1ms then we're really not "non-interruptible" for very
> long and there's less chance of subtle bugs in the way that reset
> works.
Yes, that could also work.
- Lars