All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
@ 2011-10-28  3:36 Ben Hutchings
  2011-10-31  1:59 ` Rusty Russell
                   ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Ben Hutchings @ 2011-10-28  3:36 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar
  Cc: Nick Bowler, Greg KH, Dave Jones, Rusty Russell, Randy Dunlap,
	LKML, Debian kernel maintainers

Currently lock debugging is disabled when the kernel is tainted, with
a few exceptions.  It is already recognised that this can be useful
for staging modules (TAINT_CRAP), but that also goes for out-of-tree
modules (TAINT_OOT_MODULE) so long as core kernel developers don't
have to spend time debugging them.  Also, there are several reasons
for tainting that are unlikely to introduce false locking bug reports
(e.g. TAINT_FIRMWARE_WORKAROUND).

Instead of disabling lock debugging, show the taint flags in all
lockdep and rtmutex-debug error messages.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 kernel/lockdep.c       |   23 +++++++++++++++--------
 kernel/panic.c         |   10 ----------
 kernel/rtmutex-debug.c |    1 +
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index e69434b..b2c4537 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr)
 	}
 }
 
-static void print_kernel_version(void)
+static void print_kernel_ident(void)
 {
-	printk("%s %.*s\n", init_utsname()->release,
+	printk("%s %.*s %s\n", init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
-		init_utsname()->version);
+		init_utsname()->version,
+		print_tainted());
 }
 
 static int very_verbose(struct lock_class *class)
@@ -1148,7 +1149,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,
 	printk("\n");
 	printk("======================================================\n");
 	printk("[ INFO: possible circular locking dependency detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("-------------------------------------------------------\n");
 	printk("%s/%d is trying to acquire lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -1487,7 +1488,7 @@ print_bad_irq_dependency(struct task_struct *curr,
 	printk("======================================================\n");
 	printk("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n",
 		irqclass, irqclass);
-	print_kernel_version();
+	print_kernel_ident();
 	printk("------------------------------------------------------\n");
 	printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",
 		curr->comm, task_pid_nr(curr),
@@ -1716,7 +1717,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
 	printk("\n");
 	printk("=============================================\n");
 	printk("[ INFO: possible recursive locking detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------------------\n");
 	printk("%s/%d is trying to acquire lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -2223,7 +2224,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
 	printk("\n");
 	printk("=================================\n");
 	printk("[ INFO: inconsistent lock state ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------\n");
 
 	printk("inconsistent {%s} -> {%s} usage.\n",
@@ -2288,7 +2289,7 @@ print_irq_inversion_bug(struct task_struct *curr,
 	printk("\n");
 	printk("=========================================================\n");
 	printk("[ INFO: possible irq lock inversion dependency detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------------------------------\n");
 	printk("%s/%d just changed the state of lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -3169,6 +3170,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,
 	printk("\n");
 	printk("=====================================\n");
 	printk("[ BUG: bad unlock balance detected! ]\n");
+	print_kernel_ident();
 	printk("-------------------------------------\n");
 	printk("%s/%d is trying to release lock (",
 		curr->comm, task_pid_nr(curr));
@@ -3613,6 +3615,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
 	printk("\n");
 	printk("=================================\n");
 	printk("[ BUG: bad contention detected! ]\n");
+	print_kernel_ident();
 	printk("---------------------------------\n");
 	printk("%s/%d is trying to contend lock (",
 		curr->comm, task_pid_nr(curr));
@@ -3987,6 +3990,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
 	printk("\n");
 	printk("=========================\n");
 	printk("[ BUG: held lock freed! ]\n");
+	print_kernel_ident();
 	printk("-------------------------\n");
 	printk("%s/%d is freeing memory %p-%p, with a lock still held there!\n",
 		curr->comm, task_pid_nr(curr), mem_from, mem_to-1);
@@ -4044,6 +4048,7 @@ static void print_held_locks_bug(struct task_struct *curr)
 	printk("\n");
 	printk("=====================================\n");
 	printk("[ BUG: lock held at task exit time! ]\n");
+	print_kernel_ident();
 	printk("-------------------------------------\n");
 	printk("%s/%d is exiting with locks still held!\n",
 		curr->comm, task_pid_nr(curr));
@@ -4141,6 +4146,7 @@ void lockdep_sys_exit(void)
 		printk("\n");
 		printk("================================================\n");
 		printk("[ BUG: lock held when returning to user space! ]\n");
+		print_kernel_ident();
 		printk("------------------------------------------------\n");
 		printk("%s/%d is leaving the kernel with locks still held!\n",
 				curr->comm, curr->pid);
@@ -4160,6 +4166,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 	printk("\n");
 	printk("===============================\n");
 	printk("[ INFO: suspicious RCU usage. ]\n");
+	print_kernel_ident();
 	printk("-------------------------------\n");
 	printk("%s:%d %s!\n", file, line, s);
 	printk("\nother info that might help us debug this:\n\n");
diff --git a/kernel/panic.c b/kernel/panic.c
index b2659360..ad03fb5 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -233,16 +233,6 @@ unsigned long get_taint(void)
 
 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 (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
-		printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
-
 	set_bit(flag, &tainted_mask);
 }
 EXPORT_SYMBOL(add_taint);
diff --git a/kernel/rtmutex-debug.c b/kernel/rtmutex-debug.c
index a2e7e72..077257f 100644
--- a/kernel/rtmutex-debug.c
+++ b/kernel/rtmutex-debug.c
@@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)
 
 	printk("\n============================================\n");
 	printk(  "[ BUG: circular locking deadlock detected! ]\n");
+	printk("%s\n", print_tainted());
 	printk(  "--------------------------------------------\n");
 	printk("%s/%d is deadlocking current task %s/%d\n\n",
 	       task->comm, task_pid_nr(task),
-- 
1.7.7




^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
  2011-10-28  3:36 [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error Ben Hutchings
@ 2011-10-31  1:59 ` Rusty Russell
  2011-11-16 21:52 ` Ben Hutchings
  2011-12-06  9:38 ` [tip:core/locking] lockdep, rtmutex, bug: " tip-bot for Ben Hutchings
  2 siblings, 0 replies; 24+ messages in thread
From: Rusty Russell @ 2011-10-31  1:59 UTC (permalink / raw)
  To: Ben Hutchings, Peter Zijlstra, Ingo Molnar
  Cc: Nick Bowler, Greg KH, Dave Jones, Randy Dunlap, LKML,
	Debian kernel maintainers

