[PATCH] console_print: change the function api to make it have printk style interface

[PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

Hi Folks:

I bumped into this function in printk.c a while back. As I started using
it, I wondered if we could enhance it with printk style interface that
could make it somewhat more useful. 

One straightforward thing to do here would perhaps be to define this as
a macro. Something like this:

#define console_print(fmt,msg...) printk(KERN_EMERG fmt,##msg)

However, there might be some important reason why this was not done. I
do not know. 
Anyway, keeping the existing code of using a proper C function and
exporting the symbol, here's a patch that enhances the function to use
prink style interface. To the best of my knowledge, I do not think it
will break existing code that uses this function. I did a grep through
the kernel code and didn't look like this call was used in lot of other
places. I have also built the kernel locally with this change and looks
like at least for mips, it does not break our build.

CCing Linus for his opinion.

Cheers,

Ani


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
 drivers/char/serial167.c |    2 +-
 include/linux/tty.h      |    2 +-
 kernel/printk.c          |   14 ++++++++++++--
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..029aa0d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,7 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
+extern int console_print(const char *, ...);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..eb3e4fe 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -519,7 +519,7 @@ extern int pcxe_open(struct tty_struct *tty, struct
file *filp);
 
 /* printk.c */
 
-extern void console_print(const char *);
+extern int console_print(const char *, ...);
 
 /* vt.c */
 
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..729b0ff 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,9 +1060,19 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
+int console_print(const char *fmt, ...)
 {
-	printk(KERN_EMERG "%s", s);
+	va_list args;
+	int r;
+	char _fmt[1024] = KERN_EMERG;
+
+	strncat(_fmt, fmt,sizeof(_fmt)- sizeof(KERN_EMERG));
+
+	va_start(args, fmt);
+	r = vprintk(_fmt, args);
+	va_end(args);
+
+	return r;
 }
 EXPORT_SYMBOL(console_print);
 
-- 
1.6.4

Re: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Tue, 25 Aug 2009, Anirban Sinha wrote:
>  
> -void console_print(const char *s)
> +int console_print(const char *fmt, ...)
>  {
> -	printk(KERN_EMERG "%s", s);
> +	va_list args;
> +	int r;
> +	char _fmt[1024] = KERN_EMERG;

Don't do this. That's a 1kB stack frame right there. You'll overflow the 
stack very quickly.

			Linus

Re: [PATCH] console_print: change the function api to make it have printk style interface

[Randy Dunlap]

On Tue, 25 Aug 2009 16:47:49 -0700 Anirban Sinha wrote:

> Hi Folks:
> 
> I bumped into this function in printk.c a while back. As I started using
> it, I wondered if we could enhance it with printk style interface that
> could make it somewhat more useful. 
> 
> One straightforward thing to do here would perhaps be to define this as
> a macro. Something like this:
> 
> #define console_print(fmt,msg...) printk(KERN_EMERG fmt,##msg)
> 
> However, there might be some important reason why this was not done. I
> do not know. 
> Anyway, keeping the existing code of using a proper C function and
> exporting the symbol, here's a patch that enhances the function to use
> prink style interface. To the best of my knowledge, I do not think it
> will break existing code that uses this function. I did a grep through
> the kernel code and didn't look like this call was used in lot of other
> places. I have also built the kernel locally with this change and looks
> like at least for mips, it does not break our build.
> 
> CCing Linus for his opinion.
> 
> Cheers,
> 
> Ani
> 
> 
> Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
> ---
>  drivers/char/serial167.c |    2 +-
>  include/linux/tty.h      |    2 +-
>  kernel/printk.c          |   14 ++++++++++++--
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
> index 51e7a46..029aa0d 100644
> --- a/drivers/char/serial167.c
> +++ b/drivers/char/serial167.c
> @@ -171,7 +171,7 @@ static int startup(struct cyclades_port *);
>  static void cy_throttle(struct tty_struct *);
>  static void cy_unthrottle(struct tty_struct *);
>  static void config_setup(struct cyclades_port *);
> -extern void console_print(const char *);
> +extern int console_print(const char *, ...);

Drop that.  Add
#include <linux/tty.h>
if it's not already #included.


>  #ifdef CYCLOM_SHOW_STATUS
>  static void show_status(int);
>  #endif
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index e8c6c91..eb3e4fe 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -519,7 +519,7 @@ extern int pcxe_open(struct tty_struct *tty, struct
> file *filp);
>  
>  /* printk.c */
>  
> -extern void console_print(const char *);
> +extern int console_print(const char *, ...);

Make that prototype be like printk, with the __attribute__ ((format (printf, 1, 2))):


int printk(const char * fmt, ...)
	__attribute__ ((format (printf, 1, 2)));


>  
>  /* vt.c */
>  
> diff --git a/kernel/printk.c b/kernel/printk.c
> index b4d97b5..729b0ff 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1060,9 +1060,19 @@ void __sched console_conditional_schedule(void)
>  }
>  EXPORT_SYMBOL(console_conditional_schedule);
>  
> -void console_print(const char *s)
> +int console_print(const char *fmt, ...)
>  {
> -	printk(KERN_EMERG "%s", s);
> +	va_list args;
> +	int r;
> +	char _fmt[1024] = KERN_EMERG;
> +
> +	strncat(_fmt, fmt,sizeof(_fmt)- sizeof(KERN_EMERG));
> +
> +	va_start(args, fmt);
> +	r = vprintk(_fmt, args);
> +	va_end(args);
> +
> +	return r;
>  }
>  EXPORT_SYMBOL(console_print);
>  
> -- 


---
~Randy
LPC 2009, Sept. 23-25, Portland, Oregon
http://linuxplumbersconf.org/2009/

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>
>Don't do this. That's a 1kB stack frame right there. You'll overflow
the
>stack very quickly.

Ah! Of course you are correct! Can we simply have a macro instead of a
full blown function instead? If that is not acceptable, I will think of
something else to get around it.

Ani

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Tue, 25 Aug 2009, Anirban Sinha wrote:
> 
> Ah! Of course you are correct! Can we simply have a macro instead of a
> full blown function instead? If that is not acceptable, I will think of
> something else to get around it.

A macro is what others are generally doing.

I'd suggest looking at "dev_printk()" as an example. Sure, it basically 
requires that the first argument be a string constant (so that the 
automatic concatenations of string constants in C does its job), so it's 
not a _generic_ solution, but it's a very powerful and simple one.

In other words, you'll likely have to change a few existing

	console_print(data);

to

	console_print("%s", data);

to make things work, but hey, not a big deal.

That said, quite frankly, I suspect an even better solution would be to 
just remove the whole 'console_print()' function entirely, and just make 
people use 'printk()' or 'dev_printk()' instead. The only real valid 
reason for that thing existing in the first place is likely the 
_assembler_ interface of doing a printout of an ASCII string (see 
arch/ia64/kernel/head.S).

The other uses of 'console_print()' seem totally pointless, really. It's a 
stupid legacy interface that shouldn't exist, I'm afraid.

		Linus

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>A macro is what others are generally doing.


Linus, 

I completely agree with you on both points. When I first saw this
interface, the first question that popped in my mind was "why is it even
here?" And the very next question I asked myself was "why is this isn't
a macro?" 

As far as I can see, except for arch/ia64/kernel/head.S, among the codes
that actually uses this interface, others are wrapped within #if 0 ...
#endif blocks. So effectively, this is not being used at all :)


Cheers,

Ani

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>
>Make that prototype be like printk, with the __attribute__ ((format
>(printf, 1, 2))):
>
>
>int printk(const char * fmt, ...)
>	__attribute__ ((format (printf, 1, 2)));
>
>
>

Thanks Randy. These are all very nice comments on the patch. However, I
do think that having a full blown C function is a overkill for such a
thing (except for the case which Linus has already pointed out - use
from assembly code). At the very least we should make it a macro if we
at all decide to keep the interface. I am also completely ok to remove
the interface altogether. The arcs that need that sort of function can
then implement an appropriate version (macro vs proper C function)
locally for it's use.

Since Linus has already agreed for the later, I will post a patch here
to remove the function. I am not very certain as to what we should do
about the only one single case of its genuine use - call from
ia64/head.S. What I have done is one solution. Please feel free to
debate/disagree.

Cheers,

Ani


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
arch/ia64/kernel/head.S    |    1 +
arch/ia64/kernel/head.h    |    1 +
arch/ia64/kernel/process.c |    7 +++++++
drivers/char/serial167.c   |    5 ++---
include/linux/dtlk.h       |    6 +++---
include/linux/tty.h        |    4 ----
kernel/printk.c            |    6 ------
7 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index 23f846d..e6c5c3d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
+#include "head.h"
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET				\
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
new file mode 100644
index 0000000..2e2ac68
--- /dev/null
+++ b/arch/ia64/kernel/head.h
@@ -0,0 +1 @@
+extern void console_print(const char *s);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 5d7c0e5..c2bb591 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
 		show_stack(NULL, NULL);
 }
 
