[PATCH] Correct printk %pF to work on all architectures

[PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

It was introduced by 

commit 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Jul 6 16:43:12 2008 -0700

    vsprintf: add support for '%pS' and '%pF' pointer formats


However, the current way its coded doesn't work on parisc64.  For two
reasons:  1) parisc isn't in the #ifdef and 2) parisc has a different
format for function descriptors

Make dereference_function_descriptor() more accommodating by allowing
architecture overrides.  I put the three overrides (for parisc64, ppc64
and ia64) in arch/kernel/module.c because that's where the kernel
internal linker which knows how to deal with function descriptors sits.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/ia64/kernel/module.c       |    9 +++++++++
 arch/parisc/kernel/module.c     |   13 +++++++++++++
 arch/powerpc/kernel/module_64.c |    9 +++++++++
 include/linux/kernel.h          |    3 +++
 lib/vsprintf.c                  |   12 ++++++------
 5 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 29aad34..596a862 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -31,6 +31,7 @@
 #include <linux/elf.h>
 #include <linux/moduleloader.h>
 #include <linux/string.h>
+#include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 
 #include <asm/patch.h>
@@ -941,3 +942,11 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	void *p = NULL;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+	return p;
+}
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index fdacdd4..4ad80a5 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -47,6 +47,7 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 
 #include <asm/unwind.h>
 
@@ -860,3 +861,15 @@ void module_arch_cleanup(struct module *mod)
 	deregister_unwind_table(mod);
 	module_bug_cleanup(mod);
 }
+
+#ifdef CONFIG_64BIT
+void *dereference_function_descriptor(void *ptr)
+{
+	Elf64_Fdesc *desc = ptr;
+	void *p = NULL;
+
+	if (!probe_kernel_address(&desc->addr, p))
+		ptr = p;
+	return p;
+}
+#endif
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ee6a298..60e9749 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -21,6 +21,7 @@
 #include <linux/err.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 #include <asm/module.h>
 #include <asm/uaccess.h>
 #include <asm/firmware.h>
@@ -451,3 +452,11 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
 	return 0;
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	void *p = NULL;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+	return p;
+}
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..8ff19b3 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -189,6 +189,9 @@ extern int __kernel_text_address(unsigned long addr);
 extern int kernel_text_address(unsigned long addr);
 struct pid;
 extern struct pid *session_of_pgrp(struct pid *pgrp);
+/* function descriptor handling (if any) */
+extern void *dereference_function_descriptor(void *ptr);
+
 
 #ifdef CONFIG_PRINTK
 asmlinkage int vprintk(const char *fmt, va_list args)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..cffcd95 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -513,13 +513,13 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
+/*
+ * Some architectures need special handling for pointers
+ * to functions, which are done via function descriptors
+ * Do a non weak override of this function for them
+ */
+void __weak *dereference_function_descriptor(void *ptr)
 {
-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
-	void *p;
-	if (!probe_kernel_address(ptr, p))
-		ptr = p;
-#endif
 	return ptr;
 }
 
-- 
1.5.6.5

Re: [PATCH] Correct printk %pF to work on all architectures

[Linus Torvalds]

On Wed, 3 Sep 2008, James Bottomley wrote:
> 
> Make dereference_function_descriptor() more accommodating by allowing
> architecture overrides.

Don't do it like this.

We don't want some stupid useless weak function that is empty on all sane 
platforms.

Just do

	.. declare or create an inline 'parisc_function_descriptor()' ..

	#define dereference_function_descriptor(p) parisc_function_descriptor(p)

in some arch header file. And then use

	#ifndef dereference_function_descriptor
	#define dereference_function_descriptor(p) (p)
	#endif

in the generic code, so that sane architectures don't need to do anything 
at all.

		Linus

Re: [PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

On Wed, 2008-09-03 at 14:22 -0700, Linus Torvalds wrote:
> 
> On Wed, 3 Sep 2008, James Bottomley wrote:
> > 
> > Make dereference_function_descriptor() more accommodating by allowing
> > architecture overrides.
> 
> Don't do it like this.
> 
> We don't want some stupid useless weak function that is empty on all sane 
> platforms.
> 
> Just do
> 
> 	.. declare or create an inline 'parisc_function_descriptor()' ..
> 
> 	#define dereference_function_descriptor(p) parisc_function_descriptor(p)
> 
> in some arch header file. And then use
> 
> 	#ifndef dereference_function_descriptor
> 	#define dereference_function_descriptor(p) (p)
> 	#endif
> 
> in the generic code, so that sane architectures don't need to do anything 
> at all.

Is that finally final?  because the last time I tried to do the above
for a voyager override I was told weak functions were the preferred
method ...

Anyway, it's easy to do (if a slightly larger diff) ... I have to move
the prototype from include/kernel.h to include/module.h because I need
an assured asm/xxx include before it to get the override.

It was also pointed out that I should be returning the passed in ptr,
not NULL on failure and that the return should be ptr not p.

James

---

The current way its coded doesn't work on parisc64.  For two
reasons:  1) parisc isn't in the #ifdef and 2) parisc has a different
format for function descriptors

