[PATCH 0/7] Make debugobjects fixup functions return bool type

[PATCH 0/7] Make debugobjects fixup functions return bool type

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Hello,
I am going to introduce debugobjects infrastructure to USB subsystem.
But before this, I found the code of debugobjects could be improved.
This patchset will make fixup functions return bool type instead of
int. Because fixup only need report success or no. boolean is the
'real' type.

The MAINTAINERS file doesn't include lib/debugobjects.c, so I can 
only send to Andrew Morton (akpm@linux-foundation.org) as a last
resort. :)

I will probaly have 2 other patches which send later.

Du, Changbin (7):
  debugobjects: make fixup functions return bool instead of int
  debugobjects: correct the usage of fixup call results
  workqueue: update debugobjects fixup callbacks return type
  timer: update debugobjects fixup callbacks return type
  rcu: update debugobjects fixup callbacks return type
  percpu_counter: update debugobjects fixup callbacks return type
  Documentation: update debugobjects doc

 Documentation/DocBook/debugobjects.tmpl | 26 ++++++++++---------
 include/linux/debugobjects.h            | 15 ++++++-----
 kernel/rcu/update.c                     |  6 ++---
 kernel/time/hrtimer.c                   | 18 ++++++-------
 kernel/time/timer.c                     | 30 +++++++++++-----------
 kernel/workqueue.c                      | 20 +++++++--------
 lib/debugobjects.c                      | 45 ++++++++++++++++-----------------
 lib/percpu_counter.c                    |  6 ++---
 8 files changed, 84 insertions(+), 82 deletions(-)

-- 
2.7.4

[PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

The object debugging infrastructure core provides some fixup callbacks
for the subsystem who use it. These callbacks are called from the debug
code whenever a problem in debug_object_init is detected. And
debugobjects core suppose them returns 1 when the fixup was successful,
otherwise 0. So the return type is boolean.

A bad thing is that debug_object_fixup use the return value for
arithmetic operation. It confused me that what is the reall return
type.

Reading over the whole code, I found some place do use the return value
incorrectly(see next patch). So why use bool type instead?

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 include/linux/debugobjects.h | 15 ++++++++-------
 lib/debugobjects.c           | 43 +++++++++++++++++++++----------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/linux/debugobjects.h b/include/linux/debugobjects.h
index 98ffcbd..a899f10 100644
--- a/include/linux/debugobjects.h
+++ b/include/linux/debugobjects.h
@@ -39,7 +39,8 @@ struct debug_obj {
  * @debug_hint:		function returning address, which have associated
  *			kernel symbol, to allow identify the object
  * @fixup_init:		fixup function, which is called when the init check
- *			fails
+ *			fails. All fixup functions must return true if fixup
+ *			was successful, otherwise return false
  * @fixup_activate:	fixup function, which is called when the activate check
  *			fails
  * @fixup_destroy:	fixup function, which is called when the destroy check
@@ -51,12 +52,12 @@ struct debug_obj {
  */
 struct debug_obj_descr {
 	const char		*name;
-	void *(*debug_hint)	(void *addr);
-	int (*fixup_init)	(void *addr, enum debug_obj_state state);
-	int (*fixup_activate)	(void *addr, enum debug_obj_state state);
-	int (*fixup_destroy)	(void *addr, enum debug_obj_state state);
-	int (*fixup_free)	(void *addr, enum debug_obj_state state);
-	int (*fixup_assert_init)(void *addr, enum debug_obj_state state);
+	void *(*debug_hint)(void *addr);
+	bool (*fixup_init)(void *addr, enum debug_obj_state state);
+	bool (*fixup_activate)(void *addr, enum debug_obj_state state);
+	bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
+	bool (*fixup_free)(void *addr, enum debug_obj_state state);
+	bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);
 };
 
 #ifdef CONFIG_DEBUG_OBJECTS
diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index 519b5a1..a9cee16 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -269,16 +269,15 @@ static void debug_print_object(struct debug_obj *obj, char *msg)
  * Try to repair the damage, so we have a better chance to get useful
  * debug output.
  */
-static int
-debug_object_fixup(int (*fixup)(void *addr, enum debug_obj_state state),
+static bool
+debug_object_fixup(bool (*fixup)(void *addr, enum debug_obj_state state),
 		   void * addr, enum debug_obj_state state)
 {
-	int fixed = 0;
-
-	if (fixup)
-		fixed = fixup(addr, state);
-	debug_objects_fixups += fixed;
-	return fixed;
+	if (fixup && fixup(addr, state)) {
+		debug_objects_fixups++;
+		return true;
+	}
+	return false;
 }
 
 static void debug_object_is_on_stack(void *addr, int onstack)
@@ -797,7 +796,7 @@ static __initdata struct debug_obj_descr descr_type_test;
  * fixup_init is called when:
  * - an active object is initialized
  */
-static int __init fixup_init(void *addr, enum debug_obj_state state)
+static bool __init fixup_init(void *addr, enum debug_obj_state state)
 {
 	struct self_test *obj = addr;
 
@@ -805,9 +804,9 @@ static int __init fixup_init(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		debug_object_deactivate(obj, &descr_type_test);
 		debug_object_init(obj, &descr_type_test);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -816,7 +815,7 @@ static int __init fixup_init(void *addr, enum debug_obj_state state)
  * - an active object is activated
  * - an unknown object is activated (might be a statically initialized object)
  */
-static int __init fixup_activate(void *addr, enum debug_obj_state state)
+static bool __init fixup_activate(void *addr, enum debug_obj_state state)
 {
 	struct self_test *obj = addr;
 
@@ -825,17 +824,17 @@ static int __init fixup_activate(void *addr, enum debug_obj_state state)
 		if (obj->static_init == 1) {
 			debug_object_init(obj, &descr_type_test);
 			debug_object_activate(obj, &descr_type_test);
-			return 0;
+			return false;
 		}
-		return 1;
+		return true;
 
 	case ODEBUG_STATE_ACTIVE:
 		debug_object_deactivate(obj, &descr_type_test);
 		debug_object_activate(obj, &descr_type_test);
-		return 1;
+		return true;
 
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -843,7 +842,7 @@ static int __init fixup_activate(void *addr, enum debug_obj_state state)
  * fixup_destroy is called when:
  * - an active object is destroyed
  */
-static int __init fixup_destroy(void *addr, enum debug_obj_state state)
+static bool __init fixup_destroy(void *addr, enum debug_obj_state state)
 {
 	struct self_test *obj = addr;
 
@@ -851,9 +850,9 @@ static int __init fixup_destroy(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		debug_object_deactivate(obj, &descr_type_test);
 		debug_object_destroy(obj, &descr_type_test);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -861,7 +860,7 @@ static int __init fixup_destroy(void *addr, enum debug_obj_state state)
  * fixup_free is called when:
  * - an active object is freed
  */
-static int __init fixup_free(void *addr, enum debug_obj_state state)
+static bool __init fixup_free(void *addr, enum debug_obj_state state)
 {
 	struct self_test *obj = addr;
 
@@ -869,9 +868,9 @@ static int __init fixup_free(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		debug_object_deactivate(obj, &descr_type_test);
 		debug_object_free(obj, &descr_type_test);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
-- 
2.7.4

[PATCH 2/7] debugobjects: correct the usage of fixup call results

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

If debug_object_fixup() return non-zero when problem has been
fixed. But the code got it backwards, it taks 0 as fixup
successfully. So fix it.

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 lib/debugobjects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index a9cee16..2f07c8c 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -415,7 +415,7 @@ int debug_object_activate(void *addr, struct debug_obj_descr *descr)
 			state = obj->state;
 			raw_spin_unlock_irqrestore(&db->lock, flags);
 			ret = debug_object_fixup(descr->fixup_activate, addr, state);
-			return ret ? -EINVAL : 0;
+			return ret ? 0 : -EINVAL;
 
 		case ODEBUG_STATE_DESTROYED:
 			debug_print_object(obj, "activate");
-- 
2.7.4

[PATCH 3/7] workqueue: update debugobjects fixup callbacks return type

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Update the return type to use bool instead of int, corresponding to
cheange (debugobjects: make fixup functions return bool instead of int)

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 kernel/workqueue.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 2232ae3..c3c7e69 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -437,7 +437,7 @@ static void *work_debug_hint(void *addr)
  * fixup_init is called when:
  * - an active object is initialized
  */
-static int work_fixup_init(void *addr, enum debug_obj_state state)
+static bool work_fixup_init(void *addr, enum debug_obj_state state)
 {
 	struct work_struct *work = addr;
 
@@ -445,9 +445,9 @@ static int work_fixup_init(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		cancel_work_sync(work);
 		debug_object_init(work, &work_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -456,7 +456,7 @@ static int work_fixup_init(void *addr, enum debug_obj_state state)
  * - an active object is activated
  * - an unknown object is activated (might be a statically initialized object)
  */
-static int work_fixup_activate(void *addr, enum debug_obj_state state)
+static bool work_fixup_activate(void *addr, enum debug_obj_state state)
 {
 	struct work_struct *work = addr;
 
@@ -471,16 +471,16 @@ static int work_fixup_activate(void *addr, enum debug_obj_state state)
 		if (test_bit(WORK_STRUCT_STATIC_BIT, work_data_bits(work))) {
 			debug_object_init(work, &work_debug_descr);
 			debug_object_activate(work, &work_debug_descr);
-			return 0;
+			return false;
 		}
 		WARN_ON_ONCE(1);
-		return 0;
+		return false;
 
 	case ODEBUG_STATE_ACTIVE:
 		WARN_ON(1);
 
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -488,7 +488,7 @@ static int work_fixup_activate(void *addr, enum debug_obj_state state)
  * fixup_free is called when:
  * - an active object is freed
  */
-static int work_fixup_free(void *addr, enum debug_obj_state state)
+static bool work_fixup_free(void *addr, enum debug_obj_state state)
 {
 	struct work_struct *work = addr;
 
@@ -496,9 +496,9 @@ static int work_fixup_free(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		cancel_work_sync(work);
 		debug_object_free(work, &work_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
-- 
2.7.4

[PATCH 4/7] timer: update debugobjects fixup callbacks return type

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Update the return type to use bool instead of int, corresponding to
cheange (debugobjects: make fixup functions return bool instead of int).

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 kernel/time/hrtimer.c | 18 +++++++++---------
 kernel/time/timer.c   | 30 +++++++++++++++---------------
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index fa0b983..f962a58 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -334,7 +334,7 @@ static void *hrtimer_debug_hint(void *addr)
  * fixup_init is called when:
  * - an active object is initialized
  */
-static int hrtimer_fixup_init(void *addr, enum debug_obj_state state)
+static bool hrtimer_fixup_init(void *addr, enum debug_obj_state state)
 {
 	struct hrtimer *timer = addr;
 
@@ -342,9 +342,9 @@ static int hrtimer_fixup_init(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		hrtimer_cancel(timer);
 		debug_object_init(timer, &hrtimer_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -353,19 +353,19 @@ static int hrtimer_fixup_init(void *addr, enum debug_obj_state state)
  * - an active object is activated
  * - an unknown object is activated (might be a statically initialized object)
  */
-static int hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
+static bool hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
 {
 	switch (state) {
 
 	case ODEBUG_STATE_NOTAVAILABLE:
 		WARN_ON_ONCE(1);
-		return 0;
+		return false;
 
 	case ODEBUG_STATE_ACTIVE:
 		WARN_ON(1);
 
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -373,7 +373,7 @@ static int hrtimer_fixup_activate(void *addr, enum debug_obj_state state)
  * fixup_free is called when:
  * - an active object is freed
  */
-static int hrtimer_fixup_free(void *addr, enum debug_obj_state state)
+static bool hrtimer_fixup_free(void *addr, enum debug_obj_state state)
 {
 	struct hrtimer *timer = addr;
 
@@ -381,9 +381,9 @@ static int hrtimer_fixup_free(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		hrtimer_cancel(timer);
 		debug_object_free(timer, &hrtimer_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 73164c3..be33481 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -493,7 +493,7 @@ static void *timer_debug_hint(void *addr)
  * fixup_init is called when:
  * - an active object is initialized
  */
-static int timer_fixup_init(void *addr, enum debug_obj_state state)
+static bool timer_fixup_init(void *addr, enum debug_obj_state state)
 {
 	struct timer_list *timer = addr;
 
@@ -501,9 +501,9 @@ static int timer_fixup_init(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		del_timer_sync(timer);
 		debug_object_init(timer, &timer_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -518,7 +518,7 @@ static void stub_timer(unsigned long data)
  * - an active object is activated
  * - an unknown object is activated (might be a statically initialized object)
  */
-static int timer_fixup_activate(void *addr, enum debug_obj_state state)
+static bool timer_fixup_activate(void *addr, enum debug_obj_state state)
 {
 	struct timer_list *timer = addr;
 
@@ -534,18 +534,18 @@ static int timer_fixup_activate(void *addr, enum debug_obj_state state)
 		    timer->entry.next == TIMER_ENTRY_STATIC) {
 			debug_object_init(timer, &timer_debug_descr);
 			debug_object_activate(timer, &timer_debug_descr);
-			return 0;
+			return false;
 		} else {
 			setup_timer(timer, stub_timer, 0);
-			return 1;
+			return true;
 		}
-		return 0;
+		return false;
 
 	case ODEBUG_STATE_ACTIVE:
 		WARN_ON(1);
 
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -553,7 +553,7 @@ static int timer_fixup_activate(void *addr, enum debug_obj_state state)
  * fixup_free is called when:
  * - an active object is freed
  */
-static int timer_fixup_free(void *addr, enum debug_obj_state state)
+static bool timer_fixup_free(void *addr, enum debug_obj_state state)
 {
 	struct timer_list *timer = addr;
 
@@ -561,9 +561,9 @@ static int timer_fixup_free(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		del_timer_sync(timer);
 		debug_object_free(timer, &timer_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -571,7 +571,7 @@ static int timer_fixup_free(void *addr, enum debug_obj_state state)
  * fixup_assert_init is called when:
  * - an untracked/uninit-ed object is found
  */
-static int timer_fixup_assert_init(void *addr, enum debug_obj_state state)
+static bool timer_fixup_assert_init(void *addr, enum debug_obj_state state)
 {
 	struct timer_list *timer = addr;
 
@@ -584,13 +584,13 @@ static int timer_fixup_assert_init(void *addr, enum debug_obj_state state)
 			 * is tracked in the object tracker.
 			 */
 			debug_object_init(timer, &timer_debug_descr);
-			return 0;
+			return false;
 		} else {
 			setup_timer(timer, stub_timer, 0);
-			return 1;
+			return true;
 		}
 	default:
-		return 0;
+		return false;
 	}
 }
 
-- 
2.7.4

[PATCH 5/7] rcu: update debugobjects fixup callbacks return type

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Update the return type to use bool instead of int, corresponding to
cheange (debugobjects: make fixup functions return bool instead of int).

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 kernel/rcu/update.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index ca828b4..66b75c3 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -386,7 +386,7 @@ void destroy_rcu_head(struct rcu_head *head)
  * - an unknown object is activated (might be a statically initialized object)
  * Activation is performed internally by call_rcu().
  */
-static int rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
+static bool rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
 {
 	struct rcu_head *head = addr;
 
@@ -399,9 +399,9 @@ static int rcuhead_fixup_activate(void *addr, enum debug_obj_state state)
 		 */
 		debug_object_init(head, &rcuhead_debug_descr);
 		debug_object_activate(head, &rcuhead_debug_descr);
-		return 0;
+		return false;
 	default:
-		return 1;
+		return true;
 	}
 }
 
-- 
2.7.4

[PATCH 6/7] percpu_counter: update debugobjects fixup callbacks return type

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Update the return type to use bool instead of int, corresponding to
cheange (debugobjects: make fixup functions return bool instead of int).

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 lib/percpu_counter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
index f051d69..72d3611 100644
--- a/lib/percpu_counter.c
+++ b/lib/percpu_counter.c
@@ -19,7 +19,7 @@ static DEFINE_SPINLOCK(percpu_counters_lock);
 
 static struct debug_obj_descr percpu_counter_debug_descr;
 
-static int percpu_counter_fixup_free(void *addr, enum debug_obj_state state)
+static bool percpu_counter_fixup_free(void *addr, enum debug_obj_state state)
 {
 	struct percpu_counter *fbc = addr;
 
@@ -27,9 +27,9 @@ static int percpu_counter_fixup_free(void *addr, enum debug_obj_state state)
 	case ODEBUG_STATE_ACTIVE:
 		percpu_counter_destroy(fbc);
 		debug_object_free(fbc, &percpu_counter_debug_descr);
-		return 1;
+		return true;
 	default:
-		return 0;
+		return false;
 	}
 }
 
-- 
2.7.4

[PATCH 7/7] Documentation: update debugobjects doc

[changbin.du]

From: "Du, Changbin" <changbin.du@intel.com>

Update documentation creangponding to change(debugobjects: make
fixup functions return bool instead of int).

Signed-off-by: Du, Changbin <changbin.du@intel.com>
---
 Documentation/DocBook/debugobjects.tmpl | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/Documentation/DocBook/debugobjects.tmpl b/Documentation/DocBook/debugobjects.tmpl
index 24979f6..7e4f34f 100644
--- a/Documentation/DocBook/debugobjects.tmpl
+++ b/Documentation/DocBook/debugobjects.tmpl
@@ -316,8 +316,8 @@
 	</itemizedlist>
       </para>
       <para>
-	The function returns 1 when the fixup was successful,
-	otherwise 0. The return value is used to update the
+	The function returns true when the fixup was successful,
+	otherwise false. The return value is used to update the
 	statistics.
       </para>
       <para>
@@ -341,8 +341,8 @@
 	</itemizedlist>
       </para>
       <para>
-	The function returns 1 when the fixup was successful,
-	otherwise 0. The return value is used to update the
+	The function returns true when the fixup was successful,
+	otherwise false. The return value is used to update the
 	statistics.
       </para>
       <para>
@@ -359,7 +359,8 @@
 	statically initialized object or not. In case it is it calls
 	debug_object_init() and debug_object_activate() to make the
 	object known to the tracker and marked active. In this case
-	the function should return 0 because this is not a real fixup.
+	the function should return false because this is not a real
+	fixup.
       </para>
     </sect1>
 
@@ -376,8 +377,8 @@
 	</itemizedlist>
       </para>
       <para>
-	The function returns 1 when the fixup was successful,
-	otherwise 0. The return value is used to update the
+	The function returns true when the fixup was successful,
+	otherwise false. The return value is used to update the
 	statistics.
       </para>
     </sect1>
@@ -397,8 +398,8 @@
 	</itemizedlist>
       </para>
       <para>
-	The function returns 1 when the fixup was successful,
-	otherwise 0. The return value is used to update the
+	The function returns true when the fixup was successful,
+	otherwise false. The return value is used to update the
 	statistics.
       </para>
     </sect1>
@@ -414,8 +415,8 @@
 	debug bucket.
       </para>
       <para>
-	The function returns 1 when the fixup was successful,
-	otherwise 0. The return value is used to update the
+	The function returns true when the fixup was successful,
+	otherwise false. The return value is used to update the
 	statistics.
       </para>
       <para>
@@ -427,7 +428,8 @@
 	case. The fixup function should check if this is a legitimate
 	case of a statically initialized object or not. In this case only
 	debug_object_init() should be called to make the object known to
-	the tracker. Then the function should return 0 because this is not
+	the tracker. Then the function should return false because this
+	is not
 	a real fixup.
       </para>
     </sect1>
-- 
2.7.4

Re: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
> 
> The object debugging infrastructure core provides some fixup callbacks
> for the subsystem who use it. These callbacks are called from the debug
> code whenever a problem in debug_object_init is detected. And
> debugobjects core suppose them returns 1 when the fixup was successful,
> otherwise 0. So the return type is boolean.
> 
> A bad thing is that debug_object_fixup use the return value for
> arithmetic operation. It confused me that what is the reall return

What's bad about that? The fact that it's used for arithmethic operation or
that it confused you?

> Reading over the whole code, I found some place do use the return value
> incorrectly(see next patch). So why use bool type instead?

Patches which fix a problem need to come first not in the middle of a revamp
series.
 
> +	bool (*fixup_init)(void *addr, enum debug_obj_state state);
> +	bool (*fixup_activate)(void *addr, enum debug_obj_state state);
> +	bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
> +	bool (*fixup_free)(void *addr, enum debug_obj_state state);
> +	bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);

So this change will introduce a gazillion of compile warnings because the
callbacks in the various usage sites are still having 'int' return type.

Thanks,

	tglx

RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Du, Changbin]

Hi,
> On Fri, 22 Apr 2016, changbin.du@intel.com wrote:
> > From: "Du, Changbin" <changbin.du@intel.com>
> >
> > The object debugging infrastructure core provides some fixup callbacks
> > for the subsystem who use it. These callbacks are called from the debug
> > code whenever a problem in debug_object_init is detected. And
> > debugobjects core suppose them returns 1 when the fixup was successful,
> > otherwise 0. So the return type is boolean.
> >
> > A bad thing is that debug_object_fixup use the return value for
> > arithmetic operation. It confused me that what is the reall return
> 
> What's bad about that? The fact that it's used for arithmethic operation or
> that it confused you?
> 
It confused me because this is not a common usage. I was confused that what
does he fixup function return? A countable value? But doc says return fixed
or not!
	if (fixup)
		fixed = fixup(addr, state);
	debug_objects_fixups += fixed;
In common,for int return 0 indicates success, negative for fail, positive for something
countable. So I think it is better follow this rule. Here is not of countable, it is Boolean.
So why not this?
	if (fixup && fixup(addr, state))
		debug_objects_fixups++;

> > Reading over the whole code, I found some place do use the return value
> > incorrectly(see next patch). So why use bool type instead?
> 
> Patches which fix a problem need to come first not in the middle of a revamp
> series.
> 
Thanks, I am first know of this.

> > +	bool (*fixup_init)(void *addr, enum debug_obj_state state);
> > +	bool (*fixup_activate)(void *addr, enum debug_obj_state state);
> > +	bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
> > +	bool (*fixup_free)(void *addr, enum debug_obj_state state);
> > +	bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);
> 
> So this change will introduce a gazillion of compile warnings because the
> callbacks in the various usage sites are still having 'int' return type.
> 
No, I modified all the code who use debugojects API.

> Thanks,
> 
> 	tglx

Thanks,
Du, Changbin

Re: [PATCH 2/7] debugobjects: correct the usage of fixup call results

[Thomas Gleixner]

On Fri, 22 Apr 2016, changbin.du@intel.com wrote:
> From: "Du, Changbin" <changbin.du@intel.com>
> 
> If debug_object_fixup() return non-zero when problem has been
> fixed. But the code got it backwards, it taks 0 as fixup
> successfully. So fix it.

Wrong.
 
> @@ -415,7 +415,7 @@ int debug_object_activate(void *addr, struct debug_obj_descr *descr)
>  			state = obj->state;
>  			raw_spin_unlock_irqrestore(&db->lock, flags);
>  			ret = debug_object_fixup(descr->fixup_activate, addr, state);
> -			return ret ? -EINVAL : 0;
> +			return ret ? 0 : -EINVAL;

So you need to look at the fixup_activate() callbacks.

timer_fixup_activate()

  The only state handled there is ODEBUG_STATE_NOTAVAILABLE. This can happen
  for two reasons:

   1) timer is statically allocated and initialized. That's a legitimate reason
     and it does not count as a fixup

   2) timer has not been initialized, so we have no idea what to do with it. We
     set it up with a dummy callback and return 1 because that is a fixup.

   So the return check for ret != 0 is correct. #2 is invalid

hrtimer_fixup_activate()

   There is not much we can do about it.

work_fixup_activate()

   That's similar to timer_fixup_activate(). We need to handle the statically
   allocated work gracefully.

rcuhead_fixup_activate()

   Handles the ODEBUG_STATE_NOTAVAILABLE case by tracking it. Not a fixup,
   returns 0.

   The other states are invalid and there is not much we can do about
   that. Returns 1.

I agree that this is not really intuitive, but it's correct as it is. I'm
happy to take patches which make it simpler to understand. Just blindly
changing everything to bool does not fall into that category.

Thanks,

	tglx


  

      	

RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int

[Thomas Gleixner]

On Fri, 22 Apr 2016, Du, Changbin wrote:
> > On Fri, 22 Apr 2016, changbin.du@intel.com wrote:
> > > A bad thing is that debug_object_fixup use the return value for
> > > arithmetic operation. It confused me that what is the reall return
> > 
> > What's bad about that? The fact that it's used for arithmethic operation or
> > that it confused you?
> > 
> It confused me because this is not a common usage. I was confused that what
> does he fixup function return? A countable value? But doc says return fixed
> or not!

It says return 0 for not fixed up and 1 for fixed up. The activate fixup is
special and it has been written this way to handle the static initialization
case.

> 	if (fixup)
> 		fixed = fixup(addr, state);
> 	debug_objects_fixups += fixed;

> In common,for int return 0 indicates success, negative for fail, positive
> for something countable. So I think it is better follow this rule. Here is
> not of countable, it is Boolean.

Yes, it's common for most of the code. This code has been deliberately been
written differently. I'm not opposed to change that and improve it, but just
slapping bool on it does not really make any difference.

> So why not this?
> 	if (fixup && fixup(addr, state))
> 		debug_objects_fixups++;

There is no problem with that per se.
 
> > > +	bool (*fixup_init)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_activate)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_destroy)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_free)(void *addr, enum debug_obj_state state);
> > > +	bool (*fixup_assert_init)(void *addr, enum debug_obj_state state);
> > 
> > So this change will introduce a gazillion of compile warnings because the
> > callbacks in the various usage sites are still having 'int' return type.
> > 
> No, I modified all the code who use debugojects API.

You do that in the later patches. But patches must be compilable and
functional on their own. Compiling this one will emit a gazillion of
"initialization from incompatible pointer type" warnings.

Thanks,

	tglx