[PATCH -next] bug.h: add empty warn_on_slowpath() for CONFIG_BUG=n

[PATCH -next] bug.h: add empty warn_on_slowpath() for CONFIG_BUG=n

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Add an empty function for warn_on_slowpath() when CONFIG_BUG=n so that
this build error does not happen:

linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 include/asm-generic/bug.h |    5 +++++
 1 file changed, 5 insertions(+)

--- linux-next-20080701.orig/include/asm-generic/bug.h
+++ linux-next-20080701/include/asm-generic/bug.h
@@ -81,4 +81,9 @@ extern void warn_on_slowpath(const char 
 # define WARN_ON_SMP(x)			do { } while (0)
 #endif
 
+#ifndef __ASSEMBLY__
+static inline void warn_on_slowpath(const char *file, const int line)
+{ }
+#endif
+
 #endif


---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: [PATCH -next] bug.h: add empty warn_on_slowpath() for CONFIG_BUG=n

[Andrew Morton]

On Tue, 1 Jul 2008 10:33:39 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Add an empty function for warn_on_slowpath() when CONFIG_BUG=n so that
> this build error does not happen:
> 
> linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
> linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> ---
>  include/asm-generic/bug.h |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> --- linux-next-20080701.orig/include/asm-generic/bug.h
> +++ linux-next-20080701/include/asm-generic/bug.h
> @@ -81,4 +81,9 @@ extern void warn_on_slowpath(const char 
>  # define WARN_ON_SMP(x)			do { } while (0)
>  #endif
>  
> +#ifndef __ASSEMBLY__
> +static inline void warn_on_slowpath(const char *file, const int line)
> +{ }
> +#endif
> +
>  #endif
> 

This looks like a needed-in-mainline fix?  or is there something in
linux-next which causes the bug?

Re: [PATCH -next] bug.h: add empty warn_on_slowpath() for CONFIG_BUG=n

[Randy Dunlap]

--- Original Message ---
> On Tue, 1 Jul 2008 10:33:39 -0700
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > 
> > Add an empty function for warn_on_slowpath() when CONFIG_BUG=n so that
> > this build error does not happen:
> > 
> > linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
> > linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'
> > 
> > Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> > ---
> >  include/asm-generic/bug.h |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > --- linux-next-20080701.orig/include/asm-generic/bug.h
> > +++ linux-next-20080701/include/asm-generic/bug.h
> > @@ -81,4 +81,9 @@ extern void warn_on_slowpath(const char 
> >  # define WARN_ON_SMP(x)			do { } while (0)
> >  #endif
> >  
> > +#ifndef __ASSEMBLY__
> > +static inline void warn_on_slowpath(const char *file, const int line)
> > +{ }
> > +#endif
> > +
> >  #endif
> > 
> 
> This looks like a needed-in-mainline fix?  or is there something
> in linux-next which causes the bug?

Looks like only linux-next has this code:

> > linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
> > linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'


~Randy

Re: [PATCH -next] bug.h: add empty warn_on_slowpath() for CONFIG_BUG=n

[Andrew Morton]

On Tue, 1 Jul 2008 15:23:38 -0500 (CDT)
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> --- Original Message ---
> > On Tue, 1 Jul 2008 10:33:39 -0700
> > Randy Dunlap <randy.dunlap@oracle.com> wrote:
> > 
> > > From: Randy Dunlap <randy.dunlap@oracle.com>
> > > 
> > > Add an empty function for warn_on_slowpath() when CONFIG_BUG=n so that
> > > this build error does not happen:
> > > 
> > > linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
> > > linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'
> > > 
> > > Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> > > ---
> > >  include/asm-generic/bug.h |    5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > --- linux-next-20080701.orig/include/asm-generic/bug.h
> > > +++ linux-next-20080701/include/asm-generic/bug.h
> > > @@ -81,4 +81,9 @@ extern void warn_on_slowpath(const char 
> > >  # define WARN_ON_SMP(x)			do { } while (0)
> > >  #endif
> > >  
> > > +#ifndef __ASSEMBLY__
> > > +static inline void warn_on_slowpath(const char *file, const int line)
> > > +{ }
> > > +#endif
> > > +
> > >  #endif
> > > 
> > 
> > This looks like a needed-in-mainline fix?  or is there something
> > in linux-next which causes the bug?
> 
> Looks like only linux-next has this code:
> 
> > > linux-next-20080701/drivers/acpi/utilities/utmisc.c: In function 'acpi_ut_error':
> > > linux-next-20080701/drivers/acpi/utilities/utmisc.c:1028: error: implicit declaration of function 'warn_on_slowpath'
> 

err...


commit 9e030ab0bffdc8b6d8be663b639bd5e2374537f0
Author: Len Brown <len.brown@intel.com>
Date:   Tue Jun 24 22:47:09 2008 -0400

    ACPI: add standard linux WARN_ON() output to ACPI errors and exceptions
    
    In linux-2.6.27, we expect WARN() with printk semantics
    to become available, and we'll be able to simplify
    this code.
    
    Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/drivers/acpi/utilities/utmisc.c b/drivers/acpi/utilities/utmisc.c
index 1f057b7..0a340b0 100644
--- a/drivers/acpi/utilities/utmisc.c
+++ b/drivers/acpi/utilities/utmisc.c
@@ -1028,6 +1028,7 @@ acpi_ut_error(char *module_name, u32 line_number, char *format, ...)
 {
 	va_list args;
 
+	warn_on_slowpath(module_name, line_number);
 	acpi_os_printf("ACPI Error (%s-%04d): ", module_name, line_number);
 
 	va_start(args, format);
@@ -1042,6 +1043,7 @@ acpi_ut_exception(char *module_name,
 {
 	va_list args;
 
+	warn_on_slowpath(module_name, line_number);
 	acpi_os_printf("ACPI Exception (%s-%04d): %s, ", module_name,
 		       line_number, acpi_format_exception(status));
 


Was there any particular reason for doing it this way?  It's really
quite wrong, given that an architecture can provide its own definition
of WARN_ON() and __WARN() and might not even implement
warn_on_slowpath() at all.

The usual way of doing this is to go in via the front door: WARN_ON(1).

[PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

utmisc.c needs to include asm-generic/bug.h for warn()/WARN() functions,
but it should use WARN_ON() instead of warn_on_slowpath() since
arches can provide their own implementation of WARN_ON(), which does
not have to use/provide/implement warn_on_slowpath() at all.
Just use the front door (WARN_ON).

linux-next-20080702/drivers/acpi/utilities/utmisc.c:1027: error: implicit declaration of function 'warn_on_slowpath'

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/acpi/utilities/utmisc.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- linux-next-20080702.orig/drivers/acpi/utilities/utmisc.c
+++ linux-next-20080702/drivers/acpi/utilities/utmisc.c
@@ -42,6 +42,7 @@
  */
 
 #include <linux/module.h>
+#include <asm-generic/bug.h>
 
 #include <acpi/acpi.h>
 #include <acpi/acnamesp.h>
@@ -1024,7 +1025,7 @@ acpi_ut_error(const char *module_name, u
 {
 	va_list args;
 
-	warn_on_slowpath(module_name, line_number);
+	WARN_ON(1);
 	acpi_os_printf("ACPI Error (%s-%04d): ", module_name, line_number);
 
 	va_start(args, format);
@@ -1039,7 +1040,7 @@ acpi_ut_exception(const char *module_nam
 {
 	va_list args;
 
-	warn_on_slowpath(module_name, line_number);
+	WARN_ON(1);
 	acpi_os_printf("ACPI Exception (%s-%04d): %s, ", module_name,
 		       line_number, acpi_format_exception(status));
 


---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Randy Dunlap wrote:
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> utmisc.c needs to include asm-generic/bug.h for warn()/WARN() functions,
> but it should use WARN_ON() instead of warn_on_slowpath() since
> arches can provide their own implementation of WARN_ON(), which does
> not have to use/provide/implement warn_on_slowpath() at all.
> Just use the front door (WARN_ON).
> 
> linux-next-20080702/drivers/acpi/utilities/utmisc.c:1027: error: implicit declaration of function 'warn_on_slowpath'

On what architecture did you see that?

It might be be better to just provide it on all architectures supported
by ACPI (x86, ia64). I suppose it was ia64?

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andrew Morton]

On Wed, 02 Jul 2008 20:50:04 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Randy Dunlap wrote:
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > 
> > utmisc.c needs to include asm-generic/bug.h for warn()/WARN() functions,
> > but it should use WARN_ON() instead of warn_on_slowpath() since
> > arches can provide their own implementation of WARN_ON(), which does
> > not have to use/provide/implement warn_on_slowpath() at all.
> > Just use the front door (WARN_ON).
> > 
> > linux-next-20080702/drivers/acpi/utilities/utmisc.c:1027: error: implicit declaration of function 'warn_on_slowpath'
> 
> On what architecture did you see that?

Doesn't matter.

x86, I expect.  warn_on_slowpath() isn't implemented if CONFIG_BUG=n. 
warn_on_slowpath() is an internal implementation detail of the generic
version of the WARN facility.  No other code has any business using it.

> It might be be better to just provide it on all architectures supported
> by ACPI (x86, ia64). I suppose it was ia64?

Let's use the proper interfaces.  WARN_ON().

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Andrew Morton wrote:
> On Wed, 02 Jul 2008 20:50:04 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>> Randy Dunlap wrote:
>>> From: Randy Dunlap <randy.dunlap@oracle.com>
>>>
>>> utmisc.c needs to include asm-generic/bug.h for warn()/WARN() functions,
>>> but it should use WARN_ON() instead of warn_on_slowpath() since
>>> arches can provide their own implementation of WARN_ON(), which does
>>> not have to use/provide/implement warn_on_slowpath() at all.
>>> Just use the front door (WARN_ON).
>>>
>>> linux-next-20080702/drivers/acpi/utilities/utmisc.c:1027: error: implicit declaration of function 'warn_on_slowpath'
>> On what architecture did you see that?
> 
> Doesn't matter.
> 
> x86, I expect.  warn_on_slowpath() isn't implemented if CONFIG_BUG=n. 
> warn_on_slowpath() is an internal implementation detail of the generic
> version of the WARN facility.  No other code has any business using it.
> 
>> It might be be better to just provide it on all architectures supported
>> by ACPI (x86, ia64). I suppose it was ia64?
> 
> Let's use the proper interfaces.  WARN_ON().

The problem I see here is that they are not necessarily equivalent.

In particular the function here is just a generic low level error
reporting function, but you really want to report the caller.
And that's a genuine wish. With your patch it would always
report the same function and only differ in the (potentially
unreliable) backtrace.

I think it would be better to provide a warn_on_slowpath() for
the !CONFIG_BUG case and drop this patch.

Not-Acked: ...

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andrew Morton]

On Wed, 02 Jul 2008 21:31:17 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Andrew Morton wrote:
> > On Wed, 02 Jul 2008 20:50:04 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> >> Randy Dunlap wrote:
> >>> From: Randy Dunlap <randy.dunlap@oracle.com>
> >>>
> >>> utmisc.c needs to include asm-generic/bug.h for warn()/WARN() functions,
> >>> but it should use WARN_ON() instead of warn_on_slowpath() since
> >>> arches can provide their own implementation of WARN_ON(), which does
> >>> not have to use/provide/implement warn_on_slowpath() at all.
> >>> Just use the front door (WARN_ON).
> >>>
> >>> linux-next-20080702/drivers/acpi/utilities/utmisc.c:1027: error: implicit declaration of function 'warn_on_slowpath'
> >> On what architecture did you see that?
> > 
> > Doesn't matter.
> > 
> > x86, I expect.  warn_on_slowpath() isn't implemented if CONFIG_BUG=n. 
> > warn_on_slowpath() is an internal implementation detail of the generic
> > version of the WARN facility.  No other code has any business using it.
> > 
> >> It might be be better to just provide it on all architectures supported
> >> by ACPI (x86, ia64). I suppose it was ia64?
> > 
> > Let's use the proper interfaces.  WARN_ON().
> 
> The problem I see here is that they are not necessarily equivalent.
> 
> In particular the function here is just a generic low level error
> reporting function, but you really want to report the caller.
> And that's a genuine wish. With your patch it would always
> report the same function and only differ in the (potentially
> unreliable) backtrace.
> 
> I think it would be better to provide a warn_on_slowpath() for
> the !CONFIG_BUG case and drop this patch.
> 
> Not-Acked: ...
> 

Oh.  I don't care much.  But acpi is presently causing build errors and
is abusing core kernel internals.  Please fix it.

One way would be via printk() and dump_stack().

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

[-- Attachment #1: Type: text/plain, Size: 295 bytes --]


>
> Oh.  I don't care much.

Thanks for the encouraging words.

> But acpi is presently causing build errors and
> is abusing core kernel internals.  Please fix it.
>
> One way would be via printk() and dump_stack().

Here's a patch. Can you use that one instead of Randy's please?

-Andi






[-- Attachment #2: bug-fallback --]
[-- Type: text/plain, Size: 592 bytes --]

Supply warn_on_slow_path() even for the !CONFIG_BUG case

Fix build problem with ACPI for !CONFIG_BUG. Noted by Randy Dunlap.

Signed-off-by: Andi Kleen <ak@linux.intel.com>

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 2632328..d0d83b7 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -63,6 +63,9 @@ extern void warn_on_slowpath(const char *file, const int line);
 	unlikely(__ret_warn_on);					\
 })
 #endif
+
+static inline void warn_on_slowpath(const char *file, const int line) {}
+
 #endif
 
 #define WARN_ON_ONCE(condition)	({				\

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andrew Morton]

On Wed, 02 Jul 2008 22:11:06 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Here's a patch. Can you use that one instead of Randy's please?
> 

No.

> 
> [bug-fallback  text/plain (593B)]
> Supply warn_on_slow_path() even for the !CONFIG_BUG case
> 
> Fix build problem with ACPI for !CONFIG_BUG. Noted by Randy Dunlap.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 2632328..d0d83b7 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -63,6 +63,9 @@ extern void warn_on_slowpath(const char *file, const int line);
>  	unlikely(__ret_warn_on);					\
>  })
>  #endif
> +
> +static inline void warn_on_slowpath(const char *file, const int line) {}
> +
>  #endif

There's no reason why asm/bug.h has to even include asm-generic/bug.h. 
And there's no reason why an architecture which defined __WARN needs to
define warn_on_slowpath() even if it _does_ include asm-generic/bug.h. 
And I didn't even begin to look at what might disable
WANT_WARN_ON_SLOWPATH.

This is all poking deep into the private internals of one particular
implementation of this interface.

Furthermore even if it _does_ happen to work, you've gone and coupled
the availability of this acpi debbugging feature to CONFIG_BUG, which
seems arbitrary.

If you really want to do it this way (and it sounds reasonable) then
can we please do it in a less-than-totally-hacky-and-broken way?



For example, define a new, always-available helper function in (say)
kernel/panic.c along the lines of

void emit_warning_message(?)(const char *msg, int line)


and then call that from warn_on_slowpath().  Will require that
warn_on_slowpath become inlined so we don't get a useless extra entry
in the backtraces all the time.

Or just use printk and dump_stack.

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Andrew Morton wrote:
> On Wed, 02 Jul 2008 22:11:06 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>> Here's a patch. Can you use that one instead of Randy's please?
>>
> 
> No.
> 
>> [bug-fallback  text/plain (593B)]
>> Supply warn_on_slow_path() even for the !CONFIG_BUG case
>>
>> Fix build problem with ACPI for !CONFIG_BUG. Noted by Randy Dunlap.
>>
>> Signed-off-by: Andi Kleen <ak@linux.intel.com>
>>
>> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
>> index 2632328..d0d83b7 100644
>> --- a/include/asm-generic/bug.h
>> +++ b/include/asm-generic/bug.h
>> @@ -63,6 +63,9 @@ extern void warn_on_slowpath(const char *file, const int line);
>>  	unlikely(__ret_warn_on);					\
>>  })
>>  #endif
>> +
>> +static inline void warn_on_slowpath(const char *file, const int line) {}
>> +
>>  #endif
> 
> There's no reason why asm/bug.h has to even include asm-generic/bug.h. 

I checked them all and they all do.

Besides for the ACPI case we only have to care about x86 and ia64.

But all do it anyways.


> And there's no reason why an architecture which defined __WARN needs to
> define warn_on_slowpath() even if it _does_ include asm-generic/bug.h. 
> And I didn't even begin to look at what might disable
> WANT_WARN_ON_SLOWPATH.

I think warn_on_slow_path() should be a general interface.
Perhaps with a better name though.
> 
> This is all poking deep into the private internals of one particular
> implementation of this interface.

But it's in kernel/panic.c. I don't think anyone can compile
without that. So it should be always there (unless you undefine CONFIG_BUG)


> Furthermore even if it _does_ happen to work, you've gone and coupled
> the availability of this acpi debbugging feature to CONFIG_BUG, which
> seems arbitrary.

Well not defining CONFIG_BUG means "I don't care about debugging" doesn't it?
Not having (or having only crippled) ACPI debugging in that case seems reasonable

Doesn't seem that arbitrary to me.

> 
> If you really want to do it this way (and it sounds reasonable) then
> can we please do it in a less-than-totally-hacky-and-broken way?
> 
> 
> 
> For example, define a new, always-available helper function in (say)
> kernel/panic.c along the lines of
> 
> void emit_warning_message(?)(const char *msg, int line)


Ok we can just rename warn_on_slow_path() which is already there to that new name.
Would that satisfy you?

> 
> and then call that from warn_on_slowpath().  Will require that
> warn_on_slowpath become inlined so we don't get a useless extra entry
> in the backtraces all the time.
> 
> Or just use printk and dump_stack.

I think the reason for the warn_on_slowpath() is that it produces
patterns that can be detected by Arjan's kerneloops scripts.

I'm not sure manual printk/dump_stack would have the same effect.

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andrew Morton]

On Wed, 02 Jul 2008 22:51:52 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> > 
> > If you really want to do it this way (and it sounds reasonable) then
> > can we please do it in a less-than-totally-hacky-and-broken way?
> > 
> > 
> > 
> > For example, define a new, always-available helper function in (say)
> > kernel/panic.c along the lines of
> > 
> > void emit_warning_message(?)(const char *msg, int line)
> 
> 
> Ok we can just rename warn_on_slow_path() which is already there to that new name.

Sure, that'd work.  But we should pull it outside any ifdefs so that it
is always available (IMO).

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Andrew Morton wrote:
> On Wed, 02 Jul 2008 22:51:52 +0200
> Andi Kleen <andi@firstfloor.org> wrote:
> 
>>> If you really want to do it this way (and it sounds reasonable) then
>>> can we please do it in a less-than-totally-hacky-and-broken way?
>>>
>>>
>>>
>>> For example, define a new, always-available helper function in (say)
>>> kernel/panic.c along the lines of
>>>
>>> void emit_warning_message(?)(const char *msg, int line)
>>
>> Ok we can just rename warn_on_slow_path() which is already there to that new name.
> 
> Sure, that'd work.  But we should pull it outside any ifdefs so that it
> is always available (IMO).

For !CONFIG_BUG it's reasonable to not have the calls. These people
who define it must be fighting for every byte ... So i think
the stub inline in my previous patch is fine.

What name do you want?

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andrew Morton]

On Wed, 02 Jul 2008 23:07:25 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Andrew Morton wrote:
> > On Wed, 02 Jul 2008 22:51:52 +0200
> > Andi Kleen <andi@firstfloor.org> wrote:
> > 
> >>> If you really want to do it this way (and it sounds reasonable) then
> >>> can we please do it in a less-than-totally-hacky-and-broken way?
> >>>
> >>>
> >>>
> >>> For example, define a new, always-available helper function in (say)
> >>> kernel/panic.c along the lines of
> >>>
> >>> void emit_warning_message(?)(const char *msg, int line)
> >>
> >> Ok we can just rename warn_on_slow_path() which is already there to that new name.
> > 
> > Sure, that'd work.  But we should pull it outside any ifdefs so that it
> > is always available (IMO).
> 
> For !CONFIG_BUG it's reasonable to not have the calls. These people
> who define it must be fighting for every byte ... So i think
> the stub inline in my previous patch is fine.

It should all be moved to include/linux/bug.h.  Or, perhaps,
include/linux/kernel.h.

> What name do you want?

umm, warn_with_context(), something like that?

I suppose it really should take a printk control string too.  Later.

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Len Brown]

for background...

Ingo pointed out that automated testing wasn't finding
ACPI exceptions because we were not using the standard
Linux format for oops etc.

I put this hack patch on the debug-test branch in the acpi tree,
and pulled it into the test branch for linux-next to mine
for previously ignored errors.

This commit isn't intended for 2.6.27.
(and thus is not on the release-2.6.27 branch)
hopefully that isn't abuse of linux-next...

Arjan tells me that we'll have a real WARN()
with prink semantics in 2.6.27 and so we can
simplify/standardize this when that happens.

Oh, and since I'm on sabbatical, I'm obviously
fine with whatever Andi Kleen does (or does not do here)
in my absence.

thanks,
-Len


commit 9e030ab0bffdc8b6d8be663b639bd5e2374537f0
Author: Len Brown <len.brown@intel.com>
Date:   Tue Jun 24 22:47:09 2008 -0400

    ACPI: add standard linux WARN_ON() output to ACPI errors and 
exceptions
    
    In linux-2.6.27, we expect WARN() with printk semantics
    to become available, and we'll be able to simplify
    this code.
    
    Signed-off-by: Len Brown <len.brown@intel.com>

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Len Brown wrote:

> Ingo pointed out that automated testing wasn't finding
> ACPI exceptions because we were not using the standard
> Linux format for oops etc.

Sounds more like a case of more the automated testing
needing fixing than the kernel. I'll just remove it.

> I put this hack patch on the debug-test branch in the acpi tree,
> and pulled it into the test branch for linux-next to mine
> for previously ignored errors.
> 
> This commit isn't intended for 2.6.27.
> (and thus is not on the release-2.6.27 branch)
> hopefully that isn't abuse of linux-next...
> 
> Arjan tells me that we'll have a real WARN()
> with prink semantics in 2.6.27 and so we can
> simplify/standardize this when that happens.
> 
> Oh, and since I'm on sabbatical, I'm obviously
> fine with whatever Andi Kleen does (or does not do here)
> in my absence.

Thanks for the clarification, Len. I was already puzzling
why that was changed.

And no need to watch your email that closely...

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Arjan van de Ven]

On Sun, 06 Jul 2008 13:18:56 -0400 (EDT)
Len Brown <lenb@kernel.org> wrote:
 
> Arjan tells me that we'll have a real WARN()
> with prink semantics in 2.6.27 and so we can
> simplify/standardize this when that happens.
> 
> Oh, and since I'm on sabbatical, I'm obviously
> fine with whatever Andi Kleen does (or does not do here)
> in my absence.

I'm finishing up the series to introduce (in -mm) and use it
(various extra patches).. Want me to just include the ACPI part of this
as well, or do you want to feed that separately?

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Andi Kleen]