On Fri, 28 Oct 2011 04:36:55 +0100, Ben Hutchings <ben@decadent.org.uk> wrote:
> Currently lock debugging is disabled when the kernel is tainted, with
> a few exceptions.  It is already recognised that this can be useful
> for staging modules (TAINT_CRAP), but that also goes for out-of-tree
> modules (TAINT_OOT_MODULE) so long as core kernel developers don't
> have to spend time debugging them.  Also, there are several reasons
> for tainting that are unlikely to introduce false locking bug reports
> (e.g. TAINT_FIRMWARE_WORKAROUND).
> 
> Instead of disabling lock debugging, show the taint flags in all
> lockdep and rtmutex-debug error messages.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Acked-by: Rusty Russell <rusty@rustcorp.com.au>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
  2011-10-28  3:36 [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error Ben Hutchings
  2011-10-31  1:59 ` Rusty Russell
@ 2011-11-16 21:52 ` Ben Hutchings
  2011-11-16 22:24   ` Dave Jones
  2011-11-17  8:48   ` Peter Zijlstra
  2011-12-06  9:38 ` [tip:core/locking] lockdep, rtmutex, bug: " tip-bot for Ben Hutchings
  2 siblings, 2 replies; 24+ messages in thread
From: Ben Hutchings @ 2011-11-16 21:52 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar
  Cc: Nick Bowler, Greg KH, Dave Jones, Rusty Russell, Randy Dunlap,
	LKML, Debian kernel maintainers

Please could you ack or nak this?

Ben.

On Fri, Oct 28, 2011 at 04:36:55AM +0100, Ben Hutchings wrote:
> Currently lock debugging is disabled when the kernel is tainted, with
> a few exceptions.  It is already recognised that this can be useful
> for staging modules (TAINT_CRAP), but that also goes for out-of-tree
> modules (TAINT_OOT_MODULE) so long as core kernel developers don't
> have to spend time debugging them.  Also, there are several reasons
> for tainting that are unlikely to introduce false locking bug reports
> (e.g. TAINT_FIRMWARE_WORKAROUND).
> 
> Instead of disabling lock debugging, show the taint flags in all
> lockdep and rtmutex-debug error messages.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  kernel/lockdep.c       |   23 +++++++++++++++--------
>  kernel/panic.c         |   10 ----------
>  kernel/rtmutex-debug.c |    1 +
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/lockdep.c b/kernel/lockdep.c
> index e69434b..b2c4537 100644
> --- a/kernel/lockdep.c
> +++ b/kernel/lockdep.c
> @@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr)
>  	}
>  }
>  
> -static void print_kernel_version(void)
> +static void print_kernel_ident(void)
>  {
> -	printk("%s %.*s\n", init_utsname()->release,
> +	printk("%s %.*s %s\n", init_utsname()->release,
>  		(int)strcspn(init_utsname()->version, " "),
> -		init_utsname()->version);
> +		init_utsname()->version,
> +		print_tainted());
>  }
>  
>  static int very_verbose(struct lock_class *class)
> @@ -1148,7 +1149,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,
>  	printk("\n");
>  	printk("======================================================\n");
>  	printk("[ INFO: possible circular locking dependency detected ]\n");
> -	print_kernel_version();
> +	print_kernel_ident();
>  	printk("-------------------------------------------------------\n");
>  	printk("%s/%d is trying to acquire lock:\n",
>  		curr->comm, task_pid_nr(curr));
> @@ -1487,7 +1488,7 @@ print_bad_irq_dependency(struct task_struct *curr,
>  	printk("======================================================\n");
>  	printk("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n",
>  		irqclass, irqclass);
> -	print_kernel_version();
> +	print_kernel_ident();
>  	printk("------------------------------------------------------\n");
>  	printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",
>  		curr->comm, task_pid_nr(curr),
> @@ -1716,7 +1717,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
>  	printk("\n");
>  	printk("=============================================\n");
>  	printk("[ INFO: possible recursive locking detected ]\n");
> -	print_kernel_version();
> +	print_kernel_ident();
>  	printk("---------------------------------------------\n");
>  	printk("%s/%d is trying to acquire lock:\n",
>  		curr->comm, task_pid_nr(curr));
> @@ -2223,7 +2224,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
>  	printk("\n");
>  	printk("=================================\n");
>  	printk("[ INFO: inconsistent lock state ]\n");
> -	print_kernel_version();
> +	print_kernel_ident();
>  	printk("---------------------------------\n");
>  
>  	printk("inconsistent {%s} -> {%s} usage.\n",
> @@ -2288,7 +2289,7 @@ print_irq_inversion_bug(struct task_struct *curr,
>  	printk("\n");
>  	printk("=========================================================\n");
>  	printk("[ INFO: possible irq lock inversion dependency detected ]\n");
> -	print_kernel_version();
> +	print_kernel_ident();
>  	printk("---------------------------------------------------------\n");
>  	printk("%s/%d just changed the state of lock:\n",
>  		curr->comm, task_pid_nr(curr));
> @@ -3169,6 +3170,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,
>  	printk("\n");
>  	printk("=====================================\n");
>  	printk("[ BUG: bad unlock balance detected! ]\n");
> +	print_kernel_ident();
>  	printk("-------------------------------------\n");
>  	printk("%s/%d is trying to release lock (",
>  		curr->comm, task_pid_nr(curr));
> @@ -3613,6 +3615,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
>  	printk("\n");
>  	printk("=================================\n");
>  	printk("[ BUG: bad contention detected! ]\n");
> +	print_kernel_ident();
>  	printk("---------------------------------\n");
>  	printk("%s/%d is trying to contend lock (",
>  		curr->comm, task_pid_nr(curr));
> @@ -3987,6 +3990,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
>  	printk("\n");
>  	printk("=========================\n");
>  	printk("[ BUG: held lock freed! ]\n");
> +	print_kernel_ident();
>  	printk("-------------------------\n");
>  	printk("%s/%d is freeing memory %p-%p, with a lock still held there!\n",
>  		curr->comm, task_pid_nr(curr), mem_from, mem_to-1);
> @@ -4044,6 +4048,7 @@ static void print_held_locks_bug(struct task_struct *curr)
>  	printk("\n");
>  	printk("=====================================\n");
>  	printk("[ BUG: lock held at task exit time! ]\n");
> +	print_kernel_ident();
>  	printk("-------------------------------------\n");
>  	printk("%s/%d is exiting with locks still held!\n",
>  		curr->comm, task_pid_nr(curr));
> @@ -4141,6 +4146,7 @@ void lockdep_sys_exit(void)
>  		printk("\n");
>  		printk("================================================\n");
>  		printk("[ BUG: lock held when returning to user space! ]\n");
> +		print_kernel_ident();
>  		printk("------------------------------------------------\n");
>  		printk("%s/%d is leaving the kernel with locks still held!\n",
>  				curr->comm, curr->pid);
> @@ -4160,6 +4166,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
>  	printk("\n");
>  	printk("===============================\n");
>  	printk("[ INFO: suspicious RCU usage. ]\n");
> +	print_kernel_ident();
>  	printk("-------------------------------\n");
>  	printk("%s:%d %s!\n", file, line, s);
>  	printk("\nother info that might help us debug this:\n\n");
> diff --git a/kernel/panic.c b/kernel/panic.c
> index b2659360..ad03fb5 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -233,16 +233,6 @@ unsigned long get_taint(void)
>  
>  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 (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
> -		printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
> -
>  	set_bit(flag, &tainted_mask);
>  }
>  EXPORT_SYMBOL(add_taint);
> diff --git a/kernel/rtmutex-debug.c b/kernel/rtmutex-debug.c
> index a2e7e72..077257f 100644
> --- a/kernel/rtmutex-debug.c
> +++ b/kernel/rtmutex-debug.c
> @@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)
>  
>  	printk("\n============================================\n");
>  	printk(  "[ BUG: circular locking deadlock detected! ]\n");
> +	printk("%s\n", print_tainted());
>  	printk(  "--------------------------------------------\n");
>  	printk("%s/%d is deadlocking current task %s/%d\n\n",
>  	       task->comm, task_pid_nr(task),
> -- 
> 1.7.7
> 
> 
> 

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
  2011-11-16 21:52 ` Ben Hutchings
@ 2011-11-16 22:24   ` Dave Jones
  2011-11-17  8:48   ` Peter Zijlstra
  1 sibling, 0 replies; 24+ messages in thread
From: Dave Jones @ 2011-11-16 22:24 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Peter Zijlstra, Ingo Molnar, Nick Bowler, Greg KH, Rusty Russell,
	Randy Dunlap, LKML, Debian kernel maintainers

On Wed, Nov 16, 2011 at 09:52:36PM +0000, Ben Hutchings wrote:
 > Please could you ack or nak this?
 > 
 > Ben.

sorry, thought I already had.

Reviewed-by: Dave Jones <davej@redhat.com>


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
  2011-11-16 21:52 ` Ben Hutchings
  2011-11-16 22:24   ` Dave Jones
@ 2011-11-17  8:48   ` Peter Zijlstra
  2011-11-17 15:18     ` Ben Hutchings
  1 sibling, 1 reply; 24+ messages in thread
From: Peter Zijlstra @ 2011-11-17  8:48 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Ingo Molnar, Nick Bowler, Greg KH, Dave Jones, Rusty Russell,
	Randy Dunlap, LKML, Debian kernel maintainers

On Wed, 2011-11-16 at 21:52 +0000, Ben Hutchings wrote:
> On Fri, Oct 28, 2011 at 04:36:55AM +0100, Ben Hutchings wrote:
> > Currently lock debugging is disabled when the kernel is tainted, with
> > a few exceptions.  It is already recognised that this can be useful
> > for staging modules (TAINT_CRAP), but that also goes for out-of-tree
> > modules (TAINT_OOT_MODULE) so long as core kernel developers don't
> > have to spend time debugging them.  Also, there are several reasons
> > for tainting that are unlikely to introduce false locking bug reports
> > (e.g. TAINT_FIRMWARE_WORKAROUND).



> > Instead of disabling lock debugging, show the taint flags in all
> > lockdep and rtmutex-debug error messages.


So this is two patches in one. I took the last part, the printing of the
taint flags thing, not the first part.

I did a small patch adding TAINT_FIRMWARE_WORKAROUND to the list of
TAINTS that shouldn't disable lockdep.

As for OOT_MODULE, with staging the only reason not to merge a module is
it being the wrong license and I really can't be arsed about OOT stuff
anyway, so no.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error
  2011-11-17  8:48   ` Peter Zijlstra
@ 2011-11-17 15:18     ` Ben Hutchings
  0 siblings, 0 replies; 24+ messages in thread
From: Ben Hutchings @ 2011-11-17 15:18 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ingo Molnar, Nick Bowler, Greg KH, Dave Jones, Rusty Russell,
	Randy Dunlap, LKML, Debian kernel maintainers

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

rap,On Thu, 2011-11-17 at 09:48 +0100, Peter Zijlstra wrote:
> On Wed, 2011-11-16 at 21:52 +0000, Ben Hutchings wrote:
> > On Fri, Oct 28, 2011 at 04:36:55AM +0100, Ben Hutchings wrote:
> > > Currently lock debugging is disabled when the kernel is tainted, with
> > > a few exceptions.  It is already recognised that this can be useful
> > > for staging modules (TAINT_CRAP), but that also goes for out-of-tree
> > > modules (TAINT_OOT_MODULE) so long as core kernel developers don't
> > > have to spend time debugging them.  Also, there are several reasons
> > > for tainting that are unlikely to introduce false locking bug reports
> > > (e.g. TAINT_FIRMWARE_WORKAROUND).
> 
> 
> 
> > > Instead of disabling lock debugging, show the taint flags in all
> > > lockdep and rtmutex-debug error messages.
> 
> 
> So this is two patches in one. I took the last part, the printing of the
> taint flags thing, not the first part.
> 
> I did a small patch adding TAINT_FIRMWARE_WORKAROUND to the list of
> TAINTS that shouldn't disable lockdep.
> 
> As for OOT_MODULE, with staging the only reason not to merge a module is
> it being the wrong license and I really can't be arsed about OOT stuff
> anyway, so no.

That's not true.  For example, VirtualBox OSE is GPL but would not be
accepted into staging.  But if VirtualBox developers want to use lockdep
to fix their crap, we shouldn't stop them.

Ben.

-- 
Ben Hutchings
The world is coming to an end.	Please log off.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-10-28  3:36 [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error Ben Hutchings
  2011-10-31  1:59 ` Rusty Russell
  2011-11-16 21:52 ` Ben Hutchings
@ 2011-12-06  9:38 ` tip-bot for Ben Hutchings
  2011-12-06 15:34   ` Ben Hutchings
  2 siblings, 1 reply; 24+ messages in thread
From: tip-bot for Ben Hutchings @ 2011-12-06  9:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, hpa, mingo, a.p.zijlstra, tglx, mingo, ben

Commit-ID:  fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
Gitweb:     http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
Author:     Ben Hutchings <ben@decadent.org.uk>
AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 6 Dec 2011 08:16:49 +0100

lockdep, rtmutex, bug: Show taint flags on error

Show the taint flags in all lockdep and rtmutex-debug error messages.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1319773015.6759.30.camel@deadeye
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/lockdep.c       |   23 +++++++++++++++--------
 kernel/rtmutex-debug.c |    1 +
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/kernel/lockdep.c b/kernel/lockdep.c
index 103bed8..a2ab30c 100644
--- a/kernel/lockdep.c
+++ b/kernel/lockdep.c
@@ -567,11 +567,12 @@ static void lockdep_print_held_locks(struct task_struct *curr)
 	}
 }
 
