Messages in this thread |  | | | Date | Fri, 4 Jul 2008 20:43:07 +0200 | | From | Uwe Kleine-König <> | | Subject | Re: [PATCH v2] handle failure of irqchip->set_type in setup_irq |
| |
Hello,
Andrew Morton wrote: > On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig <Uwe.Kleine-Koenig@digi.com> wrote: > > > set_type returns an int indicating success or failure, but up to now > > setup_irq ignores that. > > > > In my case this resulted in a machine hang: > > gpio-keys requested IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, but > > arm/ns9xxx can only trigger on one direction so set_type didn't touch > > the configuration which happens do default to a level sensitiveness and > > returned -EINVAL. setup_irq ignored that and unmasked the irq. This > > resulted in an endless triggering of the gpio-key interrupt service > > routine which effectively killed the machine. > > > > With this patch applied setup_irq propagates the error to the caller. > > > > Note that before in the case > > > > chip && !chip->set_type && !chip->name > > > > a NULL pointer was feed to printk. This is fixed, too. > > > > hm, OK. Do I recall there being some urgency to this? No, I didn't mention this before. (Fixing NULL-Pointers feed to printk is a bit of a reflex for me as I used to program for Solaris and if you feed a NULL-Pointer to printf there your program segfaults.) I didn't check that recently, but IIRC it's no problem here.
> > > + if (ret) { > > + char buf[100]; > > + > > + snprintf(buf, sizeof(buf), KERN_ERR > > + "setting flow type for irq %u failed (%%s)\n", > > + irq); > > + print_fn_descriptor_symbol(buf, chip->set_type); > > + } > > eww. We really mucked up that interface. That's what I thought, too. ;-) This was the nicest way I found to print the whole line in one go.
> I wonder if we can do better. > Let me think about that. I was about to suggest something like:
/* WARNING: this returns a static pointer, so you cannot use the * returned value after another call to creative_function_name */ char *creative_function_name(void *addr) { static char buf[SOME_LENGTH]; ... format symbol name into buf ...
return buf; } Then I could have used
pr_err("setting flow type for irq %u failed (%s)\n", irq, creative_function_name(chip->set_type));
which looks definitely nicer.
Thanks for taking the patch.
Best regards Uwe
-- Uwe Kleine-König, Software Engineer Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962 -- 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/
|  |