Make dereference_function_descriptor() more accommodating by allowing
architecture overrides.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/ia64/include/asm/module.h    |    4 ++++
 arch/ia64/kernel/module.c         |    9 +++++++++
 arch/parisc/kernel/module.c       |   13 +++++++++++++
 arch/powerpc/include/asm/module.h |    6 ++++++
 arch/powerpc/kernel/module_64.c   |    9 +++++++++
 include/asm-parisc/module.h       |    6 ++++++
 include/linux/module.h            |    5 +++++
 lib/vsprintf.c                    |   10 ----------
 8 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/arch/ia64/include/asm/module.h b/arch/ia64/include/asm/module.h
index d2da61e..d0328be 100644
--- a/arch/ia64/include/asm/module.h
+++ b/arch/ia64/include/asm/module.h
@@ -33,4 +33,8 @@ struct mod_arch_specific {
 
 #define ARCH_SHF_SMALL	SHF_IA_64_SHORT
 
+void *ia64_dereference_function_descriptor(void *);
+#define dereference_function_descriptor(p) \
+	ia64_dereference_function_descriptor(p)
+
 #endif /* _ASM_IA64_MODULE_H */
diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 29aad34..4aca326 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -31,6 +31,7 @@
 #include <linux/elf.h>
 #include <linux/moduleloader.h>
 #include <linux/string.h>
+#include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 
 #include <asm/patch.h>
@@ -941,3 +942,11 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+void *ia64_dereference_function_descriptor(void *ptr)
+{
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index fdacdd4..6ec3b07 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -47,6 +47,7 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 
 #include <asm/unwind.h>
 
@@ -860,3 +861,15 @@ void module_arch_cleanup(struct module *mod)
 	deregister_unwind_table(mod);
 	module_bug_cleanup(mod);
 }
+
+#ifdef CONFIG_64BIT
+void *parisc_dereference_function_descriptor(void *ptr)
+{
+	Elf64_Fdesc *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->addr, p))
+		ptr = p;
+	return ptr;
+}
+#endif
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index e5f14b1..a861b2c 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -73,5 +73,11 @@ struct exception_table_entry;
 void sort_ex_table(struct exception_table_entry *start,
 		   struct exception_table_entry *finish);
 
+#ifdef __powerpc64__
+void *powerpc64_dereference_function_descriptor(void *);
+#define dereference_function_descriptor(p) \
+	powerpc64_dereference_function_descriptor(p)
+#endif
+
 #endif /* __KERNEL__ */
 #endif	/* _ASM_POWERPC_MODULE_H */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ee6a298..e814c2a 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -21,6 +21,7 @@
 #include <linux/err.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 #include <asm/module.h>
 #include <asm/uaccess.h>
 #include <asm/firmware.h>
@@ -451,3 +452,11 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
 	return 0;
 }
