mbox: Sanitize interrupts registers

Message ID 1492729190.25766.155.camel@kernel.crashing.org
State Accepted
Headers show

Commit Message

Benjamin Herrenschmidt April 20, 2017, 10:59 p.m.
If some status interrupts are left unmasked by a previous
firmware run (either HostBoot or some other version of skiboot),
we fail to clear them and end up with a runaway SerIRQ.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

Let's merge that now. It will be obsoleted by the v2 protocol
implementation but we want a fix out ASAP.

 hw/lpc-mbox.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

Comments

Cyril Bur April 20, 2017, 11:51 p.m. | #1
On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote:
> If some status interrupts are left unmasked by a previous
> firmware run (either HostBoot or some other version of skiboot),
> we fail to clear them and end up with a runaway SerIRQ.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> Let's merge that now. It will be obsoleted by the v2 protocol
> implementation but we want a fix out ASAP.

I think this is still important even after v2 implementation gets
merged.

> 
>  hw/lpc-mbox.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
> index fd107fb..21e6eee 100644
> --- a/hw/lpc-mbox.c
> +++ b/hw/lpc-mbox.c
> @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = {
>  
>  static bool mbox_init_hw(void)
>  {
> -	/*
> -	 * Turns out there isn't anything to do.
> -	 * It might be a good idea to santise the registers though.
> -	 * TODO
> +	/* Disable all status interrupts except attentions */
> +	bmc_mbox_outb(0x00, MBOX_HOST_INT_EN_0);
> +	bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_HOST_INT_EN_1);
> +

Would it be wise to also (you do it for the control register
interrupt):

	/* 
	 * Cleanup host interrupt status bits except attention, these 	
 * bits are write 1 to clear.
	 */
	bmc_mbox_outb(0xff, MBOX_STATUS_0);
	bmc_mbox_outb(~MBOX_STATUS_ATTN, MBOX_STATUS_1);

> +	/* Cleanup host interrupt and status */
> +	bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> +
> +	/* Disable host control interrupt for now (will be
> +	 * re-enabled when needed). Clear BMC interrupts
>  	 */
> +	bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL);
> +
>  	return true;
>  }
> 
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Benjamin Herrenschmidt April 21, 2017, 12:11 a.m. | #2
On Fri, 2017-04-21 at 09:51 +1000, Cyril Bur wrote:
> On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote:
> > If some status interrupts are left unmasked by a previous
> > firmware run (either HostBoot or some other version of skiboot),
> > we fail to clear them and end up with a runaway SerIRQ.
> > 
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > 
> > Let's merge that now. It will be obsoleted by the v2 protocol
> > implementation but we want a fix out ASAP.
> 
> I think this is still important even after v2 implementation gets
> merged.

Don't we use a different interrupt with v2 anyway ? 

> > 
> >  hw/lpc-mbox.c | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
> > index fd107fb..21e6eee 100644
> > --- a/hw/lpc-mbox.c
> > +++ b/hw/lpc-mbox.c
> > @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = {
> >  
> >  static bool mbox_init_hw(void)
> >  {
> > -	/*
> > -	 * Turns out there isn't anything to do.
> > -	 * It might be a good idea to santise the registers
> > though.
> > -	 * TODO
> > +	/* Disable all status interrupts except attentions */
> > +	bmc_mbox_outb(0x00, MBOX_HOST_INT_EN_0);
> > +	bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_HOST_INT_EN_1);
> > +
> 
> Would it be wise to also (you do it for the control register
> interrupt):
> 
> 	/* 
> 	 * Cleanup host interrupt status bits except attention, these 	
>  * bits are write 1 to clear.
> 	 */
> 	bmc_mbox_outb(0xff, MBOX_STATUS_0);
> 	bmc_mbox_outb(~MBOX_STATUS_ATTN, MBOX_STATUS_1);

Not as important since we don't enable those... it won't hurt
to clear them but it won't gain anything either. Once we enable them,
the story is different.

So let's do that later with the v2 update.

Another subsequent fix we will need when we start using status
interrupts for message exchanges: Remember how the HW trigger the
interrupt by our own writes to the register ? IE not only when the BMC
writes the response but also when we write the command.

So if/when we switch to using the status interrupt instead of the host
interrupt, we need to keep it disabled until after we have written the
command (but not sent it to BMC yet) and cleared the status bit. Only
then it is safe to enable the interrupt.

> > +	/* Cleanup host interrupt and status */
> > +	bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> > +
> > +	/* Disable host control interrupt for now (will be
> > +	 * re-enabled when needed). Clear BMC interrupts
> >  	 */
> > +	bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL);
> > +
> >  	return true;
> >  }
> > 
> > 
> > _______________________________________________
> > Skiboot mailing list
> > Skiboot@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/skiboot
Michael Neuling April 21, 2017, 1:59 a.m. | #3
This is upstream as of 50e1921f98.


On Fri, 2017-04-21 at 08:59 +1000, Benjamin Herrenschmidt wrote:
> If some status interrupts are left unmasked by a previous
> firmware run (either HostBoot or some other version of skiboot),
> we fail to clear them and end up with a runaway SerIRQ.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> Let's merge that now. It will be obsoleted by the v2 protocol
> implementation but we want a fix out ASAP.
> 
>  hw/lpc-mbox.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
> index fd107fb..21e6eee 100644
> --- a/hw/lpc-mbox.c
> +++ b/hw/lpc-mbox.c
> @@ -200,11 +200,18 @@ static struct lpc_client mbox_lpc_client = {
>  
>  static bool mbox_init_hw(void)
>  {
> -	/*
> -	 * Turns out there isn't anything to do.
> -	 * It might be a good idea to santise the registers though.
> -	 * TODO
> +	/* Disable all status interrupts except attentions */
> +	bmc_mbox_outb(0x00, MBOX_HOST_INT_EN_0);
> +	bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_HOST_INT_EN_1);
> +
> +	/* Cleanup host interrupt and status */
> +	bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
> +
> +	/* Disable host control interrupt for now (will be
> +	 * re-enabled when needed). Clear BMC interrupts
>  	 */
> +	bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL);
> +
>  	return true;
>  }
> 
> 
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot

Patch

diff --git a/hw/lpc-mbox.c b/hw/lpc-mbox.c
index fd107fb..21e6eee 100644
--- a/hw/lpc-mbox.c
+++ b/hw/lpc-mbox.c
@@ -200,11 +200,18 @@  static struct lpc_client mbox_lpc_client = {
 
 static bool mbox_init_hw(void)
 {
-	/*
-	 * Turns out there isn't anything to do.
-	 * It might be a good idea to santise the registers though.
-	 * TODO
+	/* Disable all status interrupts except attentions */
+	bmc_mbox_outb(0x00, MBOX_HOST_INT_EN_0);
+	bmc_mbox_outb(MBOX_STATUS_ATTN, MBOX_HOST_INT_EN_1);
+
+	/* Cleanup host interrupt and status */
+	bmc_mbox_outb(MBOX_CTRL_INT_STATUS, MBOX_HOST_CTRL);
+
+	/* Disable host control interrupt for now (will be
+	 * re-enabled when needed). Clear BMC interrupts
 	 */
+	bmc_mbox_outb(MBOX_CTRL_INT_MASK, MBOX_BMC_CTRL);
+
 	return true;
 }