Bug in "genirq: record trigger type" - Kernel

This is a discussion on Bug in "genirq: record trigger type" - Kernel ; On Mon, 2008-10-20 at 22:05 +0000, Linux Kernel Mailing List wrote: > Gitweb: http://git.kernel.org/git/?p=linux/k...afe1398236482b > Commit: 0c5d1eb77a8be917b638344a22afe1398236482b > Parent: d6d5aeb661fc14655c417f3582ae7ec52985d2a8 > Author: David Brownell > AuthorDate: Wed Oct 1 14:46:18 2008 -0700 > Committer: Ingo Molnar > CommitDate: Thu Oct ...

+ Reply to Thread
Results 1 to 8 of 8

Thread: Bug in "genirq: record trigger type"

  1. Bug in "genirq: record trigger type"

    On Mon, 2008-10-20 at 22:05 +0000, Linux Kernel Mailing List wrote:
    > Gitweb: http://git.kernel.org/git/?p=linux/k...afe1398236482b
    > Commit: 0c5d1eb77a8be917b638344a22afe1398236482b
    > Parent: d6d5aeb661fc14655c417f3582ae7ec52985d2a8
    > Author: David Brownell
    > AuthorDate: Wed Oct 1 14:46:18 2008 -0700
    > Committer: Ingo Molnar
    > CommitDate: Thu Oct 2 10:24:09 2008 +0200


    This one is obviously broken and breaks booting on a whole bunch of
    machines (including powermac's and thus my G5, it's never good when my
    own machine breaks !).

    Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)

    > desc = irq_desc + irq;
    > - if (desc->chip->set_type) {
    > - spin_lock_irqsave(&desc->lock, flags);
    > - ret = desc->chip->set_type(irq, type);
    > - spin_unlock_irqrestore(&desc->lock, flags);
    > - }
    > + if (type == IRQ_TYPE_NONE)
    > + return 0;
    > +
    > + spin_lock_irqsave(&desc->lock, flags);
    > + ret = __irq_set_trigger(desc, irq, flags);

    ^^^^ type maybe ?

    > + spin_unlock_irqrestore(&desc->lock, flags);
    > return ret;
    > }


    I have to run so no patch until tomorrow unless somebody beats me to it.

    Cheers,
    Ben.


    --
    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/

  2. Re: Bug in "genirq: record trigger type"


    * Yinghai Lu wrote:

    > On Mon, Oct 20, 2008 at 11:32 PM, Benjamin Herrenschmidt
    > wrote:
    > > On Mon, 2008-10-20 at 22:05 +0000, Linux Kernel Mailing List wrote:
    > >> Gitweb: http://git.kernel.org/git/?p=linux/k...afe1398236482b
    > >> Commit: 0c5d1eb77a8be917b638344a22afe1398236482b
    > >> Parent: d6d5aeb661fc14655c417f3582ae7ec52985d2a8
    > >> Author: David Brownell
    > >> AuthorDate: Wed Oct 1 14:46:18 2008 -0700
    > >> Committer: Ingo Molnar
    > >> CommitDate: Thu Oct 2 10:24:09 2008 +0200

    > >
    > > This one is obviously broken and breaks booting on a whole bunch of
    > > machines (including powermac's and thus my G5, it's never good when my
    > > own machine breaks !).
    > >
    > > Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)
    > >
    > >> desc = irq_desc + irq;
    > >> - if (desc->chip->set_type) {
    > >> - spin_lock_irqsave(&desc->lock, flags);
    > >> - ret = desc->chip->set_type(irq, type);
    > >> - spin_unlock_irqrestore(&desc->lock, flags);
    > >> - }
    > >> + if (type == IRQ_TYPE_NONE)
    > >> + return 0;
    > >> +
    > >> + spin_lock_irqsave(&desc->lock, flags);
    > >> + ret = __irq_set_trigger(desc, irq, flags);

    > > ^^^^ type maybe ?
    > >
    > >> + spin_unlock_irqrestore(&desc->lock, flags);
    > >> return ret;
    > >> }

    > >
    > > I have to run so no patch until tomorrow unless somebody beats me to it.

    >
    > there is patch about it, but somehow get lost.


    should be all sorted now, the fix is below.

    Ingo

    ------------------>
    From aac4ddd11a8d0e402ddc3fbc75204cb64efa0aac Mon Sep 17 00:00:00 2001
    From: Chris Friesen
    Date: Mon, 20 Oct 2008 12:41:58 -0600
    Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type

    In set_irq_type() we want to pass the type rather than the current
    interrupt state.

    Signed-off-by: Chris Friesen
    Signed-off-by: Ingo Molnar
    ---
    kernel/irq/chip.c | 2 +-
    1 files changed, 1 insertions(+), 1 deletions(-)

    diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
    index 4895fde..3de6ea3 100644
    --- a/kernel/irq/chip.c
    +++ b/kernel/irq/chip.c
    @@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
    return 0;

    spin_lock_irqsave(&desc->lock, flags);
    - ret = __irq_set_trigger(desc, irq, flags);
    + ret = __irq_set_trigger(desc, irq, type);
    spin_unlock_irqrestore(&desc->lock, flags);
    return ret;
    }
    --
    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/

  3. [PATCH] genirq: fix set_irq_type() when recording trigger type


    * Benjamin Herrenschmidt wrote:

    > On Mon, 2008-10-20 at 22:05 +0000, Linux Kernel Mailing List wrote:
    > > Gitweb: http://git.kernel.org/git/?p=linux/k...afe1398236482b
    > > Commit: 0c5d1eb77a8be917b638344a22afe1398236482b
    > > Parent: d6d5aeb661fc14655c417f3582ae7ec52985d2a8
    > > Author: David Brownell
    > > AuthorDate: Wed Oct 1 14:46:18 2008 -0700
    > > Committer: Ingo Molnar
    > > CommitDate: Thu Oct 2 10:24:09 2008 +0200

    [...]
    > > Signed-off-by: David Brownell
    > > Signed-off-by: Andrew Morton
    > > Acked-by: Thomas Gleixner
    > > Signed-off-by: Ingo Molnar

    >
    > This one is obviously broken and breaks booting on a whole bunch of
    > machines (including powermac's and thus my G5, it's never good when my
    > own machine breaks !).
    >
    > Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)


    that patch came from -mm towards us shortly before the merge window.
    Once we had it integrated Chris Friesen reported a boot hang on a G5,
    and there's a fix pending for the bug, see it below.

    Will send it to Linus expressly. Fortunately only a minority of Linux
    users will ever run a box that uses set_irq_type() - but yes it needs to
    be fixed.

    Ingo

    ------------------------------>
    From aac4ddd11a8d0e402ddc3fbc75204cb64efa0aac Mon Sep 17 00:00:00 2001
    From: Chris Friesen
    Date: Mon, 20 Oct 2008 12:41:58 -0600
    Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type

    In set_irq_type() we want to pass the type rather than the current
    interrupt state.

    Signed-off-by: Chris Friesen
    Signed-off-by: Ingo Molnar
    ---
    kernel/irq/chip.c | 2 +-
    1 files changed, 1 insertions(+), 1 deletions(-)

    diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
    index 4895fde..3de6ea3 100644
    --- a/kernel/irq/chip.c
    +++ b/kernel/irq/chip.c
    @@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
    return 0;

    spin_lock_irqsave(&desc->lock, flags);
    - ret = __irq_set_trigger(desc, irq, flags);
    + ret = __irq_set_trigger(desc, irq, type);
    spin_unlock_irqrestore(&desc->lock, flags);
    return ret;
    }
    --
    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/

  4. Re: Bug in "genirq: record trigger type"

    On Mon, Oct 20, 2008 at 11:32 PM, Benjamin Herrenschmidt
    wrote:
    > On Mon, 2008-10-20 at 22:05 +0000, Linux Kernel Mailing List wrote:
    >> Gitweb: http://git.kernel.org/git/?p=linux/k...afe1398236482b
    >> Commit: 0c5d1eb77a8be917b638344a22afe1398236482b
    >> Parent: d6d5aeb661fc14655c417f3582ae7ec52985d2a8
    >> Author: David Brownell
    >> AuthorDate: Wed Oct 1 14:46:18 2008 -0700
    >> Committer: Ingo Molnar
    >> CommitDate: Thu Oct 2 10:24:09 2008 +0200

    >
    > This one is obviously broken and breaks booting on a whole bunch of
    > machines (including powermac's and thus my G5, it's never good when my
    > own machine breaks !).
    >
    > Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)
    >
    >> desc = irq_desc + irq;
    >> - if (desc->chip->set_type) {
    >> - spin_lock_irqsave(&desc->lock, flags);
    >> - ret = desc->chip->set_type(irq, type);
    >> - spin_unlock_irqrestore(&desc->lock, flags);
    >> - }
    >> + if (type == IRQ_TYPE_NONE)
    >> + return 0;
    >> +
    >> + spin_lock_irqsave(&desc->lock, flags);
    >> + ret = __irq_set_trigger(desc, irq, flags);

    > ^^^^ type maybe ?
    >
    >> + spin_unlock_irqrestore(&desc->lock, flags);
    >> return ret;
    >> }

    >
    > I have to run so no patch until tomorrow unless somebody beats me to it.


    there is patch about it, but somehow get lost.

    YH
    --
    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/

  5. Re: Bug in "genirq: record trigger type"

    On Tue, 2008-10-21 at 09:28 +0200, Ingo Molnar wrote:

    > From: Chris Friesen
    > Date: Mon, 20 Oct 2008 12:41:58 -0600
    > Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type
    >
    > In set_irq_type() we want to pass the type rather than the current
    > interrupt state.
    >
    > Signed-off-by: Chris Friesen
    > Signed-off-by: Ingo Molnar


    Acked-by: Benjamin Herrenschmidt

    > ---
    > kernel/irq/chip.c | 2 +-
    > 1 files changed, 1 insertions(+), 1 deletions(-)
    >
    > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
    > index 4895fde..3de6ea3 100644
    > --- a/kernel/irq/chip.c
    > +++ b/kernel/irq/chip.c
    > @@ -127,7 +127,7 @@ int set_irq_type(unsigned int irq, unsigned int type)
    > return 0;
    >
    > spin_lock_irqsave(&desc->lock, flags);
    > - ret = __irq_set_trigger(desc, irq, flags);
    > + ret = __irq_set_trigger(desc, irq, type);
    > spin_unlock_irqrestore(&desc->lock, flags);
    > return ret;
    > }


    --
    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/

  6. Re: Bug in "genirq: record trigger type"


    * Benjamin Herrenschmidt wrote:

    > On Tue, 2008-10-21 at 09:28 +0200, Ingo Molnar wrote:
    >
    > > From: Chris Friesen
    > > Date: Mon, 20 Oct 2008 12:41:58 -0600
    > > Subject: [PATCH] genirq: fix set_irq_type() when recording trigger type
    > >
    > > In set_irq_type() we want to pass the type rather than the current
    > > interrupt state.
    > >
    > > Signed-off-by: Chris Friesen
    > > Signed-off-by: Ingo Molnar

    >
    > Acked-by: Benjamin Herrenschmidt


    thx, added your ack to the commit as well.

    Ingo
    --
    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/

  7. Re: Bug in "genirq: record trigger type"

    On Monday 20 October 2008, Benjamin Herrenschmidt wrote:
    > This one is obviously broken and breaks booting on a whole bunch of
    > machines (including powermac's and thus my G5, it's never good when my
    > own machine breaks !).
    >
    > Nice to see 3 SOB's and one Ack and nobody caught the obvious bug :-)


    As you saw, that one's fixed. Chris' patch unfortunately didn't
    get integrated right away.


    I'm a bit more curious about another potential issue though ... as
    described in the patch comment:

    - Make set_irq_type() usage match request_irq() usage:
    * IRQ_TYPE_NONE should be a NOP; succeed, so irq_chip methods
    won't have to handle that case any more (many do it wrong).

    It might be a bit more accurate to say irq_chip.set_type() methods
    are *inconsistent* in handling IRQ_TYPE_NONE. Previously the
    set_irq_type() method would pass that down to irq_chip code.

    I had observed two behaviors, but I thought I observed a third one
    in some of the PowerPC code:

    (1) ignore it ... matching request_irq() usage
    (2) return an error ... nasty
    (3) assign some irq_chip-specific trigger mode

    That third behavior might cause a bit of trouble, but I think
    it was only used during platform init. Someone more attuned
    to PowerPC might want to check ...

    - Dave

    --
    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/

  8. Re: Bug in "genirq: record trigger type"

    On Tue, 2008-10-21 at 01:01 -0700, David Brownell wrote:
    >
    > I'm a bit more curious about another potential issue though ... as
    > described in the patch comment:
    >
    > - Make set_irq_type() usage match request_irq() usage:
    > * IRQ_TYPE_NONE should be a NOP; succeed, so irq_chip methods
    > won't have to handle that case any more (many do it wrong).
    >
    > It might be a bit more accurate to say irq_chip.set_type() methods
    > are *inconsistent* in handling IRQ_TYPE_NONE. Previously the
    > set_irq_type() method would pass that down to irq_chip code.
    >
    > I had observed two behaviors, but I thought I observed a third one
    > in some of the PowerPC code:
    >
    > (1) ignore it ... matching request_irq() usage
    > (2) return an error ... nasty
    > (3) assign some irq_chip-specific trigger mode
    >
    > That third behavior might cause a bit of trouble, but I think
    > it was only used during platform init. Someone more attuned
    > to PowerPC might want to check ...


    Ok, I wrote a lot of the port of powerpc stuff to genirq so I supposed
    I'm the person to do that sweep :-) I'll have a look tomorrow.

    Thanks for the heads up,

    Cheers,
    Ben.


    --
    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/

+ Reply to Thread