[PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[Mikael Pettersson]

Booting a 3.2, 3.3, or 3.4-rc4 kernel on an Atari using the
`nfeth' ethernet device triggers a WARN_ONCE() in generic irq
handling code on the first irq for that device:

WARNING: at kernel/irq/handle.c:146 handle_irq_event_percpu+0x134/0x142()
irq 3 handler nfeth_interrupt+0x0/0x194 enabled interrupts
Modules linked in:
Call Trace: [<000299b2>] warn_slowpath_common+0x48/0x6a
 [<000299c0>] warn_slowpath_common+0x56/0x6a
 [<00029a4c>] warn_slowpath_fmt+0x2a/0x32
 [<0005b34c>] handle_irq_event_percpu+0x134/0x142
 [<0005b34c>] handle_irq_event_percpu+0x134/0x142
 [<0000a584>] nfeth_interrupt+0x0/0x194
 [<001ba0a8>] schedule_preempt_disabled+0x0/0xc
 [<0005b37a>] handle_irq_event+0x20/0x2c
 [<0005add4>] generic_handle_irq+0x2c/0x3a
 [<00002ab6>] do_IRQ+0x20/0x32
 [<0000289e>] auto_irqhandler_fixup+0x4/0x6
 [<00003144>] cpu_idle+0x22/0x2e
 [<001b8a78>] printk+0x0/0x18
 [<0024d112>] start_kernel+0x37a/0x386
 [<0003021d>] __do_proc_dointvec+0xb1/0x366
 [<0003021d>] __do_proc_dointvec+0xb1/0x366
 [<0024c31e>] _sinittext+0x31e/0x9c0

After invoking the irq's handler the kernel sees !irqs_disabled()
and concludes that the handler erroneously enabled interrupts.

However, debugging shows that !irqs_disabled() is true even before
the handler is invoked, which indicates a problem in the platform
code rather than the specific driver.

The warning does not occur in 3.1 or older kernels.

It turns out that the ALLOWINT definition for Atari is incorrect.

The Atari definition of ALLOWINT is ~0x400, the stated purpose of
that is to avoid taking HSYNC interrupts.  irqs_disabled() returns
true if the 3-bit ipl & 4 is non-zero.  The nfeth interrupt runs at
ipl 3 (it's autovector 3), but 3 & 4 is zero so irqs_disabled() is
false, and the warning above is generated.

When interrupts are explicitly disabled, ipl is set to 7.  When they
are enabled, ipl is masked with ALLOWINT.  On Atari this will result
in ipl = 3, which blocks interrupts at ipl 3 and below.  So how come
nfeth interrupts at ipl 3 are received at all?  That's because ipl
is reset to 2 by Atari-specific code in default_idle(), again with
the stated purpose of blocking HSYNC interrupts.  This discrepancy
means that ipl 3 can remain blocked for longer than intended.

Both default_idle() and falcon_hblhandler() identify HSYNC with
ipl 2, and the "Atari ST/.../F030 Hardware Register Listing" agrees,
but ALLOWINT is defined as if HSYNC was ipl 3.

[As an experiment I modified default_idle() to reset ipl to 3, and
as expected that resulted in all nfeth interrupts being blocked.]

The fix is simple: define ALLOWINT as ~0x500 instead.  This makes
arch_local_irq_enable() consistent with default_idle(), and prevents
the !irqs_disabled() problems for ipl 3 interrupts.

Tested on Atari running in an Aranym VM.

Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
---
Since ipl is interpreted by HW as a numerical level, the current
simple-minded bit masking operations in arch_local_irq_enable(),
arch_irqs_disabled_flags(), and entry_mm.S aren't really correct,
but only Atari is affected, and it doesn't seem to hurt in practice.

--- linux-3.4-rc3/arch/m68k/include/asm/entry.h.~1~	2012-03-19 14:06:52.000000000 +0100
+++ linux-3.4-rc3/arch/m68k/include/asm/entry.h	2012-04-18 22:07:52.000000000 +0200
@@ -33,8 +33,8 @@
 
 /* the following macro is used when enabling interrupts */
 #if defined(MACH_ATARI_ONLY)
-	/* block out HSYNC on the atari */
-#define ALLOWINT	(~0x400)
+	/* block out HSYNC = ipl 2 on the atari */
+#define ALLOWINT	(~0x500)
 #define	MAX_NOINT_IPL	3
 #else
 	/* portable version */

Re: [PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[Michael Schmitz]

Mikael Pettersson wrote:
> The fix is simple: define ALLOWINT as ~0x500 instead.  This makes
> arch_local_irq_enable() consistent with default_idle(), and prevents
> the !irqs_disabled() problems for ipl 3 interrupts.
>
> Tested on Atari running in an Aranym VM.
>   
Tested on my Falcon/CT60.

  Michael

> Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
> ---
> Since ipl is interpreted by HW as a numerical level, the current
> simple-minded bit masking operations in arch_local_irq_enable(),
> arch_irqs_disabled_flags(), and entry_mm.S aren't really correct,
> but only Atari is affected, and it doesn't seem to hurt in practice.
>
> --- linux-3.4-rc3/arch/m68k/include/asm/entry.h.~1~	2012-03-19 14:06:52.000000000 +0100
> +++ linux-3.4-rc3/arch/m68k/include/asm/entry.h	2012-04-18 22:07:52.000000000 +0200
> @@ -33,8 +33,8 @@
>  
>  /* the following macro is used when enabling interrupts */
>  #if defined(MACH_ATARI_ONLY)
> -	/* block out HSYNC on the atari */
> -#define ALLOWINT	(~0x400)
> +	/* block out HSYNC = ipl 2 on the atari */
> +#define ALLOWINT	(~0x500)
>  #define	MAX_NOINT_IPL	3
>  #else
>  	/* portable version */
> --
> 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
>   

Re: [PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[Mikael Pettersson]

Michael Schmitz writes:
 > Mikael Pettersson wrote:
 > > The fix is simple: define ALLOWINT as ~0x500 instead.  This makes
 > > arch_local_irq_enable() consistent with default_idle(), and prevents
 > > the !irqs_disabled() problems for ipl 3 interrupts.
 > >
 > > Tested on Atari running in an Aranym VM.
 > >   
 > Tested on my Falcon/CT60.

Thanks.  May I add an explicit

Tested-by: Michael Schmitz <schmitzmic@googlemail.com> (on Falcon/CT60)

to the patch description?

/Mikael

 > 
 >   Michael
 > 
 > > Signed-off-by: Mikael Pettersson <mikpe@it.uu.se>
 > > ---
 > > Since ipl is interpreted by HW as a numerical level, the current
 > > simple-minded bit masking operations in arch_local_irq_enable(),
 > > arch_irqs_disabled_flags(), and entry_mm.S aren't really correct,
 > > but only Atari is affected, and it doesn't seem to hurt in practice.
 > >
 > > --- linux-3.4-rc3/arch/m68k/include/asm/entry.h.~1~	2012-03-19 14:06:52.000000000 +0100
 > > +++ linux-3.4-rc3/arch/m68k/include/asm/entry.h	2012-04-18 22:07:52.000000000 +0200
 > > @@ -33,8 +33,8 @@
 > >  
 > >  /* the following macro is used when enabling interrupts */
 > >  #if defined(MACH_ATARI_ONLY)
 > > -	/* block out HSYNC on the atari */
 > > -#define ALLOWINT	(~0x400)
 > > +	/* block out HSYNC = ipl 2 on the atari */
 > > +#define ALLOWINT	(~0x500)
 > >  #define	MAX_NOINT_IPL	3
 > >  #else
 > >  	/* portable version */
 > > --
 > > 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
 > >   
 > 

Re: [PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[Michael Schmitz]

Mikael,

On Thu, Apr 19, 2012 at 11:03 PM, Mikael Pettersson <mikpe@it.uu.se> wrote:
> Michael Schmitz writes:
>  > Mikael Pettersson wrote:
>  > > The fix is simple: define ALLOWINT as ~0x500 instead.  This makes
>  > > arch_local_irq_enable() consistent with default_idle(), and prevents
>  > > the !irqs_disabled() problems for ipl 3 interrupts.
>  > >
>  > > Tested on Atari running in an Aranym VM.
>  > >
>  > Tested on my Falcon/CT60.
>
> Thanks.  May I add an explicit
>
> Tested-by: Michael Schmitz <schmitzmic@googlemail.com> (on Falcon/CT60)
>
> to the patch description?

By all means - feel free to do that. Thanks for catching this one -
I've not had time for much more than quickly booting the kernel and
poking around a bit to make sure network and the like are alive but
I've got a hunch that this may resolve keyboard overruns that I did
very sporadically see in the 3.x series.

Thanks again,

  Michael

Re: [PATCH 1/2] m68k: correct the Atari ALLOWINT definition

[Geert Uytterhoeven]

On Thu, Apr 19, 2012 at 00:53, Mikael Pettersson <mikpe@it.uu.se> wrote:
> Booting a 3.2, 3.3, or 3.4-rc4 kernel on an Atari using the
> `nfeth' ethernet device triggers a WARN_ONCE() in generic irq
> handling code on the first irq for that device:
>
> WARNING: at kernel/irq/handle.c:146 handle_irq_event_percpu+0x134/0x142()
> irq 3 handler nfeth_interrupt+0x0/0x194 enabled interrupts

Many thanks for fixing this!

I will apply and queue for 3.5, with Michael's Tested-by.

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