[PATCH] pmac_zilog.c: Fix break handling, add sysrq support

[PATCH] pmac_zilog.c: Fix break handling, add sysrq support

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 872 bytes --]

Hi!

This patch fixes the BREAK handling in pmac_zilog.c, and also adds
support for SYSRQ via BREAK.

Please note that I had to remove locking in pmz_consolw_write(), since
it is called from handle_sysrq(), which is in turn called from
pmz_receive_chars(), which is called from with the lock held...

while trying to find out how other drivers handle this problem, I found
out that sn_sal_console_write() has some elaborate code that tries to
steal the lock, etc.

Either something like this is added, or the locking in pmac_zilog()
needs to be reworked.  But that's of course the maintainers' job ;)

Please consider kernel inclusion,

--
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

[-- Attachment #2: pmac_zilog.patch --]
[-- Type: text/plain, Size: 2833 bytes --]

--- linux-2.6.8-rc3-plain/drivers/serial/pmac_zilog.c	2004-08-05 14:48:36.000000000 +0200
+++ linux-2.6.8-rc3-hw1/drivers/serial/pmac_zilog.c	2004-08-06 14:37:17.003886152 +0200
@@ -29,6 +29,10 @@
  * along with this program; if not, write to the Free Software
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  *
+ * 2004-08-06 Harald Welte <laforge@gnumonks.org>
+ *	- Enable BREAK interrupt
+ *	- Add support for sysreq
+ *
  * TODO:   - Add DMA support
  *         - Defer port shutdown to a few seconds after close
  *         - maybe put something right into uap->clk_divisor
@@ -53,6 +57,7 @@
 #include <linux/slab.h>
 #include <linux/adb.h>
 #include <linux/pmu.h>
+#include <linux/sysrq.h>
 #include <asm/sections.h>
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -64,6 +69,10 @@
 #include <asm/macio.h>
 #include <asm/semaphore.h>

+#if defined (CONFIG_SERIAL_PMACZILOG_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
+#define SUPPORT_SYSRQ
+#endif
+
 #include <linux/serial.h>
 #include <linux/serial_core.h>

@@ -259,8 +268,10 @@
 		}

 		ch &= uap->parity_mask;
-		if (ch == 0 && uap->prev_status & BRK_ABRT)
+		if (ch == 0 && uap->flags & PMACZILOG_FLAG_BREAK) {
+			uap->flags &= ~PMACZILOG_FLAG_BREAK;
 			r1 |= BRK_ABRT;
+		}

 		/* A real serial line, record the character and status.  */
 		if (drop)
@@ -273,6 +284,7 @@
 		if (r1 & (PAR_ERR | Rx_OVR | CRC_ERR | BRK_ABRT)) {
 			error = 1;
 			if (r1 & BRK_ABRT) {
+				printk(KERN_DEBUG "pmz:got break !\n");
 				pmz_debug("pmz: got break !\n");
 				r1 &= ~(PAR_ERR | CRC_ERR);
 				uap->port.icount.brk++;
@@ -364,6 +376,9 @@
 		wake_up_interruptible(&uap->port.info->delta_msr_wait);
 	}

+	if (status & BRK_ABRT)
+		uap->flags |= PMACZILOG_FLAG_BREAK;
+
 	uap->prev_status = status;
 }

@@ -872,8 +887,8 @@
 	uap->curregs[R13] = 0;
 	uap->curregs[R14] = BRENAB;

-	/* Clear handshaking */
-	uap->curregs[R15] = 0;
+	/* Clear handshaking, enable BREAK interrupts */
+	uap->curregs[R15] = BRKIE;

 	/* Master interrupt enable */
 	uap->curregs[R9] |= NV | MIE;
