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 <[email protected]>
---
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.
Best regards
Uwe
kernel/irq/manage.c | 38 ++++++++++++++++++++++++--------------
1 files changed, 24 insertions(+), 14 deletions(-)
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;
--
1.5.6
--
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
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 <[email protected]>
> ---
> 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?
Best regards
Uwe
[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;
--
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
On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig <[email protected]> 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 <[email protected]>
> > ---
> > 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;
Hello Andrew,
Andrew Morton wrote:
> On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
OK.
> 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.
I thought about a dump_stack() but considered it as too much.
print_symbol() is a nice idea though.
> Did you check that all the current ->set_type() implementations are
> returning zero?
No, I don't. But for example orion5x_gpio_set_irq_type might return
-EINVAL. txx9_irq_set_type seems to have the same limitation as ns9xxx
and returns -EINVAL if more than one trigger type is requested.
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
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.
Signed-off-by: Uwe Kleine-K?nig <[email protected]>
---
Hello,
Changes since initial post:
- improve commit log (hopefully)
- move code to a dedicated function to improve readability and code
line length.
- include the symbolic name of the failing callback in the error
message.
Best regards
Uwe
kernel/irq/manage.c | 72 ++++++++++++++++++++++++++++++++++----------------
1 files changed, 49 insertions(+), 23 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 46d6611..178b966 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -270,6 +270,35 @@ void compat_irq_chip_set_default_handler(struct irq_desc *desc)
desc->handle_irq = NULL;
}
+static int __irq_set_trigger(struct irq_chip *chip, unsigned int irq,
+ unsigned long flags)
+{
+ int ret;
+
+ if (!chip || !chip->set_type) {
+ /*
+ * IRQF_TRIGGER_* but the PIC does not support multiple
+ * flow-types?
+ */
+ pr_warning("No set_type function for IRQ %d (%s)\n", irq,
+ chip ? (chip->name ? : "unknown") : "unknown");
+ return 0;
+ }
+
+ ret = chip->set_type(irq, flags & IRQF_TRIGGER_MASK);
+
+ 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);
+ }
+
+ return ret;
+}
+
/*
* Internal function to register an irqaction - typically used to
* allocate special interrupts that are part of the architecture.
@@ -281,6 +310,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,37 +368,26 @@ 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,
- new->flags & IRQF_TRIGGER_MASK);
- else
- /*
- * IRQF_TRIGGER_* but the PIC does not support
- * multiple flow-types?
- */
- printk(KERN_WARNING "No IRQF_TRIGGER set_type "
- "function for IRQ %d (%s)\n", irq,
- desc->chip ? desc->chip->name :
- "unknown");
+ ret = __irq_set_trigger(desc->chip, irq, new->flags);
+
+ if (ret) {
+ spin_unlock_irqrestore(&desc->lock, flags);
+ return ret;
+ }
+
} 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 +402,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;
--
1.5.6
On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig <[email protected]> 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?
> + 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. I wonder if we can do better.
Let me think about that.
Hello,
Andrew Morton wrote:
> On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig <[email protected]> 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
On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <[email protected]> wrote:
> > 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.
Better would be
char buf[ENOUGH /* rofl */];
pr_err("setting flow type for irq %u failed (%s)\n",
irq, render_function_name(buf, chip->set_type));
However I'm presently brewing up a plot to do this:
printk("function name is %Ss\n", (unsigned long *)chip->set_type);
which I think will work. We can also do
printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);
to replace NIPQUAD. And similar printk extensions.
Am still thinking about it though.
Andrew Morton wrote:
> On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <[email protected]> wrote:
>
> > > 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.
>
> Better would be
>
> char buf[ENOUGH /* rofl */];
>
> pr_err("setting flow type for irq %u failed (%s)\n",
> irq, render_function_name(buf, chip->set_type));
>
> However I'm presently brewing up a plot to do this:
>
> printk("function name is %Ss\n", (unsigned long *)chip->set_type);
>
> which I think will work. We can also do
>
> printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);
>
> to replace NIPQUAD. And similar printk extensions.
>
> Am still thinking about it though.
I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
Should I send a new version using %pF or is it easier if you fix up 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
On Wed, 9 Jul 2008 15:13:53 +0200 Uwe Kleine-K__nig <[email protected]> wrote:
> Andrew Morton wrote:
> > On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig <[email protected]> wrote:
> >
> > > > 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.
> >
> > Better would be
> >
> > char buf[ENOUGH /* rofl */];
> >
> > pr_err("setting flow type for irq %u failed (%s)\n",
> > irq, render_function_name(buf, chip->set_type));
> >
> > However I'm presently brewing up a plot to do this:
> >
> > printk("function name is %Ss\n", (unsigned long *)chip->set_type);
> >
> > which I think will work. We can also do
> >
> > printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);
> >
> > to replace NIPQUAD. And similar printk extensions.
> >
> > Am still thinking about it though.
> I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
yup.
> Should I send a new version using %pF or is it easier if you fix up the
> patch?
A new version would be nice, thanks. Or an incremental diff, but I can and
will turn new versions into an incremental in the twinkle of an eye, so
either is OK.
Hello,
Andrew Morton wrote:
> > > However I'm presently brewing up a plot to do this:
> > >
> > > printk("function name is %Ss\n", (unsigned long *)chip->set_type);
> > >
> > > which I think will work. We can also do
> > >
> > > printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);
> > >
> > > to replace NIPQUAD. And similar printk extensions.
> > >
> > > Am still thinking about it though.
> > I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
>
> yup.
When I saw this commit I wondered that it was you who told me to think
about this but the commiter was Linus.
> > Should I send a new version using %pF or is it easier if you fix up the
> > patch?
>
> A new version would be nice, thanks. Or an incremental diff, but I can and
> will turn new versions into an incremental in the twinkle of an eye, so
> either is OK.
voil?:
--->8---
>From c991997725fdce93352aab53ccab34f41b5afd52 Mon Sep 17 00:00:00 2001
From: Uwe Kleine-K?nig <[email protected]>
Date: Thu, 10 Jul 2008 10:03:59 +0200
Subject: [PATCH] __irq_set_trigger: use %pF to print the set_type callback's name
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
This uses the new feature of printk introduced in
0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2.
The actual output is unchanged.
Signed-off-by: Uwe Kleine-K?nig <[email protected]>
---
kernel/irq/manage.c | 11 +++--------
1 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 178b966..e01ad8e 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -287,14 +287,9 @@ static int __irq_set_trigger(struct irq_chip *chip, unsigned int irq,
ret = chip->set_type(irq, flags & IRQF_TRIGGER_MASK);
- 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);
- }
+ if (ret)
+ pr_err("setting flow type for irq %u failed (%pF)\n",
+ irq, chip->set_type);
return ret;
}
--
1.5.6
--
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
On Thu, 10 Jul 2008 10:23:36 +0200 Uwe Kleine-K__nig <[email protected]> wrote:
> Hello,
>
> Andrew Morton wrote:
> > > > However I'm presently brewing up a plot to do this:
> > > >
> > > > printk("function name is %Ss\n", (unsigned long *)chip->set_type);
> > > >
> > > > which I think will work. We can also do
> > > >
> > > > printk("IP address is %Si\n", (unsigned long *)ip_address_buffer);
> > > >
> > > > to replace NIPQUAD. And similar printk extensions.
> > > >
> > > > Am still thinking about it though.
> > > I assume the result is 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2?
> >
> > yup.
> When I saw this commit I wondered that it was you who told me to think
> about this but the commiter was Linus.
A long story. I rubbed a magic lantern ;)
> > > Should I send a new version using %pF or is it easier if you fix up the
> > > patch?
> >
> > A new version would be nice, thanks. Or an incremental diff, but I can and
> > will turn new versions into an incremental in the twinkle of an eye, so
> > either is OK.
> voila:
Thanks.