[RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[Tuo Cao]

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(-)

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;
 }
 
-- 
2.17.1

Re: [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[Greg KH]

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

Re: [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[tuo cao]

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

Re: [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[Greg KH]

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

Re: [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[tuo cao]

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!

Re: [RESEND] serial: 8250_bcm7271: move spin_lock_irqsave to spin_lock in interrupt handler

[Greg KH]

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