| Message ID | 20220822141110.17199-1-91tuocao@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler | expand |
On Mon, Aug 22, 2022 at 10:11:10PM +0800, Tuo Cao wrote:
> it is unnecessary to call spin_lock_irqsave in a interrupt handler.
Yes, but it is safer to do so, right?
Why is this change needed?
Did you test it on real hardware to verify it works?
We need a lot more information in the changelog text before being able
to accept this.
thanks,
greg k-h
No, whether it's spin_lock_irqsave() or spin_lock(), the security is the same. Since this commit:e58aa3d2d0cc01ad8d6f7f640a0670433f794922, interrupt nesting is disabled, which means interrupts has disabled in the interrupt handlers. So, it is unnecessary to call spin_lock_irqsave in a interrupt handler. And it takes less time obviously to use spin_lock()，so I think this change is needed. Finally, I'm sorry I lacked real hardware to verify it and can't provide changelog text. Thanks. Greg KH <gregkh@linuxfoundation.org> 于2022年8月22日周一 22:25写道： > > On Mon, Aug 22, 2022 at 10:11:10PM +0800, Tuo Cao wrote: > > it is unnecessary to call spin_lock_irqsave in a interrupt handler. > > Yes, but it is safer to do so, right? > > Why is this change needed? > > Did you test it on real hardware to verify it works? > > We need a lot more information in the changelog text before being able > to accept this. > > thanks, > > greg k-h
On Sat, Aug 27, 2022 at 05:42:19PM +0800, tuo cao wrote: > No, whether it's spin_lock_irqsave() or spin_lock(), the security is > the same. Since this commit:e58aa3d2d0cc01ad8d6f7f640a0670433f794922, > interrupt nesting is disabled, which means interrupts has disabled in > the interrupt handlers. So, it is unnecessary to call > spin_lock_irqsave in a interrupt handler. And it takes less time > obviously to use spin_lock()，so I think this change is needed. I have no context at all here, please never top-post :( And have you measured the time difference? Is it a real thing? > Finally, I'm sorry I lacked real hardware to verify it and can't > provide changelog text. Try to never do changes for drivers for functionality like this where you do not have the hardware to test for, until you get a lot more experience. good luck! greg k-h
Greg KH <gregkh@linuxfoundation.org> 于2022年8月30日周二 19:35写道： > > On Sat, Aug 27, 2022 at 05:42:19PM +0800, tuo cao wrote: > > No, whether it's spin_lock_irqsave() or spin_lock(), the security is > > the same. Since this commit:e58aa3d2d0cc01ad8d6f7f640a0670433f794922, > > interrupt nesting is disabled, which means interrupts has disabled in > > the interrupt handlers. So, it is unnecessary to call > > spin_lock_irqsave in a interrupt handler. And it takes less time > > obviously to use spin_lock()，so I think this change is needed. > > I have no context at all here, please never top-post :( > Sorry for causing you trouble. It should be OK this time. > And have you measured the time difference? Is it a real thing? > Yes, sir. I have measured it, it is a read thing. The test code and log have been put on Github, please check: https://github.com/tuocao1991/api_test > > Finally, I'm sorry I lacked real hardware to verify it and can't > > provide changelog text. > > Try to never do changes for drivers for functionality like this where > you do not have the hardware to test for, until you get a lot more > experience. > I got it, thanks > good luck! > > greg k-h Best Regards!
On Sun, Sep 04, 2022 at 04:48:13PM +0800, tuo cao wrote: > Greg KH <gregkh@linuxfoundation.org> 于2022年8月30日周二 19:35写道： > > > > On Sat, Aug 27, 2022 at 05:42:19PM +0800, tuo cao wrote: > > > No, whether it's spin_lock_irqsave() or spin_lock(), the security is > > > the same. Since this commit:e58aa3d2d0cc01ad8d6f7f640a0670433f794922, > > > interrupt nesting is disabled, which means interrupts has disabled in > > > the interrupt handlers. So, it is unnecessary to call > > > spin_lock_irqsave in a interrupt handler. And it takes less time > > > obviously to use spin_lock()，so I think this change is needed. > > > > I have no context at all here, please never top-post :( > > > Sorry for causing you trouble. It should be OK this time. > > > And have you measured the time difference? Is it a real thing? > > > Yes, sir. I have measured it, it is a read thing. The test code and > log have been put on Github, please check: > https://github.com/tuocao1991/api_test Did you test it for this code change? And remember, those calls are being made inside of an IRQ handler, did you measure that time difference? And that link does not show much, sorry, you are doing no real work at all, and again, not operating in an irq handler. Can you see a measurable difference with your patch applied and without it? If so, great, provide that informatin in the changelog text. If not, be very careful about changing code in stuff like this. thanks, greg k-h
diff --git a/drivers/tty/serial/8250/8250_bcm7271.c b/drivers/tty/serial/8250/8250_bcm7271.c index 8efdc271eb75..fb525c435723 100644 --- a/drivers/tty/serial/8250/8250_bcm7271.c +++ b/drivers/tty/serial/8250/8250_bcm7271.c @@ -560,7 +560,6 @@ static irqreturn_t brcmuart_isr(int irq, void *dev_id) struct uart_port *up = dev_id; struct device *dev = up->dev; struct brcmuart_priv *priv = up->private_data; - unsigned long flags; u32 interrupts; u32 rval; u32 tval; @@ -569,7 +568,7 @@ static irqreturn_t brcmuart_isr(int irq, void *dev_id) if (interrupts == 0) return IRQ_NONE; - spin_lock_irqsave(&up->lock, flags); + spin_lock(&up->lock); /* Clear all interrupts */ udma_writel(priv, REGS_DMA_ISR, UDMA_INTR_CLEAR, interrupts); @@ -583,7 +582,7 @@ static irqreturn_t brcmuart_isr(int irq, void *dev_id) if ((rval | tval) == 0) dev_warn(dev, "Spurious interrupt: 0x%x\n", interrupts); - spin_unlock_irqrestore(&up->lock, flags); + spin_unlock(&up->lock); return IRQ_HANDLED; }
it is unnecessary to call spin_lock_irqsave in a interrupt handler. Signed-off-by: Tuo Cao <91tuocao@gmail.com> --- drivers/tty/serial/8250/8250_bcm7271.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)