| Message ID | cb7c39a9-c004-4673-92e1-be4e34b85368@moroto.mountain |
|---|---|
| State | New |
| Headers | show |
| Series | serial: atmel: don't enable IRQs prematurely | expand |
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
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,
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 !
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);
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(-)