[01/16,v3] pmac_zilog: fix unexpected irq

On most 68k Macs the SCC IRQ is an autovector interrupt and cannot be 
masked. This can be a problem when pmac_zilog starts up.

For example, the serial debugging code in arch/m68k/kernel/head.S may be 
used beforehand. It disables the SCC interrupts at the chip but doesn't 
ack them. Then when a pmac_zilog port is used, the machine locks up with 
"unexpected interrupt".

This can happen in pmz_shutdown() since the irq is freed before the 
channel interrupts are disabled.

Fix this by clearing interrupt enable bits before the handler is 
uninstalled. Also move the interrupt control bit flipping into a separate 
pmz_interrupt_control() routine. Replace all instances of these operations 
with calls to this routine. Omit the zssync() calls that seem to serve no 
purpose.

Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
Acked-by: Alan Cox <alan@linux.intel.com>

---
   
Re-implemented as v2 using a simpler approach that avoids messing with the 
Master Interrupt Enable bit. As well as the ifdef problem, it turns out 
that v1 was not sufficient to entirely fix the problem.

v3 avoids needless changes to the logic and locking in the suspend and 
shutdown code and tries to keep register writes closer to their original 
sequence.

This patch has been tested on a PowerBook 520 but no PowerMacs yet.

On Wed, 2011-12-07 at 14:49 +1100, Finn Thain wrote:
> On most 68k Macs the SCC IRQ is an autovector interrupt and cannot be 
> masked. This can be a problem when pmac_zilog starts up.

Thanks. I'll test it on a powermac or two and will merge it via the
powerpc -next tree if it works out allright.

Cheers,
Ben.
 

