* [PATCH] serial: atmel: don't enable IRQs prematurely
@ 2023-06-19 9:45 Dan Carpenter
2023-06-19 11:01 ` Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-06-19 9:45 UTC (permalink / raw)
To: Elen Song
Cc: Richard Genoud, Greg Kroah-Hartman, Jiri Slaby, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Ludovic Desroches,
linux-serial, kernel-janitors
The atmel_complete_tx_dma() function disables IRQs at the start
of the function by calling spin_lock_irqsave(&port->lock, flags);
There is no need to disable them a second time using the
spin_lock_irq() function and, in fact, doing so is a bug because
it will enable IRQs prematurely when we call spin_unlock_irq().
Just use spin_lock/unlock() instead without disabling or enabling
IRQs.
Fixes: 08f738be88bb ("serial: at91: add tx dma support")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
drivers/tty/serial/atmel_serial.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 6e9192f122aa..3467a875641a 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -868,11 +868,11 @@ static void atmel_complete_tx_dma(void *arg)
dmaengine_terminate_all(chan);
uart_xmit_advance(port, atmel_port->tx_len);
- spin_lock_irq(&atmel_port->lock_tx);
+ spin_lock(&atmel_port->lock_tx);
async_tx_ack(atmel_port->desc_tx);
atmel_port->cookie_tx = -EINVAL;
atmel_port->desc_tx = NULL;
- spin_unlock_irq(&atmel_port->lock_tx);
+ spin_unlock(&atmel_port->lock_tx);
if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
uart_write_wakeup(port);
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] serial: atmel: don't enable IRQs prematurely
2023-06-19 9:45 [PATCH] serial: atmel: don't enable IRQs prematurely Dan Carpenter
@ 2023-06-19 11:01 ` Jiri Slaby
2023-06-19 11:44 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2023-06-19 11:01 UTC (permalink / raw)
To: Dan Carpenter, Elen Song
Cc: Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Ludovic Desroches,
linux-serial, kernel-janitors
On 19. 06. 23, 11:45, Dan Carpenter wrote:
> The atmel_complete_tx_dma() function disables IRQs at the start
> of the function by calling spin_lock_irqsave(&port->lock, flags);
> There is no need to disable them a second time using the
> spin_lock_irq() function and, in fact, doing so is a bug because
> it will enable IRQs prematurely when we call spin_unlock_irq().
>
> Just use spin_lock/unlock() instead without disabling or enabling
> IRQs.
>
> Fixes: 08f738be88bb ("serial: at91: add tx dma support")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> drivers/tty/serial/atmel_serial.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 6e9192f122aa..3467a875641a 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -868,11 +868,11 @@ static void atmel_complete_tx_dma(void *arg)
> dmaengine_terminate_all(chan);
> uart_xmit_advance(port, atmel_port->tx_len);
>
> - spin_lock_irq(&atmel_port->lock_tx);
> + spin_lock(&atmel_port->lock_tx);
> async_tx_ack(atmel_port->desc_tx);
> atmel_port->cookie_tx = -EINVAL;
> atmel_port->desc_tx = NULL;
> - spin_unlock_irq(&atmel_port->lock_tx);
> + spin_unlock(&atmel_port->lock_tx);
Hmm, can you ensure the DMA engine code calls this with irqs disabled?
If so, you should document it in the commit log. If not, you shyuld use
_irqsave() variant.
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] serial: atmel: don't enable IRQs prematurely
2023-06-19 11:01 ` Jiri Slaby
@ 2023-06-19 11:44 ` Dan Carpenter
2023-06-19 11:47 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-06-19 11:44 UTC (permalink / raw)
To: Jiri Slaby
Cc: Elen Song, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Ludovic Desroches,
linux-serial, kernel-janitors
On Mon, Jun 19, 2023 at 01:01:49PM +0200, Jiri Slaby wrote:
> On 19. 06. 23, 11:45, Dan Carpenter wrote:
> > The atmel_complete_tx_dma() function disables IRQs at the start
> > of the function by calling spin_lock_irqsave(&port->lock, flags);
> > There is no need to disable them a second time using the
> > spin_lock_irq() function and, in fact, doing so is a bug because
> > it will enable IRQs prematurely when we call spin_unlock_irq().
> >
> > Just use spin_lock/unlock() instead without disabling or enabling
> > IRQs.
> >
> > Fixes: 08f738be88bb ("serial: at91: add tx dma support")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > drivers/tty/serial/atmel_serial.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> > index 6e9192f122aa..3467a875641a 100644
> > --- a/drivers/tty/serial/atmel_serial.c
> > +++ b/drivers/tty/serial/atmel_serial.c
> > @@ -868,11 +868,11 @@ static void atmel_complete_tx_dma(void *arg)
> > dmaengine_terminate_all(chan);
> > uart_xmit_advance(port, atmel_port->tx_len);
> > - spin_lock_irq(&atmel_port->lock_tx);
> > + spin_lock(&atmel_port->lock_tx);
> > async_tx_ack(atmel_port->desc_tx);
> > atmel_port->cookie_tx = -EINVAL;
> > atmel_port->desc_tx = NULL;
> > - spin_unlock_irq(&atmel_port->lock_tx);
> > + spin_unlock(&atmel_port->lock_tx);
>
> Hmm, can you ensure the DMA engine code calls this with irqs disabled? If
> so, you should document it in the commit log. If not, you shyuld use
> _irqsave() variant.
>
> thanks,
Hi Jiri,
I feel like we are miscommunicating but I don't know how to improve my
commit message... The function itself disables IRQs at the start using
_irqsave(). I have left that as-is.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] serial: atmel: don't enable IRQs prematurely
2023-06-19 11:44 ` Dan Carpenter
@ 2023-06-19 11:47 ` Dan Carpenter
2023-06-20 4:50 ` Jiri Slaby
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2023-06-19 11:47 UTC (permalink / raw)
To: Jiri Slaby
Cc: Elen Song, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Ludovic Desroches,
linux-serial, kernel-janitors
On Mon, Jun 19, 2023 at 02:44:11PM +0300, Dan Carpenter wrote:
> On Mon, Jun 19, 2023 at 01:01:49PM +0200, Jiri Slaby wrote:
> > On 19. 06. 23, 11:45, Dan Carpenter wrote:
> > > The atmel_complete_tx_dma() function disables IRQs at the start
> > > of the function by calling spin_lock_irqsave(&port->lock, flags);
> > > There is no need to disable them a second time using the
> > > spin_lock_irq() function and, in fact, doing so is a bug because
> > > it will enable IRQs prematurely when we call spin_unlock_irq().
> > >
> > > Just use spin_lock/unlock() instead without disabling or enabling
> > > IRQs.
Maybe I should add a "a second time".
"Just use spin_lock/unlock() instead without disabling or enabling
IRQs a second time."
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: atmel: don't enable IRQs prematurely
2023-06-19 11:47 ` Dan Carpenter
@ 2023-06-20 4:50 ` Jiri Slaby
2023-06-21 8:19 ` Richard Genoud
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2023-06-20 4:50 UTC (permalink / raw)
To: Dan Carpenter
Cc: Elen Song, Richard Genoud, Greg Kroah-Hartman, Nicolas Ferre,
Alexandre Belloni, Claudiu Beznea, Ludovic Desroches,
linux-serial, kernel-janitors
On 19. 06. 23, 13:47, Dan Carpenter wrote:
> On Mon, Jun 19, 2023 at 02:44:11PM +0300, Dan Carpenter wrote:
>> On Mon, Jun 19, 2023 at 01:01:49PM +0200, Jiri Slaby wrote:
>>> On 19. 06. 23, 11:45, Dan Carpenter wrote:
>>>> The atmel_complete_tx_dma() function disables IRQs at the start
>>>> of the function by calling spin_lock_irqsave(&port->lock, flags);
>>>> There is no need to disable them a second time using the
>>>> spin_lock_irq() function and, in fact, doing so is a bug because
>>>> it will enable IRQs prematurely when we call spin_unlock_irq().
>>>>
>>>> Just use spin_lock/unlock() instead without disabling or enabling
>>>> IRQs.
>
> Maybe I should add a "a second time".
>
> "Just use spin_lock/unlock() instead without disabling or enabling
> IRQs a second time."
No, I'm just stupid and I apparently fail to understand written text at
times.
Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
thanks,
--
js
suse labs
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] serial: atmel: don't enable IRQs prematurely
2023-06-20 4:50 ` Jiri Slaby
@ 2023-06-21 8:19 ` Richard Genoud
0 siblings, 0 replies; 6+ messages in thread
From: Richard Genoud @ 2023-06-21 8:19 UTC (permalink / raw)
To: Jiri Slaby, Dan Carpenter
Cc: Elen Song, Greg Kroah-Hartman, Nicolas Ferre, Alexandre Belloni,
Claudiu Beznea, Ludovic Desroches, linux-serial, kernel-janitors
Le 20/06/2023 à 06:50, Jiri Slaby a écrit :
> On 19. 06. 23, 13:47, Dan Carpenter wrote:
>> On Mon, Jun 19, 2023 at 02:44:11PM +0300, Dan Carpenter wrote:
>>> On Mon, Jun 19, 2023 at 01:01:49PM +0200, Jiri Slaby wrote:
>>>> On 19. 06. 23, 11:45, Dan Carpenter wrote:
>>>>> The atmel_complete_tx_dma() function disables IRQs at the start
>>>>> of the function by calling spin_lock_irqsave(&port->lock, flags);
>>>>> There is no need to disable them a second time using the
>>>>> spin_lock_irq() function and, in fact, doing so is a bug because
>>>>> it will enable IRQs prematurely when we call spin_unlock_irq().
>>>>>
>>>>> Just use spin_lock/unlock() instead without disabling or enabling
>>>>> IRQs.
>>
>> Maybe I should add a "a second time".
>>
>> "Just use spin_lock/unlock() instead without disabling or enabling
>> IRQs a second time."
>
> No, I'm just stupid and I apparently fail to understand written text at
> times.
>
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
>
> thanks,
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-06-21 8:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-19 9:45 [PATCH] serial: atmel: don't enable IRQs prematurely Dan Carpenter
2023-06-19 11:01 ` Jiri Slaby
2023-06-19 11:44 ` Dan Carpenter
2023-06-19 11:47 ` Dan Carpenter
2023-06-20 4:50 ` Jiri Slaby
2023-06-21 8:19 ` Richard Genoud
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox