Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified

Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified

[Paul Walmsley]

On Thu, 18 Aug 2011, Pantelis Antoniou wrote:

> Let me report that with this change Beagle board fails to boot,
> hangs right on rootfs mount.

Confirmed here.

Commit f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set 
IRQF_ONESHOT if no primary handler is specified"), merged as part of 
v3.1-rc3, won't boot into userspace via MMC on either OMAP4 Panda or 
OMAP3530 BeagleBoard.  v3.1-rc2 works.


- Paul

> Users of BB should revert this until the offending driver(s) are fixed.
> 
> Regards
> 
> -- Pantelis
> 
> On Jul 7, 2011, at 11:32 PM, Sebastian Andrzej Siewior wrote:
> 
> > If no primary handler is specified then a default one is assigned which
> > always returns IRQ_WAKE_THREAD. This handler requires the IRQF_ONESHOT
> > flag on LEVEL / EIO typed irqs because the source of interrupt is not
> > disabled.
> > Since it is required for those users and there is no difference for others
> > it makes sense to add this flag unconditionally.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> > A quick grep shows that drivers/staging/iio/adc/ad7816.c is the only
> > driver in tree doing it wrong i.e. request_threaded_irq with
> > IRQF_TRIGGER_{LOW|HIGH} but without ONEHOST.
> > 
> > There are 59 users in tree which request an edge typed interrupt. 24 of
> > them specify the ONEHOST flag the others don't. Both variants are valid
> > and identical but a consistent behavior would be nice.
> > 
> > There is a total of 134 users and 87 specify the irq type directly. For
> > the remaining 47 the type depends on initial configuration of the irq chip
> > or it might be specified via the device tree, dunno. 
> > Should that patch be accepted I suggest to remove the ONESHOT flag from
> > every request_th.* which uses NULL as the primary handler so we have a
> > consistent behavior here.
> > 
> > kernel/irq/manage.c |    1 +
> > 1 files changed, 1 insertions(+), 0 deletions(-)
> > 
> > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> > index 0a7840ae..3f9cd47 100644
> > --- a/kernel/irq/manage.c
> > +++ b/kernel/irq/manage.c
> > @@ -1322,6 +1322,7 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
> > 		if (!thread_fn)
> > 			return -EINVAL;
> > 		handler = irq_default_primary_handler;
> > +		irqflags |= IRQF_ONESHOT;
> > 	}
> > 
> > 	action = kzalloc(sizeof(struct irqaction), GFP_KERNEL);
> > -- 
> > 1.7.4.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified

[Paul Walmsley]

On Fri, 19 Aug 2011, Felipe Balbi wrote:

> On Thu, Aug 18, 2011 at 07:22:39PM +0200, Sebastian Andrzej Siewior wrote:
> > Pantelis Antoniou wrote:
> > 
> > >Let me report that with this change Beagle board fails to boot,
> > >hangs right on rootfs mount.
> > 
> > Can you provide some more information about the kind of the failure and
> > used drivers?
> > 
> > >Users of BB should revert this until the offending driver(s) are fixed.
> > Which is the offending driver?
> 
> I would guess it's the lack of threaded IRQ conversion on the
> twl4030 driver. I have converted it but noone actually picked the series
> [1].
> 
> http://marc.info/?i=1309427470-605-1-git-send-email-balbi@ti.com

Just a data point: applying this series doesn't seem to fix the hang.

Sebastian, Thomas, does commit f3637a5f2e2eb391ff5757bc83fb5de8f9726464 
("irq: Always set IRQF_ONESHOT if no primary handler is specified") fix 
anything, aside from a driver in staging?


- Paul

[PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Paul Walmsley]

Convert the omap-serial hardirq handler to a threaded IRQ handler. Without 
this patch, OMAP boards which use the on-board OMAP UARTs and the 
omap-serial driver will not boot to userspace after commit 
f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set IRQF_ONESHOT if 
no primary handler is specified").  Enabling CONFIG_DEBUG_SHIRQ reveals 
'IRQ handler type mismatch' errors:

IRQ handler type mismatch for IRQ 74
current handler: serial idle
[<c001b124>] (unwind_backtrace+0x0/0xf0) from [<c009c900>] (__setup_irq+0x42c/0x464)
[<c009c900>] (__setup_irq+0x42c/0x464) from [<c009ca08>] (request_threaded_irq+0xd0/0x148)
[<c009ca08>] (request_threaded_irq+0xd0/0x148) from [<c029d398>] (serial_omap_startup+0x30/0x2dc)
[<c029d398>] (serial_omap_startup+0x30/0x2dc) from [<c0295a38>] (uart_startup+0x5c/0x1ac)

(etc.)

It turns out that the omap-serial code used one threaded IRQ
handler[1][2] and one non-threaded IRQ handler[3] that shared the same
IRQ.  During the 3.1-rc series, a patch was merged[4] that caused
IRQF_ONESHOT to be set on the threaded handler, but the non-threaded
handler did not have IRQF_ONESHOT set.  Since interrupt handlers
that share the same IRQ must also share the presence or absence of
IRQF_ONESHOT[5], this new commit caused a mismatch that prevented the
non-threaded IRQ from registering.

Fix by converting the non-threaded IRQ handler in omap-serial.c to a
threaded IRQ handler.  Ideally we would not have to make this type of
change during the -rc series, but the commit that caused this behavior
was itself merged between v3.1-rc2 and v3.1-rc3.  In the long term, it
would be good to get rid of the shared IRQ handler hack in
arch/arm/mach-omap2/serial.c.

This change has been boot-tested on OMAP3530 BeagleBoard and
OMAP4430ES2 PandaBoard, and static suspend has been lightly tested on
the BeagleBoard.

Pantelis Antoniou <pantelis.antoniou@gmail.com> originally reported
the boot failure.

1. arch/arm/mach-omap2/serial.c line 550, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-omap2/serial.c;h=466fc722fa0f39f03b8d93cf84e4dae4f57fd029;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l550

2. arch/arm/mach-omap2/serial.c line 563, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-omap2/serial.c;h=466fc722fa0f39f03b8d93cf84e4dae4f57fd029;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l563

3. drivers/tty/serial/omap-serial.c line 464, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/tty/serial/omap-serial.c;h=c37df8d0fa2819261dffccc5bc4d0180b9531f49;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l464

4. Commit f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set
   IRQF_ONESHOT if no primary handler is specified")

5. Gleixner, Thomas. _[patch 2/5] genirq: Allow shared oneshot
   interrupts_.  http://lkml.org/lkml/2011/2/23/511

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Pantelis Antoniou <pantelis.antoniou@gmail.com>
Cc: Govindraj.R <govindraj.raja@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/omap-serial.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index c37df8d..e83a769 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -461,8 +461,8 @@ static int serial_omap_startup(struct uart_port *port)
 	/*
 	 * Allocate the IRQ
 	 */
-	retval = request_irq(up->port.irq, serial_omap_irq, up->port.irqflags,
-				up->name, up);
+	retval = request_threaded_irq(up->port.irq, NULL, serial_omap_irq,
+				      up->port.irqflags, up->name, up);
 	if (retval)
 		return retval;
 
-- 
1.7.5.4

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Sebastian Andrzej Siewior]

* Paul Walmsley | 2011-08-22 23:10:21 [-0600]:

>IRQ handler type mismatch for IRQ 74

>It turns out that the omap-serial code used one threaded IRQ
>handler[1][2] and one non-threaded IRQ handler[3] that shared the same
>IRQ.  During the 3.1-rc series, a patch was merged[4] that caused
>IRQF_ONESHOT to be set on the threaded handler, but the non-threaded
>handler did not have IRQF_ONESHOT set.  Since interrupt handlers

Is the type of IRQ74 edge or line/eoi?
Since you registered the serial handler as threaded and the primary
handler did _not_ disable the irq source (due to the absence of the
IRQF_ONESHOT flag or a custom primary handler which does this by
changing a bit in the hardware) it is more or less luck that you did not
hang forever on the first serial interrupt. Well, it does not happen
with edge typed interrupt lines.

>that share the same IRQ must also share the presence or absence of
>IRQF_ONESHOT[5], this new commit caused a mismatch that prevented the
>non-threaded IRQ from registering.

In theory it should be okay if the handler without ONESHOT is either
primary only or primary + threaded and the primary disables the irq
source.

Sebastian

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Alan Cox]

On Mon, 22 Aug 2011 23:10:21 -0600 (MDT)
Paul Walmsley <paul@pwsan.com> wrote:

> 
> Convert the omap-serial hardirq handler to a threaded IRQ handler. Without 
> this patch, OMAP boards which use the on-board OMAP UARTs and the 
> omap-serial driver will not boot to userspace after commit 
> f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set IRQF_ONESHOT if 
> no primary handler is specified").  Enabling CONFIG_DEBUG_SHIRQ reveals 
> 'IRQ handler type mismatch' errors:

There are multiple other drivers reporting all these problems - the
faulty irq change should be reverted at this point not the drivers
fiddled with.

Alan

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Linus Torvalds]

On Tue, Aug 23, 2011 at 1:57 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Mon, 22 Aug 2011 23:10:21 -0600 (MDT)
> Paul Walmsley <paul@pwsan.com> wrote:
>
>>
>> Convert the omap-serial hardirq handler to a threaded IRQ handler. Without
>> this patch, OMAP boards which use the on-board OMAP UARTs and the
>> omap-serial driver will not boot to userspace after commit
>> f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set IRQF_ONESHOT if
>> no primary handler is specified").  Enabling CONFIG_DEBUG_SHIRQ reveals
>> 'IRQ handler type mismatch' errors:
>
> There are multiple other drivers reporting all these problems - the
> faulty irq change should be reverted at this point not the drivers
> fiddled with.

Agreed. It's too late to try to fix random drivers.

The real issue seems to be that clue:

    "Enabling CONFIG_DEBUG_SHIRQ reveals 'IRQ handler type mismatch' errors"

iow, we have a shared irq, and forcing the IRQF_ONESHOT bit is
resulting in those shared interrupts now having different values of
IRQF_ONESHOT, so this test triggers:

                /*
                 * Can't share interrupts unless both agree to and are
                 * the same type (level, edge, polarity). So both flag
                 * fields must have IRQF_SHARED set and the bits which
                 * set the trigger type must match. Also all must
                 * agree on ONESHOT.
                 */
                if (!((old->flags & new->flags) & IRQF_SHARED) ||
                    ((old->flags ^ new->flags) & IRQF_TRIGGER_MASK) ||
                    ((old->flags ^ new->flags) & IRQF_ONESHOT)) {
                        old_name = old->name;
                        goto mismatch;
                }

and the irq isn't installed at all (setup_irq returns with EBUSY).

So the commit that caused this problem was simply bogus. The commit log says

                       "Since it is required for those users and
    there is no difference for others it makes sense to add this flag
    unconditionally."

but the "there is no difference for others" seems to be total crap,
exactly because it results in this IRQF mismatch.

So I think that commit should just be reverted. Thomas?

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

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Thomas Gleixner]

Linus,

On Tue, 23 Aug 2011, Linus Torvalds wrote:
> but the "there is no difference for others" seems to be total crap,
> exactly because it results in this IRQF mismatch.
> 
> So I think that commit should just be reverted. Thomas?

Yes. I missed that detail when I applied that patch. Reverting it
seems to be the only sensible solution.

Thanks,

	tglx

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Felipe Balbi]

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

Hi,

