On top fixes for my last patches

On top fixes for my last patches

[Thomas Renninger]

Hi,

I was a bit too quick... Adding Andrew's suggestions let
two bugs slip in.

The missing statics make the kernel not compile if ondemand
and conservative are compiled with =y.

The "replace WARN_ONCE with printk" I simply overlooked and
is just to avoid the stack_dump in the message.

Both could be fixed by replacing things in the patches themselves
(without a newline),
but as they were already committed, on top patches are probably
better?

Sorry and thanks,

   Thomas

[PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 83515f1..5aa832f 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -1247,12 +1247,12 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
 			 * thing gets introduced
 			 */
 			if (!print_once) {
-				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
-					  "does not provide ACPI _PSS objects "
-					  "in a way that Linux understands. "
-					  "Please report this to the Linux ACPI"
-					  " maintainers and complain to your "
-					  "BIOS vendor.\n");
+				printk(KERN_ERR FW_BUG PFX "Your BIOS "
+				       "does not provide ACPI _PSS objects "
+				       "in a way that Linux understands. "
+				       "Please report this to the Linux ACPI"
+				       " maintainers and complain to your "
+				       "BIOS vendor.\n");
 				print_once++;
 			}
 			goto err_out;
-- 
1.6.0.2

[PATCH 2/2] CPUFREQ: Use static or it won't compile if conservative and ondemand are set =y

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/cpufreq/cpufreq_conservative.c |    2 +-
 drivers/cpufreq/cpufreq_ondemand.c     |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index 3aa99e6..8d541c6 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -60,7 +60,7 @@ static unsigned int def_sampling_rate;
  *   - MIN_STAT_SAMPLING_RATE
  * To avoid that userspace shoots itself.
 */
-unsigned int minimum_sampling_rate(void)
+static unsigned int minimum_sampling_rate(void)
 {
 	return max(def_sampling_rate / 10, MIN_STAT_SAMPLING_RATE);
 }
diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
index 3220749..338f428 100644
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -58,7 +58,7 @@ static unsigned int def_sampling_rate;
  *   - MIN_STAT_SAMPLING_RATE
  * To avoid that userspace shoots itself.
 */
-unsigned int minimum_sampling_rate(void)
+static unsigned int minimum_sampling_rate(void)
 {
 	return max(def_sampling_rate / 10, MIN_STAT_SAMPLING_RATE);
 }
-- 
1.6.0.2

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Ingo Molnar]

* Thomas Renninger <trenn@suse.de> wrote:

> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> index 83515f1..5aa832f 100644
> --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> @@ -1247,12 +1247,12 @@ static int __cpuinit powernowk8_cpu_init(struct cpufreq_policy *pol)
>  			 * thing gets introduced
>  			 */
>  			if (!print_once) {
> -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> -					  "does not provide ACPI _PSS objects "
> -					  "in a way that Linux understands. "
> -					  "Please report this to the Linux ACPI"
> -					  " maintainers and complain to your "
> -					  "BIOS vendor.\n");
> +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> +				       "does not provide ACPI _PSS objects "
> +				       "in a way that Linux understands. "
> +				       "Please report this to the Linux ACPI"
> +				       " maintainers and complain to your "
> +				       "BIOS vendor.\n");
>  				print_once++;

hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
essence is)

So please use WARN_ONCE(), and indent it all one tab to the left which will 
solve at least part of that ugly 6-line split up thing. And if it's a 
WARN_ONCE() then kerneloops.org will pick it up too.

	Ingo

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Thomas Renninger]