+/* local support for deprecated console_print */
+void 
+console_print(const char *s)
+{
+	printk(KERN_EMERG "%s", s);
+}
+
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
in_syscall)
 {
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..5942a9d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
@@ -245,7 +244,7 @@ void SP(char *data)
 {
 	unsigned long flags;
 	local_irq_save(flags);
-	console_print(data);
+	printk(KERN_EMERG "%s", data);
 	local_irq_restore(flags);
 }
 
@@ -255,7 +254,7 @@ void CP(char data)
 	unsigned long flags;
 	local_irq_save(flags);
 	scrn[0] = data;
-	console_print(scrn);
+	printk(KERN_EMERG "%c", scrn);
 	local_irq_restore(flags);
 }				/* CP */
 
diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
index 2896d90..4f93c45 100644
--- a/include/linux/dtlk.h
+++ b/include/linux/dtlk.h
@@ -4,15 +4,15 @@
 	{ \
 	  if(dtlk_trace) \
 	  { \
-	    console_print(text); \
-	    console_print("\n"); \
+	    printk(KERN_EMERG "%s", text);	\
+	    printk(KERN_EMERG "\n");		\
 	  } \
 	}
 
 #define TRACE_CHR(chr) \
 	{ \
 	  if(dtlk_trace) \
-	    console_print(chr); \
+	    printk(KERN_EMERG "%c", chr); \
 	} \
 
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..ba191e3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -517,10 +517,6 @@ extern void serial_console_init(void);
 
 extern int pcxe_open(struct tty_struct *tty, struct file *filp);
 
