Make debugobjects fixup functions return bool type
Subsystems (7)
cover Make debugobjects fixup functions return bool type
From: "Du, Changbin" <[email protected]>
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 ([email protected]) 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(-)
Signature · 1 line
2.7.4
PATCH 1/7 debugobjects: make fixup functions return bool instead of int
From: "Du, Changbin" <[email protected]>
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?
--- include/linux/debugobjects.h | 15 ++++++++------- lib/debugobjects.c | 43 +++++++++++++++++++++---------------------- 2 files changed, 29 insertions(+), 29 deletions(-)
include/linux/debugobjects.h +8 -7
@@ -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
lib/debugobjects.c +21 -22
109 lines not on this page · raw message
Signature · 1 line
2.7.4
Re: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int
Quoted 10 lines · On Fri, 22 Apr 2016, [email protected] wrote:
From: "Du, Changbin" <[email protected]>
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_initis 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_fixupuse 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?
Quoted 2 lines
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.
include/linux/debugobjects.h struct debug_obj { · line 60
Quoted 5 lines
include/linux/debugobjects.h +5
-0+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
Hi,
Quoted 9 lines · On Fri, 22 Apr 2016, [email protected] wrote:
Quoted 8 lines
From: "Du, Changbin" <[email protected]>
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_initis detected. And debugobjects core suppose them returns 1 when the fixup was successful, otherwise 0. So the return type is boolean.
Quoted 6 lines
Quoted 2 lines
A bad thing is that
debug_object_fixupuse the return value for arithmetic operation. It confused me that what is the reall returnWhat'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++;
Quoted 6 lines
Quoted 2 lines
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.
Quoted 9 lines
include/linux/debugobjects.h struct debug_obj { · line 60
Quoted 5 lines
include/linux/debugobjects.h +5
-0+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.
Quoted 3 lines
Thanks,
tglx
Thanks,
Du, Changbin
RE: [PATCH 1/7] debugobjects: make fixup functions return bool instead of int
Quoted 10 lines · Changbin
Quoted 7 lines · On Fri, 22 Apr 2016, [email protected] wrote:
Quoted 2 lines
A bad thing is that
debug_object_fixupuse the return value for arithmetic operation. It confused me that what is the reall returnWhat'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.
Quoted 7 lines
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.
Quoted 3 lines
So why not this?
if (fixup && fixup(addr, state)) debug_objects_fixups++;
There is no problem with that per se.
Quoted 10 lines
Quoted 9 lines
include/linux/debugobjects.h struct debug_obj { · line 60
Quoted 5 lines
include/linux/debugobjects.h +5
-0+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
PATCH 2/7 debugobjects: correct the usage of fixup call results
In reply to [email protected]From: "Du, Changbin" <[email protected]>
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.
--- lib/debugobjects.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/debugobjects.c +1 -1
@@ -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" );
Signature · 1 line
2.7.4
Re: [PATCH 2/7] debugobjects: correct the usage of fixup call results
Quoted 5 lines · On Fri, 22 Apr 2016, [email protected] wrote:
From: "Du, Changbin" <[email protected]>
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.
lib/debugobjects.c int debug_object_activate(void *addr, struct debug_obj_descr *descr) · line 418
Quoted 6 lines
lib/debugobjects.c +1
-1@@ -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
PATCH 3/7 workqueue: update debugobjects fixup callbacks return type
In reply to [email protected]From: "Du, Changbin" <[email protected]>
Update the return type to use bool instead of int, corresponding to cheange (debugobjects: make fixup functions return bool instead of int)
--- kernel/workqueue.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
kernel/workqueue.c +10 -10
@@ -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; } }
Signature · 1 line
2.7.4
PATCH 4/7 timer: update debugobjects fixup callbacks return type
In reply to [email protected]From: "Du, Changbin" <[email protected]>
Update the return type to use bool instead of int, corresponding to cheange (debugobjects: make fixup functions return bool instead of int).
--- kernel/time/hrtimer.c | 18 +++++++++--------- kernel/time/timer.c | 30 +++++++++++++++--------------- 2 files changed, 24 insertions(+), 24 deletions(-)
kernel/time/hrtimer.c +9 -9
@@ -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; } }
kernel/time/timer.c +15 -15
@@ -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; } }
Signature · 1 line
2.7.4
PATCH 5/7 rcu: update debugobjects fixup callbacks return type
In reply to [email protected]From: "Du, Changbin" <[email protected]>
Update the return type to use bool instead of int, corresponding to cheange (debugobjects: make fixup functions return bool instead of int).
--- kernel/rcu/update.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/rcu/update.c +3 -3
@@ -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; } }
Signature · 1 line
2.7.4
PATCH 6/7 percpu_counter: update debugobjects fixup callbacks return type
In reply to [email protected]From: "Du, Changbin" <[email protected]>
Update the return type to use bool instead of int, corresponding to cheange (debugobjects: make fixup functions return bool instead of int).
--- lib/percpu_counter.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/percpu_counter.c +3 -3
@@ -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; } }
Signature · 1 line
2.7.4
PATCH 7/7 Documentation: update debugobjects doc
In reply to [email protected]From: "Du, Changbin" <[email protected]>
Update documentation creangponding to change(debugobjects: make fixup functions return bool instead of int).
--- Documentation/DocBook/debugobjects.tmpl | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
Documentation/DocBook/debugobjects.tmpl +14 -12
@@ -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>
Signature · 1 line
2.7.4