On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> 
> * Thomas Renninger <trenn@suse.de> wrote:
> 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > ---
> >  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c 
b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > index 83515f1..5aa832f 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > @@ -1247,12 +1247,12 @@ static int __cpuinit powernowk8_cpu_init(struct 
cpufreq_policy *pol)
> >  			 * thing gets introduced
> >  			 */
> >  			if (!print_once) {
> > -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > -					  "does not provide ACPI _PSS objects "
> > -					  "in a way that Linux understands. "
> > -					  "Please report this to the Linux ACPI"
> > -					  " maintainers and complain to your "
> > -					  "BIOS vendor.\n");
> > +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > +				       "does not provide ACPI _PSS objects "
> > +				       "in a way that Linux understands. "
> > +				       "Please report this to the Linux ACPI"
> > +				       " maintainers and complain to your "
> > +				       "BIOS vendor.\n");
> >  				print_once++;
> 
> hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
> essence is)
> 
> So please use WARN_ONCE(), and indent it all one tab to the left which will 
> solve at least part of that ugly 6-line split up thing. And if it's a 
> WARN_ONCE() then kerneloops.org will pick it up too.
No.
This happens if your BIOS is older than your CPU and you then miss cpufreq.
This often happens on very new machines/CPUs. It could also happen that you
have to wait a month or so until your vendor offers a new BIOS.

We want to tell the user that it's not the kernel's fault, but we better do
not spit out a huge backtrace, which is worthless anyway as it's the BIOS
which is broken.

     Thomas

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Thomas Renninger]

On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> 
> * Thomas Renninger <trenn@suse.de> wrote:
> 
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > ---
> >  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c 
b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > index 83515f1..5aa832f 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > @@ -1247,12 +1247,12 @@ static int __cpuinit powernowk8_cpu_init(struct 
cpufreq_policy *pol)
> >  			 * thing gets introduced
> >  			 */
> >  			if (!print_once) {
> > -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > -					  "does not provide ACPI _PSS objects "
> > -					  "in a way that Linux understands. "
> > -					  "Please report this to the Linux ACPI"
> > -					  " maintainers and complain to your "
> > -					  "BIOS vendor.\n");
> > +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > +				       "does not provide ACPI _PSS objects "
> > +				       "in a way that Linux understands. "
> > +				       "Please report this to the Linux ACPI"
> > +				       " maintainers and complain to your "
> > +				       "BIOS vendor.\n");
> >  				print_once++;
> 
> hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
> essence is)
Looking at WARN() again:
#ifndef __WARN
#ifndef __ASSEMBLY__
extern void warn_slowpath(const char *file, const int line,
                const char *fmt, ...) __attribute__((format(printf, 3, 4)));
#define WANT_WARN_ON_SLOWPATH
#endif
#define __WARN()                warn_slowpath(__FILE__, __LINE__, NULL)
#define __WARN_printf(arg...)   warn_slowpath(__FILE__, __LINE__, arg)
#else
#define __WARN_printf(arg...)   do { printk(arg); __WARN(); } while (0)
#endif

WARN_ONCE does throw a backtrace (warn_slowpath does) or I missed
something...
Thus WARN_ONCE makes a big difference to printk_once() (which does not
exist? but would be neat...) and prints out the backtrace, right?

Thanks,

     Thomas

> 
> So please use WARN_ONCE(), and indent it all one tab to the left which will 
> solve at least part of that ugly 6-line split up thing. And if it's a 
> WARN_ONCE() then kerneloops.org will pick it up too.
> 

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Ingo Molnar]

* Thomas Renninger <trenn@suse.de> wrote:

> On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> > 
> > * Thomas Renninger <trenn@suse.de> wrote:
> > 
> > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > ---
> > >  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
> > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c 
> b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > index 83515f1..5aa832f 100644
> > > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > @@ -1247,12 +1247,12 @@ static int __cpuinit powernowk8_cpu_init(struct 
> cpufreq_policy *pol)
> > >  			 * thing gets introduced
> > >  			 */
> > >  			if (!print_once) {
> > > -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > > -					  "does not provide ACPI _PSS objects "
> > > -					  "in a way that Linux understands. "
> > > -					  "Please report this to the Linux ACPI"
> > > -					  " maintainers and complain to your "
> > > -					  "BIOS vendor.\n");
> > > +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > > +				       "does not provide ACPI _PSS objects "
> > > +				       "in a way that Linux understands. "
> > > +				       "Please report this to the Linux ACPI"
> > > +				       " maintainers and complain to your "
> > > +				       "BIOS vendor.\n");
> > >  				print_once++;
> > 
> > hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
> > essence is)
> > 
> > So please use WARN_ONCE(), and indent it all one tab to the left which will 
> > solve at least part of that ugly 6-line split up thing. And if it's a 
> > WARN_ONCE() then kerneloops.org will pick it up too.
> No.
> This happens if your BIOS is older than your CPU and you then miss cpufreq.
> This often happens on very new machines/CPUs. It could also happen that you
> have to wait a month or so until your vendor offers a new BIOS.
> 
> We want to tell the user that it's not the kernel's fault, but we better do
> not spit out a huge backtrace, which is worthless anyway as it's the BIOS
> which is broken.

That's fine and we can do that, but the text does not suggest that at all.

The text says "please report this to the Linux ACPI maintainers" and that 
Linux does not understand this - and closes with the suggestion that this 
should be reported to the BIOS vendor too. That tells, to the user, that at 
minimum Linux is confused.

Such text directs the bugreports to _us_ kernel maintainers, not to the BIOS 
vendors.

A much clearer text and implementation would be to do something like:

static const char ACPI_PSS_BIOS_BUG_MSG[] =
  KERN_ERR "Your BIOS does not provide compatible ACPI _PSS objects.\n"
  KERN_ERR "Complain to your BIOS vendor. This is not a kernel bug.\n";

[...]

  			if (!print_once) {
				printk(ACPI_PSS_BIOS_BUG_MSG);
				print_once = 1;
			}

Note the improvements:

 - No more ugly linebreaks.

 - print_once++ was a poor solution as well - the standard thing is to set
   'once' flags to 1 - once and forever.

 - The 6-line split-up warning message does not obscure the code 
   itself anymore. The error condition is clear and clean and visually 
   unintrusive.

 - The original message text had no linebreak and was about two full lines
   long when printed - in a single line. If the kernel prints such messages 
   that looks sloppy and confusing. If watched via a serial line then the 
   overlong portion can even be missed at first sight.

 - If someone hits that warning and sees it in the kernel log, then a
   git grep ""Your BIOS does not provide compatible ACPI _PSS objects"
   will come up with arch/x86/kernel/cpu/cpufreq/powernow-k8.c. With the 
   original code it would come up empty and the user/developer would perhaps 
   thing that it's perhaps the distro kernel that prints that warning, not 
   the upstream kernel.

Could you please fix it in that fashion? Thanks,

	Ingo

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Ingo Molnar]

* Thomas Renninger <trenn@suse.de> wrote:

> Looking at WARN() again:
> #ifndef __WARN
> #ifndef __ASSEMBLY__
> extern void warn_slowpath(const char *file, const int line,
>                 const char *fmt, ...) __attribute__((format(printf, 3, 4)));
> #define WANT_WARN_ON_SLOWPATH
> #endif
> #define __WARN()                warn_slowpath(__FILE__, __LINE__, NULL)
> #define __WARN_printf(arg...)   warn_slowpath(__FILE__, __LINE__, arg)
> #else
> #define __WARN_printf(arg...)   do { printk(arg); __WARN(); } while (0)
> #endif
> 
> WARN_ONCE does throw a backtrace (warn_slowpath does) or I missed
> something...
> Thus WARN_ONCE makes a big difference to printk_once() (which does not
> exist? but would be neat...) and prints out the backtrace, right?

yes, correct. We use WARN()/WARN_ONCE() in places where an error is 
surprising and where we want to print a backtrace too.

In this case you are right to point out that it's not a kernel bug but a 
BIOS environment bug, and that the text itself uniquely identifies the place 
it comes from. So using a printk is perfectly fine.

There's a few cleanups necessary with the printk solution too though, see my 
previous mail for the details.