@@ -1919,10 +1934,11 @@
 static void pmz_console_write(struct console *con, const char *s, unsigned int count)
 {
 	struct uart_pmac_port *uap = &pmz_ports[con->index];
-	unsigned long flags;
 	int i;

-	spin_lock_irqsave(&uap->port.lock, flags);
+	/* Don't do any locking here, since we might get called from within
+	 * sysreq() handling and already hold a lock.  Ideally we would need
+	 * something like the 'steal lock' code in sn_sal_console_write -HW */

 	/* Turn of interrupts and enable the transmitter. */
 	write_zsreg(uap, R1, uap->curregs[1] & ~TxINT_ENAB);
@@ -1943,8 +1959,6 @@
 	/* Restore the values in the registers. */
 	write_zsreg(uap, R1, uap->curregs[1]);
 	/* Don't disable the transmitter. */
-
-	spin_unlock_irqrestore(&uap->port.lock, flags);
 }

 /*

Re: [PATCH] pmac_zilog.c: Fix break handling, add sysrq support

[Benjamin Herrenschmidt]

On Fri, 2004-08-06 at 20:43, Harald Welte wrote:
> Hi!
>
> This patch fixes the BREAK handling in pmac_zilog.c, and also adds
> support for SYSRQ via BREAK.
>
> Please note that I had to remove locking in pmz_consolw_write(), since
> it is called from handle_sysrq(), which is in turn called from
> pmz_receive_chars(), which is called from with the lock held...
>
> while trying to find out how other drivers handle this problem, I found
> out that sn_sal_console_write() has some elaborate code that tries to
> steal the lock, etc.

I noticed some locking bugs in other drivers too... it may be better to
drop the lock when calling handle_sysrq, no ?

> Either something like this is added, or the locking in pmac_zilog()
> needs to be reworked.  But that's of course the maintainers' job ;)
>
> Please consider kernel inclusion,

I'll have a look, thanks much.

Ben.

> 	Harald
>
> --- linux-2.6.8-rc3-plain/drivers/serial/pmac_zilog.c	2004-08-05 14:48:36.000000000 +0200
> +++ linux-2.6.8-rc3-hw1/drivers/serial/pmac_zilog.c	2004-08-06 14:37:17.003886152 +0200
> @@ -29,6 +29,10 @@
>   * along with this program; if not, write to the Free Software
>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>   *
> + * 2004-08-06 Harald Welte <laforge@gnumonks.org>
> + *	- Enable BREAK interrupt
> + *	- Add support for sysreq
> + *
>   * TODO:   - Add DMA support
>   *         - Defer port shutdown to a few seconds after close
>   *         - maybe put something right into uap->clk_divisor
> @@ -53,6 +57,7 @@
>  #include <linux/slab.h>
>  #include <linux/adb.h>
>  #include <linux/pmu.h>
> +#include <linux/sysrq.h>
>  #include <asm/sections.h>
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -64,6 +69,10 @@
>  #include <asm/macio.h>
>  #include <asm/semaphore.h>
>
> +#if defined (CONFIG_SERIAL_PMACZILOG_CONSOLE) && defined(CONFIG_MAGIC_SYSRQ)
> +#define SUPPORT_SYSRQ
> +#endif
> +
>  #include <linux/serial.h>
>  #include <linux/serial_core.h>
>
> @@ -259,8 +268,10 @@
>  		}
>
>  		ch &= uap->parity_mask;
> -		if (ch == 0 && uap->prev_status & BRK_ABRT)
> +		if (ch == 0 && uap->flags & PMACZILOG_FLAG_BREAK) {
> +			uap->flags &= ~PMACZILOG_FLAG_BREAK;
>  			r1 |= BRK_ABRT;
> +		}
>
>  		/* A real serial line, record the character and status.  */
>  		if (drop)
> @@ -273,6 +284,7 @@
>  		if (r1 & (PAR_ERR | Rx_OVR | CRC_ERR | BRK_ABRT)) {
>  			error = 1;
>  			if (r1 & BRK_ABRT) {
> +				printk(KERN_DEBUG "pmz:got break !\n");
>  				pmz_debug("pmz: got break !\n");
>  				r1 &= ~(PAR_ERR | CRC_ERR);
>  				uap->port.icount.brk++;
> @@ -364,6 +376,9 @@
>  		wake_up_interruptible(&uap->port.info->delta_msr_wait);
>  	}
>
> +	if (status & BRK_ABRT)
> +		uap->flags |= PMACZILOG_FLAG_BREAK;
> +
>  	uap->prev_status = status;
>  }
>
> @@ -872,8 +887,8 @@
>  	uap->curregs[R13] = 0;
>  	uap->curregs[R14] = BRENAB;
>
> -	/* Clear handshaking */
> -	uap->curregs[R15] = 0;
> +	/* Clear handshaking, enable BREAK interrupts */
> +	uap->curregs[R15] = BRKIE;
>
>  	/* Master interrupt enable */
>  	uap->curregs[R9] |= NV | MIE;
> @@ -1919,10 +1934,11 @@
>  static void pmz_console_write(struct console *con, const char *s, unsigned int count)
>  {
>  	struct uart_pmac_port *uap = &pmz_ports[con->index];
> -	unsigned long flags;
>  	int i;
>
> -	spin_lock_irqsave(&uap->port.lock, flags);
> +	/* Don't do any locking here, since we might get called from within
> +	 * sysreq() handling and already hold a lock.  Ideally we would need
> +	 * something like the 'steal lock' code in sn_sal_console_write -HW */
>
>  	/* Turn of interrupts and enable the transmitter. */
>  	write_zsreg(uap, R1, uap->curregs[1] & ~TxINT_ENAB);
> @@ -1943,8 +1959,6 @@
>  	/* Restore the values in the registers. */
>  	write_zsreg(uap, R1, uap->curregs[1]);
>  	/* Don't disable the transmitter. */
> -
> -	spin_unlock_irqrestore(&uap->port.lock, flags);
>  }
>
>  /*
--
Benjamin Herrenschmidt <benh@kernel.crashing.org>


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [PATCH] pmac_zilog.c: Fix break handling, add sysrq support

[Harald Welte]

On Fri, Aug 06, 2004 at 08:44:38PM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2004-08-06 at 20:43, Harald Welte wrote:
> > This patch fixes the BREAK handling in pmac_zilog.c, and also adds
> > support for SYSRQ via BREAK.
> >
> > Please note that I had to remove locking in pmz_consolw_write(),
> > since it is called from handle_sysrq(), which is in turn called from
> > pmz_receive_chars(), which is called from with the lock held...
> >
> > while trying to find out how other drivers handle this problem, I
> > found out that sn_sal_console_write() has some elaborate code that
> > tries to steal the lock, etc.
>
> I noticed some locking bugs in other drivers too... it may be better
> to drop the lock when calling handle_sysrq, no ?

Yes, I agree, that sounds way cleaner than just not lock at all in
console_write().  But then of course you could still runt into a race
condition where another thread is already spinning for the lock.  As
soon as we drop the lock, the other thread would grab it... and when
console_write() is called via handle_sysrq(), it's already too late,
isn't it?

I think this is a generic issue that all serial drivers supporting
sysrq() face.  Instead of putting some special different solution into
each driver, I'd say the code from sn_sal_console_write() needs to be
generalized.

Cc'ing the serial maintainer exactly for this reason.

Russell, what is the preferred solution for this deadlock-prone
situation?

> I'll have a look, thanks much.
> Ben.
--
- Harald Welte <laforge@gnumonks.org>               http://www.gnumonks.org/
============================================================================
Programming is like sex: One mistake and you have to support it your lifetime

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [PATCH] pmac_zilog.c: Fix break handling, add sysrq support

[Benjamin Herrenschmidt]

>
> Yes, I agree, that sounds way cleaner than just not lock at all in
> console_write().  But then of course you could still runt into a race
> condition where another thread is already spinning for the lock.  As
> soon as we drop the lock, the other thread would grab it... and when
> console_write() is called via handle_sysrq(), it's already too late,
> isn't it?

Ugh ? handle_sysrq() is called from irq, this should be an irqsafe
lock, another thread on another cpu would just cause us to wait a bit...

> I think this is a generic issue that all serial drivers supporting
> sysrq() face.  Instead of putting some special different solution into
> each driver, I'd say the code from sn_sal_console_write() needs to be
> generalized.
>
> Cc'ing the serial maintainer exactly for this reason.
>
> Russell, what is the preferred solution for this deadlock-prone
> situation?
>
> > I'll have a look, thanks much.
> > Ben.
--
Benjamin Herrenschmidt <benh@kernel.crashing.org>


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/