On Tue, Mar 16, 2021 at 11:11:43AM +0100, Krzysztof Kozlowski wrote:

On 16/03/2021 10:56, Johan Hovold wrote:

quoted

On Tue, Mar 16, 2021 at 10:47:53AM +0100, Krzysztof Kozlowski wrote:

quoted

On 16/03/2021 10:02, Johan Hovold wrote:

quoted

On Mon, Mar 15, 2021 at 07:12:12PM +0100, Krzysztof Kozlowski wrote:

quoted

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 (local).

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.

Right, this is messy and we've been playing whack-a-mole with this for
years (as usual) it seems.

Some version of 2 above is probably what we want but the sysrq bits
aren't handled uniformly either (e.g. since 596f63da42b9 ("serial: 8250:
Process sysrq at port unlock time")).

Johan