* [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ @ 2020-11-19 9:01 Tian Tao 2020-11-19 9:03 ` Jiri Slaby 2020-11-20 8:23 ` Johan Hovold 0 siblings, 2 replies; 7+ messages in thread From: Tian Tao @ 2020-11-19 9:01 UTC (permalink / raw) To: gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel 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); stat = owl_uart_read(port, OWL_UART_STAT); @@ -268,7 +267,7 @@ static irqreturn_t owl_uart_irq(int irq, void *dev_id) stat |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP; owl_uart_write(port, stat, OWL_UART_STAT); - spin_unlock_irqrestore(&port->lock, flags); + spin_unlock(&port->lock); return IRQ_HANDLED; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-19 9:01 [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ Tian Tao @ 2020-11-19 9:03 ` Jiri Slaby 2020-11-20 8:23 ` Johan Hovold 1 sibling, 0 replies; 7+ messages in thread From: Jiri Slaby @ 2020-11-19 9:03 UTC (permalink / raw) To: Tian Tao, gregkh, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel On 19. 11. 20, 10:01, Tian Tao wrote: > 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> Reviewed-by: Jiri Slaby <jirislaby@kernel.org> > --- > 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); > > stat = owl_uart_read(port, OWL_UART_STAT); > > @@ -268,7 +267,7 @@ static irqreturn_t owl_uart_irq(int irq, void *dev_id) > stat |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP; > owl_uart_write(port, stat, OWL_UART_STAT); > > - spin_unlock_irqrestore(&port->lock, flags); > + spin_unlock(&port->lock); > > return IRQ_HANDLED; > } > -- js suse labs ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-19 9:01 [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ Tian Tao 2020-11-19 9:03 ` Jiri Slaby @ 2020-11-20 8:23 ` Johan Hovold 2020-11-20 11:25 ` tiantao (H) 1 sibling, 1 reply; 7+ messages in thread From: Johan Hovold @ 2020-11-20 8:23 UTC (permalink / raw) To: Tian Tao Cc: gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote: > 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. > stat = owl_uart_read(port, OWL_UART_STAT); > > @@ -268,7 +267,7 @@ static irqreturn_t owl_uart_irq(int irq, void *dev_id) > stat |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP; > owl_uart_write(port, stat, OWL_UART_STAT); > > - spin_unlock_irqrestore(&port->lock, flags); > + spin_unlock(&port->lock); > > return IRQ_HANDLED; > } Johan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-20 8:23 ` Johan Hovold @ 2020-11-20 11:25 ` tiantao (H) 2020-11-20 12:49 ` Johan Hovold 0 siblings, 1 reply; 7+ messages in thread From: tiantao (H) @ 2020-11-20 11:25 UTC (permalink / raw) To: Johan Hovold, Tian Tao Cc: gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel 在 2020/11/20 16:23, Johan Hovold 写道: > On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote: >> 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. 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; > >> stat = owl_uart_read(port, OWL_UART_STAT); >> >> @@ -268,7 +267,7 @@ static irqreturn_t owl_uart_irq(int irq, void *dev_id) >> stat |= OWL_UART_STAT_RIP | OWL_UART_STAT_TIP; >> owl_uart_write(port, stat, OWL_UART_STAT); >> >> - spin_unlock_irqrestore(&port->lock, flags); >> + spin_unlock(&port->lock); >> >> return IRQ_HANDLED; >> } > > Johan > > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-20 11:25 ` tiantao (H) @ 2020-11-20 12:49 ` Johan Hovold 2020-11-20 20:00 ` David Laight 0 siblings, 1 reply; 7+ messages in thread From: Johan Hovold @ 2020-11-20 12:49 UTC (permalink / raw) To: tiantao (H), Thomas Gleixner Cc: Johan Hovold, Tian Tao, gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel On Fri, Nov 20, 2020 at 07:25:03PM +0800, tiantao (H) wrote: > 在 2020/11/20 16:23, Johan Hovold 写道: > > On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote: > >> 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. > 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). Thomas, any comments? Johan ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-20 12:49 ` Johan Hovold @ 2020-11-20 20:00 ` David Laight 2020-11-21 15:17 ` Johan Hovold 0 siblings, 1 reply; 7+ messages in thread From: David Laight @ 2020-11-20 20:00 UTC (permalink / raw) To: 'Johan Hovold', tiantao (H), Thomas Gleixner Cc: Tian Tao, gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel From: Johan Hovold > Sent: 20 November 2020 12:50 > > On Fri, Nov 20, 2020 at 07:25:03PM +0800, tiantao (H) wrote: > > 在 2020/11/20 16:23, Johan Hovold 写道: > > > On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote: > > >> 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. > > > 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ 2020-11-20 20:00 ` David Laight @ 2020-11-21 15:17 ` Johan Hovold 0 siblings, 0 replies; 7+ messages in thread From: Johan Hovold @ 2020-11-21 15:17 UTC (permalink / raw) To: David Laight Cc: 'Johan Hovold', tiantao (H), Thomas Gleixner, Tian Tao, gregkh, jirislaby, afaerber, manivannan.sadhasivam, linux-serial, linux-arm-kernel, linux-kernel On Fri, Nov 20, 2020 at 08:00:05PM +0000, David Laight wrote: > From: Johan Hovold > > Sent: 20 November 2020 12:50 > > > > On Fri, Nov 20, 2020 at 07:25:03PM +0800, tiantao (H) wrote: > > > 在 2020/11/20 16:23, Johan Hovold 写道: > > > > On Thu, Nov 19, 2020 at 05:01:29PM +0800, Tian Tao wrote: > > > >> 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. > > > > > 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. Not all interrupts run as threads with "threadirqs" so the lock can potentially still be taken in hard IRQ context also with forced threading. 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). If a driver exposes an interface that can be called in hard IRQ context, it must use spin_lock_irqsave() in its interrupt handler (or use IRQF_NO_THREAD) because of "threadirqs". Johan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-11-21 15:17 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-19 9:01 [PATCH] tty: serial: replace spin_lock_irqsave by spin_lock in hard IRQ Tian Tao 2020-11-19 9:03 ` Jiri Slaby 2020-11-20 8:23 ` Johan Hovold 2020-11-20 11:25 ` tiantao (H) 2020-11-20 12:49 ` Johan Hovold 2020-11-20 20:00 ` David Laight 2020-11-21 15:17 ` Johan Hovold
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).