-/* printk.c */
-
-extern void console_print(const char *);
-
 /* vt.c */
 
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..32e15ce 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,12 +1060,6 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
-{
-	printk(KERN_EMERG "%s", s);
-}
-EXPORT_SYMBOL(console_print);
-
 void console_unblank(void)
 {
 	struct console *c;

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Linus Torvalds]

On Wed, 26 Aug 2009, Anirban Sinha wrote:
> 
> Since Linus has already agreed for the later, I will post a patch here
> to remove the function. I am not very certain as to what we should do
> about the only one single case of its genuine use - call from
> ia64/head.S. What I have done is one solution. Please feel free to
> debate/disagree.

Looks ok to me, but I'm not going to apply it in late -rc, since it 
doesn't really fix any bug. Feel free to re-send after 2.6.31 is out, or 
try to get it into one of the trees to be merged later.

		Linus

RE: [PATCH] console_print: change the function api to make it have printk style interface

[Anirban Sinha]

>Looks ok to me, but I'm not going to apply it in late -rc, since it
>doesn't really fix any bug. Feel free to re-send after 2.6.31 is out,
or
>try to get it into one of the trees to be merged later.
>

Thanks Linus. I will resend the patch once 2.6.31 is out of the door.

Ani

[PATCH] cleanup legacy console_print function

[Anirban Sinha]

Hi Linus:

>>Looks ok to me, but I'm not going to apply it in late -rc, since it
>>doesn't really fix any bug. Feel free to re-send after 2.6.31 is out,
>or
>>try to get it into one of the trees to be merged later.
>>
>
>Thanks Linus. I will resend the patch once 2.6.31 is out of the door.

Here's the patch again:


Subject: cleanup legacy console_print function

console_print() is an old legacy interface mostly unused in the entire
kernel tree. It's best to clean up its existing use and let developers
usew their own implementation of it as they feel fit.


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
arch/ia64/kernel/head.S    |    1 +
arch/ia64/kernel/head.h    |    1 +
arch/ia64/kernel/process.c |    7 +++++++
drivers/char/serial167.c   |    5 ++---
include/linux/dtlk.h       |    6 +++---
include/linux/tty.h        |    4 ----
kernel/printk.c            |    6 ------
7 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index 23f846d..e6c5c3d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
+#include "head.h"
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET				\
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
new file mode 100644
index 0000000..2e2ac68
--- /dev/null
+++ b/arch/ia64/kernel/head.h
@@ -0,0 +1 @@
+extern void console_print(const char *s);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 5d7c0e5..c2bb591 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
 		show_stack(NULL, NULL);
 }
 
+/* local support for deprecated console_print */
+void 
+console_print(const char *s)
+{
+	printk(KERN_EMERG "%s", s);
+}
+
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
in_syscall)
 {
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..5942a9d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
@@ -245,7 +244,7 @@ void SP(char *data)
 {
 	unsigned long flags;
 	local_irq_save(flags);
-	console_print(data);
+	printk(KERN_EMERG "%s", data);
 	local_irq_restore(flags);
 }
 
@@ -255,7 +254,7 @@ void CP(char data)
 	unsigned long flags;
 	local_irq_save(flags);
 	scrn[0] = data;
-	console_print(scrn);
+	printk(KERN_EMERG "%c", scrn);
 	local_irq_restore(flags);
 }				/* CP */
 
diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
index 2896d90..4f93c45 100644
--- a/include/linux/dtlk.h
+++ b/include/linux/dtlk.h
@@ -4,15 +4,15 @@
 	{ \
 	  if(dtlk_trace) \
 	  { \
-	    console_print(text); \
-	    console_print("\n"); \
+	    printk(KERN_EMERG "%s", text);	\
+	    printk(KERN_EMERG "\n");		\
 	  } \
 	}
 
 #define TRACE_CHR(chr) \
 	{ \
 	  if(dtlk_trace) \
-	    console_print(chr); \
+	    printk(KERN_EMERG "%c", chr); \
 	} \
 
 #endif
diff --git a/include/linux/tty.h b/include/linux/tty.h
index e8c6c91..ba191e3 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -517,10 +517,6 @@ extern void serial_console_init(void);
 
 extern int pcxe_open(struct tty_struct *tty, struct file *filp);
 
-/* printk.c */
-
-extern void console_print(const char *);
-
 /* vt.c */
 
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
diff --git a/kernel/printk.c b/kernel/printk.c
index b4d97b5..32e15ce 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1060,12 +1060,6 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
-{
-	printk(KERN_EMERG "%s", s);
-}
-EXPORT_SYMBOL(console_print);
-
 void console_unblank(void)
 {
 	struct console *c;

Re: [PATCH] cleanup legacy console_print function

[Linus Torvalds]

On Fri, 11 Sep 2009, Anirban Sinha wrote:
>
> -           console_print(text); \
> -           console_print("\n"); \
> +           printk(KERN_EMERG "%s", text);      \
> +           printk(KERN_EMERG "\n");            \

Just clean it up to be

	printk(KERN_EMERG "%s\n", text);

instead while at it.

>  #define TRACE_CHR(chr) \
>  	{ \
>  	  if(dtlk_trace) \
> -	    console_print(chr); \
> +	    printk(KERN_EMERG "%c", chr); \

That can't be right. The translation is to use "%s", not "%c".

		Linus

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>On Fri, 11 Sep 2009, Anirban Sinha wrote:
>>
>> -           console_print(text); \
>> -           console_print("\n"); \
>> +           printk(KERN_EMERG "%s", text);      \
>> +           printk(KERN_EMERG "\n");            \
>
>Just clean it up to be
>
>	printk(KERN_EMERG "%s\n", text);
>
>instead while at it.
>
>>  #define TRACE_CHR(chr) \
>>  	{ \
>>  	  if(dtlk_trace) \
>> -	    console_print(chr); \
>> +	    printk(KERN_EMERG "%c", chr); \
>
>That can't be right. The translation is to use "%s", not "%c".

Better still, for both the above cases, I will simply remove those macro
definitions. They are enclosed within {#if0 #endif}, so no code could
possibly be using it. If anyone has any strong reasons for keeping it
still, please raise your voice now.

Cheers,

Ani

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>
>The diffstat doesn't match the patch (doesn't include a change to
>process.c) ... but the patch itself is OK.
>
>Acked-by: Tony Luck <tony.luck@intel.com>

Yeah, my bad. I had a local change in that file which I also committed.
Anyway, here it is again:


Subject: Fix ia64 build breakage. 

On ia64 it is not necessary to provide any kind of "extern" declaration
before using external symbols.

Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
Acked-by: Tony Luck <tony.luck@intel.com>
---
arch/ia64/kernel/head.S |    1 -
arch/ia64/kernel/head.h |    1 -
2 files changed, 0 insertions(+), 2 deletions(-)
delete mode 100644 arch/ia64/kernel/head.h

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index e6c5c3d..23f846d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,7 +34,6 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
-#include "head.h"

 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET                            \
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
deleted file mode 100644
index 2e2ac68..0000000
--- a/arch/ia64/kernel/head.h
+++ /dev/null
@@ -1 +0,0 @@
-extern void console_print(const char *s);
--
1.6.4

Re: [PATCH] cleanup legacy console_print function

[Tony Luck]

On Tue, Sep 15, 2009 at 11:25 AM, Anirban Sinha
<ASinha@zeugmasystems.com> wrote:
>  arch/ia64/kernel/head.S    |    1 -
>  arch/ia64/kernel/head.h    |    1 -
>  arch/ia64/kernel/process.c |    2 +-
>  3 files changed, 1 insertions(+), 3 deletions(-)

The diffstat doesn't match the patch (doesn't include a change to
process.c) ... but the patch itself is OK.

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

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>On ia64 (and I think most other assemblers) it is not
>necessary to provide any kind of "extern" declaration
>before using external symbols.


Ok, here goes:

From: Anirban Sinha <asinha@zeugmasystems.com>
Date: Tue, 15 Sep 2009 11:19:29 -0700
Subject: Fix ia64 build breakage. 

On ia64 it is not necessary to provide any kind of "extern" declaration
before using external symbols.


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
 arch/ia64/kernel/head.S    |    1 -
 arch/ia64/kernel/head.h    |    1 -
 arch/ia64/kernel/process.c |    2 +-
 3 files changed, 1 insertions(+), 3 deletions(-)
 delete mode 100644 arch/ia64/kernel/head.h

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index e6c5c3d..23f846d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,7 +34,6 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
-#include "head.h"

 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET                            \
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
deleted file mode 100644
index 2e2ac68..0000000
--- a/arch/ia64/kernel/head.h
+++ /dev/null
@@ -1 +0,0 @@
-extern void console_print(const char *s);
--
1.6.4

RE: [PATCH] cleanup legacy console_print function

[Luck, Tony]

> Will it resolve the function symbol though? I dunno.
>
> I can drop the head.h completely and add:
>
> #ifndef __ASSEMBLY__
> extern void console_print(const char *s);
> #endif
>
> to head.S if that is a better option.

head.S is in assembler (hence the ".S" suffix).  So adding
code wrapped inside #ifndef __ASSEMBLY__ to it would be a
no-op.

On ia64 (and I think most other assemblers) it is not
necessary to provide any kind of "extern" declaration
before using external symbols.

-Tony

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>Umm.  Your new file "head.h" is only included by head.S ... perhaps it
>would be
>better to delete head.h entirely and drop the #include from head.S?
>

Will it resolve the function symbol though? I dunno.

I can drop the head.h completely and add:

#ifndef __ASSEMBLY__
 extern void console_print(const char *s);
#endif

to head.S if that is a better option.

  Ani

Re: [PATCH] cleanup legacy console_print function

[Tony Luck]

> --- a/arch/ia64/kernel/head.h
> +++ b/arch/ia64/kernel/head.h
> @@ -1 +1,3 @@
> +#ifndef __ASSEMBLY__
>  extern void console_print(const char *s);
> +#endif

Umm.  Your new file "head.h" is only included by head.S ... perhaps it would be
better to delete head.h entirely and drop the #include from head.S?

-Tony

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

Oops! Sorry for breaking the build. I do not have a intel 64 machine to
try my change. However, I believe this will help:

diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
index 2e2ac68..e762125 100644
--- a/arch/ia64/kernel/head.h
+++ b/arch/ia64/kernel/head.h
@@ -1 +1,3 @@
+#ifndef __ASSEMBLY__
 extern void console_print(const char *s);
+#endif


Can you please try this patch and test if this fixed it. If it does, I
will go ahead, commit the change locally and post the final patch.

   Ani


>-----Original Message-----
>From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
>Sent: Tuesday, September 15, 2009 10:27 AM
>To: Anirban Sinha
>Cc: Linus Torvalds; linux-kernel@vger.kernel.org; Randy Dunlap; linux-
>ia64@vger.kernel.org; Luck, Tony
>Subject: Re: [PATCH] cleanup legacy console_print function
>
>On Monday 14 September 2009 12:28:21 pm Anirban Sinha wrote:
>> From e096c1774450f8348a7af3b7e47800710a490212 Mon Sep 17 00:00:00
2001
>> From: Anirban Sinha <asinha@zeugmasystems.com>
>> Date: Mon, 14 Sep 2009 11:13:37 -0700
>> Subject: cleanup console_print()
>>
>> console_print() is an old legacy interface mostly unused in the
entire
>> kernel tree. It's best to clean up its existing use and let
developers
>> use their own implementation of it as they feel fit.
>
>This broke the ia64 build, please fix:
>
>arch/ia64/kernel/head.h: Assembler messages:
>arch/ia64/kernel/head.h:1: Error: Unknown opcode `extern void
>console_print(const char*s)'
>make[1]: *** [arch/ia64/kernel/head.o] Error 1
>make[1]: *** Waiting for unfinished jobs....
>
>Bjorn
>
>
>> ---
>>  arch/ia64/kernel/head.S    |    1 +
>>  arch/ia64/kernel/head.h    |    1 +
>>  arch/ia64/kernel/process.c |    7 +++++++
>>  drivers/char/serial167.c   |    5 ++---
>>  include/linux/dtlk.h       |   19 -------------------
>>  include/linux/tty.h        |    4 ----
>>  kernel/printk.c            |    6 ------
>>  7 files changed, 11 insertions(+), 32 deletions(-)
>>  create mode 100644 arch/ia64/kernel/head.h
>>
>> diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
>> index 23f846d..e6c5c3d 100644
>> --- a/arch/ia64/kernel/head.S
>> +++ b/arch/ia64/kernel/head.S
>> @@ -34,6 +34,7 @@
>>  #include <asm/mca_asm.h>
>>  #include <linux/init.h>
>>  #include <linux/linkage.h>
>> +#include "head.h"
>>
>>  #ifdef CONFIG_HOTPLUG_CPU
>>  #define SAL_PSR_BITS_TO_SET				\
>> diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
>> new file mode 100644
>> index 0000000..2e2ac68
>> --- /dev/null
>> +++ b/arch/ia64/kernel/head.h
>> @@ -0,0 +1 @@
>> +extern void console_print(const char *s);
>> diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
>> index 89969e9..b34bef7 100644
>> --- a/arch/ia64/kernel/process.c
>> +++ b/arch/ia64/kernel/process.c
>> @@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
>>  		show_stack(NULL, NULL);
>>  }
>>
>> +/* local support for deprecated console_print */
>> +void
>> +console_print(const char *s)
>> +{
>> +	printk(KERN_EMERG "%s", s);
>> +}
>> +
>>  void
>>  do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
>> in_syscall)
>>  {
>> diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
>> index 51e7a46..5942a9d 100644
>> --- a/drivers/char/serial167.c
>> +++ b/drivers/char/serial167.c
>> @@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
>>  static void cy_throttle(struct tty_struct *);
>>  static void cy_unthrottle(struct tty_struct *);
>>  static void config_setup(struct cyclades_port *);
>> -extern void console_print(const char *);
>>  #ifdef CYCLOM_SHOW_STATUS
>>  static void show_status(int);
>>  #endif
>> @@ -245,7 +244,7 @@ void SP(char *data)
>>  {
>>  	unsigned long flags;
>>  	local_irq_save(flags);
>> -	console_print(data);
>> +	printk(KERN_EMERG "%s", data);
>>  	local_irq_restore(flags);
>>  }
>>
>> @@ -255,7 +254,7 @@ void CP(char data)
>>  	unsigned long flags;
>>  	local_irq_save(flags);
>>  	scrn[0] = data;
>> -	console_print(scrn);
>> +	printk(KERN_EMERG "%c", scrn);
>>  	local_irq_restore(flags);
>>  }				/* CP */
>>
>> diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
>> index 2896d90..22a7b9a 100644
>> --- a/include/linux/dtlk.h
>> +++ b/include/linux/dtlk.h
>> @@ -1,22 +1,3 @@
>> -#if 0
>> -
>> -#define TRACE_TXT(text) \
>> -	{ \
>> -	  if(dtlk_trace) \
>> -	  { \
>> -	    console_print(text); \
>> -	    console_print("\n"); \
>> -	  } \
>> -	}
>> -
>> -#define TRACE_CHR(chr) \
>> -	{ \
>> -	  if(dtlk_trace) \
>> -	    console_print(chr); \
>> -	} \
>> -
>> -#endif
>> -
>>  #define DTLK_MINOR	0
>>  #define DTLK_IO_EXTENT	0x02
>>
>> diff --git a/include/linux/tty.h b/include/linux/tty.h
>> index 0d3974f..a916a31 100644
>> --- a/include/linux/tty.h
>> +++ b/include/linux/tty.h
>> @@ -519,10 +519,6 @@ extern void serial_console_init(void);
>>
>>  extern int pcxe_open(struct tty_struct *tty, struct file *filp);
>>
>> -/* printk.c */
>> -
>> -extern void console_print(const char *);
>> -
>>  /* vt.c */
>>
>>  extern int vt_ioctl(struct tty_struct *tty, struct file *file,
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index e10d193..602033a 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -1075,12 +1075,6 @@ void __sched
console_conditional_schedule(void)
>>  }
>>  EXPORT_SYMBOL(console_conditional_schedule);
>>
>> -void console_print(const char *s)
>> -{
>> -	printk(KERN_EMERG "%s", s);
>> -}
>> -EXPORT_SYMBOL(console_print);
>> -
>>  void console_unblank(void)
>>  {
>>  	struct console *c;
>

Re: [PATCH] cleanup legacy console_print function

[Bjorn Helgaas]

On Monday 14 September 2009 12:28:21 pm Anirban Sinha wrote:
> From e096c1774450f8348a7af3b7e47800710a490212 Mon Sep 17 00:00:00 2001
> From: Anirban Sinha <asinha@zeugmasystems.com>
> Date: Mon, 14 Sep 2009 11:13:37 -0700
> Subject: cleanup console_print()
> 
> console_print() is an old legacy interface mostly unused in the entire
> kernel tree. It's best to clean up its existing use and let developers
> use their own implementation of it as they feel fit.

This broke the ia64 build, please fix:

arch/ia64/kernel/head.h: Assembler messages:
arch/ia64/kernel/head.h:1: Error: Unknown opcode `extern void console_print(const char*s)'
make[1]: *** [arch/ia64/kernel/head.o] Error 1
make[1]: *** Waiting for unfinished jobs....

Bjorn


> ---
>  arch/ia64/kernel/head.S    |    1 +
>  arch/ia64/kernel/head.h    |    1 +
>  arch/ia64/kernel/process.c |    7 +++++++
>  drivers/char/serial167.c   |    5 ++---
>  include/linux/dtlk.h       |   19 -------------------
>  include/linux/tty.h        |    4 ----
>  kernel/printk.c            |    6 ------
>  7 files changed, 11 insertions(+), 32 deletions(-)
>  create mode 100644 arch/ia64/kernel/head.h
> 
> diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
> index 23f846d..e6c5c3d 100644
> --- a/arch/ia64/kernel/head.S
> +++ b/arch/ia64/kernel/head.S
> @@ -34,6 +34,7 @@
>  #include <asm/mca_asm.h>
>  #include <linux/init.h>
>  #include <linux/linkage.h>
> +#include "head.h"
>  
>  #ifdef CONFIG_HOTPLUG_CPU
>  #define SAL_PSR_BITS_TO_SET				\
> diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
> new file mode 100644
> index 0000000..2e2ac68
> --- /dev/null
> +++ b/arch/ia64/kernel/head.h
> @@ -0,0 +1 @@
> +extern void console_print(const char *s);
> diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
> index 89969e9..b34bef7 100644
> --- a/arch/ia64/kernel/process.c
> +++ b/arch/ia64/kernel/process.c
> @@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
>  		show_stack(NULL, NULL);
>  }
>  
> +/* local support for deprecated console_print */
> +void 
> +console_print(const char *s)
> +{
> +	printk(KERN_EMERG "%s", s);
> +}
> +
>  void
>  do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
> in_syscall)
>  {
> diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
> index 51e7a46..5942a9d 100644
> --- a/drivers/char/serial167.c
> +++ b/drivers/char/serial167.c
> @@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
>  static void cy_throttle(struct tty_struct *);
>  static void cy_unthrottle(struct tty_struct *);
>  static void config_setup(struct cyclades_port *);
> -extern void console_print(const char *);
>  #ifdef CYCLOM_SHOW_STATUS
>  static void show_status(int);
>  #endif
> @@ -245,7 +244,7 @@ void SP(char *data)
>  {
>  	unsigned long flags;
>  	local_irq_save(flags);
> -	console_print(data);
> +	printk(KERN_EMERG "%s", data);
>  	local_irq_restore(flags);
>  }
>  
> @@ -255,7 +254,7 @@ void CP(char data)
>  	unsigned long flags;
>  	local_irq_save(flags);
>  	scrn[0] = data;
> -	console_print(scrn);
> +	printk(KERN_EMERG "%c", scrn);
>  	local_irq_restore(flags);
>  }				/* CP */
>  
> diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
> index 2896d90..22a7b9a 100644
> --- a/include/linux/dtlk.h
> +++ b/include/linux/dtlk.h
> @@ -1,22 +1,3 @@
> -#if 0
> -
> -#define TRACE_TXT(text) \
> -	{ \
> -	  if(dtlk_trace) \
> -	  { \
> -	    console_print(text); \
> -	    console_print("\n"); \
> -	  } \
> -	}
> -
> -#define TRACE_CHR(chr) \
> -	{ \
> -	  if(dtlk_trace) \
> -	    console_print(chr); \
> -	} \
> -
> -#endif
> -
>  #define DTLK_MINOR	0
>  #define DTLK_IO_EXTENT	0x02
>  
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 0d3974f..a916a31 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -519,10 +519,6 @@ extern void serial_console_init(void);
>  
>  extern int pcxe_open(struct tty_struct *tty, struct file *filp);
>  
> -/* printk.c */
> -
> -extern void console_print(const char *);
> -
>  /* vt.c */
>  
>  extern int vt_ioctl(struct tty_struct *tty, struct file *file,
> diff --git a/kernel/printk.c b/kernel/printk.c
> index e10d193..602033a 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1075,12 +1075,6 @@ void __sched console_conditional_schedule(void)
>  }
>  EXPORT_SYMBOL(console_conditional_schedule);
>  
> -void console_print(const char *s)
> -{
> -	printk(KERN_EMERG "%s", s);
> -}
> -EXPORT_SYMBOL(console_print);
> -
>  void console_unblank(void)
>  {
>  	struct console *c;

RE: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

>
>Here you go!
>

Thanks Linus for committing the patch.

    Ani

Re: [PATCH] cleanup legacy console_print function

[Anirban Sinha]

Here you go!


>From e096c1774450f8348a7af3b7e47800710a490212 Mon Sep 17 00:00:00 2001
From: Anirban Sinha <asinha@zeugmasystems.com>
Date: Mon, 14 Sep 2009 11:13:37 -0700
Subject: cleanup console_print()

console_print() is an old legacy interface mostly unused in the entire
kernel tree. It's best to clean up its existing use and let developers
use their own implementation of it as they feel fit.


Signed-off-by: Anirban Sinha <asinha@zeugmasystems.com>
---
 arch/ia64/kernel/head.S    |    1 +
 arch/ia64/kernel/head.h    |    1 +
 arch/ia64/kernel/process.c |    7 +++++++
 drivers/char/serial167.c   |    5 ++---
 include/linux/dtlk.h       |   19 -------------------
 include/linux/tty.h        |    4 ----
 kernel/printk.c            |    6 ------
 7 files changed, 11 insertions(+), 32 deletions(-)
 create mode 100644 arch/ia64/kernel/head.h

diff --git a/arch/ia64/kernel/head.S b/arch/ia64/kernel/head.S
index 23f846d..e6c5c3d 100644
--- a/arch/ia64/kernel/head.S
+++ b/arch/ia64/kernel/head.S
@@ -34,6 +34,7 @@
 #include <asm/mca_asm.h>
 #include <linux/init.h>
 #include <linux/linkage.h>
+#include "head.h"
 
 #ifdef CONFIG_HOTPLUG_CPU
 #define SAL_PSR_BITS_TO_SET				\
diff --git a/arch/ia64/kernel/head.h b/arch/ia64/kernel/head.h
new file mode 100644
index 0000000..2e2ac68
--- /dev/null
+++ b/arch/ia64/kernel/head.h
@@ -0,0 +1 @@
+extern void console_print(const char *s);
diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 89969e9..b34bef7 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -161,6 +161,13 @@ show_regs (struct pt_regs *regs)
 		show_stack(NULL, NULL);
 }
 
