On Mon, 7 Feb 2022 23:41:49 +0800 Jia-Ju Bai <baijiaju1990@xxxxxxxxx> wrote: > Hello, > > My static analysis tool reports a possible deadlock in the opt3001 > driver in Linux 5.16: > > opt3001_read_raw() > mutex_lock(&opt->lock); --> Line 399 (Lock A) > opt3001_get_lux() > wait_event_timeout(opt->result_ready_queue, ...) --> Line 276 (Wait X) > mutex_lock(&opt->lock); --> Line 412 (Unlock A) > > opt3001_irq() > mutex_lock(&opt->lock); --> Line 693 (Lock A) > mutex_unlock(&opt->lock); --> Line 730 (Unlock A) > wake_up(&opt->result_ready_queue); --> Line 733 (Wake X) > > When opt3001_read_raw() is executed, "Wait X" is performed by holding > "Lock A". If opt3001_irq() is executed at this time, "Wake X" cannot be > performed to wake up "Wait X" in opt3001_read_raw(), because "Lock A" > has been already hold by opt3001_read_raw(), causing a possible deadlock. > I find that "Wait X" is performed with a timeout, to relieve the > possible deadlock; but I think this timeout can cause inefficient execution. Hi Jia-Ju Bai, There is a quirk in here thatyou haven't mentioned. The "magic" opt->ok_to_ignore_lock. So there are two cases: 1) No irq in use. In that case the opt3001_irq() will never run and we just sleep + check a status flag. That's the best we can do without an interrupt. 2) irq in use, the ok_to_ignore_lock = true statement occurs and in the opt3001_irq() the locks are never taken hence no deadlock. It is a very odd bit of code though so I'd be surprised if a static analyser hadn't highlighted it as a possible deadlock! Now, I'm not immediately sure why the driver is done like this as opposed to a more complete wait_for_completion() in read_raw(), complete() in the irq handler and do the actual read of the data back in read_raw(). It's probably related to the other interrupt sources that we need to differentiate from in the interrupt handler. The lock definition is missing an documentation of exactly what it's scope is which definitely doesn't help us understand this unusual structure. Thanks, Jonathan > > I am not quite sure whether this possible problem is real and how to fix > it if it is real. > Any feedback would be appreciated, thanks :) > > > Best wishes, > Jia-Ju Bai
