From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934267AbcIEOPi (ORCPT ); Mon, 5 Sep 2016 10:15:38 -0400 Received: from mail.ispras.ru ([83.149.199.45]:55056 "EHLO mail.ispras.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932636AbcIEOPe (ORCPT ); Mon, 5 Sep 2016 10:15:34 -0400 Subject: Re: A potential bug in drivers/iio/light/opt3001.ko To: Jonathan Cameron References: <61728463-8bd8-c68e-ef4a-7a3e0ddd7899@kernel.org> Cc: Hartmut Knaack , Lars-Peter Clausen , Peter Meerwald-Stadler , linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, Alexander Koch , Vaishali Thakkar , ldv-project@linuxtesting.org, Andreas Dannenberg From: Pavel Andrianov Message-ID: <0bb361fc-5912-eae2-e081-05e45f774798@ispras.ru> Date: Mon, 5 Sep 2016 17:15:30 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <61728463-8bd8-c68e-ef4a-7a3e0ddd7899@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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