handle failure of irqchip->set_type in setup_irq

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.

kernel/irq/manage.c +24 -14

index 46d6611..bc990a1 100644
@@ -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,

[extending the Cc: list]

Quoted 6 lines · Uwe Kleine-K__nig

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.

Quoted 7 lines

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

[1] kernel/irq/manage.c is modified by linux-next patch. The relevant commit is:
1840475... genirq: Expose default irq affinity mask (take 3)

Quoted 76 lines

kernel/irq/manage.c +24 -14

index 46d6611..bc990a1 100644
@@ -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;

Quoted 29 lines · On Wed, 2 Jul 2008 11:17:57 +0200 Uwe Kleine-K__nig…

Hello,

[extending the Cc: list]

Quoted 6 lines · Uwe Kleine-K__nig

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.

Quoted 7 lines

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.

Quoted 4 lines

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?

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.

Quoted 13 lines · On Fri, 4 Jul 2008 12:46:34 +0200 Uwe Kleine-K__nig…

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.

Quoted 6 lines

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?

kernel/irq/manage.c void compat_irq_chip_set_default_handler(struct irq_desc *desc) · line 297

Quoted 9 lines

kernel/irq/manage.c +8 -0

+     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.

Quoted 2 lines

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.

Quoted 16 lines · On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig…

Quoted 2 lines

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;
      }

Quoted 6 lines

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.

Quoted 36 lines · Andrew Morton

Quoted 16 lines · On Fri, 4 Jul 2008 20:43:07 +0200 Uwe Kleine-K__nig…

Quoted 2 lines

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;
      }

Quoted 6 lines

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);

Quoted 7 lines

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?

Quoted 6 lines

Quoted 6 lines

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

Quoted 6 lines

Quoted 5 lines

        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.

Quoted 2 lines

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.

From c991997725fdce93352aab53ccab34f41b5afd52 Mon Sep 17 00:00:00 2001

Hello,

Quoted 8 lines · Andrew Morton

Quoted 8 lines

Quoted 8 lines

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);

Quoted 6 lines

Quoted 4 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 2 lines

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:

so I think we could easily just say that we extend %p in various ways:

- %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of pointers.

Quoted 7 lines · Linus Torvalds

so I think we could easily just say that we extend %p in various ways:

- %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of pointers.

So here's a totally untested example patch of this, which could probably easily be extended to to other things.

I actually made it '%pF' and '%pS' for a Function descriptor pointer and normal Symbolic pointer respectively, because of the stupid things ia64 and PPC64 do with the pointer indirection through function descriptors.

That function descriptor indirection is totally untested, and I did it with a

	pagefault_disable();
	__get_user(..)
	pagefault_enable();

thing because I thought it would be nice if printk() was always safe, and passing bogus function pointers to '%pF' should try to work, but quite frankly, I didn't even check that that part compiles, much less works.

ia64/ppc lists cc'd, just in case somebody wants to test this.

NOTE! There are no current actual users of this, but the _idea_ is that we should be able to just do

	printk("%pF\n", desc->handle_irq);

instead of using

	print_symbol("%s\n", (unsigned long)desc->handle_irq);

The latter is from kernel/irq/internals.h, and actually looks incorrect - shouldn't it use print_fn_descriptor_symbol(), since it's a C level function pointer? We should use "print_symbol()" for return pointers we find on the stack and data pointers, but not for stuff that actually has a C type that is a function pointer?

Somebody who cares about the insane function descriptors should take a deeper look.

NOTE AGAIN! UNTESTED! I could easily have screwed up printk() _entirely_, since I had to factor out the string handling into a function of its own.

Linus

---
 lib/vsprintf.c |  102 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 28 deletions(-)

lib/vsprintf.c +1 -0

index 6021757..148b656 100644
@@ -22,6 +22,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>

lib/vsprintf.c +48 -0