+/* local support for deprecated console_print */
+void 
+console_print(const char *s)
+{
+	printk(KERN_EMERG "%s", s);
+}
+
 void
 do_notify_resume_user(sigset_t *unused, struct sigscratch *scr, long
in_syscall)
 {
diff --git a/drivers/char/serial167.c b/drivers/char/serial167.c
index 51e7a46..5942a9d 100644
--- a/drivers/char/serial167.c
+++ b/drivers/char/serial167.c
@@ -171,7 +171,6 @@ static int startup(struct cyclades_port *);
 static void cy_throttle(struct tty_struct *);
 static void cy_unthrottle(struct tty_struct *);
 static void config_setup(struct cyclades_port *);
-extern void console_print(const char *);
 #ifdef CYCLOM_SHOW_STATUS
 static void show_status(int);
 #endif
@@ -245,7 +244,7 @@ void SP(char *data)
 {
 	unsigned long flags;
 	local_irq_save(flags);
-	console_print(data);
+	printk(KERN_EMERG "%s", data);
 	local_irq_restore(flags);
 }
 
@@ -255,7 +254,7 @@ void CP(char data)
 	unsigned long flags;
 	local_irq_save(flags);
 	scrn[0] = data;
-	console_print(scrn);
+	printk(KERN_EMERG "%c", scrn);
 	local_irq_restore(flags);
 }				/* CP */
 