printk_once() would be nice indeed - it's a frequent construct.

	Ingo

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Thomas Renninger]

On Thursday 05 February 2009 13:33:31 Ingo Molnar wrote:
> 
> * Thomas Renninger <trenn@suse.de> wrote:
> 
> > On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> > > 
> > > * Thomas Renninger <trenn@suse.de> wrote:
> > > 
> > > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > > ---
> > > >  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
> > > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c 
> > b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > index 83515f1..5aa832f 100644
> > > > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > @@ -1247,12 +1247,12 @@ static int __cpuinit 
powernowk8_cpu_init(struct 
> > cpufreq_policy *pol)
> > > >  			 * thing gets introduced
> > > >  			 */
> > > >  			if (!print_once) {
> > > > -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > > > -					  "does not provide ACPI _PSS objects "
> > > > -					  "in a way that Linux understands. "
> > > > -					  "Please report this to the Linux ACPI"
> > > > -					  " maintainers and complain to your "
> > > > -					  "BIOS vendor.\n");
> > > > +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > > > +				       "does not provide ACPI _PSS objects "
> > > > +				       "in a way that Linux understands. "
> > > > +				       "Please report this to the Linux ACPI"
> > > > +				       " maintainers and complain to your "
> > > > +				       "BIOS vendor.\n");
> > > >  				print_once++;
> > > 
> > > hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
> > > essence is)
> > > 
> > > So please use WARN_ONCE(), and indent it all one tab to the left which 
will 
> > > solve at least part of that ugly 6-line split up thing. And if it's a 
> > > WARN_ONCE() then kerneloops.org will pick it up too.
> > No.
> > This happens if your BIOS is older than your CPU and you then miss 
cpufreq.
> > This often happens on very new machines/CPUs. It could also happen that 
you
> > have to wait a month or so until your vendor offers a new BIOS.
> > 
> > We want to tell the user that it's not the kernel's fault, but we better 
do
> > not spit out a huge backtrace, which is worthless anyway as it's the BIOS
> > which is broken.
> 
> That's fine and we can do that, but the text does not suggest that at all.
> 
> The text says "please report this to the Linux ACPI maintainers" and that 
> Linux does not understand this - and closes with the suggestion that this 
> should be reported to the BIOS vendor too. That tells, to the user, that at 
> minimum Linux is confused.
> 
> Such text directs the bugreports to _us_ kernel maintainers, not to the BIOS 
> vendors.
> 
> A much clearer text and implementation would be to do something like:
> 
> static const char ACPI_PSS_BIOS_BUG_MSG[] =
>   KERN_ERR "Your BIOS does not provide compatible ACPI _PSS objects.\n"
>   KERN_ERR "Complain to your BIOS vendor. This is not a kernel bug.\n";

Yep, even better:
   KERN_ERR FW_BUG "Incompatible ACPI _PSS objects.\n"
   KERN_ERR FW_BUG "Complain to your BIOS vendor.\n";

Then distributions easily can do:
dmesg |grep '[Firmware Bug]'

and reject the BIOS to get certified or throw a bug back to the
vendor.

I expect the long version still comes from the times when one
could not be sure whether it's the Linux ACPI subsystem or the
BIOS table which is wrong. I agree, that mentioning the kernel to
possibly be fault, should be deleted.

> [...]
> 
>   			if (!print_once) {
> 				printk(ACPI_PSS_BIOS_BUG_MSG);
> 				print_once = 1;
> 			}
> 
> Note the improvements:
> 
>  - No more ugly linebreaks.
> 
>  - print_once++ was a poor solution as well - the standard thing is to set
>    'once' flags to 1 - once and forever.
Hm, it's only incremented once, I do not see why this is a poor solution.
perfect would be printk_once() similar to WARN_ONCE.
Andrew mentioned a discussion about implementing such a thing.
IMO it would be worth it, I needed something like that three times in the
last 7 patches.

