Messages in this thread |  | | | Date | Wed, 2 Jul 2008 02:49:26 -0700 | | From | Andrew Morton <> | | Subject | Re: [PATCH] handle failure of irqchip->set_type in setup_irq |
| |
On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> wrote:
> Hello, > > [extending the Cc: list] > > Uwe Kleine-K__nig wrote: > > set_type returns an int but currently setup_irq ignores that. > > > > To save me from undoing some changes setting the IRQ_NO_BALANCING bit in > > desc->flags, setting IRQ_PER_CPU in desc->status and adding the new > > action to desc is only done after set_type succeeded. > > > > Signed-off-by: Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> > > --- > > Hello, > > > > in my case set_type returns an error because gpio-key tries to use > > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING and my irq-chip can only do > > one of these. > > up to now I didn't get any response for this patch. It addresses a real > problem for my platform so I really like to have a fix in. > > In mmotm of 2008-07-01-21-57 kernel/irq/manage.c is touched[1], > too, but git was able to automerge the change correctly when applying it > on top of mmotm. > > For your convenience you can find the patch again at the end of this > mail. > > Andrew: Do you can take this patch into mm? >
From a brief squint the patch seems to be reasonable. But the changelog is a bit mangled. Perhaps you could have another go when resending it. Explan more clearly under what circumststances your ->set_type() implementation can fail and why you require the core code to handle this.
Perhaps we want a dump_stack() on the error path so we can see who goofed. Or a print_symbol() of desc->chip->set_type. Or perhaps not.
Did you check that all the current ->set_type() implementations are returning zero?
> > [1] kernel/irq/manage.c is modified by linux-next patch. The relevant > commit is: > 1840475... genirq: Expose default irq affinity mask (take 3) > > > > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > > index 46d6611..bc990a1 100644 > > --- a/kernel/irq/manage.c > > +++ b/kernel/irq/manage.c > > @@ -281,6 +281,7 @@ int setup_irq(unsigned int irq, struct irqaction *new) > > const char *old_name = NULL; > > unsigned long flags; > > int shared = 0; > > + int ret; > > > > if (irq >= NR_IRQS) > > return -EINVAL; > > @@ -338,26 +339,23 @@ int setup_irq(unsigned int irq, struct irqaction *new) > > shared = 1; > > } > > > > - *p = new; > > - > > - /* Exclude IRQ from balancing */ > > - if (new->flags & IRQF_NOBALANCING) > > - desc->status |= IRQ_NO_BALANCING; > > - > > if (!shared) { > > irq_chip_set_defaults(desc->chip); > > > > -#if defined(CONFIG_IRQ_PER_CPU) > > - if (new->flags & IRQF_PERCPU) > > - desc->status |= IRQ_PER_CPU; > > -#endif > > - > > /* Setup the type (level, edge polarity) if configured: */ > > if (new->flags & IRQF_TRIGGER_MASK) { > > - if (desc->chip && desc->chip->set_type) > > - desc->chip->set_type(irq, > > + if (desc->chip && desc->chip->set_type) { > > + ret = desc->chip->set_type(irq, > > new->flags & IRQF_TRIGGER_MASK); > > - else > > + if (ret) { > > + pr_err("setting flow type for irq %u " > > + "failed\n", irq); > > + spin_unlock_irqrestore(&desc->lock, > > + flags); > > + return ret; > > + } > > + > > + } else > > /* > > * IRQF_TRIGGER_* but the PIC does not support > > * multiple flow-types? > > @@ -369,6 +367,11 @@ int setup_irq(unsigned int irq, struct irqaction *new) > > } else > > compat_irq_chip_set_default_handler(desc); > > > > +#if defined(CONFIG_IRQ_PER_CPU) > > + if (new->flags & IRQF_PERCPU) > > + desc->status |= IRQ_PER_CPU; > > +#endif > > + > > desc->status &= ~(IRQ_AUTODETECT | IRQ_WAITING | > > IRQ_INPROGRESS | IRQ_SPURIOUS_DISABLED); > > > > @@ -383,6 +386,13 @@ int setup_irq(unsigned int irq, struct irqaction *new) > > /* Undo nested disables: */ > > desc->depth = 1; > > } > > + > > + *p = new; > > + > > + /* Exclude IRQ from balancing */ > > + if (new->flags & IRQF_NOBALANCING) > > + desc->status |= IRQ_NO_BALANCING; > > + > > /* Reset broken irq detection when installing new handler */ > > desc->irq_count = 0; > > desc->irqs_unhandled = 0;
|  |