-static void print_kernel_version(void)
+static void print_kernel_ident(void)
 {
-	printk("%s %.*s\n", init_utsname()->release,
+	printk("%s %.*s %s\n", init_utsname()->release,
 		(int)strcspn(init_utsname()->version, " "),
-		init_utsname()->version);
+		init_utsname()->version,
+		print_tainted());
 }
 
 static int very_verbose(struct lock_class *class)
@@ -1149,7 +1150,7 @@ print_circular_bug_header(struct lock_list *entry, unsigned int depth,
 	printk("\n");
 	printk("======================================================\n");
 	printk("[ INFO: possible circular locking dependency detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("-------------------------------------------------------\n");
 	printk("%s/%d is trying to acquire lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -1488,7 +1489,7 @@ print_bad_irq_dependency(struct task_struct *curr,
 	printk("======================================================\n");
 	printk("[ INFO: %s-safe -> %s-unsafe lock order detected ]\n",
 		irqclass, irqclass);
-	print_kernel_version();
+	print_kernel_ident();
 	printk("------------------------------------------------------\n");
 	printk("%s/%d [HC%u[%lu]:SC%u[%lu]:HE%u:SE%u] is trying to acquire:\n",
 		curr->comm, task_pid_nr(curr),
@@ -1717,7 +1718,7 @@ print_deadlock_bug(struct task_struct *curr, struct held_lock *prev,
 	printk("\n");
 	printk("=============================================\n");
 	printk("[ INFO: possible recursive locking detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------------------\n");
 	printk("%s/%d is trying to acquire lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -2224,7 +2225,7 @@ print_usage_bug(struct task_struct *curr, struct held_lock *this,
 	printk("\n");
 	printk("=================================\n");
 	printk("[ INFO: inconsistent lock state ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------\n");
 
 	printk("inconsistent {%s} -> {%s} usage.\n",
@@ -2289,7 +2290,7 @@ print_irq_inversion_bug(struct task_struct *curr,
 	printk("\n");
 	printk("=========================================================\n");
 	printk("[ INFO: possible irq lock inversion dependency detected ]\n");
-	print_kernel_version();
+	print_kernel_ident();
 	printk("---------------------------------------------------------\n");
 	printk("%s/%d just changed the state of lock:\n",
 		curr->comm, task_pid_nr(curr));
@@ -3170,6 +3171,7 @@ print_unlock_inbalance_bug(struct task_struct *curr, struct lockdep_map *lock,
 	printk("\n");
 	printk("=====================================\n");
 	printk("[ BUG: bad unlock balance detected! ]\n");
+	print_kernel_ident();
 	printk("-------------------------------------\n");
 	printk("%s/%d is trying to release lock (",
 		curr->comm, task_pid_nr(curr));
@@ -3614,6 +3616,7 @@ print_lock_contention_bug(struct task_struct *curr, struct lockdep_map *lock,
 	printk("\n");
 	printk("=================================\n");
 	printk("[ BUG: bad contention detected! ]\n");
+	print_kernel_ident();
 	printk("---------------------------------\n");
 	printk("%s/%d is trying to contend lock (",
 		curr->comm, task_pid_nr(curr));
@@ -3988,6 +3991,7 @@ print_freed_lock_bug(struct task_struct *curr, const void *mem_from,
 	printk("\n");
 	printk("=========================\n");
 	printk("[ BUG: held lock freed! ]\n");
+	print_kernel_ident();
 	printk("-------------------------\n");
 	printk("%s/%d is freeing memory %p-%p, with a lock still held there!\n",
 		curr->comm, task_pid_nr(curr), mem_from, mem_to-1);
@@ -4045,6 +4049,7 @@ static void print_held_locks_bug(struct task_struct *curr)
 	printk("\n");
 	printk("=====================================\n");
 	printk("[ BUG: lock held at task exit time! ]\n");
+	print_kernel_ident();
 	printk("-------------------------------------\n");
 	printk("%s/%d is exiting with locks still held!\n",
 		curr->comm, task_pid_nr(curr));
@@ -4142,6 +4147,7 @@ void lockdep_sys_exit(void)
 		printk("\n");
 		printk("================================================\n");
 		printk("[ BUG: lock held when returning to user space! ]\n");
+		print_kernel_ident();
 		printk("------------------------------------------------\n");
 		printk("%s/%d is leaving the kernel with locks still held!\n",
 				curr->comm, curr->pid);
@@ -4161,6 +4167,7 @@ void lockdep_rcu_suspicious(const char *file, const int line, const char *s)
 	printk("\n");
 	printk("===============================\n");
 	printk("[ INFO: suspicious RCU usage. ]\n");
+	print_kernel_ident();
 	printk("-------------------------------\n");
 	printk("%s:%d %s!\n", file, line, s);
 	printk("\nother info that might help us debug this:\n\n");
diff --git a/kernel/rtmutex-debug.c b/kernel/rtmutex-debug.c
index 8eafd1b..16502d3 100644
--- a/kernel/rtmutex-debug.c
+++ b/kernel/rtmutex-debug.c
@@ -101,6 +101,7 @@ void debug_rt_mutex_print_deadlock(struct rt_mutex_waiter *waiter)
 
 	printk("\n============================================\n");
 	printk(  "[ BUG: circular locking deadlock detected! ]\n");
+	printk("%s\n", print_tainted());
 	printk(  "--------------------------------------------\n");
 	printk("%s/%d is deadlocking current task %s/%d\n\n",
 	       task->comm, task_pid_nr(task),

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06  9:38 ` [tip:core/locking] lockdep, rtmutex, bug: " tip-bot for Ben Hutchings
@ 2011-12-06 15:34   ` Ben Hutchings
  2011-12-06 17:48     ` Peter Zijlstra
  2011-12-06 17:54     ` Ingo Molnar
  0 siblings, 2 replies; 24+ messages in thread
From: Ben Hutchings @ 2011-12-06 15:34 UTC (permalink / raw)
  To: mingo, hpa, linux-kernel, a.p.zijlstra, tglx, mingo; +Cc: linux-tip-commits

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

On Tue, 2011-12-06 at 01:38 -0800, tip-bot for Ben Hutchings wrote:
> Commit-ID:  fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> Gitweb:     http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> Author:     Ben Hutchings <ben@decadent.org.uk>
> AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Tue, 6 Dec 2011 08:16:49 +0100
> 
> lockdep, rtmutex, bug: Show taint flags on error
> 
> Show the taint flags in all lockdep and rtmutex-debug error messages.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Link: http://lkml.kernel.org/r/1319773015.6759.30.camel@deadeye
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
[...]

If you disagree with a patch, do not silently drop parts of it.  I
demand that you remove my 'Signed-off-by' as this is not the change I
submitted.

Ben.

-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 15:34   ` Ben Hutchings
@ 2011-12-06 17:48     ` Peter Zijlstra
  2011-12-06 17:55       ` Alan Cox
  2011-12-06 17:54     ` Ingo Molnar
  1 sibling, 1 reply; 24+ messages in thread
From: Peter Zijlstra @ 2011-12-06 17:48 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: mingo, hpa, linux-kernel, tglx, mingo, linux-tip-commits

On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:
> If you disagree with a patch, do not silently drop parts of it.  I
> demand that you remove my 'Signed-off-by' as this is not the change I
> submitted. 

The easy solution is that I never take patches from you again, ever.
Consider that done. I'll let Ingo see if he can remove your SOB.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 15:34   ` Ben Hutchings
  2011-12-06 17:48     ` Peter Zijlstra
@ 2011-12-06 17:54     ` Ingo Molnar
  2011-12-06 18:14       ` Nick Bowler
  1 sibling, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 17:54 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: mingo, hpa, linux-kernel, a.p.zijlstra, tglx, linux-tip-commits


* Ben Hutchings <ben@decadent.org.uk> wrote:

> On Tue, 2011-12-06 at 01:38 -0800, tip-bot for Ben Hutchings wrote:
> > Commit-ID:  fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> > Gitweb:     http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> > Author:     Ben Hutchings <ben@decadent.org.uk>
> > AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100
> > Committer:  Ingo Molnar <mingo@elte.hu>
> > CommitDate: Tue, 6 Dec 2011 08:16:49 +0100
> > 
> > lockdep, rtmutex, bug: Show taint flags on error
> > 
> > Show the taint flags in all lockdep and rtmutex-debug error messages.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Link: http://lkml.kernel.org/r/1319773015.6759.30.camel@deadeye
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> [...]
> 
> If you disagree with a patch, do not silently drop parts of 
> it.  I demand that you remove my 'Signed-off-by' as this is 
> not the change I submitted.

FYI, there's no "I will only sign off on a patch doing two 
things if it's applied in full" kind of condition in the SOB 
definition, allowing that would break the GPL: people have the 
right to take your modifications to the GPL-ed kernel and modify 
it further.

Your original patch did two things. Peter did the sensible 
thing: he split out the print_kernel_ident() changes from your 
patch which stand on their own and kept your authorship in place 
- that is what the above patch does.

Once you send it out the SOB is valid and people can (and 
typically will) modify it - and if you are still the main author 
(which you are here 100%) then keeping you as the author is the 
proper approach - Peter added his own SOB after yours.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:48     ` Peter Zijlstra
@ 2011-12-06 17:55       ` Alan Cox
  2011-12-06 17:56         ` Peter Zijlstra
                           ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Alan Cox @ 2011-12-06 17:55 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Ben Hutchings, mingo, hpa, linux-kernel, tglx, mingo, linux-tip-commits

On Tue, 06 Dec 2011 18:48:51 +0100
Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:
> > If you disagree with a patch, do not silently drop parts of it.  I
> > demand that you remove my 'Signed-off-by' as this is not the change I
> > submitted. 
> 
> The easy solution is that I never take patches from you again, ever.
> Consider that done. I'll let Ingo see if he can remove your SOB.

In which case you are presumably ceasing to be a maintainer for that
code ? Your statement above appears to be inconsistent with the rôle of a
maintainer.

Alan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:55       ` Alan Cox
@ 2011-12-06 17:56         ` Peter Zijlstra
  2011-12-06 17:59         ` Ingo Molnar
  2011-12-06 18:04         ` Ingo Molnar
  2 siblings, 0 replies; 24+ messages in thread
From: Peter Zijlstra @ 2011-12-06 17:56 UTC (permalink / raw)
  To: Alan Cox
  Cc: Ben Hutchings, mingo, hpa, linux-kernel, tglx, mingo, linux-tip-commits

On Tue, 2011-12-06 at 17:55 +0000, Alan Cox wrote:
> On Tue, 06 Dec 2011 18:48:51 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:
> > > If you disagree with a patch, do not silently drop parts of it.  I
> > > demand that you remove my 'Signed-off-by' as this is not the change I
> > > submitted. 
> > 
> > The easy solution is that I never take patches from you again, ever.
> > Consider that done. I'll let Ingo see if he can remove your SOB.
> 
> In which case you are presumably ceasing to be a maintainer for that
> code ? Your statement above appears to be inconsistent with the rôle of a
> maintainer.

There's plenty people who have various other people blacklisted in the
procmail. My blacklist is shorter than some, Merkey isn't actually on it
for instance.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:55       ` Alan Cox
  2011-12-06 17:56         ` Peter Zijlstra
@ 2011-12-06 17:59         ` Ingo Molnar
  2011-12-06 18:17           ` Ben Hutchings
  2011-12-06 18:04         ` Ingo Molnar
  2 siblings, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 17:59 UTC (permalink / raw)
  To: Alan Cox
  Cc: Peter Zijlstra, Ben Hutchings, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits


* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Tue, 06 Dec 2011 18:48:51 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:

> > > If you disagree with a patch, do not silently drop parts 
> > > of it.  I demand that you remove my 'Signed-off-by' as 
> > > this is not the change I submitted.
> > 
> > The easy solution is that I never take patches from you 
> > again, ever. Consider that done. I'll let Ingo see if he can 
> > remove your SOB.
> 
> In which case you are presumably ceasing to be a maintainer 
> for that code ? Your statement above appears to be 
> inconsistent with the rôle of a maintainer.

What Peter did was rather sensible: he split a patch that did 
two things into two and applied one standalone, uncontroversial 
half of it and kept the part of the part of the changelog that 
related to that change.

What Peter probably could have done is to add one more line 
before his SOB:

 [ split out the patch from the original submission ]
 Signed-off-by: Peter Zijlstra <peterz@infradead.org>

Otherwise Ben Hutchings's objection here makes little sense.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:55       ` Alan Cox
  2011-12-06 17:56         ` Peter Zijlstra
  2011-12-06 17:59         ` Ingo Molnar
@ 2011-12-06 18:04         ` Ingo Molnar
  2 siblings, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 18:04 UTC (permalink / raw)
  To: Alan Cox
  Cc: Peter Zijlstra, Ben Hutchings, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits


* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Tue, 06 Dec 2011 18:48:51 +0100
> Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:
> > > If you disagree with a patch, do not silently drop parts of it.  I
> > > demand that you remove my 'Signed-off-by' as this is not the change I
> > > submitted. 
> > 
> > The easy solution is that I never take patches from you again, ever.
> > Consider that done. I'll let Ingo see if he can remove your SOB.
> 
> In which case you are presumably ceasing to be a maintainer for that
> code ? Your statement above appears to be inconsistent with the rôle of a
> maintainer.

Just for the record, Peter is co-maintaining the lockdep code 
with me (and is doing a superb job with that) and as such he has 
no obligation whatsoever to interact with every contributor - as 
long as at least one maintainer considers every patch submitted.

Personally, reading various past discussions i can understand 
Peter not wanting to work with Ben Hutchings for some time - 
i'll look at his future patches and will consider them.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:54     ` Ingo Molnar
@ 2011-12-06 18:14       ` Nick Bowler
  2011-12-06 18:20         ` Ingo Molnar
                           ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Nick Bowler @ 2011-12-06 18:14 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Ben Hutchings, mingo, hpa, linux-kernel, a.p.zijlstra, tglx,
	linux-tip-commits

On 2011-12-06 18:54 +0100, Ingo Molnar wrote:
> * Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Tue, 2011-12-06 at 01:38 -0800, tip-bot for Ben Hutchings wrote:
> > > Commit-ID:  fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> > > Gitweb:     http://git.kernel.org/tip/fbdc4b9a6c29befbcca65e5366e5aaf2abb7a013
> > > Author:     Ben Hutchings <ben@decadent.org.uk>
> > > AuthorDate: Fri, 28 Oct 2011 04:36:55 +0100
> > > Committer:  Ingo Molnar <mingo@elte.hu>
> > > CommitDate: Tue, 6 Dec 2011 08:16:49 +0100
> > > 
> > > lockdep, rtmutex, bug: Show taint flags on error
> > > 
> > > Show the taint flags in all lockdep and rtmutex-debug error messages.
> > > 
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > > Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > Link: http://lkml.kernel.org/r/1319773015.6759.30.camel@deadeye
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > [...]
> > 
> > If you disagree with a patch, do not silently drop parts of 
> > it.  I demand that you remove my 'Signed-off-by' as this is 
> > not the change I submitted.
> 
> FYI, there's no "I will only sign off on a patch doing two 
> things if it's applied in full" kind of condition in the SOB 
> definition, allowing that would break the GPL: people have the 
> right to take your modifications to the GPL-ed kernel and modify 
> it further.

If you want to play the GPL card, you will note that the GPL requires
modified versions of a work to carry prominent notices that they have
been modified, including the date of any change.

A signoff by a maintainer does not imply that any modifications have
been made, so I don't think that counts as "prominent".

> Your original patch did two things. Peter did the sensible 
> thing: he split out the print_kernel_ident() changes from your 
> patch which stand on their own and kept your authorship in place 
> - that is what the above patch does.

In which case, the changelog should have been amended to state that
it's a modification of Ben's original submission.

> Once you send it out the SOB is valid and people can (and 
> typically will) modify it - and if you are still the main author 
> (which you are here 100%) then keeping you as the author is the 
> proper approach - Peter added his own SOB after yours.

Linux convention is to add a signoff *and* a description of any changes
to the original submission in square brackets.

Cheers,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 17:59         ` Ingo Molnar
@ 2011-12-06 18:17           ` Ben Hutchings
  2011-12-06 18:28             ` Ingo Molnar
  0 siblings, 1 reply; 24+ messages in thread
From: Ben Hutchings @ 2011-12-06 18:17 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Alan Cox, Peter Zijlstra, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits

On Tue, Dec 06, 2011 at 06:59:19PM +0100, Ingo Molnar wrote:
> 
> * Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
> > On Tue, 06 Dec 2011 18:48:51 +0100
> > Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > On Tue, 2011-12-06 at 15:34 +0000, Ben Hutchings wrote:
> 
> > > > If you disagree with a patch, do not silently drop parts 
> > > > of it.  I demand that you remove my 'Signed-off-by' as 
> > > > this is not the change I submitted.
> > > 
> > > The easy solution is that I never take patches from you 
> > > again, ever. Consider that done. I'll let Ingo see if he can 
> > > remove your SOB.
> > 
> > In which case you are presumably ceasing to be a maintainer 
> > for that code ? Your statement above appears to be 
> > inconsistent with the rôle of a maintainer.
> 
> What Peter did was rather sensible: he split a patch that did 
> two things into two and applied one standalone, uncontroversial 
> half of it and kept the part of the part of the changelog that 
> related to that change.
[...]

I can accept that if the change from my submission is commented
in the commit message.  (I sometimes do that with other people's
patches.)  But this was modified without comment.

Giving credit for authorship is important, but it is also
important not to say someone is the sole author of something
when they aren't and haven't approved the subsequent changes
(cf. Alan Smithee).
 
Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:14       ` Nick Bowler
@ 2011-12-06 18:20         ` Ingo Molnar
  2011-12-06 18:21         ` Alan Cox
  2011-12-06 18:23         ` Ben Hutchings
  2 siblings, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 18:20 UTC (permalink / raw)
  To: Nick Bowler
  Cc: Ben Hutchings, mingo, hpa, linux-kernel, a.p.zijlstra, tglx,
	linux-tip-commits


* Nick Bowler <nbowler@elliptictech.com> wrote:

> > Your original patch did two things. Peter did the sensible 
> > thing: he split out the print_kernel_ident() changes from 
> > your patch which stand on their own and kept your authorship 
> > in place - that is what the above patch does.
> 
> In which case, the changelog should have been amended to state 
> that it's a modification of Ben's original submission.

Which is what i said in my reply to Alan:

 | What Peter probably could have done is to add one more line
 | before his SOB:
 |
 |   [ split out the patch from the original submission ]
 |   Signed-off-by: Peter Zijlstra <peterz@infradead.org>
 |
 | Otherwise Ben Hutchings's objection here makes little sense.

No line of code was added by Peter - it's all Ben's changes.

Note that the commit in question:

  fbdc4b9a6c29: lockdep, rtmutex, bug: Show taint flags on error

is actually a good one and i think even Ben actually thinks 
those changes are good. Nothing was added - Ben only wants 
*more* to be done in a single patch and is being silly about the 
SOB and is asking it to be removed.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:14       ` Nick Bowler
  2011-12-06 18:20         ` Ingo Molnar
@ 2011-12-06 18:21         ` Alan Cox
  2011-12-06 18:34           ` Ingo Molnar
  2011-12-06 18:23         ` Ben Hutchings
  2 siblings, 1 reply; 24+ messages in thread
From: Alan Cox @ 2011-12-06 18:21 UTC (permalink / raw)
  To: linux-kernel; +Cc: Ingo Molnar, Ben Hutchings, mingo, hpa, a.p.zijlstra, tglx

> FYI, there's no "I will only sign off on a patch doing two 
> things if it's applied in full" kind of condition in the SOB 
> definition, allowing that would break the GPL: people have the 
> right to take your modifications to the GPL-ed kernel and modify 
> it further.

That gets into moral rights, and at least in the EU representing someone
as the author of something they are not isn't a good idea even before you
get to any contractual or licensing questions. I don't believe the EU is
unusual in this point either.

Alan

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:14       ` Nick Bowler
  2011-12-06 18:20         ` Ingo Molnar
  2011-12-06 18:21         ` Alan Cox
@ 2011-12-06 18:23         ` Ben Hutchings
  2 siblings, 0 replies; 24+ messages in thread
From: Ben Hutchings @ 2011-12-06 18:23 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Nick Bowler, mingo, hpa, linux-kernel, a.p.zijlstra, tglx,
	linux-tip-commits

On Tue, Dec 06, 2011 at 01:14:22PM -0500, Nick Bowler wrote:
> On 2011-12-06 18:54 +0100, Ingo Molnar wrote:
[...]
> > Once you send it out the SOB is valid and people can (and 
> > typically will) modify it - and if you are still the main author 
> > (which you are here 100%) then keeping you as the author is the 
> > proper approach - Peter added his own SOB after yours.
> 
> Linux convention is to add a signoff *and* a description of any changes
> to the original submission in square brackets.

Yes, I would also be satisfied with that.  (So my 'demand' was a bit
overstated.  Sorry about that, but *quietly* changing my code is like
misquoing me and it does get me pretty angry.)
 
Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:17           ` Ben Hutchings
@ 2011-12-06 18:28             ` Ingo Molnar
  2011-12-06 19:14               ` Ingo Molnar
  0 siblings, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 18:28 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Alan Cox, Peter Zijlstra, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits


* Ben Hutchings <ben@decadent.org.uk> wrote:

> > What Peter did was rather sensible: he split a patch that 
> > did two things into two and applied one standalone, 
> > uncontroversial half of it and kept the part of the part of 
> > the changelog that related to that change.
> [...]
> 
> I can accept that if the change from my submission is 
> commented in the commit message.  (I sometimes do that with 
> other people's patches.)  But this was modified without 
> comment.

That was not what you requested though:

> > > > > If you disagree with a patch, do not silently drop 
> > > > > parts of it.  I demand that you remove my 
> > > > > 'Signed-off-by' as this is not the change I submitted.

What happened is that you did two things in a single patch and 
Peter applied the uncontroversial, unrelated bits in full.

> Giving credit for authorship is important, but it is also 
> important not to say someone is the sole author of something 
> when they aren't and haven't approved the subsequent changes 
> (cf. Alan Smithee).

It's all hunks submitted by you, you are the only author.

He did not modify your changes - you submitted several changes. 

You are actually the sole author of those changes in the commit. 
Peter only split up your original patch - he was actually being 
very constructive and 100% helpful with upstreaming your debug 
printout changes - which makes your original objection doubly 
offensive.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:21         ` Alan Cox
@ 2011-12-06 18:34           ` Ingo Molnar
  0 siblings, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 18:34 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel, Ben Hutchings, mingo, hpa, a.p.zijlstra, tglx


* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > FYI, there's no "I will only sign off on a patch doing two 
> > things if it's applied in full" kind of condition in the SOB 
> > definition, allowing that would break the GPL: people have 
> > the right to take your modifications to the GPL-ed kernel 
> > and modify it further.
> 
> That gets into moral rights, and at least in the EU 
> representing someone as the author of something they are not 
> [..]

Alan, stop being silly - you clearly don't know what you are 
talking about.

The fact is, *every single line of code* in that commit was 
written by Ben Hutchinson. Peter was being excessively helpful, 
polite and did Ben a favor by splitting out the debug-printks 
from the original submission and upstreaming them.

What he did not do was to apply an unrelated, still under 
discussion change mixed into that debug-printouts patch.

Look at the original submission by Ben on lkml:

 Subject: [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error

The hunk that Peter left out of that patch was not declared in 
the patch title and was unrelated to the rest of the patch:

--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -233,16 +233,6 @@ unsigned long get_taint(void)

 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 (flag != TAINT_CRAP && flag != TAINT_WARN && __debug_locks_off())
-               printk(KERN_WARNING "Disabling lock debugging due to kernel taint\n");
-
        set_bit(flag, &tainted_mask);

Criticising Peter for not applying that change which the patch 
title did not even mention and then sulkingly asking the SOB to 
be removed is silly and offensive square two.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 18:28             ` Ingo Molnar
@ 2011-12-06 19:14               ` Ingo Molnar
  2011-12-06 21:13                 ` Ben Hutchings
  0 siblings, 1 reply; 24+ messages in thread
From: Ingo Molnar @ 2011-12-06 19:14 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Alan Cox, Peter Zijlstra, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits


Anyway, SOB flaming aside, i think there's now consensus that we 
don't want to disable lockdep for TAINT_OOT modules.

Mind (re-)sending a delta patch against tip:master that excludes 
that taint flag in addition to the already excluded 
TAINT_FIRMWARE case? That should close this rather verbose 
chapter of taint flag handling changes for good ;-)

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 19:14               ` Ingo Molnar
@ 2011-12-06 21:13                 ` Ben Hutchings
  2011-12-07  7:49                   ` Ingo Molnar
  0 siblings, 1 reply; 24+ messages in thread
From: Ben Hutchings @ 2011-12-06 21:13 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Alan Cox, Peter Zijlstra, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits

On Tue, Dec 06, 2011 at 08:14:58PM +0100, Ingo Molnar wrote:
> 
> Anyway, SOB flaming aside, i think there's now consensus that we 
> don't want to disable lockdep for TAINT_OOT modules.
 
OK, glad we could work this out.

> Mind (re-)sending a delta patch against tip:master that excludes 
> that taint flag in addition to the already excluded 
> TAINT_FIRMWARE case? That should close this rather verbose 
> chapter of taint flag handling changes for good ;-)

I originally assumed that lock debugging was being disabled on a
tainted kernel in order to save kernel developers from having to
handle false lockdep errors that are due to external bugs.  The
intent of my proposed change was to achieve this same effect by
showing taint flags instead.  Although the immediate motivation
for this was the new TAINT_OOT_MODULE, I don't see that it is
important for any taint flag to have this disabling effect now.

If you want to ensure that lock debugging is not available to
proprietary modules, I don't have a problem with that, although there
is no legal or practical way for us to stop proprietary module
developers evading this check and doing lock debugging in the privacy
of their offices.

So what I would like to do is either:

1. No longer disable lock debugging due to any taint
or
2. Disable lock debugging only when TAINT_PROPRIETARY_MODULE is set

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [tip:core/locking] lockdep, rtmutex, bug: Show taint flags on error
  2011-12-06 21:13                 ` Ben Hutchings
@ 2011-12-07  7:49                   ` Ingo Molnar
  0 siblings, 0 replies; 24+ messages in thread
From: Ingo Molnar @ 2011-12-07  7:49 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: Alan Cox, Peter Zijlstra, mingo, hpa, linux-kernel, tglx,
	linux-tip-commits


* Ben Hutchings <ben@decadent.org.uk> wrote:

> So what I would like to do is either:
> 
> 1. No longer disable lock debugging due to any taint
> or
> 2. Disable lock debugging only when TAINT_PROPRIETARY_MODULE is set

I'd like a whitelist - i.e. disable on taint by default, but 
allow case by case exceptions such as OOT_MODULE.

Nor am i singling out bin-only modules, look at the current set 
of taint flags:

#define TAINT_PROPRIETARY_MODULE        0
#define TAINT_FORCED_MODULE             1
#define TAINT_UNSAFE_SMP                2
#define TAINT_FORCED_RMMOD              3
#define TAINT_MACHINE_CHECK             4
#define TAINT_BAD_PAGE                  5
#define TAINT_USER                      6
#define TAINT_DIE                       7
#define TAINT_OVERRIDDEN_ACPI_TABLE     8
#define TAINT_WARN                      9
#define TAINT_CRAP                      10
#define TAINT_FIRMWARE_WORKAROUND       11
#define TAINT_OOT_MODULE                12

*all* except TAINT_FIRMWARE_WORKAROUND, TAINT_CRAP and OOT 
should result in lockdep disabling. (perhaps 
OVERRIDDEN_ACPI_TABLE, as a variant of FIRMWARE_WORKAROUND, 
could be added as an exception too)

Module forcing causes various lifetime issues and lockdep tracks 
lifetime so can get confused on that. Unsafe SMP - enough said. 
Lockdep also wants to disable on the first WARN_ON() - which is 
typically the sign of some initial badness, which should be 
investigated before any lockdep splat is looked into.

Since most of the taint flags show deep kernel or hardware 
troubles i'd like most new taint flags to be lockdep off by 
default. As new taint flags are introduced they can be added to 
the whitelist when justified.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2011-12-07  7:50 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-28  3:36 [PATCH 1/2] lockdep,rtmutex,bug: Show taint flags on error Ben Hutchings
2011-10-31  1:59 ` Rusty Russell
2011-11-16 21:52 ` Ben Hutchings
2011-11-16 22:24   ` Dave Jones
2011-11-17  8:48   ` Peter Zijlstra
2011-11-17 15:18     ` Ben Hutchings
2011-12-06  9:38 ` [tip:core/locking] lockdep, rtmutex, bug: " tip-bot for Ben Hutchings
2011-12-06 15:34   ` Ben Hutchings
2011-12-06 17:48     ` Peter Zijlstra
2011-12-06 17:55       ` Alan Cox
2011-12-06 17:56         ` Peter Zijlstra
2011-12-06 17:59         ` Ingo Molnar
2011-12-06 18:17           ` Ben Hutchings
2011-12-06 18:28             ` Ingo Molnar
2011-12-06 19:14               ` Ingo Molnar
2011-12-06 21:13                 ` Ben Hutchings
2011-12-07  7:49                   ` Ingo Molnar
2011-12-06 18:04         ` Ingo Molnar
2011-12-06 17:54     ` Ingo Molnar
2011-12-06 18:14       ` Nick Bowler
2011-12-06 18:20         ` Ingo Molnar
2011-12-06 18:21         ` Alan Cox
2011-12-06 18:34           ` Ingo Molnar
2011-12-06 18:23         ` Ben Hutchings

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.