diff mbox series

[v2] serial/pmac_zilog: Remove flawed mitigation for rx irq flood

Message ID 0df45bedded1249f6c6ec2c2fb0d9879da1841b7.1712273040.git.fthain@linux-m68k.org
State Superseded
Headers show
Series [v2] serial/pmac_zilog: Remove flawed mitigation for rx irq flood | expand

Commit Message

Finn Thain April 4, 2024, 11:24 p.m. UTC
The mitigation was intended to stop the irq completely. That may be
better than a hard lock-up but it turns out that you get a crash anyway
if you're using pmac_zilog as a serial console:

ttyPZ0: pmz: rx irq flood !
BUG: spinlock recursion on CPU#0, swapper/0

That's because the pr_err() call in pmz_receive_chars() results in
pmz_console_write() attempting to lock a spinlock already locked in
pmz_interrupt(). With CONFIG_DEBUG_SPINLOCK=y, this produces a fatal
BUG splat. The spinlock in question is the one in struct uart_port.

Even when it's not fatal, the serial port rx function ceases to work.
Also, the iteration limit doesn't play nicely with QEMU, as can be
seen in the bug report linked below.

A web search for other reports of the error message "pmz: rx irq flood"
didn't produce anything. So I don't think this code is needed any more.
Remove it.

Link: https://github.com/vivier/qemu-m68k/issues/44
Link: https://lore.kernel.org/all/1078874617.9746.36.camel@gaston/
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
Changed since v1:
 - Reworked commit log according to comments from Andy Shevchenko.
---
 drivers/tty/serial/pmac_zilog.c | 14 --------------
 1 file changed, 14 deletions(-)

Comments

Michael Ellerman April 5, 2024, 3:10 a.m. UTC | #1
Finn Thain <fthain@linux-m68k.org> writes:
> The mitigation was intended to stop the irq completely. That may be
> better than a hard lock-up but it turns out that you get a crash anyway
> if you're using pmac_zilog as a serial console:
>
> ttyPZ0: pmz: rx irq flood !
> BUG: spinlock recursion on CPU#0, swapper/0
>
> That's because the pr_err() call in pmz_receive_chars() results in
> pmz_console_write() attempting to lock a spinlock already locked in
> pmz_interrupt(). With CONFIG_DEBUG_SPINLOCK=y, this produces a fatal
> BUG splat. The spinlock in question is the one in struct uart_port.
>
> Even when it's not fatal, the serial port rx function ceases to work.
> Also, the iteration limit doesn't play nicely with QEMU, as can be
> seen in the bug report linked below.
>
> A web search for other reports of the error message "pmz: rx irq flood"
> didn't produce anything. So I don't think this code is needed any more.
> Remove it.

Yeah I think you're probably right.

I assume you have tested this on an actual pmac, as well as qemu?

cheers
Finn Thain April 6, 2024, 3:21 a.m. UTC | #2
On Fri, 5 Apr 2024, Michael Ellerman wrote:

> I assume you have tested this on an actual pmac, as well as qemu?
> 

I tested the patched driver and its console functionality using Zilog SCC 
hardware in a Mac IIci, as well as QEMU's q800 virtual machine.

That should suffice from a code coverage point-of-view, since 
pmz_receive_chars() is portable and independent of CONFIG_PPC_PMAC.

Moreover, I don't know how to get my PowerMac G3 to execute the kludge 
that's to be removed here. I can't prove it's impossible, though.
Michael Ellerman April 8, 2024, 5:29 a.m. UTC | #3
Finn Thain <fthain@linux-m68k.org> writes:
> On Fri, 5 Apr 2024, Michael Ellerman wrote:
>
>> I assume you have tested this on an actual pmac, as well as qemu?
>> 
>
> I tested the patched driver and its console functionality using Zilog SCC 
> hardware in a Mac IIci, as well as QEMU's q800 virtual machine.
>
> That should suffice from a code coverage point-of-view, since 
> pmz_receive_chars() is portable and independent of CONFIG_PPC_PMAC.
>
> Moreover, I don't know how to get my PowerMac G3 to execute the kludge 
> that's to be removed here. I can't prove it's impossible, though.

Thanks. That's good enough for me.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers
diff mbox series

Patch

diff --git a/drivers/tty/serial/pmac_zilog.c b/drivers/tty/serial/pmac_zilog.c
index c8bf08c19c64..77691fbbf779 100644
--- a/drivers/tty/serial/pmac_zilog.c
+++ b/drivers/tty/serial/pmac_zilog.c
@@ -210,7 +210,6 @@  static bool pmz_receive_chars(struct uart_pmac_port *uap)
 {
 	struct tty_port *port;
 	unsigned char ch, r1, drop, flag;
-	int loops = 0;
 
 	/* Sanity check, make sure the old bug is no longer happening */
 	if (uap->port.state == NULL) {
@@ -291,24 +290,11 @@  static bool pmz_receive_chars(struct uart_pmac_port *uap)
 		if (r1 & Rx_OVR)
 			tty_insert_flip_char(port, 0, TTY_OVERRUN);
 	next_char:
-		/* We can get stuck in an infinite loop getting char 0 when the
-		 * line is in a wrong HW state, we break that here.
-		 * When that happens, I disable the receive side of the driver.
-		 * Note that what I've been experiencing is a real irq loop where
-		 * I'm getting flooded regardless of the actual port speed.
-		 * Something strange is going on with the HW
-		 */
-		if ((++loops) > 1000)
-			goto flood;
 		ch = read_zsreg(uap, R0);
 		if (!(ch & Rx_CH_AV))
 			break;
 	}
 
-	return true;
- flood:
-	pmz_interrupt_control(uap, 0);
-	pmz_error("pmz: rx irq flood !\n");
 	return true;
 }