RE: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ

From: Johan Hovold

Sent: 20 November 2020 12:50

On Fri, Nov 20, 2020 at 07:25:03PM +0800, tiantao (H) wrote:

quoted

在 2020/11/20 16:23, Johan Hovold 写道:

quoted

On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote:

quoted

The code has been in a irq-disabled context since it is hard IRQ. There
is no necessity to do it again.

Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
---
  drivers/tty/serial/owl-uart.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/owl-uart.c b/drivers/tty/serial/owl-uart.c
index c149f8c3..472fdaf 100644
--- a/drivers/tty/serial/owl-uart.c
+++ b/drivers/tty/serial/owl-uart.c

@@ -251,10 +251,9 @@ static void owl_uart_receive_chars(struct uart_port *port)
  static irqreturn_t owl_uart_irq(int irq, void *dev_id)
  {
  	struct uart_port *port = dev_id;
-	unsigned long flags;
  	u32 stat;

-	spin_lock_irqsave(&port->lock, flags);
+	spin_lock(&port->lock);

Same thing here; this will break with forced irq threading (i.e.
"threadirqs") since the console code can still end up being called from
interrupt context.

quoted

As the following code shows, owl_uart_irq does not run in the irq
threading context.
  ret = request_irq(port->irq, owl_uart_irq, IRQF_TRIGGER_HIGH,
                         "owl-uart", port);
         if (ret)
                 return ret;

It still runs in a thread when interrupts are forced to be threaded
using the kernel parameter "threadirqs".

We just had a revert of a change like yours after lockdep reported the
resulting lock inversion with forced interrupt threading.

Whether drivers should have to care about "threadirqs" is a somewhat
different question. Not sure how that's even supposed to work generally
unless we mass-convert drivers to spin_lock_irqsave() (or mark their
interrupts IRQF_NO_THREAD).

Isn't that backwards?

You need to use the 'irqsave' variant in code that might run with
interrupts enabled because an interrupt might try to acquire the
same lock having interrupted the code that already holds the lock.

If interrupts run as separate threads that can never happen.
So in that case all code can use the non-irqsave call.

So either lockdep is broken or you have a different bug.

OTOH the additional cost of using the 'irqsave' form inside
an ISR is probably minimal.
So while not necessary it does no harm and saves the reader
having to worry about whether a particular function might
be called from an ISR.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)