> 
>  - The 6-line split-up warning message does not obscure the code 
>    itself anymore. The error condition is clear and clean and visually 
>    unintrusive.
> 
>  - The original message text had no linebreak and was about two full lines
>    long when printed - in a single line. If the kernel prints such messages 
>    that looks sloppy and confusing. If watched via a serial line then the 
>    overlong portion can even be missed at first sight.
> 
>  - If someone hits that warning and sees it in the kernel log, then a
>    git grep ""Your BIOS does not provide compatible ACPI _PSS objects"
>    will come up with arch/x86/kernel/cpu/cpufreq/powernow-k8.c. With the 
>    original code it would come up empty and the user/developer would perhaps 
>    thing that it's perhaps the distro kernel that prints that warning, not 
>    the upstream kernel.
> 
> Could you please fix it in that fashion? Thanks,
I fully agree with the "no line break" and not "not grepable" issues.
I send something new, maybe not today.

Thanks,

      Thomas

[tip:core/printk] printk: introduce printk_once()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> printk_once() would be nice indeed - it's a frequent construct.

Something like the patch below?

	Ingo

---------------->
Author:     Ingo Molnar <mingo@elte.hu>
AuthorDate: Thu, 5 Feb 2009 13:45:43 +0100
Commit:     Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 5 Feb 2009 13:52:29 +0100

printk: introduce printk_once()

This pattern shows up frequently in the kernel:

  static int once = 1;
  ...

		if (once) {
			once = 0;
			printk(KERN_ERR "message\n");
		}
  ...

So add a printk_once() helper macro that reduces this to a single line
of:

		printk_once(KERN_ERR "message\n");

It works analogously to WARN_ONCE() & friends. (We use a macro not
an inline because vararg expansion in inlines looks awkward and the
macro is simple enough.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>


---
 include/linux/kernel.h |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 343df9e..3c183d9 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -242,6 +242,19 @@ extern struct ratelimit_state printk_ratelimit_state;
 extern int printk_ratelimit(void);
 extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 				   unsigned int interval_msec);
+
+/*
+ * Print a one-time message (analogous to WARN_ONCE() et al):
+ */
+#define printk_once(x...) ({			\
+	static int __print_once = 1;		\
+						\
+	if (__print_once) {			\
+		__print_once = 0;		\
+		printk(x);			\
+	}					\
+})
+
 #else
 static inline int vprintk(const char *s, va_list args)
 	__attribute__ ((format (printf, 1, 0)));
@@ -253,6 +266,10 @@ static inline int printk_ratelimit(void) { return 0; }
 static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
 					  unsigned int interval_msec)	\
 		{ return false; }
+
+/* No effect, but we still get type checking even in the !PRINTK case: */
+#define printk_once(x...) printk(x)
+
 #endif
 
 extern int printk_needs_cpu(int cpu);

Re: [PATCH 1/2] CPUFREQ: powernow-k8: Forgot to use printk instead of WARN_ONCE in last patch

[Ingo Molnar]

* Thomas Renninger <trenn@suse.de> wrote:

> On Thursday 05 February 2009 13:33:31 Ingo Molnar wrote:
> > 
> > * Thomas Renninger <trenn@suse.de> wrote:
> > 
> > > On Thursday 05 February 2009 13:02:03 Ingo Molnar wrote:
> > > > 
> > > > * Thomas Renninger <trenn@suse.de> wrote:
> > > > 
> > > > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > > > ---
> > > > >  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |   12 ++++++------
> > > > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c 
> > > b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > index 83515f1..5aa832f 100644
> > > > > --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> > > > > @@ -1247,12 +1247,12 @@ static int __cpuinit 
> powernowk8_cpu_init(struct 
> > > cpufreq_policy *pol)
> > > > >  			 * thing gets introduced
> > > > >  			 */
> > > > >  			if (!print_once) {
> > > > > -				WARN_ONCE(1, KERN_ERR FW_BUG PFX "Your BIOS "
> > > > > -					  "does not provide ACPI _PSS objects "
> > > > > -					  "in a way that Linux understands. "
> > > > > -					  "Please report this to the Linux ACPI"
> > > > > -					  " maintainers and complain to your "
> > > > > -					  "BIOS vendor.\n");
> > > > > +				printk(KERN_ERR FW_BUG PFX "Your BIOS "
> > > > > +				       "does not provide ACPI _PSS objects "
> > > > > +				       "in a way that Linux understands. "
> > > > > +				       "Please report this to the Linux ACPI"
> > > > > +				       " maintainers and complain to your "
> > > > > +				       "BIOS vendor.\n");
> > > > >  				print_once++;
> > > > 
> > > > hm, why the open-coded WARN_ONCE? (which print_once flag + the printk in 
> > > > essence is)
> > > > 
> > > > So please use WARN_ONCE(), and indent it all one tab to the left which 
> will 
> > > > solve at least part of that ugly 6-line split up thing. And if it's a 
> > > > WARN_ONCE() then kerneloops.org will pick it up too.
> > > No.
> > > This happens if your BIOS is older than your CPU and you then miss 
> cpufreq.
> > > This often happens on very new machines/CPUs. It could also happen that 
> you
> > > have to wait a month or so until your vendor offers a new BIOS.
> > > 
> > > We want to tell the user that it's not the kernel's fault, but we better 
> do
> > > not spit out a huge backtrace, which is worthless anyway as it's the BIOS
> > > which is broken.
> > 
> > That's fine and we can do that, but the text does not suggest that at all.
> > 
> > The text says "please report this to the Linux ACPI maintainers" and that 
> > Linux does not understand this - and closes with the suggestion that this 
> > should be reported to the BIOS vendor too. That tells, to the user, that at 
> > minimum Linux is confused.
> > 
> > Such text directs the bugreports to _us_ kernel maintainers, not to the BIOS 
> > vendors.
> > 
> > A much clearer text and implementation would be to do something like:
> > 
> > static const char ACPI_PSS_BIOS_BUG_MSG[] =
> >   KERN_ERR "Your BIOS does not provide compatible ACPI _PSS objects.\n"
> >   KERN_ERR "Complain to your BIOS vendor. This is not a kernel bug.\n";
> 
> Yep, even better:
>    KERN_ERR FW_BUG "Incompatible ACPI _PSS objects.\n"
>    KERN_ERR FW_BUG "Complain to your BIOS vendor.\n";
> 
> Then distributions easily can do:
> dmesg |grep '[Firmware Bug]'
> 
> and reject the BIOS to get certified or throw a bug back to the
> vendor.
> 
> I expect the long version still comes from the times when one
> could not be sure whether it's the Linux ACPI subsystem or the
> BIOS table which is wrong. I agree, that mentioning the kernel to
> possibly be fault, should be deleted.
> 
> > [...]
> > 
> >   			if (!print_once) {
> > 				printk(ACPI_PSS_BIOS_BUG_MSG);
> > 				print_once = 1;
> > 			}
> > 
> > Note the improvements:
> > 
> >  - No more ugly linebreaks.
> > 
> >  - print_once++ was a poor solution as well - the standard thing is to set
> >    'once' flags to 1 - once and forever.
> Hm, it's only incremented once, I do not see why this is a poor solution.

It's not a huge issue - it's just somewhat suboptimal as a coding construct 
because it's a bit dissimilar to how things are done typically. When i first 
saw it i had to look again because it looked a bit odd - first i was unsure 
whether the ++ has an actual _meaning_.

It seems like an insignifican detail (which it really is, if looked at in 
isolation), but we have a huge, 10 MLOC kernel full of code. So we really 
want elegant-looking, pleasant, predictable, efficient and standard patterns 
of code everywhere.

> perfect would be printk_once() similar to WARN_ONCE.
> Andrew mentioned a discussion about implementing such a thing.
> IMO it would be worth it, I needed something like that three times in the
> last 7 patches.

Yeah - i just posted it.

> >  - The 6-line split-up warning message does not obscure the code 
> >    itself anymore. The error condition is clear and clean and visually 
> >    unintrusive.
> > 
> >  - The original message text had no linebreak and was about two full lines
> >    long when printed - in a single line. If the kernel prints such messages 
> >    that looks sloppy and confusing. If watched via a serial line then the 
> >    overlong portion can even be missed at first sight.
> > 
> >  - If someone hits that warning and sees it in the kernel log, then a
> >    git grep ""Your BIOS does not provide compatible ACPI _PSS objects"
> >    will come up with arch/x86/kernel/cpu/cpufreq/powernow-k8.c. With the 
> >    original code it would come up empty and the user/developer would perhaps 
> >    thing that it's perhaps the distro kernel that prints that warning, not 
> >    the upstream kernel.
> > 
> > Could you please fix it in that fashion? Thanks,
> I fully agree with the "no line break" and not "not grepable" issues.
> I send something new, maybe not today.

Thanks.

	Ingo

Re: [tip:core/printk] printk: introduce printk_once()

[Thomas Renninger]

On Thursday 05 February 2009 14:04:20 Ingo Molnar wrote:
> 
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> > printk_once() would be nice indeed - it's a frequent construct.
> 
> Something like the patch below?
Yes, nice.
Sorry, I can't test it right now, I really have to do something else.
I can give it a test in some hours or tomorrow.

I'd also wait a week with the WARN_ONCE cleanup until this is
in linux-next and then fix it up correctly just "once" :)