+
+void *powerpc64_dereference_function_descriptor(void *ptr)
+{
+	void *p;
+	if (!probe_kernel_address(ptr, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/include/asm-parisc/module.h b/include/asm-parisc/module.h
index c2cb49e..f5f971b 100644
--- a/include/asm-parisc/module.h
+++ b/include/asm-parisc/module.h
@@ -29,4 +29,10 @@ struct mod_arch_specific
 	struct unwind_table *unwind;
 };
 
+#ifdef CONFIG_64BIT
+void *parisc_dereference_function_descriptor(void *);
+#define dereference_function_descriptor(p) \
+	parisc_dereference_function_descriptor(p)
+#endif
+
 #endif /* _ASM_PARISC_MODULE_H */
diff --git a/include/linux/module.h b/include/linux/module.h
index 68e0955..a549f89 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -76,6 +76,11 @@ void sort_extable(struct exception_table_entry *start,
 		  struct exception_table_entry *finish);
 void sort_main_extable(void);
 
+/* function descriptor handling (if any) */
+#ifndef dereference_function_descriptor
+#define dereference_function_descriptor(p) (p)
+#endif
+
 #ifdef MODULE
 #define MODULE_GENERIC_TABLE(gtype,name)			\
 extern const struct gtype##_id __mod_##gtype##_table		\
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..0c47629 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -513,16 +513,6 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
-{
-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
-	void *p;
-	if (!probe_kernel_address(ptr, p))
-		ptr = p;
-#endif
-	return ptr;
-}
-
 static char *symbol_string(char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {
 	unsigned long value = (unsigned long) ptr;
-- 
1.5.6.5

Re: [PATCH] Correct printk %pF to work on all architectures

[Linus Torvalds]

On Wed, 3 Sep 2008, James Bottomley wrote:
> 
> Is that finally final?  because the last time I tried to do the above
> for a voyager override I was told weak functions were the preferred
> method ...

Weak functions are fine IF THEY DO SOMETHING REAL AND SHOULD BE FUNCTIONS 
IN THE FIRST PLACE!

IOW, if the default behaviour is actually something that should be a 
function, then a weak function is the simplest and most appropriate way of 
doing thigns.

But if the default behaviour is to not do anything at all, then a weak 
function simply doesn't work - because it will always generate that stupid 
and useless function call. And then you have to have per-architecture 
inline functions.

And in order to avoid having all 99 architectures that don't care at all 
add an empty inline function, then you essentially have to use a #define 
to allow us to detect at compile-time that no function existed.

> Anyway, it's easy to do (if a slightly larger diff) ... I have to move
> the prototype from include/kernel.h to include/module.h because I need
> an assured asm/xxx include before it to get the override.

I don't really see what this has to do with module.h, though.

Why do this in <linux/module.h>?  Why not just do it in lib/vsptintf.c 
which is the only place that cares? None of this needs to pollute the 
generic header files that simply don't care.

			Linus

Re: [PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

On Wed, 2008-09-03 at 15:54 -0700, Linus Torvalds wrote:
> > Anyway, it's easy to do (if a slightly larger diff) ... I have to move
> > the prototype from include/kernel.h to include/module.h because I need
> > an assured asm/xxx include before it to get the override.
> 
> I don't really see what this has to do with module.h, though.
> 
> Why do this in <linux/module.h>?  Why not just do it in lib/vsptintf.c 
> which is the only place that cares? None of this needs to pollute the 
> generic header files that simply don't care.

You want me to pull the elf header files into lib/vsprintf.c and have
something like

static inline void *dereference_function_descritpor(void *ptr)
{
#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
	void *p;
	if (!probe_kernel_address(ptr, p))
		ptr = p;
#elif defined(CONFIG_PARISC) && defined(CONFIG_64BITS)
	Elf64_Fptr *desc = ptr;
	void *p;
	if (!probe_kernel_address(&desc->addr, p))
		ptr = p;
#endif
	...

?

Because it just looks rather tacky ...

James

Re: [PATCH] Correct printk %pF to work on all architectures

[Linus Torvalds]

On Wed, 3 Sep 2008, James Bottomley wrote:
> 
> You want me to pull the elf header files into lib/vsprintf.c and have
> something like

No.

I want you to stop polluting <linux/module.h> with total and utter crap.

Please tell my WHY the hell you have

	diff --git a/include/linux/module.h b/include/linux/module.h
	index 68e0955..a549f89 100644
	--- a/include/linux/module.h
	+++ b/include/linux/module.h
	@@ -76,6 +76,11 @@ void sort_extable(struct exception_table_entry *start,
	                  struct exception_table_entry *finish);
	 void sort_main_extable(void);
	 
	+/* function descriptor handling (if any) */
	+#ifndef dereference_function_descriptor
	+#define dereference_function_descriptor(p) (p)
	+#endif
	+
	 #ifdef MODULE
	 #define MODULE_GENERIC_TABLE(gtype,name)                       \
	 extern const struct gtype##_id __mod_##gtype##_table           \

in your patch? What the _hell_ does that have to do with "module.h"?

Why the heck don't you just have that in the ONLY place that cares, namely 
lib/vfprintf.c?

In other words, WHY did you do something as stupid and totally 
unexplainable as

	diff --git a/lib/vsprintf.c b/lib/vsprintf.c
	index d8d1d11..0c47629 100644
	--- a/lib/vsprintf.c
	+++ b/lib/vsprintf.c
	@@ -513,16 +513,6 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
	        return buf;
	 }
	 
	-static inline void *dereference_function_descriptor(void *ptr)
	-{
	-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
	-       void *p;
	-       if (!probe_kernel_address(ptr, p))
	-               ptr = p;
	-#endif
	-       return ptr;
	-}
	-

when that thing _used_ to be in a place where it made sense? Why didn't 
you just change that already existing code to use a #ifdef instead?

Why do you move that to <linux/module.h>? It makes no sense.

And why do you put the arch-specific defines in <asm/module.h>? That makes 
no sense either. WHY?

As far as I can tell, the _only_ reason you happened to pick 
<linux/module.h> was literally that it is the first of our header files 
that lib/vsprintf.c includes. And quite frankly, that makes no sense. 

That's about as sensible as putting it into <linux/string.h>. 

Put those things in some _sane_ place. That means:

 - default non-implementation in lib/vsprintf.c, since there is no point 
   in putting it anywhere else, when it's not used anywhere else.

 - arch-specific implementations can go into some more sensible asm header 
   file that is more relevant. Maybe <asm/processor.h>?

IOW, I'm complaining about your totally senseless and apparently random 
choice of location.

			Linus

Re: [PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

On Wed, 2008-09-03 at 16:15 -0700, Linus Torvalds wrote:
> 
> On Wed, 3 Sep 2008, James Bottomley wrote:
> > 
> > You want me to pull the elf header files into lib/vsprintf.c and have
> > something like
> 
> No.
> 
> I want you to stop polluting <linux/module.h> with total and utter crap.
> 
> Please tell my WHY the hell you have
> 
> 	diff --git a/include/linux/module.h b/include/linux/module.h
> 	index 68e0955..a549f89 100644
> 	--- a/include/linux/module.h
> 	+++ b/include/linux/module.h
> 	@@ -76,6 +76,11 @@ void sort_extable(struct exception_table_entry *start,
> 	                  struct exception_table_entry *finish);
> 	 void sort_main_extable(void);
> 	 
> 	+/* function descriptor handling (if any) */
> 	+#ifndef dereference_function_descriptor
> 	+#define dereference_function_descriptor(p) (p)
> 	+#endif
> 	+
> 	 #ifdef MODULE
> 	 #define MODULE_GENERIC_TABLE(gtype,name)                       \
> 	 extern const struct gtype##_id __mod_##gtype##_table           \
> 
> in your patch? What the _hell_ does that have to do with "module.h"?
> 
> Why the heck don't you just have that in the ONLY place that cares, namely 
> lib/vfprintf.c?

Oh ... because Arjan has a patch to export
dereference_function_descriptor.  I suppose I could make him do the
heavy lifting, but it seemed sensible to make it easy for him (and me)
by putting it in a header.

http://marc.info/?l=linux-kernel&m=121976793429869

It's already in Ingo's tree, so it will be upstream soon.

> In other words, WHY did you do something as stupid and totally 
> unexplainable as
> 
> 	diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> 	index d8d1d11..0c47629 100644
> 	--- a/lib/vsprintf.c
> 	+++ b/lib/vsprintf.c
> 	@@ -513,16 +513,6 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
> 	        return buf;
> 	 }
> 	 
> 	-static inline void *dereference_function_descriptor(void *ptr)
> 	-{
> 	-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
> 	-       void *p;
> 	-       if (!probe_kernel_address(ptr, p))
> 	-               ptr = p;
> 	-#endif
> 	-       return ptr;
> 	-}
> 	-
> 
> when that thing _used_ to be in a place where it made sense? Why didn't 
> you just change that already existing code to use a #ifdef instead?
> 
> Why do you move that to <linux/module.h>? It makes no sense.
> 
> And why do you put the arch-specific defines in <asm/module.h>? That makes 
> no sense either. WHY?
> 
> As far as I can tell, the _only_ reason you happened to pick 
> <linux/module.h> was literally that it is the first of our header files 
> that lib/vsprintf.c includes. And quite frankly, that makes no sense. 
> 
> That's about as sensible as putting it into <linux/string.h>. 
> 
> Put those things in some _sane_ place. That means:
> 
>  - default non-implementation in lib/vsprintf.c, since there is no point 
>    in putting it anywhere else, when it's not used anywhere else.
> 
>  - arch-specific implementations can go into some more sensible asm header 
>    file that is more relevant. Maybe <asm/processor.h>?
> 
> IOW, I'm complaining about your totally senseless and apparently random 
> choice of location.

Because arch/../kernel/module.c is where we put the in-kernel linker
which uses the function descriptors and already processes them.

The original patch put the prototype in kernel.h, but kernel.h doesn't
include too many files before it, so if I'm putting the arch
implementation in module.c, it makes sense to me to put the headers in
linux/module.h and asm/module.h being the natural pair belonging to
arch/kernel/module.c

So if I use asm/processor.h, you want me to add that to linux/kernel.h
and move the prototypes back in there?

James

Re: [PATCH] Correct printk %pF to work on all architectures

[Linus Torvalds]

On Wed, 3 Sep 2008, James Bottomley wrote:
> 
> Oh ... because Arjan has a patch to export
> dereference_function_descriptor.  I suppose I could make him do the
> heavy lifting, but it seemed sensible to make it easy for him (and me)
> by putting it in a header.
> 
> http://marc.info/?l=linux-kernel&m=121976793429869

Ahh.

NOW it all starts to make sense.

Or perhaps not sense, but I at least understand why people want to move it 
around. The kernel.h location kind of goes together with that 
core_kernel_text() thing, although it seems to be more of a "random 
collection of routines" thing than anything else (but hey, that's the very 
definition of "kernel.h" for you).

The module.h location still seems to be more of a "oh, both 
kernel/extable.c and lib/vsprintf.c already included <linux/module.h>" and 
it's a bit sad, since it really has nothing at all to do with modules.

Grr. It does seem like we don't have any kind of "abi" header file. 
<linux/kernel.h> and <asm/processor.h> has various random things.

So yea, there doesn't seem to be any _obvious_ place that makes sense.

		Linus

Not-very-strong-opinion: How about <asm/sections.h>? That does seem to be 
where we already hide things like "in_kernel_text()" at least on powerpc. 
In fact, since we already always have a generic version, the patch would 
actually be something like

 - in <asm-generic/sections.h>, just do

	#define dereference_function_descriptor(p) (p)

 - in architectures that want to override it

	#undef dereference_function_descriptor

   followed by 

	static inline void *dereference_function_descriptor(..) ..
	or
	#define dereference_function_descriptor my_fn_dereference

   since they all include the generic one as a base

Hmm? I do admit that "<asm/sections.h>" doesn't really strike me as a very 
natural name for this, but kernel/extable.c does already include it for 
other reasons, and it's at least no worse than module.h.

Re: [PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

On Wed, 2008-09-03 at 17:01 -0700, Linus Torvalds wrote:
> 
> On Wed, 3 Sep 2008, James Bottomley wrote:
> > 
> > Oh ... because Arjan has a patch to export
> > dereference_function_descriptor.  I suppose I could make him do the
> > heavy lifting, but it seemed sensible to make it easy for him (and me)
> > by putting it in a header.
> > 
> > http://marc.info/?l=linux-kernel&m=121976793429869
> 
> Ahh.
> 
> NOW it all starts to make sense.
> 
> Or perhaps not sense, but I at least understand why people want to move it 
> around. The kernel.h location kind of goes together with that 
> core_kernel_text() thing, although it seems to be more of a "random 
> collection of routines" thing than anything else (but hey, that's the very 
> definition of "kernel.h" for you).
> 
> The module.h location still seems to be more of a "oh, both 
> kernel/extable.c and lib/vsprintf.c already included <linux/module.h>" and 
> it's a bit sad, since it really has nothing at all to do with modules.
> 
> Grr. It does seem like we don't have any kind of "abi" header file. 
> <linux/kernel.h> and <asm/processor.h> has various random things.
> 
> So yea, there doesn't seem to be any _obvious_ place that makes sense.
> 
> 		Linus
> 
> Not-very-strong-opinion: How about <asm/sections.h>? That does seem to be 
> where we already hide things like "in_kernel_text()" at least on powerpc. 
> In fact, since we already always have a generic version, the patch would 
> actually be something like
> 
>  - in <asm-generic/sections.h>, just do
> 
> 	#define dereference_function_descriptor(p) (p)
> 
>  - in architectures that want to override it
> 
> 	#undef dereference_function_descriptor
> 
>    followed by 
> 
> 	static inline void *dereference_function_descriptor(..) ..
> 	or
> 	#define dereference_function_descriptor my_fn_dereference
> 
>    since they all include the generic one as a base
> 
> Hmm? I do admit that "<asm/sections.h>" doesn't really strike me as a very 
> natural name for this, but kernel/extable.c does already include it for 
> other reasons, and it's at least no worse than module.h.

Well, good as any other I suppose.  So this is what you want?

James

---

It was introduced by 

commit 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Jul 6 16:43:12 2008 -0700

    vsprintf: add support for '%pS' and '%pF' pointer formats


However, the current way its coded doesn't work on parisc64.  For two
reasons:  1) parisc isn't in the #ifdef and 2) parisc has a different
format for function descriptors

Make dereference_function_descriptor() more accommodating by allowing
architecture overrides.  I put the three overrides (for parisc64, ppc64
and ia64) in arch/kernel/module.c because that's where the kernel
internal linker which knows how to deal with function descriptors sits.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/ia64/include/asm/sections.h    |    3 +++
 arch/ia64/kernel/module.c           |   12 ++++++++++++
 arch/parisc/kernel/module.c         |   14 ++++++++++++++
 arch/powerpc/include/asm/sections.h |    3 +++
 arch/powerpc/kernel/module_64.c     |   13 ++++++++++++-
 include/asm-generic/sections.h      |    6 ++++++
 include/asm-parisc/sections.h       |    5 +++++
 lib/vsprintf.c                      |   11 +----------
 8 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 7286e4a..a7acad2 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -21,5 +21,8 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_b
 extern char __start_unwind[], __end_unwind[];
 extern char __start_ivt_text[], __end_ivt_text[];
 
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+
 #endif /* _ASM_IA64_SECTIONS_H */
 
diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 29aad34..545626f 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -31,9 +31,11 @@
 #include <linux/elf.h>
 #include <linux/moduleloader.h>
 #include <linux/string.h>
+#include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 
 #include <asm/patch.h>
+#include <asm/sections.h>
 #include <asm/unaligned.h>
 
 #define ARCH_MODULE_DEBUG 0
@@ -941,3 +943,13 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	struct fdesc *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->ip, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index fdacdd4..44138c3 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -47,7 +47,9 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 
+#include <asm/sections.h>
 #include <asm/unwind.h>
 
 #if 0
@@ -860,3 +862,15 @@ void module_arch_cleanup(struct module *mod)
 	deregister_unwind_table(mod);
 	module_bug_cleanup(mod);
 }
+
+#ifdef CONFIG_64BIT
+void *dereference_function_descriptor(void *ptr)
+{
+	Elf64_Fdesc *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->addr, p))
+		ptr = p;
+	return ptr;
+}
+#endif
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 916018e..7710e9e 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -16,6 +16,9 @@ static inline int in_kernel_text(unsigned long addr)
 	return 0;
 }
 
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+
 #endif
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ee6a298..ad79de2 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -21,8 +21,9 @@
 #include <linux/err.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 #include <asm/module.h>
