On 16/03/2021 10:56, Johan Hovold wrote: > 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. Thanks, I understand now. I see three patterns shared by serial drivers: 1. Do not take the lock in console_write() handler, 2. Take the lock like: if (port->sysrq) locked = 0; else if (oops_in_progress) locked = spin_trylock_irqsave(&port->lock, flags); else spin_lock_irqsave(&port->lock, flags) 3. Take the lock like above but preceded with local_irq_save(). It seems the choice of pattern depends which driver was used as a base. Best regards, Krzysztof
