On Tue, Mar 16, 2021 at 10:47:53AM +0100, Krzysztof Kozlowski wrote: > On 16/03/2021 10:02, Johan Hovold wrote: > > On Mon, Mar 15, 2021 at 07:12:12PM +0100, Krzysztof Kozlowski wrote: > >> Since interrupt handler is called with disabled local interrupts, there > >> is no need to use the spinlock primitives disabling interrupts as well. > > > > This isn't generally true due to "threadirqs" and that can lead to > > deadlocks if the console code is called from hard irq context. > > > > Now, this is *not* the case for this particular driver since it doesn't > > even bother to take the port lock in console_write(). That should > > probably be fixed instead. > > > > See https://lore.kernel.org/r/X7kviiRwuxvPxC8O@localhost. > > Thanks for the link, quite interesting! For one type of device we have > two interrupts (RX and TX) so I guess it's a valid point/risk. However > let me try to understand it more. > > Assuming we had only one interrupt line, how this interrupt handler with > threadirqs could be called from hardirq context?
No, it's console_write() which can end up being called in hard irq context and if that path takes the port lock after the now threaded interrupt handler has been preempted you have a deadlock. > You wrote there: > > For console drivers this can even happen for the same interrupt as the > > generic interrupt code can call printk(), and so can any other handler > > that isn't threaded (e.g. hrtimers or explicit IRQF_NO_THREAD). > > However I replaced here only interrupt handler's spin lock to non-irq. > This code path will be executed only when interrupt is masked therefore > for one interrupt line there is *no possibility of*: > > -> s3c64xx_serial_handle_irq > - interrupts are masked > - s3c24xx_serial_tx_irq > - spin_lock() > -> hrtimers or other IRQF_NO_THREAD > - console_write() or something > - s3c64xx_serial_handle_irq You don't end up in s3c64xx_serial_handle_irq() here. It's just that console_write() (typically) takes the port lock which is already held by the preempted s3c24xx_serial_tx_irq(). > - s3c24xx_serial_tx_irq > - spin_lock() Johan