-#include <asm/uaccess.h>
+#include <asm/sections.h>
 #include <asm/firmware.h>
 #include <asm/code-patching.h>
 #include <linux/sort.h>
@@ -451,3 +452,13 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
 	return 0;
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	struct ppc64_opd_entry *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->funcaddr, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 8feeae1..79a7ff9 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -14,4 +14,10 @@ extern char __kprobes_text_start[], __kprobes_text_end[];
 extern char __initdata_begin[], __initdata_end[];
 extern char __start_rodata[], __end_rodata[];
 
+/* function descriptor handling (if any).  Override
+ * in asm/sections.h */
+#ifndef dereference_function_descriptor
+#define dereference_function_descriptor(p) (p)
+#endif
+
 #endif /* _ASM_GENERIC_SECTIONS_H_ */
diff --git a/include/asm-parisc/sections.h b/include/asm-parisc/sections.h
index fdd43ec..9d13c35 100644
--- a/include/asm-parisc/sections.h
+++ b/include/asm-parisc/sections.h
@@ -4,4 +4,9 @@
 /* nothing to see, move along */
 #include <asm-generic/sections.h>
 
+#ifdef CONFIG_64BIT
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+#endif
+
 #endif
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..c399bc1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
+#include <asm/sections.h>	/* for dereference_function_descriptor() */
 
 /* Works only for digits and letters, but small and fast */
 #define TOLOWER(x) ((x) | 0x20)
