Re: [tip:core/debugobjects] debugobjects: Be smarter about static objects

Thomas Gleixner
2011-12-13T20:37:40Z

On Tue, 13 Dec 2011, Stephen Boyd wrote:
>
> This code is only slightly confusing

Maybe we should tell that the guy who wrote it :)

> static int __init fixup_activate(void *addr, enum debug_obj_state state)
> {
> struct self_test *obj = addr;
>
> switch (state) {
> case ODEBUG_STATE_NOTAVAILABLE:
> if (obj->static_init == 1) {
> debug_object_init(obj, &descr_type_test);
> debug_object_activate(obj, &descr_type_test);
> /*
> * Real code should return 0 here ! This is
> * not a fixup of some bad behaviour. We
> * merily call the debug_init function to keep
> * track of the object.
> */
> return 1;
> } else {
> /* Real code needs to emit a warning here */
> }
> return 0;
>
>
> It seems that it does the complete opposite of what it should do, i.e.
> return 1 when the fixup is static and not actually a problem and return
> 0 otherwise. Because of this return 1, debug_object_activate() thinks
> there was a problem in the fixup and then it ups the warning count
> because this patch added a warning print for static objects.

Hmm, I think that was because I had not implemented that static
warning thing back then. So yes, it's backwards and should be fixed
proper:

> diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> index 77cb245..0ab9ae8 100644
> --- a/lib/debugobjects.c
> +++ b/lib/debugobjects.c
> @@ -818,17 +818,9 @@ 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);
> - /*
> - * Real code should return 0 here ! This is
> - * not a fixup of some bad behaviour. We
> - * merily call the debug_init function to keep
> - * track of the object.
> - */
> - return 1;
> - } else {
> - /* Real code needs to emit a warning here */
> + return 0;
> }
> - return 0;
> + return 1;
>
> case ODEBUG_STATE_ACTIVE:
> debug_object_deactivate(obj, &descr_type_test);
> @@ -967,7 +959,7 @@ static void __init debug_objects_selftest(void)
>
> obj.static_init = 1;
> debug_object_activate(&obj, &descr_type_test);
> - if (check_results(&obj, ODEBUG_STATE_ACTIVE, ++fixups, warnings))
> + if (check_results(&obj, ODEBUG_STATE_ACTIVE, fixups, warnings))
> goto out;
> debug_object_init(&obj, &descr_type_test);
> if (check_results(&obj, ODEBUG_STATE_INIT, ++fixups, ++warnings))
>
>
>
> This would make the fixup function for a static NOTAVAILABLE object
> return 0 and 1 appropriately and corrects the fixup and warning checking
> to reflect that nothing was in need of fixing.

Yes, the other thing works, but is butt ugly.

> Why was the fixup for selftest inverted?

See above plus laziness I assume :)

Can you please resend with a changelong ?

Thanks,

tglx