+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	char sym[KSYM_SYMBOL_LEN];
+	switch (*fmt) {
+	case 'F':	/* Function pointer */
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+		{ void *p;
+		pagefault_disable();
+		if (!__get_user((void **)ptr, &p))
+			ptr = p;
+		pagefault_enable();
+		}
+#endif
+	/* Fallthrough */
+	case 'S':	/* Other pointer */

probe_kernel_address() should be usable here.

lib/vsprintf.c static char *number(char *buf, char *end, unsigned long long num, int base, int · line 491

Quoted 6 lines

lib/vsprintf.c +6 -0

+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";

hm, is that needed for other reasons than "it will fault"?

otherwise we could walk the whole string with probe_kernel_address() before we do anything with it.

If this takes off we might want a register-your-printk-handler interface. Maybe.

We also jump through hoops to print things like sector_t and resource_size_t. They always need to be cast to `unsiged long long', which generates additional stack space and text in some setups.

And then there's the perennial "need to cast u64 to unsigned long long to print it". If we were to do

	printk("%SL", (void *)some_u64);

then that's still bloody ugly, but it'll save a little text-n-stack.

Quoted 6 lines · On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds…

Quoted 6 lines

so I think we could easily just say that we extend %p in various ways:

- %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of pointers.

If this takes off we might want a register-your-printk-handler interface. Maybe.

We also jump through hoops to print things like sector_t and resource_size_t. They always need to be cast to `unsiged long long', which generates additional stack space and text in some setups.

And then there's the perennial "need to cast u64 to unsigned long long to print it". If we were to do

	printk("%SL", (void *)some_u64);

then that's still bloody ugly, but it'll save a little text-n-stack.

Quoted 11 lines · Andrew Morton

Quoted 6 lines · On Fri, 4 Jul 2008 13:02:05 -0700 (PDT) Linus Torvalds…

Quoted 6 lines

so I think we could easily just say that we extend %p in various ways:

- %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of pointers.

If this takes off we might want a register-your-printk-handler interface. Maybe.

Quoted 3 lines

We also jump through hoops to print things like sector_t and resource_size_t. They always need to be cast to `unsiged long long', which generates additional stack space and text in some setups.

The thing is that GCC checks types. So it's fine to add "print this pointer specially", but you can't in general add new printf arguments without also hacking GCC. Unless you use -Wno-format, and require sparse to check special kernel types.

Quoted 6 lines

And then there's the perennial "need to cast u64 to unsigned long long to print it". If we were to do

	printk("%SL", (void *)some_u64);

then that's still bloody ugly, but it'll save a little text-n-stack.

u64 is easy to fix, and I don't know why we haven't. Just make it unsigned long long on all architectures.

(heck, let's cc lkml - avoid having to go through all this again)

It would be excellent if gcc had an extension system so that you could add new printf control chars and maybe even tell gcc how to check them. But of course, if that were to happen, we couldn't use it for 4-5 years.

What I had initially proposed was to abuse %S, which takes a wchar_t*. gcc accepts `unsigned long *' for %S.

- there's a cast, so you could accidentally do

	printk("here is an inode: %Si\n", (unsigned long *)dentry);

- there's a cast, and they're ugly

- gcc cannot of course check that the arg matches the control string

Unfortunately (and this seems weird), gcc printf checking will not accept a void* for %S: it _has_ to be wchar_t*, and the checker won't permit void* substitution for that.

Anyway, Linus took all that and said "let's abuse %p instead". It _will_ accept any pointer so we can instead do:

	printk("here is an inode: %pi\n", inode);

which is nicer.

Quoted 2 lines

u64 is easy to fix, and I don't know why we haven't. Just make it unsigned long long on all architectures.

Yeah. Why don't we do that?

[PATCH] Make u64 long long on all architectures

It is currently awkward to print a u64 type. Some architectures use unsigned long while others use unsigned long long. Since unsigned long long is 64-bit for all existing Linux architectures, change those that use long to use long long. Note that this applies only within the kernel. If u64 is being used in a C++ method definition, the symbol mangling would change.

Signed-off-by Matthew Wilcox <[email protected]>

include/asm-generic/int-l64.h +0 -0

index 2af9b75..32f07bd 100644
@@ -23,8 +23,13 @@ typedef unsigned short __u16;
 typedef __signed__ int __s32;
 typedef unsigned int __u32;

Quoted 18 lines · On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <[email protected]> wrote:

[PATCH] Make u64 long long on all architectures

It is currently awkward to print a u64 type. Some architectures use unsigned long while others use unsigned long long. Since unsigned long long is 64-bit for all existing Linux architectures, change those that use long to use long long. Note that this applies only within the kernel. If u64 is being used in a C++ method definition, the symbol mangling would change.

Signed-off-by Matthew Wilcox <[email protected]>

include/asm-generic/int-l64.h +0 -0

index 2af9b75..32f07bd 100644
@@ -23,8 +23,13 @@ typedef unsigned short __u16;
 typedef __signed__ int __s32;
 typedef unsigned int __u32;

include/asm-generic/int-l64.h typedef unsigned short __u16; · line 34

Quoted 10 lines

include/asm-generic/int-l64.h +5 -0

 
+#ifdef __KERNEL__
+typedef __signed__ long long __s64;
+typedef unsigned long long __u64;
+#else
 typedef __signed__ long __s64;
 typedef unsigned long __u64;
+#endif
 
 #endif /* __ASSEMBLY__ */

This is (IMO) a desirable change and will prevent a heck of a lot of goofing around, and will permit a lot of prior goofing around to be removed.

But I bet there are lots of instalces of printk("%l", some_u64) down in arch code where the type of u64 _is_ known which will now spew warnings.

Oh well.

Quoted 38 lines · Andrew Morton

Quoted 18 lines · On Fri, 4 Jul 2008 20:03:51 -0600 Matthew Wilcox <[email protected]> wrote:

[PATCH] Make u64 long long on all architectures

It is currently awkward to print a u64 type. Some architectures use unsigned long while others use unsigned long long. Since unsigned long long is 64-bit for all existing Linux architectures, change those that use long to use long long. Note that this applies only within the kernel. If u64 is being used in a C++ method definition, the symbol mangling would change.

Signed-off-by Matthew Wilcox <[email protected]>

include/asm-generic/int-l64.h +0 -0

index 2af9b75..32f07bd 100644
@@ -23,8 +23,13 @@ typedef unsigned short __u16;
 typedef __signed__ int __s32;
 typedef unsigned int __u32;

include/asm-generic/int-l64.h typedef unsigned short __u16; · line 34

Quoted 10 lines

include/asm-generic/int-l64.h +5 -0

 
+#ifdef __KERNEL__
+typedef __signed__ long long __s64;
+typedef unsigned long long __u64;
+#else
 typedef __signed__ long __s64;
 typedef unsigned long __u64;
+#endif
 
 #endif /* __ASSEMBLY__ */

This is (IMO) a desirable change and will prevent a heck of a lot of goofing around, and will permit a lot of prior goofing around to be removed.

But I bet there are lots of instalces of printk("%l", some_u64) down in arch code where the type of u64 _is_ known which will now spew warnings.

Quoted 1 line

Oh well.

As a rough estimate:

concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l 635

Someone's gonna get a lot of git points for fixing all those. Might keep the speeling fix crowd busy for a while.

concordia powerpc(master) $ find arch/powerpc/ ! -name '*32.*' | xargs grep "%l" | grep -v "%ll" | wc -l
635

Someone's gonna get a lot of git points for fixing all those. Might keep
the speeling fix crowd busy for a

This is (IMO) a desirable change and will prevent a heck of a lot of goofing around, and will permit a lot of prior goofing around to be removed.

But I bet there are lots of instalces of printk("%l", some_u64) down in arch code where the type of u64 _is_ known which will now spew warnings.

Quoted 8 lines

Quoted 3 lines

We also jump through hoops to print things like sector_t and resource_size_t. They always need to be cast to `unsiged long long', which generates additional stack space and text in some setups.

The thing is that GCC checks types. So it's fine to add "print this pointer specially", but you can't in general add new printf arguments without also hacking GCC. Unless you use -Wno-format, and require sparse to check special kernel types.

It would be excellent if gcc had an extension system so that you could add new printf control chars and maybe even tell gcc how to check them. But of course, if that were to happen, we couldn't use it for 4-5 years.

What I had initially proposed was to abuse %S, which takes a wchar_t*. gcc accepts `unsigned long *' for %S.

Then, we put the kernel-specific control char after the S, so we can print an inode (rofl) with

	struct inode *inode;

	printk("here is an inode: %Si\n", (unsigned long *)inode);

Downsides are:

- there's a cast, so you could accidentally do

	printk("here is an inode: %Si\n", (unsigned long *)dentry);

- there's a cast, and they're ugly

- gcc cannot of course check that the arg matches the control string

Unfortunately (and this seems weird), gcc printf checking will not accept a void* for %S: it _has_ to be wchar_t*, and the checker won't permit void* substitution for that.

We don't know how much interest there would be in churning NIPQUAD from the net guys. Interestingly, there's also %C (wint_t) which is a 32-bit quantity. So we could just go and say "%C prints an ipv4 address" and be done with it. But there's no way of doing that for ipv6 addresses so things would become asymmetrical there.

Quoted 6 lines · Andrew Morton

We don't know how much interest there would be in churning NIPQUAD from the net guys. Interestingly, there's also %C (wint_t) which is a 32-bit quantity. So we could just go and say "%C prints an ipv4 address" and be done with it. But there's no way of doing that for ipv6 addresses so things would become asymmetrical there.

       struct in6_addr src;
       printk("Source address: %p{ipv6}\n", &src);

How about %p{feature}?

Quoted 8 lines · Andrew Morton

Quoted 6 lines

We don't know how much interest there would be in churning NIPQUAD from the net guys. Interestingly, there's also %C (wint_t) which is a 32-bit quantity. So we could just go and say "%C prints an ipv4 address" and be done with it. But there's no way of doing that for ipv6 addresses so things would become asymmetrical there.

Quoted 4 lines

       struct in6_addr src;
       printk("Source address: %p{ipv6}\n", &src);

How about %p{feature}?

Something like this?

lib/vsprintf.c +8 -0

+{
+	if (!strncmp(fmt, "sym", fmtlen)) {
+		char name[KSYM_SYMBOL_LEN];
+		int len;
+		int i;
+
+		len = sprint_symbol(name, (unsigned long) arg);
+		if (len < 0)

lib/vsprintf.c +4 -0

+
+	return buf;
+}
+

static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)

{
	/* we are called with base 8, 10 or 16, only, thus don't need "G..."  */

Quoted 13 lines · Vegard Nossum

Quoted 12 lines · Andrew Morton

Quoted 6 lines

We don't know how much interest there would be in churning NIPQUAD from the net guys. Interestingly, there's also %C (wint_t) which is a 32-bit quantity. So we could just go and say "%C prints an ipv4 address" and be done with it. But there's no way of doing that for ipv6 addresses so things would become asymmetrical there.

       struct in6_addr src;
       printk("Source address: %p{ipv6}\n", &src);

How about %p{feature}?

lib/vsprintf.c static noinline char* put_dec(char *buf, unsigned long long num) · line 371

Quoted 4 lines

Something like this?

lib/vsprintf.c +2 -0

+static char *custom_format(char *buf, char *end,
+      const char *fmt, unsigned int fmtlen, void *arg)

I'd use const void *arg here, so nobody gets the idea that you could modify the argument while printing :)

Quoted 2 lines

lib/vsprintf.c +2 -0

+{
+      if (!strncmp(fmt, "sym", fmtlen)) {

Quoted 1 line

+ }

And I assume it's then as simple as

       } else if (strncmp(fmt, "nip6", fmtlen) = 0) {
               snprintf(buf, end - buf (+1?), NIP6_FMT in expanded form,
                        NIP6 in expanded form(arg));
       }

Hm, that's going to get a big function when everyone adds their fmttypes.

Quoted 8 lines

lib/vsprintf.c +4 -0

+
+      return buf;
+}
+

static char *number(char *buf, char *end, unsigned long long num, int base, int size, int precision, int type)

{
      /* we are called with base 8, 10 or 16, only, thus don't need "G..."  */

lib/vsprintf.c int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) · line 682

Quoted 10 lines

lib/vsprintf.c +7 -0

@@ -648,6 +673,25 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
                              continue;
 
                      case 'p':
+                              if (fmt[1] = '{') {
+                                      const char *cfmt;
+
+                                      /* Skip the '%{' */
+                                      ++fmt;
+                                      ++fmt;
+

Not this?

       /* Skip the '%p{' */
       fmt += 3;

I think the most elegant solution would be a macro similar to the initcall macros, that adds the custom extensions to a table which is defined by a special linker section. This allows complete decentralization, but I don't think it's possible to do binary search on the names anymore.

Quoted 9 lines · Jan Engelhardt

Quoted 6 lines · Andrew Morton

We don't know how much interest there would be in churning NIPQUAD from the net guys. Interestingly, there's also %C (wint_t) which is a 32-bit quantity. So we could just go and say "%C prints an ipv4 address" and be done with it. But there's no way of doing that for ipv6 addresses so things would become asymmetrical there.

Quoted 4 lines

       struct in6_addr src;
       printk("Source address: %p{ipv6}\n", &src);

How about %p{feature}?

(It's hard on the stack, yes, I know. We should fix kallsyms.)

Quoted 2 lines

Quoted 2 lines

How about %p{feature}?

No.

I _expressly_ chose '%p[alphanumeric]*' because it's basically totally insane to have that in a *real* printk() string: the end result would be totally unreadable.

So, and what do you do when you run out of alphanumeric characters?

Quoted 2 lines · Vegard Nossum

Quoted 1 line · Jan Engelhardt

How about %p{feature}?

No.

I _expressly_ chose '%p[alphanumeric]*' because it's basically totally insane to have that in a *real* printk() string: the end result would be totally unreadable.

In contrast, '%p[specialchar]' is not unreadable, and in fact we have lots of those already in the kernel. In fact, there are 40 occurrences of '%p{' in the kernel, just grep for it (especially the AFS code seems to be very happy to use that kind of printout in its debug statements).

So it makes perfect sense to have a _real_ printk string that says

       "BUG: Dentry %p{i=%lx,n=%s}"

where we have that '%p{...' sequence: the end result is easily parseable. In contrast, anybody who uses '%pS' or something like that and expects a pointer name to be immediately followed by teh letter 'S' is simply insane, because the end result is an unreadable mess.

Quoted 2 lines

(It's hard on the stack, yes, I know. We should fix kallsyms.)

Not just that, but it's broken when KALLSYMS is disabled. Look at what sprint_symbol() becomes.

The patch I already sent out is about a million times better, because it avoids all these issues, and knows about subtle issues like the difference between a direct pointer and a pointer to a function descriptor.

Single letters are bad because it hurts readability and limits the usefulness of the extension.</MHO>

Quoted 2 lines

Single letters are bad because it hurts readability and limits the usefulness of the extension.</MHO>

I think you need a little warning noise that goes off in your head that means "I might be overdesigning this". Linus' code is elegant and solves a problem nicely.

Quoted 2 lines · Vegard Nossum

Quoted 2 lines

Single letters are bad because it hurts readability and limits the usefulness of the extension.</MHO>

Quoted 3 lines · Matthew Wilcox

I think you need a little warning noise that goes off in your head that means "I might be overdesigning this". Linus' code is elegant and solves a problem nicely.

Am I the only one who missed Linus' patch? Did it make it to the list?

u64 is easy to fix, and I don't know why we haven't. Just make it unsigned long long on all architectures.

Quoted 7 lines · Linus Torvalds

so I think we could easily just say that we extend %p in various ways:

- %pS - print pointer as a symbol

and leave tons of room for future extensions for different kinds of pointers.

So here's a totally untested example patch of this, which could probably easily be extended to to other things.

I actually made it '%pF' and '%pS' for a Function descriptor pointer and normal Symbolic pointer respectively, because of the stupid things ia64 and PPC64 do with the pointer indirection through function descriptors.

That function descriptor indirection is totally untested, and I did it with a

	pagefault_disable();
	__get_user(..)
	pagefault_enable();

thing because I thought it would be nice if printk() was always safe, and passing bogus function pointers to '%pF' should try to work, but quite frankly, I didn't even check that that part compiles, much less works.

ia64/ppc lists cc'd, just in case somebody wants to test this.

NOTE! There are no current actual users of this, but the _idea_ is that we should be able to just do

	printk("%pF\n", desc->handle_irq);

instead of using

	print_symbol("%s\n", (unsigned long)desc->handle_irq);

The latter is from kernel/irq/internals.h, and actually looks incorrect - shouldn't it use print_fn_descriptor_symbol(), since it's a C level function pointer? We should use "print_symbol()" for return pointers we find on the stack and data pointers, but not for stuff that actually has a C type that is a function pointer?

Somebody who cares about the insane function descriptors should take a deeper look.

NOTE AGAIN! UNTESTED! I could easily have screwed up printk() _entirely_, since I had to factor out the string handling into a function of its own.

Linus

---
 lib/vsprintf.c |  102 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 74 insertions(+), 28 deletions(-)

lib/vsprintf.c +74 -29

index 6021757..148b656 100644
@@ -22,6 +22,7 @@
 #include <linux/string.h>
 #include <linux/ctype.h>
 #include <linux/kernel.h>
+#include <linux/kallsyms.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>

@@ -482,6 +483,72 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
 	return buf;
 }
 
+static char *string(char *buf, char *end, char *s, int field_width, int precision, int flags)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "<NULL>";
+
+	len = strnlen(s, precision);
+
+	if (!(flags & LEFT)) {
+		while (len < field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
+/*
+ * Show a '%p' thing.  A kernel extension is that the '%p' is followed
+ * by an extra set of alphanumeric characters that are extended format
+ * specifiers.  Right now we just handle 'F' (for symbolic Function
+ * pointers) and 'S' (for Symbolic data pointers), but this can easily
+ * be extended in the future (network address types etc).
+ *
+ * The difference between 'S' and 'F' is that on ia64 and ppc64 function
+ * pointers are really function descriptors, which contain a pointer the
+ * real address. 
+ */
+static char *pointer(const char *fmt, char *buf, char *end, void *ptr, int base, int size, int precision, int type)
+{
+	char sym[KSYM_SYMBOL_LEN];
+	switch (*fmt) {
+	case 'F':	/* Function pointer */
+#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
+		{ void *p;
+		pagefault_disable();
+		if (!__get_user((void **)ptr, &p))
+			ptr = p;
+		pagefault_enable();
+		}
+#endif
+	/* Fallthrough */
+	case 'S':	/* Other pointer */
+#if CONFIG_KALLSYMS
+		sprint_symbol(sym, (unsigned long) ptr);
+		return string(buf, end, sym, size, precision, type);
+#else
+		type |= SPECIAL;
+		break;
+#endif
+	}
+	return number(buf, end, (unsigned long long) ptr, base, size, precision, type);
+}
+
 /**
  * vsnprintf - Format a string and place it in a buffer
  * @buf: The buffer to place the result into

@@ -502,11 +569,9 @@ static char *number(char *buf, char *end, unsigned long long num, int base, int
  */
 int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 {
-	int len;
 	unsigned long long num;
-	int i, base;
+	int base;
 	char *str, *end, c;
-	const char *s;
 
 	int flags;		/* flags to number() */
 

@@ -622,29 +687,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 				continue;
 
 			case 's':
-				s = va_arg(args, char *);
-				if ((unsigned long)s < PAGE_SIZE)
-					s = "<NULL>";
-
-				len = strnlen(s, precision);
-
-				if (!(flags & LEFT)) {
-					while (len < field_width--) {
-						if (str < end)
-							*str = ' ';
-						++str;
-					}
-				}
-				for (i = 0; i < len; ++i) {
-					if (str < end)
-						*str = *s;
-					++str; ++s;
-				}
-				while (len < field_width--) {
-					if (str < end)
-						*str = ' ';
-					++str;
-				}
+				str = string(str, end, va_arg(args, char *), field_width, precision, flags);
 				continue;
 
 			case 'p':

@@ -653,9 +696,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 					field_width = 2*sizeof(void *);
 					flags |= ZEROPAD;
 				}
-				str = number(str, end,
-						(unsigned long) va_arg(args, void *),
+				str = pointer(fmt+1, str, end,
+						va_arg(args, void *),
 						16, field_width, precision, flags);
+				/* Skip all alphanumeric pointer suffixes */
+				while (isalnum(fmt[1]))
+					fmt++;
 				continue;
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-ia64" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

It's also true for parisc, fwiw. Added a cc to them.

That function descriptor indirection is totally untested, and I did it with a

        pagefault_disable();
        __get_user(..)
        pagefault_enable();

thing because I thought it would be nice if printk() was always safe, and passing bogus function pointers to '%pF' should try to work, but quite frankly, I didn't even check that that part compiles, much less works.

ia64/ppc lists cc'd, just in case somebody wants to test this.

I'll give it a try using probe_kernel_address() instead on monday.

Still all happily untested, of course. And still with no actual users converted.

Quoted 2 lines · Linus Torvalds

Still all happily untested, of course. And still with no actual users converted.

Ok, it's tested, and here's an example usage conversion.

The diffstat pretty much says it all. It _does_ change the format of the stack trace entry a bit, but I don't think it's for the worse (unless it breaks things like the oops tracker - Arjan?)

It changes the symbol-in-module format from

	:ext3:add_dirent_to_buf+0x6c/0x26c

to

	add_dirent_to_buf+0x6c/0x26c [ext3]

but quite frankly, the latter was the standard format anyway (it's what "sprint_symbol()" gives you), and traps_64.c was the odd man out.

Quoted 2 lines

Still all happily untested, of course. And still with no actual users converted.

Ok, it's tested, and here's an example usage conversion.

The diffstat pretty much says it all. It _does_ change the format of the stack trace entry a bit, but I don't think it's for the worse (unless it breaks things like the oops tracker - Arjan?)

It changes the symbol-in-module format from

	:ext3:add_dirent_to_buf+0x6c/0x26c

to

	add_dirent_to_buf+0x6c/0x26c [ext3]

but quite frankly, the latter was the standard format anyway (it's what "sprint_symbol()" gives you), and traps_64.c was the odd man out.

In fact, traps_32.c already uses the standard print_symbol() format, so it really was an issue of the 64-bit code being odd (and I assume that this also means that it cannot break the oops tracker, since it already had to be able to handle both formats).

I also removed the KALLSYMS dependency, so if KALLSYMS isn't enabled it will now give the same hex format twice, but I doubt we really care (such stack traces are unreadable whether it shows up once or twice, and the simplicity is worth it).

If people do just a few more conversions like this, then the 52 added lines in lib/vsnprintf.c are more than made up for by removed lines elsewhere (and more readable source code).

applied (with the commit message below) to tip/x86/debug for v2.6.27 merging, thanks Linus. Can i add your SOB too?

Quoted 3 lines · Ingo Molnar

applied (with the commit message below) to tip/x86/debug for v2.6.27 merging, thanks Linus. Can i add your SOB too?

Sure, add my S-O-B. But I hope/assuem that you also added my earlier patch that added the support for '%pS' too? I'm not entirely sure that should go in an x86-specific branch, since it has nothing x86-specific in it.

yeah, agreed, combined it's not an x86 topic anymore.

[ There's some lkml trouble so i've missed the earlier patch. I'm not
sure the email problem is on my side, see how incomplete the discussion is on lkml.org as well:

     http://lkml.org/lkml/2008/6/25/170   ]

Anyway, i have have added this second patch of yours to tip/core/printk and moved your first patch over to that topic (which relies on it).

Quoted 2 lines · Benjamin Herrenschmidt

I'll give it a try using probe_kernel_address() instead on monday.

Here's the updated patch which uses probe_kernel_address() instead (and moves the whole #ifdef mess out of the code that wants it and into a helper function - and maybe we should then put that helper into kallsyms.h, but that's a different issue).

Still all happily untested, of course. And still with no actual users converted.

Quoted 10 lines · Linus Torvalds

Quoted 2 lines · Benjamin Herrenschmidt

I'll give it a try using probe_kernel_address() instead on monday.

Here's the updated patch which uses probe_kernel_address() instead (and moves the whole #ifdef mess out of the code that wants it and into a helper function - and maybe we should then put that helper into kallsyms.h, but that's a different issue).

Quoted 2 lines

Still all happily untested, of course. And still with no actual users converted.

Did a few tests and it seems to work. I'll stick a patch converting powerpc to use %pS for oops display in -next.

Quoted 15 lines · On Mon, 07 Jul 2008 11:14:36 +1000 Benjamin Herrenschmidt…

Quoted 12 lines · Linus Torvalds

Quoted 2 lines · Benjamin Herrenschmidt

I'll give it a try using probe_kernel_address() instead on monday.

Here's the updated patch which uses probe_kernel_address() instead (and moves the whole #ifdef mess out of the code that wants it and into a helper function - and maybe we should then put that helper into kallsyms.h, but that's a different issue).

Still all happily untested, of course. And still with no actual users converted.

Quoted 2 lines

Did a few tests and it seems to work. I'll stick a patch converting powerpc to use %pS for oops display in -next.

After you post it to linuxppc-dev and get review comments, of course ...

And after the patch to make %pS work goes in ...

Wasn't that already merged via the trivial scheduler fixes tree or something? ;)

Quoted 2 lines

Did a few tests and it seems to work. I'll stick a patch converting powerpc to use %pS for oops display in -next.

After you post it to linuxppc-dev and get review comments, of course ...

And after the patch to make %pS work goes in ...