Messages in this thread | | | Date | Tue, 13 Dec 2011 21:37:27 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [tip:core/debugobjects] debugobjects: Be smarter about static objects |
| |
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
| |