* [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint
@ 2009-04-09 21:27 Frederic Weisbecker
2009-04-09 21:27 ` [PATCH 2/2] lockdep: choose to continue lock debugging despite taint Frederic Weisbecker
2009-04-10 12:12 ` [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Ingo Molnar
0 siblings, 2 replies; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-09 21:27 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Frederic Weisbecker, Peter Zijlstra
Impact: provide useful missing info for developers
Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...
But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.
This patch adds this missing warning.
Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..9e7420a 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
void add_taint(unsigned flag)
{
- /* can't trust the integrity of the kernel anymore: */
- debug_locks = 0;
+ /*
+ * Can't trust the integrity of the kernel anymore.
+ * We don't call directly debug_locks_off() because the issue
+ * is not necessarily serious enough to set oops_in_progress to 1
+ */
+ if (xchg(&debug_locks, 0))
+ printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
set_bit(flag, &tainted_mask);
}
EXPORT_SYMBOL(add_taint);
--
1.6.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-09 21:27 [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
@ 2009-04-09 21:27 ` Frederic Weisbecker
2009-04-10 12:15 ` Ingo Molnar
2009-04-10 12:12 ` [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Ingo Molnar
1 sibling, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-09 21:27 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Frederic Weisbecker, LTP, Peter Zijlstra
Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or anyone who might develop a kernel that happens
to be tainted.
Just provide an option to let one choose to continue lock-debugging
after any taint.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
diff --git a/kernel/panic.c b/kernel/panic.c
index 9e7420a..eadc0b5 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,6 +213,7 @@ unsigned long get_taint(void)
void add_taint(unsigned flag)
{
+#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
/*
* Can't trust the integrity of the kernel anymore.
* We don't call directly debug_locks_off() because the issue
@@ -220,6 +221,7 @@ void add_taint(unsigned flag)
*/
if (xchg(&debug_locks, 0))
printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+#endif
set_bit(flag, &tainted_mask);
}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index c6e854f..599888b 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -436,6 +436,20 @@ config PROVE_LOCKING
For more details, see Documentation/lockdep-design.txt.
+config LOCKDEP_IGNORE_TAINT
+ bool "Lock debugging: continue despite taints"
+ depends on LOCKDEP
+ default n
+ help
+ If your kernel becomes tainted because of a warning,
+ a staging or proprietary module loaded, etc... lockdep
+ will be disabled by default. Select this option if you
+ want lockdep to continue running despite a taint. If
+ you are working on a staging driver, experiencing
+ warnings but focusing on something else, or whatever
+ reason that makes you developing on a kernel that is
+ tainted, you might want to say Y here.
+
config LOCKDEP
bool
depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT
--
1.6.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint
2009-04-09 21:27 [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
2009-04-09 21:27 ` [PATCH 2/2] lockdep: choose to continue lock debugging despite taint Frederic Weisbecker
@ 2009-04-10 12:12 ` Ingo Molnar
2009-04-10 13:34 ` Frederic Weisbecker
1 sibling, 1 reply; 15+ messages in thread
From: Ingo Molnar @ 2009-04-10 12:12 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: LKML, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Impact: provide useful missing info for developers
>
> Kernel taint can occur in several situations such as warnings,
> load of prorietary or staging modules, bad page, etc...
>
> But when such taint happens, a developer might still be working on
> the kernel, expecting that lockdep is still enabled. But a taint
> disables lockdep without ever warning about it.
> Such a kernel behaviour doesn't really help for kernel development.
>
> This patch adds this missing warning.
>
> Since the taint is done most of the time after the main message that
> explain the real source issue, it seems safe to warn about it inside
> add_taint() so that it appears at last, without hurting the main
> information.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
>
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 3fd8c5b..9e7420a 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -213,8 +213,14 @@ unsigned long get_taint(void)
>
> void add_taint(unsigned flag)
> {
> - /* can't trust the integrity of the kernel anymore: */
> - debug_locks = 0;
> + /*
> + * Can't trust the integrity of the kernel anymore.
> + * We don't call directly debug_locks_off() because the issue
> + * is not necessarily serious enough to set oops_in_progress to 1
> + */
> + if (xchg(&debug_locks, 0))
> + printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> +
nice idea - but please use the proper debug_locks_off() construct
instead of an open-coded xchg(). Something like:
if (debug_locks_off())
printk(...);
should do the trick.
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-09 21:27 ` [PATCH 2/2] lockdep: choose to continue lock debugging despite taint Frederic Weisbecker
@ 2009-04-10 12:15 ` Ingo Molnar
2009-04-10 13:38 ` Frederic Weisbecker
0 siblings, 1 reply; 15+ messages in thread
From: Ingo Molnar @ 2009-04-10 12:15 UTC (permalink / raw)
To: Frederic Weisbecker, Greg KH; +Cc: LKML, LTP, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Lockdep is disabled after any kernel taints. This might be
> convenient to ignore bad locking issues which sources come from
> outside the kernel tree. Nevertheless, it might be a frustrating
> experience for the staging developers or anyone who might develop
> a kernel that happens to be tainted.
Good point. Not having lockdep coverage for drivers/staging/ just
prolongs their transition - not good.
But instead of this:
> void add_taint(unsigned flag)
> {
> +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> /*
> * Can't trust the integrity of the kernel anymore.
> * We don't call directly debug_locks_off() because the issue
> @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> */
> if (xchg(&debug_locks, 0))
> printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> +#endif
I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e.
something like:
if (!(flag & TAINT_CRAP) && debug_locks_off())
printk(...);
will do the trick.
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint
2009-04-10 12:12 ` [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Ingo Molnar
@ 2009-04-10 13:34 ` Frederic Weisbecker
2009-04-10 13:38 ` Ingo Molnar
0 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-10 13:34 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Peter Zijlstra
On Fri, Apr 10, 2009 at 02:12:43PM +0200, Ingo Molnar wrote:
>
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> > Impact: provide useful missing info for developers
> >
> > Kernel taint can occur in several situations such as warnings,
> > load of prorietary or staging modules, bad page, etc...
> >
> > But when such taint happens, a developer might still be working on
> > the kernel, expecting that lockdep is still enabled. But a taint
> > disables lockdep without ever warning about it.
> > Such a kernel behaviour doesn't really help for kernel development.
> >
> > This patch adds this missing warning.
> >
> > Since the taint is done most of the time after the main message that
> > explain the real source issue, it seems safe to warn about it inside
> > add_taint() so that it appears at last, without hurting the main
> > information.
> >
> > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> >
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 3fd8c5b..9e7420a 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -213,8 +213,14 @@ unsigned long get_taint(void)
> >
> > void add_taint(unsigned flag)
> > {
> > - /* can't trust the integrity of the kernel anymore: */
> > - debug_locks = 0;
> > + /*
> > + * Can't trust the integrity of the kernel anymore.
> > + * We don't call directly debug_locks_off() because the issue
> > + * is not necessarily serious enough to set oops_in_progress to 1
> > + */
> > + if (xchg(&debug_locks, 0))
> > + printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > +
>
> nice idea - but please use the proper debug_locks_off() construct
> instead of an open-coded xchg(). Something like:
>
> if (debug_locks_off())
> printk(...);
>
> should do the trick.
>
> Ingo
Yeah, I first wanted to do so but was shy about the oops_in_progress = 1
inside debug_locks_off(). Isn't it a problem?
Thanks,
Frederic.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint
2009-04-10 13:34 ` Frederic Weisbecker
@ 2009-04-10 13:38 ` Ingo Molnar
0 siblings, 0 replies; 15+ messages in thread
From: Ingo Molnar @ 2009-04-10 13:38 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: LKML, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Fri, Apr 10, 2009 at 02:12:43PM +0200, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> > > Impact: provide useful missing info for developers
> > >
> > > Kernel taint can occur in several situations such as warnings,
> > > load of prorietary or staging modules, bad page, etc...
> > >
> > > But when such taint happens, a developer might still be working on
> > > the kernel, expecting that lockdep is still enabled. But a taint
> > > disables lockdep without ever warning about it.
> > > Such a kernel behaviour doesn't really help for kernel development.
> > >
> > > This patch adds this missing warning.
> > >
> > > Since the taint is done most of the time after the main message that
> > > explain the real source issue, it seems safe to warn about it inside
> > > add_taint() so that it appears at last, without hurting the main
> > > information.
> > >
> > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> > >
> > > diff --git a/kernel/panic.c b/kernel/panic.c
> > > index 3fd8c5b..9e7420a 100644
> > > --- a/kernel/panic.c
> > > +++ b/kernel/panic.c
> > > @@ -213,8 +213,14 @@ unsigned long get_taint(void)
> > >
> > > void add_taint(unsigned flag)
> > > {
> > > - /* can't trust the integrity of the kernel anymore: */
> > > - debug_locks = 0;
> > > + /*
> > > + * Can't trust the integrity of the kernel anymore.
> > > + * We don't call directly debug_locks_off() because the issue
> > > + * is not necessarily serious enough to set oops_in_progress to 1
> > > + */
> > > + if (xchg(&debug_locks, 0))
> > > + printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > +
> >
> > nice idea - but please use the proper debug_locks_off() construct
> > instead of an open-coded xchg(). Something like:
> >
> > if (debug_locks_off())
> > printk(...);
> >
> > should do the trick.
> >
> > Ingo
>
> Yeah, I first wanted to do so but was shy about the
> oops_in_progress = 1 inside debug_locks_off(). Isn't it a problem?
hm, indeed. How about providing a __debug_locks_off() primitive that
only does the xchg and none of the oops_in_progress and
console_verbose() calls?
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-10 12:15 ` Ingo Molnar
@ 2009-04-10 13:38 ` Frederic Weisbecker
2009-04-10 13:45 ` Ingo Molnar
0 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-10 13:38 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Greg KH, LKML, LTP, Peter Zijlstra
On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
>
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> > Lockdep is disabled after any kernel taints. This might be
> > convenient to ignore bad locking issues which sources come from
> > outside the kernel tree. Nevertheless, it might be a frustrating
> > experience for the staging developers or anyone who might develop
> > a kernel that happens to be tainted.
>
> Good point. Not having lockdep coverage for drivers/staging/ just
> prolongs their transition - not good.
>
> But instead of this:
>
> > void add_taint(unsigned flag)
> > {
> > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > /*
> > * Can't trust the integrity of the kernel anymore.
> > * We don't call directly debug_locks_off() because the issue
> > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > */
> > if (xchg(&debug_locks, 0))
> > printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > +#endif
>
> I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e.
> something like:
>
> if (!(flag & TAINT_CRAP) && debug_locks_off())
> printk(...);
>
> will do the trick.
>
> Ingo
Ok, but this is not only about staging. It's also about TAINT_WARN.
Just imagine that you report a warning to a maintainer, and while
you are waiting for it to be fixed, you can't use lockdep for your
own needs.
Hm?
Frederic.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-10 13:38 ` Frederic Weisbecker
@ 2009-04-10 13:45 ` Ingo Molnar
2009-04-11 1:15 ` Frederic Weisbecker
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Ingo Molnar @ 2009-04-10 13:45 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: Greg KH, LKML, LTP, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> > > Lockdep is disabled after any kernel taints. This might be
> > > convenient to ignore bad locking issues which sources come from
> > > outside the kernel tree. Nevertheless, it might be a frustrating
> > > experience for the staging developers or anyone who might develop
> > > a kernel that happens to be tainted.
> >
> > Good point. Not having lockdep coverage for drivers/staging/ just
> > prolongs their transition - not good.
> >
> > But instead of this:
> >
> > > void add_taint(unsigned flag)
> > > {
> > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > > /*
> > > * Can't trust the integrity of the kernel anymore.
> > > * We don't call directly debug_locks_off() because the issue
> > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > > */
> > > if (xchg(&debug_locks, 0))
> > > printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > +#endif
> >
> > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e.
> > something like:
> >
> > if (!(flag & TAINT_CRAP) && debug_locks_off())
> > printk(...);
> >
> > will do the trick.
> >
> > Ingo
>
>
> Ok, but this is not only about staging. It's also about
> TAINT_WARN. Just imagine that you report a warning to a
> maintainer, and while you are waiting for it to be fixed, you
> can't use lockdep for your own needs.
>
> Hm?
We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-10 13:45 ` Ingo Molnar
@ 2009-04-11 1:15 ` Frederic Weisbecker
2009-04-11 1:37 ` Frederic Weisbecker
2009-04-11 1:17 ` [PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
2009-04-11 1:17 ` [PATCH 2/2 v2] lockdep: continue lock debugging despite some taints Frederic Weisbecker
2 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-11 1:15 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Greg KH, LKML, LTP, Peter Zijlstra
On Fri, Apr 10, 2009 at 03:45:20PM +0200, Ingo Molnar wrote:
>
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> > On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> > >
> > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > >
> > > > Lockdep is disabled after any kernel taints. This might be
> > > > convenient to ignore bad locking issues which sources come from
> > > > outside the kernel tree. Nevertheless, it might be a frustrating
> > > > experience for the staging developers or anyone who might develop
> > > > a kernel that happens to be tainted.
> > >
> > > Good point. Not having lockdep coverage for drivers/staging/ just
> > > prolongs their transition - not good.
> > >
> > > But instead of this:
> > >
> > > > void add_taint(unsigned flag)
> > > > {
> > > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > > > /*
> > > > * Can't trust the integrity of the kernel anymore.
> > > > * We don't call directly debug_locks_off() because the issue
> > > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > > > */
> > > > if (xchg(&debug_locks, 0))
> > > > printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > > +#endif
> > >
> > > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e.
> > > something like:
> > >
> > > if (!(flag & TAINT_CRAP) && debug_locks_off())
> > > printk(...);
> > >
> > > will do the trick.
> > >
> > > Ingo
> >
> >
> > Ok, but this is not only about staging. It's also about
> > TAINT_WARN. Just imagine that you report a warning to a
> > maintainer, and while you are waiting for it to be fixed, you
> > can't use lockdep for your own needs.
> >
> > Hm?
>
> We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).
>
> Ingo
Fine :-)
See the v2 on further mails in this thread.
Side request: do you think you could merge them on kill-the-BKL tree?
Thanks!
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint
2009-04-10 13:45 ` Ingo Molnar
2009-04-11 1:15 ` Frederic Weisbecker
@ 2009-04-11 1:17 ` Frederic Weisbecker
2009-04-12 14:45 ` [tip:core/urgent] " Frederic Weisbecker
2009-04-11 1:17 ` [PATCH 2/2 v2] lockdep: continue lock debugging despite some taints Frederic Weisbecker
2 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-11 1:17 UTC (permalink / raw)
To: Ingo Molnar; +Cc: LKML, Frederic Weisbecker, Peter Zijlstra
Impact: provide useful missing info for developers
Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...
But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.
This patch adds this missing warning.
Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.
v2: Use a generic helper to disable lockdep instead of an open coded
xchg().
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
---
include/linux/debug_locks.h | 7 +++++++
kernel/panic.c | 10 ++++++++--
lib/debug_locks.c | 2 +-
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/linux/debug_locks.h b/include/linux/debug_locks.h
index 096476f..493dedb 100644
--- a/include/linux/debug_locks.h
+++ b/include/linux/debug_locks.h
@@ -2,12 +2,19 @@
#define __LINUX_DEBUG_LOCKING_H
#include <linux/kernel.h>
+#include <asm/atomic.h>
struct task_struct;
extern int debug_locks;
extern int debug_locks_silent;
+
+static inline int __debug_locks_off(void)
+{
+ return xchg(&debug_locks, 0);
+}
+
/*
* Generic 'turn off all lock debugging' function:
*/
diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..940ca14 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
void add_taint(unsigned flag)
{
- /* can't trust the integrity of the kernel anymore: */
- debug_locks = 0;
+ /*
+ * Can't trust the integrity of the kernel anymore.
+ * We don't call directly debug_locks_off() because the issue
+ * is not necessarily serious enough to set oops_in_progress to 1
+ */
+ if (__debug_locks_off())
+ printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
set_bit(flag, &tainted_mask);
}
EXPORT_SYMBOL(add_taint);
diff --git a/lib/debug_locks.c b/lib/debug_locks.c
index 0218b46..bc3b117 100644
--- a/lib/debug_locks.c
+++ b/lib/debug_locks.c
@@ -36,7 +36,7 @@ int debug_locks_silent;
*/
int debug_locks_off(void)
{
- if (xchg(&debug_locks, 0)) {
+ if (__debug_locks_off()) {
if (!debug_locks_silent) {
oops_in_progress = 1;
console_verbose();
--
1.6.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2 v2] lockdep: continue lock debugging despite some taints
2009-04-10 13:45 ` Ingo Molnar
2009-04-11 1:15 ` Frederic Weisbecker
2009-04-11 1:17 ` [PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
@ 2009-04-11 1:17 ` Frederic Weisbecker
2009-04-12 14:45 ` [tip:core/urgent] " Frederic Weisbecker
2 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-11 1:17 UTC (permalink / raw)
To: Ingo Molnar
Cc: LKML, Frederic Weisbecker, LTP, Peter Zijlstra, Greg KH,
Peter Zijlstra, Greg KH
Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or those who experience a warning but are focused
on another things that require lockdep.
The v2 of this patch simply don't disable anymore lockdep in case
of TAINT_CRAP and TAINT_WARN events.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg KH <gregkh@suse.de>
---
kernel/panic.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index 940ca14..934fb37 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -217,8 +217,10 @@ void add_taint(unsigned flag)
* Can't trust the integrity of the kernel anymore.
* We don't call directly debug_locks_off() because the issue
* is not necessarily serious enough to set oops_in_progress to 1
+ * Also we want to keep up lockdep for staging development and
+ * post-warning case.
*/
- if (__debug_locks_off())
+ if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
set_bit(flag, &tainted_mask);
--
1.6.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-11 1:15 ` Frederic Weisbecker
@ 2009-04-11 1:37 ` Frederic Weisbecker
2009-04-12 14:09 ` Ingo Molnar
0 siblings, 1 reply; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-11 1:37 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Greg KH, LKML, LTP, Peter Zijlstra
On Sat, Apr 11, 2009 at 03:15:06AM +0200, Frederic Weisbecker wrote:
> On Fri, Apr 10, 2009 at 03:45:20PM +0200, Ingo Molnar wrote:
> >
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> > > On Fri, Apr 10, 2009 at 02:15:15PM +0200, Ingo Molnar wrote:
> > > >
> > > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > >
> > > > > Lockdep is disabled after any kernel taints. This might be
> > > > > convenient to ignore bad locking issues which sources come from
> > > > > outside the kernel tree. Nevertheless, it might be a frustrating
> > > > > experience for the staging developers or anyone who might develop
> > > > > a kernel that happens to be tainted.
> > > >
> > > > Good point. Not having lockdep coverage for drivers/staging/ just
> > > > prolongs their transition - not good.
> > > >
> > > > But instead of this:
> > > >
> > > > > void add_taint(unsigned flag)
> > > > > {
> > > > > +#ifndef CONFIG_LOCKDEP_IGNORE_TAINT
> > > > > /*
> > > > > * Can't trust the integrity of the kernel anymore.
> > > > > * We don't call directly debug_locks_off() because the issue
> > > > > @@ -220,6 +221,7 @@ void add_taint(unsigned flag)
> > > > > */
> > > > > if (xchg(&debug_locks, 0))
> > > > > printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
> > > > > +#endif
> > > >
> > > > I'd suggest to not do the debug_locks_off() call if TAINT_CRAP. I.e.
> > > > something like:
> > > >
> > > > if (!(flag & TAINT_CRAP) && debug_locks_off())
> > > > printk(...);
> > > >
> > > > will do the trick.
> > > >
> > > > Ingo
> > >
> > >
> > > Ok, but this is not only about staging. It's also about
> > > TAINT_WARN. Just imagine that you report a warning to a
> > > maintainer, and while you are waiting for it to be fixed, you
> > > can't use lockdep for your own needs.
> > >
> > > Hm?
> >
> > We can exclude TAINT_WARN too - i.e. (TAINT_CRAP|TAINT_WARN).
> >
> > Ingo
>
>
> Fine :-)
>
> See the v2 on further mails in this thread.
>
> Side request: do you think you could merge them on kill-the-BKL tree?
Sorry, forget about this side request. I thought merging tip:master
into tip:core/kill-the-BKL would make me suffer a rain of conflicts
but actually I've just tried and I only encountered a very simple
conflict in sched.c so it's very easy to manage it locally. Then
we can keep the history of kill-the-BKL, for now it's good as is.
Frederic.
> Thanks!
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] lockdep: choose to continue lock debugging despite taint
2009-04-11 1:37 ` Frederic Weisbecker
@ 2009-04-12 14:09 ` Ingo Molnar
0 siblings, 0 replies; 15+ messages in thread
From: Ingo Molnar @ 2009-04-12 14:09 UTC (permalink / raw)
To: Frederic Weisbecker; +Cc: Greg KH, LKML, LTP, Peter Zijlstra
* Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Side request: do you think you could merge them on kill-the-BKL
> > tree?
>
> Sorry, forget about this side request. I thought merging
> tip:master into tip:core/kill-the-BKL would make me suffer a rain
> of conflicts but actually I've just tried and I only encountered a
> very simple conflict in sched.c so it's very easy to manage it
> locally. Then we can keep the history of kill-the-BKL, for now
> it's good as is.
Ok - that's how we like topics anyway: as independent trees which
happen to be mostly conflict-free when merged into tip:master.
Ingo
^ permalink raw reply [flat|nested] 15+ messages in thread
* [tip:core/urgent] lockdep: warn about lockdep disabling after kernel taint
2009-04-11 1:17 ` [PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
@ 2009-04-12 14:45 ` Frederic Weisbecker
0 siblings, 0 replies; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-12 14:45 UTC (permalink / raw)
To: linux-tip-commits; +Cc: linux-kernel, hpa, mingo, fweisbec, peterz, tglx, mingo
Commit-ID: 9eeba6138cefc0435695463ddadb0d95e0a6bcd2
Gitweb: http://git.kernel.org/tip/9eeba6138cefc0435695463ddadb0d95e0a6bcd2
Author: Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sat, 11 Apr 2009 03:17:17 +0200
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 12 Apr 2009 16:10:51 +0200
lockdep: warn about lockdep disabling after kernel taint
Impact: provide useful missing info for developers
Kernel taint can occur in several situations such as warnings,
load of prorietary or staging modules, bad page, etc...
But when such taint happens, a developer might still be working on
the kernel, expecting that lockdep is still enabled. But a taint
disables lockdep without ever warning about it.
Such a kernel behaviour doesn't really help for kernel development.
This patch adds this missing warning.
Since the taint is done most of the time after the main message that
explain the real source issue, it seems safe to warn about it inside
add_taint() so that it appears at last, without hurting the main
information.
v2: Use a generic helper to disable lockdep instead of an
open coded xchg().
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
LKML-Reference: <1239412638-6739-1-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/debug_locks.h | 7 +++++++
kernel/panic.c | 10 ++++++++--
lib/debug_locks.c | 2 +-
3 files changed, 16 insertions(+), 3 deletions(-)
diff --git a/include/linux/debug_locks.h b/include/linux/debug_locks.h
index 096476f..493dedb 100644
--- a/include/linux/debug_locks.h
+++ b/include/linux/debug_locks.h
@@ -2,12 +2,19 @@
#define __LINUX_DEBUG_LOCKING_H
#include <linux/kernel.h>
+#include <asm/atomic.h>
struct task_struct;
extern int debug_locks;
extern int debug_locks_silent;
+
+static inline int __debug_locks_off(void)
+{
+ return xchg(&debug_locks, 0);
+}
+
/*
* Generic 'turn off all lock debugging' function:
*/
diff --git a/kernel/panic.c b/kernel/panic.c
index 3fd8c5b..940ca14 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -213,8 +213,14 @@ unsigned long get_taint(void)
void add_taint(unsigned flag)
{
- /* can't trust the integrity of the kernel anymore: */
- debug_locks = 0;
+ /*
+ * Can't trust the integrity of the kernel anymore.
+ * We don't call directly debug_locks_off() because the issue
+ * is not necessarily serious enough to set oops_in_progress to 1
+ */
+ if (__debug_locks_off())
+ printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
+
set_bit(flag, &tainted_mask);
}
EXPORT_SYMBOL(add_taint);
diff --git a/lib/debug_locks.c b/lib/debug_locks.c
index 0218b46..bc3b117 100644
--- a/lib/debug_locks.c
+++ b/lib/debug_locks.c
@@ -36,7 +36,7 @@ int debug_locks_silent;
*/
int debug_locks_off(void)
{
- if (xchg(&debug_locks, 0)) {
+ if (__debug_locks_off()) {
if (!debug_locks_silent) {
oops_in_progress = 1;
console_verbose();
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [tip:core/urgent] lockdep: continue lock debugging despite some taints
2009-04-11 1:17 ` [PATCH 2/2 v2] lockdep: continue lock debugging despite some taints Frederic Weisbecker
@ 2009-04-12 14:45 ` Frederic Weisbecker
0 siblings, 0 replies; 15+ messages in thread
From: Frederic Weisbecker @ 2009-04-12 14:45 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, peterz, ltp-list, fweisbec, gregkh,
tglx, mingo
Commit-ID: 574bbe782057fdf0490dc7dec906a2dc26363e20
Gitweb: http://git.kernel.org/tip/574bbe782057fdf0490dc7dec906a2dc26363e20
Author: Frederic Weisbecker <fweisbec@gmail.com>
AuthorDate: Sat, 11 Apr 2009 03:17:18 +0200
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Sun, 12 Apr 2009 16:10:52 +0200
lockdep: continue lock debugging despite some taints
Impact: broaden lockdep checks
Lockdep is disabled after any kernel taints. This might be convenient
to ignore bad locking issues which sources come from outside the kernel
tree. Nevertheless, it might be a frustrating experience for the
staging developers or those who experience a warning but are focused
on another things that require lockdep.
The v2 of this patch simply don't disable anymore lockdep in case
of TAINT_CRAP and TAINT_WARN events.
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: LTP <ltp-list@lists.sourceforge.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg KH <gregkh@suse.de>
LKML-Reference: <1239412638-6739-2-git-send-email-fweisbec@gmail.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
kernel/panic.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/kernel/panic.c b/kernel/panic.c
index 940ca14..934fb37 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -217,8 +217,10 @@ void add_taint(unsigned flag)
* Can't trust the integrity of the kernel anymore.
* We don't call directly debug_locks_off() because the issue
* is not necessarily serious enough to set oops_in_progress to 1
+ * Also we want to keep up lockdep for staging development and
+ * post-warning case.
*/
- if (__debug_locks_off())
+ if (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
printk(KERN_WARNING "Disabling lockdep due to kernel taint\n");
set_bit(flag, &tainted_mask);
^ permalink raw reply related [flat|nested] 15+ messages in thread
end of thread, other threads:[~2009-04-12 14:46 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-04-09 21:27 [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
2009-04-09 21:27 ` [PATCH 2/2] lockdep: choose to continue lock debugging despite taint Frederic Weisbecker
2009-04-10 12:15 ` Ingo Molnar
2009-04-10 13:38 ` Frederic Weisbecker
2009-04-10 13:45 ` Ingo Molnar
2009-04-11 1:15 ` Frederic Weisbecker
2009-04-11 1:37 ` Frederic Weisbecker
2009-04-12 14:09 ` Ingo Molnar
2009-04-11 1:17 ` [PATCH 1/2 v2] lockdep: warn about lockdep disabling after kernel taint Frederic Weisbecker
2009-04-12 14:45 ` [tip:core/urgent] " Frederic Weisbecker
2009-04-11 1:17 ` [PATCH 2/2 v2] lockdep: continue lock debugging despite some taints Frederic Weisbecker
2009-04-12 14:45 ` [tip:core/urgent] " Frederic Weisbecker
2009-04-10 12:12 ` [PATCH 1/2] lockdep: warn about lockdep disabling after kernel taint Ingo Molnar
2009-04-10 13:34 ` Frederic Weisbecker
2009-04-10 13:38 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).