Arjan van de Ven wrote:
> On Sun, 06 Jul 2008 13:18:56 -0400 (EDT)
> Len Brown <lenb@kernel.org> wrote:
>  
>> Arjan tells me that we'll have a real WARN()
>> with prink semantics in 2.6.27 and so we can
>> simplify/standardize this when that happens.
>>
>> Oh, and since I'm on sabbatical, I'm obviously
>> fine with whatever Andi Kleen does (or does not do here)
>> in my absence.
> 
> I'm finishing up the series to introduce (in -mm) and use it
> (various extra patches).. Want me to just include the ACPI part of this
> as well, or do you want to feed that separately?

I'm dropping the ACPI part because we're getting too many
reports of scary looking backtraces and ACPI exceptions are actually
not that exceptional.

-Andi

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Arjan van de Ven]

On Mon, 07 Jul 2008 08:19:08 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> I'm dropping the ACPI part because we're getting too many
> reports of scary looking backtraces and ACPI exceptions are actually
> not that exceptional.

when I talked to Len about it he implied/said that these would only
happen for ACPI code bugs (and not bios bugs). Maybe he was
overestimating what the code would actually do; if so it would imo be
well worth splitting out ACPI code assertions versions things that are
BIOS caused (if only to be able to inform the user more consistently)


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH -next] acpi utmisc: use WARN_ON() instead of warn_on_slowpath()

