| Message ID | 1269126340.18314.115.camel@localhost (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Ben Hutchings <ben@decadent.org.uk> writes: > WARN() is used in some places to report firmware or hardware bugs that > are then worked-around. These bugs do not affect the stability of the > kernel and should not set the usual TAINT_WARN flag. To allow for > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > flag as argument. > > Architectures that implement warnings using trap instructions instead > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > instead of __WARN(). I guess this should enforce that at least some taint flag is set? (e.g. with a BUILD_BUG_ON) -Andi
On Sun, 2010-03-21 at 20:10 +0100, Andi Kleen wrote: > Ben Hutchings <ben@decadent.org.uk> writes: > > > WARN() is used in some places to report firmware or hardware bugs that > > are then worked-around. These bugs do not affect the stability of the > > kernel and should not set the usual TAINT_WARN flag. To allow for > > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > > flag as argument. > > > > Architectures that implement warnings using trap instructions instead > > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > > instead of __WARN(). > > I guess this should enforce that at least some taint flag is set? > (e.g. with a BUILD_BUG_ON) I'm being a bit sloppy with the wording here. The TAINT_* macros are actually bit numbers, not flags. I could define a TAINT_MAX and add: BUILD_BUG_ON(taint < 0 || taint > TAINT_MAX); Not sure that that's really worth doing though. Ben.
On Sat, 20 Mar 2010 23:05:40 +0000 Ben Hutchings <ben@decadent.org.uk> wrote: > WARN() is used in some places to report firmware or hardware bugs that > are then worked-around. These bugs do not affect the stability of the > kernel and should not set the usual TAINT_WARN flag. To allow for > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > flag as argument. > > Architectures that implement warnings using trap instructions instead > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > instead of __WARN(). When you say they "must now implement", I assume that you mean that they _do_ now implement, and that no additional architecture work is needed. > The architecture-specific changes here are untested and need to be > reviewed by architecture maintainers. That would be nice.
On Sat, Mar 20, 2010 at 11:05:40PM +0000, Ben Hutchings wrote: > WARN() is used in some places to report firmware or hardware bugs that > are then worked-around. These bugs do not affect the stability of the > kernel and should not set the usual TAINT_WARN flag. To allow for > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > flag as argument. > > Architectures that implement warnings using trap instructions instead > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > instead of __WARN(). > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > --- > The architecture-specific changes here are untested and need to be > reviewed by architecture maintainers. > I'm a bit confused about how this is supposed to work, the TAINT_xxx values are bit positions presently from 0 to 10, while BUGFLAG_xxx are ranged from 0 up. You've set up BUGFLAG_TAINT() to that the TAINT_xxx value is shifted up 8 bits but neglected the fact that the trap type is 16-bits on most (all?) of the platforms using trap-based BUG handling. If the 'taint' in question is just the TAINT_xxx value by itself and will never be a bitmap then that's fine, but there's certainly not enough room to pass the bitmap in on top of the bugflag otherwise (I don't know if this is your intention or not though). Also note that some platforms (like SH) implement additional bugflags, so we at least want to keep the lower byte available for architecture private use. Having said that, the current patch does work for me, although I'm a bit nervous about someone thinking it's ok to pass in a taint bitmap here. Tested-by: Paul Mundt <lethal@linux-sh.org>
On Mon, 2010-03-22 at 22:47 -0400, Andrew Morton wrote: > On Sat, 20 Mar 2010 23:05:40 +0000 Ben Hutchings <ben@decadent.org.uk> wrote: > > > WARN() is used in some places to report firmware or hardware bugs that > > are then worked-around. These bugs do not affect the stability of the > > kernel and should not set the usual TAINT_WARN flag. To allow for > > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > > flag as argument. > > > > Architectures that implement warnings using trap instructions instead > > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > > instead of __WARN(). > > When you say they "must now implement", I assume that you mean that > they _do_ now implement, and that no additional architecture work is > needed. Right, I believe I fixed-up all the current architectures. There might be more architectures out there, unmerged as yet. Ben.
On Tue, 2010-03-23 at 16:45 +0900, Paul Mundt wrote: > On Sat, Mar 20, 2010 at 11:05:40PM +0000, Ben Hutchings wrote: > > WARN() is used in some places to report firmware or hardware bugs that > > are then worked-around. These bugs do not affect the stability of the > > kernel and should not set the usual TAINT_WARN flag. To allow for > > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > > flag as argument. > > > > Architectures that implement warnings using trap instructions instead > > of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) > > instead of __WARN(). > > > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > --- > > The architecture-specific changes here are untested and need to be > > reviewed by architecture maintainers. > > > I'm a bit confused about how this is supposed to work, the TAINT_xxx > values are bit positions presently from 0 to 10, while BUGFLAG_xxx are > ranged from 0 up. You've set up BUGFLAG_TAINT() to that the TAINT_xxx > value is shifted up 8 bits but neglected the fact that the trap type is > 16-bits on most (all?) of the platforms using trap-based BUG handling. > > If the 'taint' in question is just the TAINT_xxx value by itself and will > never be a bitmap then that's fine, but there's certainly not enough room > to pass the bitmap in on top of the bugflag otherwise (I don't know if > this is your intention or not though). Yes, the taint value must be a bit number not a flag. Sloppy wording on my part. > Also note that some platforms (like SH) implement additional bugflags, so > we at least want to keep the lower byte available for architecture > private use. I noticed, that's why I started at 8 not 1. > Having said that, the current patch does work for me, although I'm a bit > nervous about someone thinking it's ok to pass in a taint bitmap here. We can maybe use BUILD_BUG_ON() here as the taint bit is already required to be a compile-time constant. Ben. > Tested-by: Paul Mundt <lethal@linux-sh.org> >
On Sat, 2010-03-20 at 23:05 +0000, Ben Hutchings wrote: > WARN() is used in some places to report firmware or hardware bugs that > are then worked-around. These bugs do not affect the stability of the > kernel and should not set the usual TAINT_WARN flag. To allow for > this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint > flag as argument. .. > The architecture-specific changes here are untested and need to be > reviewed by architecture maintainers. I'm not one of them, but this at least builds on powerpc FWIW. cheers
diff --git a/arch/parisc/include/asm/bug.h b/arch/parisc/include/asm/bug.h index 75e46c5..72cfdb0 100644 --- a/arch/parisc/include/asm/bug.h +++ b/arch/parisc/include/asm/bug.h @@ -44,7 +44,7 @@ #endif #ifdef CONFIG_DEBUG_BUGVERBOSE -#define __WARN() \ +#define __WARN_TAINT(taint) \ do { \ asm volatile("\n" \ "1:\t" PARISC_BUG_BREAK_ASM "\n" \ @@ -54,11 +54,11 @@ "\t.org 2b+%c3\n" \ "\t.popsection" \ : : "i" (__FILE__), "i" (__LINE__), \ - "i" (BUGFLAG_WARNING), \ + "i" (BUGFLAG_TAINT(taint)), \ "i" (sizeof(struct bug_entry)) ); \ } while(0) #else -#define __WARN() \ +#define __WARN_TAINT(taint) \ do { \ asm volatile("\n" \ "1:\t" PARISC_BUG_BREAK_ASM "\n" \ @@ -67,7 +67,7 @@ "\t.short %c0\n" \ "\t.org 2b+%c1\n" \ "\t.popsection" \ - : : "i" (BUGFLAG_WARNING), \ + : : "i" (BUGFLAG_TAINT(taint)), \ "i" (sizeof(struct bug_entry)) ); \ } while(0) #endif diff --git a/arch/powerpc/include/asm/bug.h b/arch/powerpc/include/asm/bug.h index 2c15212..065c590 100644 --- a/arch/powerpc/include/asm/bug.h +++ b/arch/powerpc/include/asm/bug.h @@ -85,12 +85,12 @@ } \ } while (0) -#define __WARN() do { \ +#define __WARN_TAINT(taint) do { \ __asm__ __volatile__( \ "1: twi 31,0,0\n" \ _EMIT_BUG_ENTRY \ : : "i" (__FILE__), "i" (__LINE__), \ - "i" (BUGFLAG_WARNING), \ + "i" (BUGFLAG_TAINT(taint)), \ "i" (sizeof(struct bug_entry))); \ } while (0) @@ -104,7 +104,7 @@ "1: "PPC_TLNEI" %4,0\n" \ _EMIT_BUG_ENTRY \ : : "i" (__FILE__), "i" (__LINE__), \ - "i" (BUGFLAG_WARNING), \ + "i" (BUGFLAG_TAINT(TAINT_WARN)), \ "i" (sizeof(struct bug_entry)), \ "r" (__ret_warn_on)); \ } \ diff --git a/arch/s390/include/asm/bug.h b/arch/s390/include/asm/bug.h index 9beeb9d..bf90d1f 100644 --- a/arch/s390/include/asm/bug.h +++ b/arch/s390/include/asm/bug.h @@ -46,18 +46,18 @@ unreachable(); \ } while (0) -#define __WARN() do { \ - __EMIT_BUG(BUGFLAG_WARNING); \ +#define __WARN_TAINT(taint) do { \ + __EMIT_BUG(BUGFLAG_TAINT(taint)); \ } while (0) #define WARN_ON(x) ({ \ int __ret_warn_on = !!(x); \ if (__builtin_constant_p(__ret_warn_on)) { \ if (__ret_warn_on) \ - __EMIT_BUG(BUGFLAG_WARNING); \ + __WARN(); \ } else { \ if (unlikely(__ret_warn_on)) \ - __EMIT_BUG(BUGFLAG_WARNING); \ + __WARN(); \ } \ unlikely(__ret_warn_on); \ }) diff --git a/arch/sh/include/asm/bug.h b/arch/sh/include/asm/bug.h index d02c01b..6323f86 100644 --- a/arch/sh/include/asm/bug.h +++ b/arch/sh/include/asm/bug.h @@ -48,7 +48,7 @@ do { \ "i" (sizeof(struct bug_entry))); \ } while (0) -#define __WARN() \ +#define __WARN_TAINT(taint) \ do { \ __asm__ __volatile__ ( \ "1:\t.short %O0\n" \ @@ -57,7 +57,7 @@ do { \ : "n" (TRAPA_BUG_OPCODE), \ "i" (__FILE__), \ "i" (__LINE__), \ - "i" (BUGFLAG_WARNING), \ + "i" (BUGFLAG_TAINT(taint)), \ "i" (sizeof(struct bug_entry))); \ } while (0) diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h index 18c435d..c2c9ba0 100644 --- a/include/asm-generic/bug.h +++ b/include/asm-generic/bug.h @@ -25,7 +25,10 @@ struct bug_entry { }; #endif /* __ASSEMBLY__ */ -#define BUGFLAG_WARNING (1<<0) +#define BUGFLAG_WARNING (1 << 0) +#define BUGFLAG_TAINT(taint) (BUGFLAG_WARNING | ((taint) << 8)) +#define BUG_GET_TAINT(bug) ((bug)->flags >> 8) + #endif /* CONFIG_GENERIC_BUG */ /* @@ -56,17 +59,25 @@ struct bug_entry { * appear at runtime. Use the versions with printk format strings * to provide better diagnostics. */ -#ifndef __WARN +#ifndef __WARN_TAINT #ifndef __ASSEMBLY__ extern void warn_slowpath_fmt(const char *file, const int line, const char *fmt, ...) __attribute__((format(printf, 3, 4))); +extern void warn_slowpath_fmt_taint(const char *file, const int line, + unsigned taint, const char *fmt, ...) + __attribute__((format(printf, 4, 5))); extern void warn_slowpath_null(const char *file, const int line); #define WANT_WARN_ON_SLOWPATH #endif #define __WARN() warn_slowpath_null(__FILE__, __LINE__) #define __WARN_printf(arg...) warn_slowpath_fmt(__FILE__, __LINE__, arg) +#define __WARN_printf_taint(taint, arg...) \ + warn_slowpath_fmt_taint(__FILE__, __LINE__, taint, arg) #else +#define __WARN() __WARN_TAINT(TAINT_WARN) #define __WARN_printf(arg...) do { printk(arg); __WARN(); } while (0) +#define __WARN_printf_taint(taint, arg...) \ + do { printk(arg); __WARN_TAINT(taint); } while (0) #endif #ifndef WARN_ON @@ -87,6 +98,13 @@ extern void warn_slowpath_null(const char *file, const int line); }) #endif +#define WARN_TAINT(condition, taint, format...) ({ \ + int __ret_warn_on = !!(condition); \ + if (unlikely(__ret_warn_on)) \ + __WARN_printf_taint(taint, format); \ + unlikely(__ret_warn_on); \ +}) + #else /* !CONFIG_BUG */ #ifndef HAVE_ARCH_BUG #define BUG() do {} while(0) @@ -110,6 +128,8 @@ extern void warn_slowpath_null(const char *file, const int line); }) #endif +#define WARN_TAINT(condition, taint, format...) WARN_ON(condition) + #endif #define WARN_ON_ONCE(condition) ({ \ @@ -132,6 +152,16 @@ extern void warn_slowpath_null(const char *file, const int line); unlikely(__ret_warn_once); \ }) +#define WARN_TAINT_ONCE(condition, taint, format...) ({ \ + static bool __warned; \ + int __ret_warn_once = !!(condition); \ + \ + if (unlikely(__ret_warn_once)) \ + if (WARN_TAINT(!__warned, taint, format)) \ + __warned = true; \ + unlikely(__ret_warn_once); \ +}) + #define WARN_ON_RATELIMIT(condition, state) \ WARN_ON((condition) && __ratelimit(state)) diff --git a/kernel/panic.c b/kernel/panic.c index 13d966b..8b821bc 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -365,7 +365,8 @@ struct slowpath_args { va_list args; }; -static void warn_slowpath_common(const char *file, int line, void *caller, struct slowpath_args *args) +static void warn_slowpath_common(const char *file, int line, void *caller, + unsigned taint, struct slowpath_args *args) { const char *board; @@ -381,7 +382,7 @@ static void warn_slowpath_common(const char *file, int line, void *caller, struc print_modules(); dump_stack(); print_oops_end_marker(); - add_taint(TAINT_WARN); + add_taint(taint); } void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...) @@ -390,14 +391,29 @@ void warn_slowpath_fmt(const char *file, int line, const char *fmt, ...) args.fmt = fmt; va_start(args.args, fmt); - warn_slowpath_common(file, line, __builtin_return_address(0), &args); + warn_slowpath_common(file, line, __builtin_return_address(0), + TAINT_WARN, &args); va_end(args.args); } EXPORT_SYMBOL(warn_slowpath_fmt); +void warn_slowpath_fmt_taint(const char *file, int line, + unsigned taint, const char *fmt, ...) +{ + struct slowpath_args args; + + args.fmt = fmt; + va_start(args.args, fmt); + warn_slowpath_common(file, line, __builtin_return_address(0), + taint, &args); + va_end(args.args); +} +EXPORT_SYMBOL(warn_slowpath_fmt_taint); + void warn_slowpath_null(const char *file, int line) { - warn_slowpath_common(file, line, __builtin_return_address(0), NULL); + warn_slowpath_common(file, line, __builtin_return_address(0), + TAINT_WARN, NULL); } EXPORT_SYMBOL(warn_slowpath_null); #endif diff --git a/lib/bug.c b/lib/bug.c index 300e41a..f13daf4 100644 --- a/lib/bug.c +++ b/lib/bug.c @@ -165,7 +165,7 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs) (void *)bugaddr); show_regs(regs); - add_taint(TAINT_WARN); + add_taint(BUG_GET_TAINT(bug)); return BUG_TRAP_TYPE_WARN; }
WARN() is used in some places to report firmware or hardware bugs that are then worked-around. These bugs do not affect the stability of the kernel and should not set the usual TAINT_WARN flag. To allow for this, add WARN_TAINT() and WARN_TAINT_ONCE() macros that take a taint flag as argument. Architectures that implement warnings using trap instructions instead of calls to warn_slowpath_*() must now implement __WARN_TAINT(taint) instead of __WARN(). Signed-off-by: Ben Hutchings <ben@decadent.org.uk> --- The architecture-specific changes here are untested and need to be reviewed by architecture maintainers. Ben. arch/parisc/include/asm/bug.h | 8 ++++---- arch/powerpc/include/asm/bug.h | 6 +++--- arch/s390/include/asm/bug.h | 8 ++++---- arch/sh/include/asm/bug.h | 4 ++-- include/asm-generic/bug.h | 34 ++++++++++++++++++++++++++++++++-- kernel/panic.c | 24 ++++++++++++++++++++---- lib/bug.c | 2 +- 7 files changed, 66 insertions(+), 20 deletions(-)