[PATCH] serial: atmel: don't enable IRQs prematurely

[PATCH] serial: atmel: don't enable IRQs prematurely

[Dan Carpenter]

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

Re: [PATCH] serial: atmel: don't enable IRQs prematurely

[Jiri Slaby]

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

Re: [PATCH] serial: atmel: don't enable IRQs prematurely

[Dan Carpenter]

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

Re: [PATCH] serial: atmel: don't enable IRQs prematurely

[Dan Carpenter]

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

Re: [PATCH] serial: atmel: don't enable IRQs prematurely

[Jiri Slaby]

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

Re: [PATCH] serial: atmel: don't enable IRQs prematurely

[Richard Genoud]

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 !