powernow-k8 printing a backtrace in linux-next in broken BIOS case
for a week or two shouldn't be an issue.

No need to answer, just tell me if this does not work out.

Thanks for your suggestions,

   Thomas
 
> 	Ingo
> 
> ---------------->
> Author:     Ingo Molnar <mingo@elte.hu>
> AuthorDate: Thu, 5 Feb 2009 13:45:43 +0100
> Commit:     Ingo Molnar <mingo@elte.hu>
> CommitDate: Thu, 5 Feb 2009 13:52:29 +0100
> 
> printk: introduce printk_once()
> 
> This pattern shows up frequently in the kernel:
> 
>   static int once = 1;
>   ...
> 
> 		if (once) {
> 			once = 0;
> 			printk(KERN_ERR "message\n");
> 		}
>   ...
> 
> So add a printk_once() helper macro that reduces this to a single line
> of:
> 
> 		printk_once(KERN_ERR "message\n");
> 
> It works analogously to WARN_ONCE() & friends. (We use a macro not
> an inline because vararg expansion in inlines looks awkward and the
> macro is simple enough.)
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> 
> ---
>  include/linux/kernel.h |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 343df9e..3c183d9 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -242,6 +242,19 @@ extern struct ratelimit_state printk_ratelimit_state;
>  extern int printk_ratelimit(void);
>  extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
>  				   unsigned int interval_msec);
> +
> +/*
> + * Print a one-time message (analogous to WARN_ONCE() et al):
> + */
> +#define printk_once(x...) ({			\
> +	static int __print_once = 1;		\
> +						\
> +	if (__print_once) {			\
> +		__print_once = 0;		\
> +		printk(x);			\
> +	}					\
> +})
> +
>  #else
>  static inline int vprintk(const char *s, va_list args)
>  	__attribute__ ((format (printf, 1, 0)));
> @@ -253,6 +266,10 @@ static inline int printk_ratelimit(void) { return 0; }
>  static inline bool printk_timed_ratelimit(unsigned long *caller_jiffies, \
>  					  unsigned int interval_msec)	\
>  		{ return false; }
> +
> +/* No effect, but we still get type checking even in the !PRINTK case: */
> +#define printk_once(x...) printk(x)
> +
>  #endif
>  
>  extern int printk_needs_cpu(int cpu);
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>