> For example, the serial debugging code in arch/m68k/kernel/head.S may be 
> used beforehand. It disables the SCC interrupts at the chip but doesn't 
> ack them. Then when a pmac_zilog port is used, the machine locks up with 
> "unexpected interrupt".
> 
> This can happen in pmz_shutdown() since the irq is freed before the 
> channel interrupts are disabled.
> 
> Fix this by clearing interrupt enable bits before the handler is 
> uninstalled. Also move the interrupt control bit flipping into a separate 
> pmz_interrupt_control() routine. Replace all instances of these operations 
> with calls to this routine. Omit the zssync() calls that seem to serve no 
> purpose.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Acked-by: Alan Cox <alan@linux.intel.com>
> 
> ---
>    
> Re-implemented as v2 using a simpler approach that avoids messing with the 
> Master Interrupt Enable bit. As well as the ifdef problem, it turns out 
> that v1 was not sufficient to entirely fix the problem.
> 
> v3 avoids needless changes to the logic and locking in the suspend and 
> shutdown code and tries to keep register writes closer to their original 
> sequence.
> 
> This patch has been tested on a PowerBook 520 but no PowerMacs yet.
> 
> 
> Index: linux-git/drivers/tty/serial/pmac_zilog.c
> ===================================================================
> --- linux-git.orig/drivers/tty/serial/pmac_zilog.c	2011-12-07 12:36:32.000000000 +1100
> +++ linux-git/drivers/tty/serial/pmac_zilog.c	2011-12-07 14:29:21.000000000 +1100
> @@ -216,6 +216,18 @@ static void pmz_maybe_update_regs(struct
>  	}
>  }
>  
> +static void pmz_interrupt_control(struct uart_pmac_port *uap, int enable)
> +{
> +	if (enable) {
> +		uap->curregs[1] |= INT_ALL_Rx | TxINT_ENAB;
> +		if (!ZS_IS_EXTCLK(uap))
> +			uap->curregs[1] |= EXT_INT_ENAB;
> +	} else {
> +		uap->curregs[1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> +	}
> +	write_zsreg(uap, R1, uap->curregs[1]);
> +}
> +
>  static struct tty_struct *pmz_receive_chars(struct uart_pmac_port *uap)
>  {
>  	struct tty_struct *tty = NULL;
> @@ -339,9 +351,7 @@ static struct tty_struct *pmz_receive_ch
>  
>  	return tty;
>   flood:
> -	uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> -	zssync(uap);
> +	pmz_interrupt_control(uap, 0);
>  	pmz_error("pmz: rx irq flood !\n");
>  	return tty;
>  }
> @@ -990,12 +1000,9 @@ static int pmz_startup(struct uart_port
>  	if (ZS_IS_IRDA(uap))
>  		pmz_irda_reset(uap);
>  
> -	/* Enable interrupts emission from the chip */
> +	/* Enable interrupt requests for the channel */
>  	spin_lock_irqsave(&port->lock, flags);
> -	uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -	if (!ZS_IS_EXTCLK(uap))
> -		uap->curregs[R1] |= EXT_INT_ENAB;
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> +	pmz_interrupt_control(uap, 1);
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
>  	pmz_debug("pmz: startup() done.\n");
> @@ -1015,6 +1022,25 @@ static void pmz_shutdown(struct uart_por
>  
>  	mutex_lock(&pmz_irq_mutex);
>  
> +	spin_lock_irqsave(&port->lock, flags);
> +
> +	if (!ZS_IS_ASLEEP(uap)) {
> +		/* Disable interrupt requests for the channel */
> +		pmz_interrupt_control(uap, 0);
> +
> +		if (!ZS_IS_CONS(uap)) {
> +			/* Disable receiver and transmitter */
> +			uap->curregs[R3] &= ~RxENABLE;
> +			uap->curregs[R5] &= ~TxENABLE;
> +
> +			/* Disable break assertion */
> +			uap->curregs[R5] &= ~SND_BRK;
> +			pmz_maybe_update_regs(uap);
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&port->lock, flags);
> +
>  	/* Release interrupt handler */
>  	free_irq(uap->port.irq, uap);
>  
> @@ -1025,29 +1051,8 @@ static void pmz_shutdown(struct uart_por
>  	if (!ZS_IS_OPEN(uap->mate))
>  		pmz_get_port_A(uap)->flags &= ~PMACZILOG_FLAG_IS_IRQ_ON;
>  
> -	/* Disable interrupts */
> -	if (!ZS_IS_ASLEEP(uap)) {
> -		uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -		zssync(uap);
> -	}
> -
> -	if (ZS_IS_CONS(uap) || ZS_IS_ASLEEP(uap)) {
> -		spin_unlock_irqrestore(&port->lock, flags);
> -		mutex_unlock(&pmz_irq_mutex);
> -		return;
> -	}
> -
> -	/* Disable receiver and transmitter.  */
> -	uap->curregs[R3] &= ~RxENABLE;
> -	uap->curregs[R5] &= ~TxENABLE;
> -
> -	/* Disable all interrupts and BRK assertion.  */
> -	uap->curregs[R5] &= ~SND_BRK;
> -	pmz_maybe_update_regs(uap);
> -
> -	/* Shut the chip down */
> -	pmz_set_scc_power(uap, 0);
> +	if (!ZS_IS_ASLEEP(uap) && !ZS_IS_CONS(uap))
> +		pmz_set_scc_power(uap, 0);	/* Shut the chip down */
>  
>  	spin_unlock_irqrestore(&port->lock, flags);
>  
> @@ -1352,19 +1357,15 @@ static void pmz_set_termios(struct uart_
>  	spin_lock_irqsave(&port->lock, flags);	
>  
>  	/* Disable IRQs on the port */
> -	uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> -	write_zsreg(uap, R1, uap->curregs[R1]);
> +	pmz_interrupt_control(uap, 0);
>  
>  	/* Setup new port configuration */
>  	__pmz_set_termios(port, termios, old);
>  
>  	/* Re-enable IRQs on the port */
> -	if (ZS_IS_OPEN(uap)) {
> -		uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -		if (!ZS_IS_EXTCLK(uap))
> -			uap->curregs[R1] |= EXT_INT_ENAB;
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -	}
> +	if (ZS_IS_OPEN(uap))
> +		pmz_interrupt_control(uap, 1);
> +
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
>  
> @@ -1671,14 +1672,17 @@ static int pmz_suspend(struct macio_dev
>  	spin_lock_irqsave(&uap->port.lock, flags);
>  
>  	if (ZS_IS_OPEN(uap) || ZS_IS_CONS(uap)) {
> -		/* Disable receiver and transmitter.  */
> +		/* Disable interrupt requests for the channel */
> +		pmz_interrupt_control(uap, 0);
> +
> +		/* Disable receiver and transmitter */
>  		uap->curregs[R3] &= ~RxENABLE;
>  		uap->curregs[R5] &= ~TxENABLE;
>  
> -		/* Disable all interrupts and BRK assertion.  */
> -		uap->curregs[R1] &= ~(EXT_INT_ENAB | TxINT_ENAB | RxINT_MASK);
> +		/* Disable break assertion */
>  		uap->curregs[R5] &= ~SND_BRK;
>  		pmz_load_zsregs(uap, uap->curregs);
> +
>  		uap->flags |= PMACZILOG_FLAG_IS_ASLEEP;
>  		mb();
>  	}
> @@ -1738,14 +1742,6 @@ static int pmz_resume(struct macio_dev *
>  	/* Take care of config that may have changed while asleep */
>  	__pmz_set_termios(&uap->port, &uap->termios_cache, NULL);
>  
> -	if (ZS_IS_OPEN(uap)) {
> -		/* Enable interrupts */		
> -		uap->curregs[R1] |= INT_ALL_Rx | TxINT_ENAB;
> -		if (!ZS_IS_EXTCLK(uap))
> -			uap->curregs[R1] |= EXT_INT_ENAB;
> -		write_zsreg(uap, R1, uap->curregs[R1]);
> -	}
> -
>  	spin_unlock_irqrestore(&uap->port.lock, flags);
>  
>  	if (ZS_IS_CONS(uap))
> @@ -1757,6 +1753,12 @@ static int pmz_resume(struct macio_dev *
>  		enable_irq(uap->port.irq);
>  	}
>  
> +	if (ZS_IS_OPEN(uap)) {
> +		spin_lock_irqsave(&uap->port.lock, flags);
> +		pmz_interrupt_control(uap, 1);
> +		spin_unlock_irqrestore(&uap->port.lock, flags);
> +	}
> +
>   bail:
>  	mutex_unlock(&state->port.mutex);
>  	mutex_unlock(&pmz_irq_mutex);

On Wed, 2011-12-07 at 14:49 +1100, Finn Thain wrote:
> On most 68k Macs the SCC IRQ is an autovector interrupt and cannot be 
> masked. This can be a problem when pmac_zilog starts up.
> 
> For example, the serial debugging code in arch/m68k/kernel/head.S may be 
> used beforehand. It disables the SCC interrupts at the chip but doesn't 
> ack them. Then when a pmac_zilog port is used, the machine locks up with 
> "unexpected interrupt".
> 
> This can happen in pmz_shutdown() since the irq is freed before the 
> channel interrupts are disabled.
> 
> Fix this by clearing interrupt enable bits before the handler is 
> uninstalled. Also move the interrupt control bit flipping into a separate 
> pmz_interrupt_control() routine. Replace all instances of these operations 
> with calls to this routine. Omit the zssync() calls that seem to serve no 
> purpose.
> 
> Signed-off-by: Finn Thain <fthain@telegraphics.com.au>
> Acked-by: Alan Cox <alan@linux.intel.com>
> 
> ---

So basic operations seem to work, I've applied the patch to
powerpc-next.

However, the internal modem on my Pismo powerbook doesn't appear to
survive suspend/resume. I'll dig into that and merge a fixup patch asap.

Cheers,
Ben.

On Thu, 2011-12-08 at 15:20 +1100, Benjamin Herrenschmidt wrote:

> So basic operations seem to work, I've applied the patch to
> powerpc-next.
> 
> However, the internal modem on my Pismo powerbook doesn't appear to
> survive suspend/resume. I'll dig into that and merge a fixup patch asap.

BTW. I applied anyway because suspend/resume was already broken (you
spotted that we don't clear the suspended flag for example).

Fixing the flag alone helps a bit. We can't use the modem if we
suspend/resume with the open port, but closing and re-opening works.

Lockdep also picked-up a A->B B->A between the port mutex and the pmz
irq mutex on suspend.

I'll try to fix all these, and will let you know (I may not have time
today).

Cheers,
Ben.

On Thu, 8 Dec 2011, Benjamin Herrenschmidt wrote:

> On Thu, 2011-12-08 at 15:20 +1100, Benjamin Herrenschmidt wrote:
> 
> > So basic operations seem to work, I've applied the patch to 
> > powerpc-next.

Then I guess Geert should not push this for 3.3 -- or does it make no 
difference?

> > However, the internal modem on my Pismo powerbook doesn't appear to 
> > survive suspend/resume. I'll dig into that and merge a fixup patch 
> > asap.
> 
> BTW. I applied anyway because suspend/resume was already broken (you 
> spotted that we don't clear the suspended flag for example).
> 
> Fixing the flag alone helps a bit. We can't use the modem if we 
> suspend/resume with the open port,

If the SCC IRQ counters change across suspend/resume, perhaps the modem 
itself is not powering up...

> but closing and re-opening works.

Maybe the modem wants a transition on DTR or similar, but it hasn't had 
time to initialise when that happens during SCC resumption.

If so, calling pmz_shutdown() then pmz_startup() from the tail of 
pmz_resume() without delay should probably fail to revive it...

> 
> Lockdep also picked-up a A->B B->A between the port mutex and the pmz 
> irq mutex on suspend.
> 
> I'll try to fix all these, and will let you know (I may not have time 
> today).

Thanks.

Finn

> 
> Cheers,
> Ben.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-m68k" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Hi Finn,

On Thu, Dec 8, 2011 at 12:26, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Thu, 8 Dec 2011, Benjamin Herrenschmidt wrote:
>> On Thu, 2011-12-08 at 15:20 +1100, Benjamin Herrenschmidt wrote:
>> > So basic operations seem to work, I've applied the patch to
>> > powerpc-next.
>
> Then I guess Geert should not push this for 3.3 -- or does it make no
> difference?

I do not plan to push it myself, that's why it's not in my for-next branch.
The for-3.3 is just indicative.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

On Thu, 2011-12-08 at 22:26 +1100, Finn Thain wrote:
> 
> Maybe the modem wants a transition on DTR or similar, but it hasn't had 
> time to initialise when that happens during SCC resumption.
> 
> If so, calling pmz_shutdown() then pmz_startup() from the tail of 
> pmz_resume() without delay should probably fail to revive it... 

Well, we power the modem down and back up... but it's possible that we
fail to re-enable something, I'll check. That used to work (at least
with macserial, maybe I never tried this specific torture with pmz...).
I'll figure it out eventually.

Cheers,
Ben.