On Mon, Aug 22, 2011 at 11:10:21PM -0600, Paul Walmsley wrote:
> 
> Convert the omap-serial hardirq handler to a threaded IRQ handler. Without 
> this patch, OMAP boards which use the on-board OMAP UARTs and the 
> omap-serial driver will not boot to userspace after commit 
> f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set IRQF_ONESHOT if 
> no primary handler is specified").  Enabling CONFIG_DEBUG_SHIRQ reveals 
> 'IRQ handler type mismatch' errors:
> 
> IRQ handler type mismatch for IRQ 74
> current handler: serial idle
> [<c001b124>] (unwind_backtrace+0x0/0xf0) from [<c009c900>] (__setup_irq+0x42c/0x464)
> [<c009c900>] (__setup_irq+0x42c/0x464) from [<c009ca08>] (request_threaded_irq+0xd0/0x148)
> [<c009ca08>] (request_threaded_irq+0xd0/0x148) from [<c029d398>] (serial_omap_startup+0x30/0x2dc)
> [<c029d398>] (serial_omap_startup+0x30/0x2dc) from [<c0295a38>] (uart_startup+0x5c/0x1ac)
> 
> (etc.)
> 
> It turns out that the omap-serial code used one threaded IRQ
> handler[1][2] and one non-threaded IRQ handler[3] that shared the same
> IRQ.  During the 3.1-rc series, a patch was merged[4] that caused
> IRQF_ONESHOT to be set on the threaded handler, but the non-threaded
> handler did not have IRQF_ONESHOT set.  Since interrupt handlers
> that share the same IRQ must also share the presence or absence of
> IRQF_ONESHOT[5], this new commit caused a mismatch that prevented the
> non-threaded IRQ from registering.
> 
> Fix by converting the non-threaded IRQ handler in omap-serial.c to a
> threaded IRQ handler.  Ideally we would not have to make this type of
> change during the -rc series, but the commit that caused this behavior
> was itself merged between v3.1-rc2 and v3.1-rc3.  In the long term, it
> would be good to get rid of the shared IRQ handler hack in
> arch/arm/mach-omap2/serial.c.
> 
> This change has been boot-tested on OMAP3530 BeagleBoard and
> OMAP4430ES2 PandaBoard, and static suspend has been lightly tested on
> the BeagleBoard.
> 
> Pantelis Antoniou <pantelis.antoniou@gmail.com> originally reported
> the boot failure.
> 
> 1. arch/arm/mach-omap2/serial.c line 550, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-omap2/serial.c;h=466fc722fa0f39f03b8d93cf84e4dae4f57fd029;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l550
> 
> 2. arch/arm/mach-omap2/serial.c line 563, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=arch/arm/mach-omap2/serial.c;h=466fc722fa0f39f03b8d93cf84e4dae4f57fd029;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l563
> 
> 3. drivers/tty/serial/omap-serial.c line 464, as of Linux v3.1-rc3.  http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/tty/serial/omap-serial.c;h=c37df8d0fa2819261dffccc5bc4d0180b9531f49;hb=fcb8ce5cfe30ca9ca5c9a79cdfe26d1993e65e0c#l464
> 
> 4. Commit f3637a5f2e2eb391ff5757bc83fb5de8f9726464 ("irq: Always set
>    IRQF_ONESHOT if no primary handler is specified")
> 
> 5. Gleixner, Thomas. _[patch 2/5] genirq: Allow shared oneshot
>    interrupts_.  http://lkml.org/lkml/2011/2/23/511
> 
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Pantelis Antoniou <pantelis.antoniou@gmail.com>
> Cc: Govindraj.R <govindraj.raja@ti.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/tty/serial/omap-serial.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index c37df8d..e83a769 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -461,8 +461,8 @@ static int serial_omap_startup(struct uart_port *port)
>  	/*
>  	 * Allocate the IRQ
>  	 */
> -	retval = request_irq(up->port.irq, serial_omap_irq, up->port.irqflags,
> -				up->name, up);
> +	retval = request_threaded_irq(up->port.irq, NULL, serial_omap_irq,
> +				      up->port.irqflags, up->name, up);

if you're not running on a slow bus, you should use top half to check if
IRQs are really from this device.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] tty: omap-serial: fix boot hang by converting to use a threaded IRQ handler (was Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified)

[Paul Walmsley]

On Tue, 23 Aug 2011, Felipe Balbi wrote:

> if you're not running on a slow bus, you should use top half to check if
> IRQs are really from this device.

I don't think this applies in this case, since the IRQ isn't shared 
between different hardware devices - it's shared for the purposes of a 
software hack.


- Paul

Re: [PATCH] irq: always set IRQF_ONESHOT if no primary handler is specified

[Felipe Balbi]

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

Hi,

On Mon, Aug 22, 2011 at 05:45:06PM -0600, Paul Walmsley wrote:
> On Fri, 19 Aug 2011, Felipe Balbi wrote:
> 
> > On Thu, Aug 18, 2011 at 07:22:39PM +0200, Sebastian Andrzej Siewior wrote:
> > > Pantelis Antoniou wrote:
> > > 
> > > >Let me report that with this change Beagle board fails to boot,
> > > >hangs right on rootfs mount.
> > > 
> > > Can you provide some more information about the kind of the failure and
> > > used drivers?
> > > 
> > > >Users of BB should revert this until the offending driver(s) are fixed.
> > > Which is the offending driver?
> > 
> > I would guess it's the lack of threaded IRQ conversion on the
> > twl4030 driver. I have converted it but noone actually picked the series
> > [1].
> > 
> > http://marc.info/?i=1309427470-605-1-git-send-email-balbi@ti.com
> 
> Just a data point: applying this series doesn't seem to fix the hang.

a wild guess which wasn't right :-p

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]