diff --git a/include/linux/dtlk.h b/include/linux/dtlk.h
index 2896d90..22a7b9a 100644
--- a/include/linux/dtlk.h
+++ b/include/linux/dtlk.h
@@ -1,22 +1,3 @@
-#if 0
-
-#define TRACE_TXT(text) \
-	{ \
-	  if(dtlk_trace) \
-	  { \
-	    console_print(text); \
-	    console_print("\n"); \
-	  } \
-	}
-
-#define TRACE_CHR(chr) \
-	{ \
-	  if(dtlk_trace) \
-	    console_print(chr); \
-	} \
-
-#endif
-
 #define DTLK_MINOR	0
 #define DTLK_IO_EXTENT	0x02
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 0d3974f..a916a31 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -519,10 +519,6 @@ extern void serial_console_init(void);
 
 extern int pcxe_open(struct tty_struct *tty, struct file *filp);
 
-/* printk.c */
-
-extern void console_print(const char *);
-
 /* vt.c */
 
 extern int vt_ioctl(struct tty_struct *tty, struct file *file,
diff --git a/kernel/printk.c b/kernel/printk.c
index e10d193..602033a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1075,12 +1075,6 @@ void __sched console_conditional_schedule(void)
 }
 EXPORT_SYMBOL(console_conditional_schedule);
 
-void console_print(const char *s)
-{
-	printk(KERN_EMERG "%s", s);
-}
-EXPORT_SYMBOL(console_print);
-
 void console_unblank(void)
 {
 	struct console *c;
-- 
1.6.4