[Thomas Renninger]

On Monday 07 July 2008 09:03:20 Arjan van de Ven wrote:
> On Mon, 07 Jul 2008 08:19:08 +0200
>
> Andi Kleen <andi@firstfloor.org> wrote:
> > I'm dropping the ACPI part because we're getting too many
> > reports of scary looking backtraces and ACPI exceptions are actually
> > not that exceptional.
>
> when I talked to Len about it he implied/said that these would only
> happen for ACPI code bugs (and not bios bugs).

drivers/acpi/processor_core.c:852:              ACPI_EXCEPTION((AE_INFO, status,
drivers/acpi/processor_core.c-853-                              "Processor Device is not present"));

and

drivers/acpi/parser/psloop.c-895-                                       if (status == AE_AML_NO_RETURN_VALUE) {
drivers/acpi/parser/psloop.c:896:                                               ACPI_EXCEPTION((AE_INFO, status,
drivers/acpi/parser/psloop.c-897-                                                               "Invoked method did not return a value"));

do very much look like BIOS bugs.

> Maybe he was 
> overestimating what the code would actually do; if so it would imo be
> well worth splitting out ACPI code assertions versions things that are
> BIOS caused (if only to be able to inform the user more consistently)

You want to split BIOS bug and kernel bug ACPI messages?
This is a good idea.
But this should be started in ACPICA code first.
ACPI_EXCEPTION is an ACPICA interface having an
ACPI_FIRMWARE_BUG for relevant messages would be nice.
There are probably also a lot ACPI_DEBUG_PRINT((ACPI_DB_INFO,
info messages suppressing bugs.
Being able to mark them with a ACPI_FIRMWARE_BUG interface, 
would be great.

As suggested some days ago, I'd like to have an interface in the kernel
to report firmware bugs via printk, netlink, both (or compiled out) to
userspace.
As soon as ACPICA can differ and invoke e.g.
ACPI_FIRMWARE_BUG, it can easily be extended to use a kernel
interface to send firmware bugs to userspace as such.

If you are already working in this area or have suggestions, please let me
know or keep me in CC.
Otherwise I will try to come up with some patches sooner or later.

Thanks,

         Thomas

BTW: I saw a lot people used:
ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
again in linux (not ACPICA code!) which is wrong.

These are suppressed if ACPI_DEBUG is not compiled in.
It does not make sense to use a debug interface for errors.
IMO ACPI_DB_ERROR and ACPI_DB_WARN
should be reverted or somehow enforced that
it cannot be used in kernel code anymore, maybe they should be renamed
into DB_DEBUG and DB_WHATEVER?

Here these are again used, some look like sever errors (on a first glance),
some not:

grep -n1 DB_ERROR drivers/acpi/*
drivers/acpi/acpi_memhotplug.c-456-             if (result)
drivers/acpi/acpi_memhotplug.c:457:                     ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/acpi_memhotplug.c-458-                             "Error in acpi_memory_enable_device\n"));
--
drivers/acpi/cm_sbs.c-54-       } else {
drivers/acpi/cm_sbs.c:55:               ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/cm_sbs.c-56-                                 "Cannot create %s\n", ACPI_AC_CLASS));
--
drivers/acpi/cm_sbs.c-85-       } else {
drivers/acpi/cm_sbs.c:86:               ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/cm_sbs.c-87-                                 "Cannot create %s\n", ACPI_BATTERY_CLASS));
--
drivers/acpi/fan.c-323- if (result) {
drivers/acpi/fan.c:324:         ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/fan.c-325-                           "Error reading fan power state\n"));
--
drivers/acpi/osl.c-726- if (!queue_work(queue, &dpc->work)) {
drivers/acpi/osl.c:727:         ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/osl.c-728-                   "Call to queue_work() failed.\n"));
--
drivers/acpi/processor_perflib.c-515-   if (!psd || (psd->type != ACPI_TYPE_PACKAGE)) {
drivers/acpi/processor_perflib.c:516:           ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _PSD data\n"));
drivers/acpi/processor_perflib.c-517-           result = -EFAULT;
--
drivers/acpi/processor_perflib.c-521-   if (psd->package.count != 1) {
drivers/acpi/processor_perflib.c:522:           ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _PSD data\n"));
drivers/acpi/processor_perflib.c-523-           result = -EFAULT;
--
drivers/acpi/processor_perflib.c-534-   if (ACPI_FAILURE(status)) {
drivers/acpi/processor_perflib.c:535:           ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _PSD data\n"));
drivers/acpi/processor_perflib.c-536-           result = -EFAULT;
--
drivers/acpi/processor_perflib.c-540-   if (pdomain->num_entries != ACPI_PSD_REV0_ENTRIES) {
drivers/acpi/processor_perflib.c:541:           ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Unknown _PSD:num_entries\n"));
drivers/acpi/processor_perflib.c-542-           result = -EFAULT;
--
drivers/acpi/processor_perflib.c-546-   if (pdomain->revision != ACPI_PSD_REV0_REVISION) {
drivers/acpi/processor_perflib.c:547:           ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Unknown _PSD:revision\n"));
drivers/acpi/processor_perflib.c-548-           result = -EFAULT;
--
drivers/acpi/processor_throttling.c-530-        if (!tsd || (tsd->type != ACPI_TYPE_PACKAGE)) {
drivers/acpi/processor_throttling.c:531:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _TSD data\n"));
drivers/acpi/processor_throttling.c-532-                result = -EFAULT;
--
drivers/acpi/processor_throttling.c-536-        if (tsd->package.count != 1) {
drivers/acpi/processor_throttling.c:537:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _TSD data\n"));
drivers/acpi/processor_throttling.c-538-                result = -EFAULT;
--
drivers/acpi/processor_throttling.c-549-        if (ACPI_FAILURE(status)) {
drivers/acpi/processor_throttling.c:550:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Invalid _TSD data\n"));
drivers/acpi/processor_throttling.c-551-                result = -EFAULT;
--
drivers/acpi/processor_throttling.c-555-        if (pdomain->num_entries != ACPI_TSD_REV0_ENTRIES) {
drivers/acpi/processor_throttling.c:556:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Unknown _TSD:num_entries\n"));
drivers/acpi/processor_throttling.c-557-                result = -EFAULT;
--
drivers/acpi/processor_throttling.c-561-        if (pdomain->revision != ACPI_TSD_REV0_REVISION) {
drivers/acpi/processor_throttling.c:562:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Unknown _TSD:revision\n"));
drivers/acpi/processor_throttling.c-563-                result = -EFAULT;
--
drivers/acpi/scan.c-467-        if(result)
drivers/acpi/scan.c:468:                ACPI_DEBUG_PRINT((ACPI_DB_ERROR, "Error creating sysfs interface for device %s\n", device->dev.bus_id));
drivers/acpi/scan.c-469-
--
drivers/acpi/thermal.c-1174-    if (ACPI_FAILURE(status)) {
drivers/acpi/thermal.c:1175:            ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/thermal.c-1176-                            "Error attaching device data\n"));
--
drivers/acpi/video.c-1532-              if (ACPI_FAILURE(status)) {
drivers/acpi/video.c:1533:                      ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/video.c-1534-                                        "Error installing notify handler\n"));
--
drivers/acpi/video.c-2005-      if (ACPI_FAILURE(status)) {
drivers/acpi/video.c:2006:              ACPI_DEBUG_PRINT((ACPI_DB_ERROR,
drivers/acpi/video.c-2007-                                "Error installing notify handler\n"));