Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) >>> -> opt3001_get_lux() >>> .. >>> ->i2c_smbus_write_word_swapped() >>> Now an interrupt comes >>> -> opt3001_irq >>>-> mutex_lock(>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(>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
Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) >>> -> opt3001_get_lux() >>> .. >>> ->i2c_smbus_write_word_swapped() >>> Now an interrupt comes >>> -> opt3001_irq >>>-> mutex_lock(>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(>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
Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) -> opt3001_get_lux() .. ->i2c_smbus_write_word_swapped() Now an interrupt comes -> opt3001_irq -> mutex_lock(>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(>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: andria...@ispras.ru
Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) -> opt3001_get_lux() .. ->i2c_smbus_write_word_swapped() Now an interrupt comes -> opt3001_irq -> mutex_lock(>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(>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: andria...@ispras.ru
Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) > -> opt3001_get_lux() > .. > ->i2c_smbus_write_word_swapped() > Now an interrupt comes > -> opt3001_irq >-> mutex_lock(>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(>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
Re: A potential bug in drivers/iio/light/opt3001.ko
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(>lock) > -> opt3001_get_lux() > .. > ->i2c_smbus_write_word_swapped() > Now an interrupt comes > -> opt3001_irq >-> mutex_lock(>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(>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
A potential bug in drivers/iio/light/opt3001.ko
Hi! There is a bug in drivers/iio/light/opt3001.ko. Regard such case: Thread 1 Thread 2 -> opt3001_read_raw -> mutex_lock(>lock) -> opt3001_get_lux() .. ->i2c_smbus_write_word_swapped() Now an interrupt comes -> opt3001_irq -> mutex_lock(>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(>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: andria...@ispras.ru
A potential bug in drivers/iio/light/opt3001.ko
Hi! There is a bug in drivers/iio/light/opt3001.ko. Regard such case: Thread 1 Thread 2 -> opt3001_read_raw -> mutex_lock(>lock) -> opt3001_get_lux() .. ->i2c_smbus_write_word_swapped() Now an interrupt comes -> opt3001_irq -> mutex_lock(>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(>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: andria...@ispras.ru