@@ -513,16 +514,6 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
-{
-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
-	void *p;
-	if (!probe_kernel_address(ptr, p))
-		ptr = p;
-#endif
-	return ptr;
-}
-
 static char *symbol_string(char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {
 	unsigned long value = (unsigned long) ptr;
-- 
1.5.6.5

Re: [PATCH] Correct printk %pF to work on all architectures

[Benjamin Herrenschmidt]

> Make dereference_function_descriptor() more accommodating by allowing
> architecture overrides.  I put the three overrides (for parisc64, ppc64
> and ia64) in arch/kernel/module.c because that's where the kernel
> internal linker which knows how to deal with function descriptors sits.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

RE: [PATCH] Correct printk %pF to work on all architectures

[Luck, Tony]

>> Make dereference_function_descriptor() more accommodating by allowing
>> architecture overrides.  I put the three overrides (for parisc64, ppc64
>> and ia64) in arch/kernel/module.c because that's where the kernel
>> internal linker which knows how to deal with function descriptors sits.
>>
>> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

ia64 bits still build, boot and work too.

Acked-by: Tony Luck <tony.luck@intel.com>

-Tony

RE: [PATCH] Correct printk %pF to work on all architectures

[James Bottomley]

OK, so could we get this in to -rc5 please?  It's a bug fix for parisc
since we're currently printing rubbish.

James

---
From: James Bottomley <James.Bottomley@HansenPartnership.com>
Date: Wed, 3 Sep 2008 20:43:36 -0500
Subject: lib: Correct printk %pF to work on all architectures

It was introduced by

commit 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Jul 6 16:43:12 2008 -0700

    vsprintf: add support for '%pS' and '%pF' pointer formats


However, the current way its coded doesn't work on parisc64.  For two
reasons:  1) parisc isn't in the #ifdef and 2) parisc has a different
format for function descriptors

Make dereference_function_descriptor() more accommodating by allowing
architecture overrides.  I put the three overrides (for parisc64, ppc64
and ia64) in arch/kernel/module.c because that's where the kernel
internal linker which knows how to deal with function descriptors sits.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Acked-by: Tony Luck <tony.luck@intel.com>
---
 arch/ia64/include/asm/sections.h    |    3 +++
 arch/ia64/kernel/module.c           |   12 ++++++++++++
 arch/parisc/kernel/module.c         |   14 ++++++++++++++
 arch/powerpc/include/asm/sections.h |    3 +++
 arch/powerpc/kernel/module_64.c     |   13 ++++++++++++-
 include/asm-generic/sections.h      |    6 ++++++
 include/asm-parisc/sections.h       |    5 +++++
 lib/vsprintf.c                      |   11 +----------
 8 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/arch/ia64/include/asm/sections.h b/arch/ia64/include/asm/sections.h
index 7286e4a..a7acad2 100644
--- a/arch/ia64/include/asm/sections.h
+++ b/arch/ia64/include/asm/sections.h
@@ -21,5 +21,8 @@ extern char __start_gate_brl_fsys_bubble_down_patchlist[], __end_gate_brl_fsys_b
 extern char __start_unwind[], __end_unwind[];
 extern char __start_ivt_text[], __end_ivt_text[];
 
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+
 #endif /* _ASM_IA64_SECTIONS_H */
 
diff --git a/arch/ia64/kernel/module.c b/arch/ia64/kernel/module.c
index 29aad34..545626f 100644
--- a/arch/ia64/kernel/module.c
+++ b/arch/ia64/kernel/module.c
@@ -31,9 +31,11 @@
 #include <linux/elf.h>
 #include <linux/moduleloader.h>
 #include <linux/string.h>
+#include <linux/uaccess.h>
 #include <linux/vmalloc.h>
 
 #include <asm/patch.h>
+#include <asm/sections.h>
 #include <asm/unaligned.h>
 
 #define ARCH_MODULE_DEBUG 0
@@ -941,3 +943,13 @@ module_arch_cleanup (struct module *mod)
 	if (mod->arch.core_unw_table)
 		unw_remove_unwind_table(mod->arch.core_unw_table);
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	struct fdesc *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->ip, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/arch/parisc/kernel/module.c b/arch/parisc/kernel/module.c
index fdacdd4..44138c3 100644
--- a/arch/parisc/kernel/module.c
+++ b/arch/parisc/kernel/module.c
@@ -47,7 +47,9 @@
 #include <linux/string.h>
 #include <linux/kernel.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 
+#include <asm/sections.h>
 #include <asm/unwind.h>
 
 #if 0
@@ -860,3 +862,15 @@ void module_arch_cleanup(struct module *mod)
 	deregister_unwind_table(mod);
 	module_bug_cleanup(mod);
 }
