* A potential bug in drivers/iio/light/opt3001.ko
@ 2016-08-31 10:23 Pavel Andrianov
2016-09-03 16:38 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Pavel Andrianov @ 2016-08-31 10:23 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, linux-kernel, Alexander Koch, Vaishali Thakkar,
ldv-project
Hi!
There is a bug in drivers/iio/light/opt3001.ko. Regard such case:
Thread 1 Thread 2
-> opt3001_read_raw
-> mutex_lock(&opt->lock)
-> opt3001_get_lux()
..
->i2c_smbus_write_word_swapped()
Now an interrupt comes
-> opt3001_irq
-> mutex_lock(&opt->lock)
This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.
Regard another case:
Thread 1 Thread 2
-> opt3001_read_raw
-> mutex_lock(&opt->lock)
-> opt3001_get_lux()
..
-> i2c_smbus_write_word_swapped()
opt->ok_to_ignore_lock = true;
Now an interrupt comes
-> opt3001_irq
..
opt->result_ready = true
wake_up()
opt->result_ready = false;
wait_event_timeout()
In this case the first thread misses the result and waits until timeout
expires.
--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov@ispras.ru
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: A potential bug in drivers/iio/light/opt3001.ko
2016-08-31 10:23 A potential bug in drivers/iio/light/opt3001.ko Pavel Andrianov
@ 2016-09-03 16:38 ` Jonathan Cameron
2016-09-05 14:15 ` Pavel Andrianov
0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Cameron @ 2016-09-03 16:38 UTC (permalink / raw)
To: Pavel Andrianov
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, linux-kernel, Alexander Koch, Vaishali Thakkar,
ldv-project, Andreas Dannenberg
On 31/08/16 11:23, Pavel Andrianov wrote:
> Hi!
>
> There is a bug in drivers/iio/light/opt3001.ko. Regard such case:
>
> Thread 1 Thread 2
> -> opt3001_read_raw
> -> mutex_lock(&opt->lock)
> -> opt3001_get_lux()
> ..
> ->i2c_smbus_write_word_swapped()
> Now an interrupt comes
> -> opt3001_irq
> -> mutex_lock(&opt->lock)
>
> This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.
Good find. Will need reordering to set the ok_to_ignore_lock first.
Whether it ever actually happens will depend on just how long that EOC
interrupt takes to happen. Still it's a theoretical problem with
a fairly simple fix so let's fix it.
>
> Regard another case:
>
> Thread 1 Thread 2
> -> opt3001_read_raw
> -> mutex_lock(&opt->lock)
> -> opt3001_get_lux()
> ..
> -> i2c_smbus_write_word_swapped()
> opt->ok_to_ignore_lock = true;
> Now an interrupt comes
> -> opt3001_irq
> ..
> opt->result_ready = true
> wake_up()
> opt->result_ready = false;
> wait_event_timeout()
>
> In this case the first thread misses the result and waits until timeout expires.
>
Agreed - looks like some reordering is needed here as well.
Jonathan
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: A potential bug in drivers/iio/light/opt3001.ko
2016-09-03 16:38 ` Jonathan Cameron
@ 2016-09-05 14:15 ` Pavel Andrianov
2016-09-05 20:04 ` Jonathan Cameron
0 siblings, 1 reply; 4+ messages in thread
From: Pavel Andrianov @ 2016-09-05 14:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, linux-kernel, Alexander Koch, Vaishali Thakkar,
ldv-project, Andreas Dannenberg
03.09.2016 19:38, Jonathan Cameron пишет:
> On 31/08/16 11:23, Pavel Andrianov wrote:
>> Hi!
>>
>> There is a bug in drivers/iio/light/opt3001.ko. Regard such case:
>>
>> Thread 1 Thread 2
>> -> opt3001_read_raw
>> -> mutex_lock(&opt->lock)
>> -> opt3001_get_lux()
>> ..
>> ->i2c_smbus_write_word_swapped()
>> Now an interrupt comes
>> -> opt3001_irq
>> -> mutex_lock(&opt->lock)
>>
>> This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.
> Good find. Will need reordering to set the ok_to_ignore_lock first.
> Whether it ever actually happens will depend on just how long that EOC
> interrupt takes to happen. Still it's a theoretical problem with
> a fairly simple fix so let's fix it.
>>
>> Regard another case:
>>
>> Thread 1 Thread 2
>> -> opt3001_read_raw
>> -> mutex_lock(&opt->lock)
>> -> opt3001_get_lux()
>> ..
>> -> i2c_smbus_write_word_swapped()
>> opt->ok_to_ignore_lock = true;
>> Now an interrupt comes
>> -> opt3001_irq
>> ..
>> opt->result_ready = true
>> wake_up()
>> opt->result_ready = false;
>> wait_event_timeout()
>>
>> In this case the first thread misses the result and waits until timeout expires.
>>
> Agreed - looks like some reordering is needed here as well.
>
> Jonathan
>
In opt3001_get_lux has a comment, that i2c_smbus_write_word_swapped
(line 246) enables interrupt mechanism. If an interrupt can not arise
before the function, the assignments to both of flags should be moved
before i2c_smbus_write_word_swapped and this is the best fix for both of
issues.
Do you know if my assumption is correct and interrupts are disabled
before i2c_smbus_write_word_swapped call?
--
Pavel Andrianov
Linux Verification Center, ISPRAS
web: http://linuxtesting.org
e-mail: andrianov@ispras.ru
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: A potential bug in drivers/iio/light/opt3001.ko
2016-09-05 14:15 ` Pavel Andrianov
@ 2016-09-05 20:04 ` Jonathan Cameron
0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Cameron @ 2016-09-05 20:04 UTC (permalink / raw)
To: Pavel Andrianov
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
linux-iio, linux-kernel, Alexander Koch, Vaishali Thakkar,
ldv-project, Andreas Dannenberg
On 05/09/16 15:15, Pavel Andrianov wrote:
> 03.09.2016 19:38, Jonathan Cameron пишет:
>> On 31/08/16 11:23, Pavel Andrianov wrote:
>>> Hi!
>>>
>>> There is a bug in drivers/iio/light/opt3001.ko. Regard such case:
>>>
>>> Thread 1 Thread 2
>>> -> opt3001_read_raw
>>> -> mutex_lock(&opt->lock)
>>> -> opt3001_get_lux()
>>> ..
>>> ->i2c_smbus_write_word_swapped()
>>> Now an interrupt comes
>>> -> opt3001_irq
>>> -> mutex_lock(&opt->lock)
>>>
>>> This is a deadlock, as the flag ok_to_ignore_lock has not been set yet.
>> Good find. Will need reordering to set the ok_to_ignore_lock first.
>> Whether it ever actually happens will depend on just how long that EOC
>> interrupt takes to happen. Still it's a theoretical problem with
>> a fairly simple fix so let's fix it.
>>>
>>> Regard another case:
>>>
>>> Thread 1 Thread 2
>>> -> opt3001_read_raw
>>> -> mutex_lock(&opt->lock)
>>> -> opt3001_get_lux()
>>> ..
>>> -> i2c_smbus_write_word_swapped()
>>> opt->ok_to_ignore_lock = true;
>>> Now an interrupt comes
>>> -> opt3001_irq
>>> ..
>>> opt->result_ready = true
>>> wake_up()
>>> opt->result_ready = false;
>>> wait_event_timeout()
>>>
>>> In this case the first thread misses the result and waits until timeout expires.
>>>
>> Agreed - looks like some reordering is needed here as well.
>>
>> Jonathan
>>
>
> In opt3001_get_lux has a comment, that i2c_smbus_write_word_swapped
> (line 246) enables interrupt mechanism. If an interrupt can not arise
> before the function, the assignments to both of flags should be moved
> before i2c_smbus_write_word_swapped and this is the best fix for both
> of issues. Do you know if my assumption is correct and interrupts are
> disabled before i2c_smbus_write_word_swapped call?
Andreas, can you confirm this for us?
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-05 20:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-31 10:23 A potential bug in drivers/iio/light/opt3001.ko Pavel Andrianov
2016-09-03 16:38 ` Jonathan Cameron
2016-09-05 14:15 ` Pavel Andrianov
2016-09-05 20:04 ` 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).