Since interrupt handler is called with disabled local interrupts, there
is no need to use the spinlock primitives disabling interrupts as well.
Suggested-by: Andy Shevchenko <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/tty/serial/samsung_tty.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index 80df842bf4c7..d9e4b67a12a0 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -715,13 +715,12 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
struct s3c24xx_uart_dma *dma = ourport->dma;
struct tty_struct *tty = tty_port_tty_get(&ourport->port.state->port);
struct tty_port *t = &port->state->port;
- unsigned long flags;
struct dma_tx_state state;
utrstat = rd_regl(port, S3C2410_UTRSTAT);
rd_regl(port, S3C2410_UFSTAT);
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock(&port->lock);
if (!(utrstat & S3C2410_UTRSTAT_TIMEOUT)) {
s3c64xx_start_rx_dma(ourport);
@@ -750,7 +749,7 @@ static irqreturn_t s3c24xx_serial_rx_chars_dma(void *dev_id)
wr_regl(port, S3C2410_UTRSTAT, S3C2410_UTRSTAT_TIMEOUT);
finish:
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock(&port->lock);
return IRQ_HANDLED;
}
@@ -846,11 +845,10 @@ static irqreturn_t s3c24xx_serial_rx_chars_pio(void *dev_id)
{
struct s3c24xx_uart_port *ourport = dev_id;
struct uart_port *port = &ourport->port;
- unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock(&port->lock);
s3c24xx_serial_rx_drain_fifo(ourport);
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock(&port->lock);
return IRQ_HANDLED;
}
@@ -934,13 +932,12 @@ static irqreturn_t s3c24xx_serial_tx_irq(int irq, void *id)
{
struct s3c24xx_uart_port *ourport = id;
struct uart_port *port = &ourport->port;
- unsigned long flags;
- spin_lock_irqsave(&port->lock, flags);
+ spin_lock(&port->lock);
s3c24xx_serial_tx_chars(ourport);
- spin_unlock_irqrestore(&port->lock, flags);
+ spin_unlock(&port->lock);
return IRQ_HANDLED;
}
--
2.25.1
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?
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
- s3c24xx_serial_tx_irq
- spin_lock()
Best regards,
Krzysztof
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
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.
Johan
On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <[email protected]> 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.
Finn, Barry, something to check I think?
--
With Best Regards,
Andy Shevchenko
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
On Tue, Mar 16, 2021 at 11:11:43AM +0100, Krzysztof Kozlowski wrote:
> 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.
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
> -----Original Message-----
> From: Andy Shevchenko [mailto:[email protected]]
> Sent: Tuesday, March 16, 2021 10:41 PM
> To: Johan Hovold <[email protected]>; Finn Thain <[email protected]>;
> Song Bao Hua (Barry Song) <[email protected]>
> Cc: Krzysztof Kozlowski <[email protected]>; Greg
> Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>;
> linux-arm Mailing List <[email protected]>; Linux Samsung
> SOC <[email protected]>; open list:SERIAL DRIVERS
> <[email protected]>; Linux Kernel Mailing List
> <[email protected]>; Hector Martin <[email protected]>; Arnd
> Bergmann <[email protected]>
> Subject: Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in
> interrupt handlers
>
> On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <[email protected]> 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.
>
> Finn, Barry, something to check I think?
My understanding is that spin_lock_irqsave can't protect the context
the console_write() is called in hardirq for threaded_irq case mainly
for preempt-rt scenarios as spin_lock_irqsave doesn't disable irq in
that case at all.
See:
https://www.kernel.org/doc/html/latest/locking/locktypes.html
spinlock_t and PREEMPT_RT
On a PREEMPT_RT kernel spinlock_t is mapped to a separate implementation
based on rt_mutex which changes the semantics:
Preemption is not disabled.
The hard interrupt related suffixes for spin_lock / spin_unlock operations
(_irq, _irqsave / _irqrestore) do not affect the CPU’s interrupt disabled
state.
So if console_write() can interrupt our code in hardirq, we should
move to raw_spin_lock_irqsave for this driver.
I think it is almost always wrong to call spin_lock_irqsave in hardirq.
>
> --
> With Best Regards,
> Andy Shevchenko
Thanks
Barry
On Fri, Mar 19, 2021 at 06:36:39AM +0000, Song Bao Hua (Barry Song) wrote:
>
>
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:[email protected]]
> > Sent: Tuesday, March 16, 2021 10:41 PM
> > To: Johan Hovold <[email protected]>; Finn Thain <[email protected]>;
> > Song Bao Hua (Barry Song) <[email protected]>
> > Cc: Krzysztof Kozlowski <[email protected]>; Greg
> > Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>;
> > linux-arm Mailing List <[email protected]>; Linux Samsung
> > SOC <[email protected]>; open list:SERIAL DRIVERS
> > <[email protected]>; Linux Kernel Mailing List
> > <[email protected]>; Hector Martin <[email protected]>; Arnd
> > Bergmann <[email protected]>
> > Subject: Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in
> > interrupt handlers
> >
> > On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <[email protected]> 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.
> >
> > Finn, Barry, something to check I think?
>
> My understanding is that spin_lock_irqsave can't protect the context
> the console_write() is called in hardirq for threaded_irq case mainly
> for preempt-rt scenarios as spin_lock_irqsave doesn't disable irq in
> that case at all.
Forced threaded interrupts have so far run with interrupts enabled and
spin_lock_irqsave() would suffice on non-RT. This is about to change
though so that drivers don't need to worry about "threadirqs":
https://lore.kernel.org/r/[email protected]
> See:
> https://www.kernel.org/doc/html/latest/locking/locktypes.html
> spinlock_t and PREEMPT_RT
> On a PREEMPT_RT kernel spinlock_t is mapped to a separate implementation
> based on rt_mutex which changes the semantics:
> Preemption is not disabled.
> The hard interrupt related suffixes for spin_lock / spin_unlock operations
> (_irq, _irqsave / _irqrestore) do not affect the CPU’s interrupt disabled
> state.
>
> So if console_write() can interrupt our code in hardirq, we should
> move to raw_spin_lock_irqsave for this driver.
No, no. RT handles this by deferring console writes apparently.
> I think it is almost always wrong to call spin_lock_irqsave in hardirq.
Again, no. It's even been a requirement due to "threadirqs" in some
cases (e.g. hrtimers) up until now (or rather until the above patch is
in mainline).
Johan
On Fri, Mar 19, 2021 at 10:09 AM Johan Hovold <[email protected]> wrote:
>
> On Fri, Mar 19, 2021 at 06:36:39AM +0000, Song Bao Hua (Barry Song) wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:[email protected]]
> > > Sent: Tuesday, March 16, 2021 10:41 PM
> > > To: Johan Hovold <[email protected]>; Finn Thain <[email protected]>;
> > > Song Bao Hua (Barry Song) <[email protected]>
> > > Cc: Krzysztof Kozlowski <[email protected]>; Greg
> > > Kroah-Hartman <[email protected]>; Jiri Slaby <[email protected]>;
> > > linux-arm Mailing List <[email protected]>; Linux Samsung
> > > SOC <[email protected]>; open list:SERIAL DRIVERS
> > > <[email protected]>; Linux Kernel Mailing List
> > > <[email protected]>; Hector Martin <[email protected]>; Arnd
> > > Bergmann <[email protected]>
> > > Subject: Re: [PATCH] tty: serial: samsung_tty: remove spinlock flags in
> > > interrupt handlers
> > >
> > > On Tue, Mar 16, 2021 at 11:02 AM Johan Hovold <[email protected]> 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.
> > >
> > > Finn, Barry, something to check I think?
> >
> > My understanding is that spin_lock_irqsave can't protect the context
> > the console_write() is called in hardirq for threaded_irq case mainly
> > for preempt-rt scenarios as spin_lock_irqsave doesn't disable irq in
> > that case at all.
>
> Forced threaded interrupts have so far run with interrupts enabled and
> spin_lock_irqsave() would suffice on non-RT. This is about to change
> though so that drivers don't need to worry about "threadirqs":
>
> https://lore.kernel.org/r/[email protected]
>
> > See:
> > https://www.kernel.org/doc/html/latest/locking/locktypes.html
> > spinlock_t and PREEMPT_RT
> > On a PREEMPT_RT kernel spinlock_t is mapped to a separate implementation
> > based on rt_mutex which changes the semantics:
> > Preemption is not disabled.
> > The hard interrupt related suffixes for spin_lock / spin_unlock operations
> > (_irq, _irqsave / _irqrestore) do not affect the CPU’s interrupt disabled
> > state.
> >
> > So if console_write() can interrupt our code in hardirq, we should
> > move to raw_spin_lock_irqsave for this driver.
>
> No, no. RT handles this by deferring console writes apparently.
>
> > I think it is almost always wrong to call spin_lock_irqsave in hardirq.
>
> Again, no. It's even been a requirement due to "threadirqs" in some
> cases (e.g. hrtimers) up until now (or rather until the above patch is
> in mainline).
By the way, a good question Imre (Cc'ed) and I have discussed is the
in-kernel documentation, i.e.
https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html.
Should it be adjusted to reality?
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 19, 2021 at 12:09:34PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 19, 2021 at 10:09 AM Johan Hovold <[email protected]> wrote:
> > > I think it is almost always wrong to call spin_lock_irqsave in
> > > hardirq.
> >
> > Again, no. It's even been a requirement due to "threadirqs" in some
> > cases (e.g. hrtimers) up until now (or rather until the above patch is
> > in mainline).
>
> By the way, a good question Imre (Cc'ed) and I have discussed is the
> in-kernel documentation, i.e.
> https://www.kernel.org/doc/html/latest/kernel-hacking/locking.html.
> Should it be adjusted to reality?
Once forced threading disables interrupts (as it should have all along)
we don't need to worry about this anymore. But yeah, otherwise it should
be documented.
Johan
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.
>
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
Interrupts are now disabled also with forced interrupt threading even if
this never was an issue for this driver which currently doesn't take the
port lock in the console paths.
Reviewed-by: Johan Hovold <[email protected]>
Johan