+
+#ifdef CONFIG_64BIT
+void *dereference_function_descriptor(void *ptr)
+{
+	Elf64_Fdesc *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->addr, p))
+		ptr = p;
+	return ptr;
+}
+#endif
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 916018e..7710e9e 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -16,6 +16,9 @@ static inline int in_kernel_text(unsigned long addr)
 	return 0;
 }
 
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+
 #endif
 
 #endif /* __KERNEL__ */
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index ee6a298..ad79de2 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -21,8 +21,9 @@
 #include <linux/err.h>
 #include <linux/vmalloc.h>
 #include <linux/bug.h>
+#include <linux/uaccess.h>
 #include <asm/module.h>
-#include <asm/uaccess.h>
+#include <asm/sections.h>
 #include <asm/firmware.h>
 #include <asm/code-patching.h>
 #include <linux/sort.h>
@@ -451,3 +452,13 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 
 	return 0;
 }
+
+void *dereference_function_descriptor(void *ptr)
+{
+	struct ppc64_opd_entry *desc = ptr;
+	void *p;
+
+	if (!probe_kernel_address(&desc->funcaddr, p))
+		ptr = p;
+	return ptr;
+}
diff --git a/include/asm-generic/sections.h b/include/asm-generic/sections.h
index 8feeae1..79a7ff9 100644
--- a/include/asm-generic/sections.h
+++ b/include/asm-generic/sections.h
@@ -14,4 +14,10 @@ extern char __kprobes_text_start[], __kprobes_text_end[];
 extern char __initdata_begin[], __initdata_end[];
 extern char __start_rodata[], __end_rodata[];
 
+/* function descriptor handling (if any).  Override
+ * in asm/sections.h */
+#ifndef dereference_function_descriptor
+#define dereference_function_descriptor(p) (p)
+#endif
+
 #endif /* _ASM_GENERIC_SECTIONS_H_ */
diff --git a/include/asm-parisc/sections.h b/include/asm-parisc/sections.h
index fdd43ec..9d13c35 100644
--- a/include/asm-parisc/sections.h
+++ b/include/asm-parisc/sections.h
@@ -4,4 +4,9 @@
 /* nothing to see, move along */
 #include <asm-generic/sections.h>
 
+#ifdef CONFIG_64BIT
+#undef dereference_function_descriptor
+void *dereference_function_descriptor(void *);
+#endif
+
 #endif
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index d8d1d11..c399bc1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/div64.h>
+#include <asm/sections.h>	/* for dereference_function_descriptor() */
 
 /* Works only for digits and letters, but small and fast */
 #define TOLOWER(x) ((x) | 0x20)
@@ -513,16 +514,6 @@ static char *string(char *buf, char *end, char *s, int field_width, int precisio
 	return buf;
 }
 
-static inline void *dereference_function_descriptor(void *ptr)
-{
-#if defined(CONFIG_IA64) || defined(CONFIG_PPC64)
-	void *p;
-	if (!probe_kernel_address(ptr, p))
-		ptr = p;
-#endif
-	return ptr;
-}
-
 static char *symbol_string(char *buf, char *end, void *ptr, int field_width, int precision, int flags)
 {
 	unsigned long value = (unsigned long) ptr;
-- 
1.5.5.1

Re: [PATCH] Correct printk %pF to work on all architectures

[Kyle McMartin]

On Tue, Sep 09, 2008 at 09:12:41AM -0500, James Bottomley wrote:
> OK, so could we get this in to -rc5 please?  It's a bug fix for parisc
> since we're currently printing rubbish.
> 

While I suppose it's a "parisc" patch, I'm not going to try to push it
unless either Linus just applies it, or we get an ack from the ppc/ia64
folks too.

... That said, please apply it, Linus. :)

Acked-by: Kyle McMartin <kyle@mcmartin.ca>

regards, Kyle

Re: [PATCH] Correct printk %pF to work on all architectures

[Benjamin Herrenschmidt]

On Tue, 2008-09-09 at 14:08 -0400, Kyle McMartin wrote:
> On Tue, Sep 09, 2008 at 09:12:41AM -0500, James Bottomley wrote:
> > OK, so could we get this in to -rc5 please?  It's a bug fix for parisc
> > since we're currently printing rubbish.
> > 
> 
> While I suppose it's a "parisc" patch, I'm not going to try to push it
> unless either Linus just applies it, or we get an ack from the ppc/ia64
> folks too.
> 
> ... That said, please apply it, Linus. :)
> 
> Acked-by: Kyle McMartin <kyle@mcmartin.ca>

Got one from us already but here it is again

Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Cheers,
Ben.