Re: [PATCH] PERF(kernel): Cleanup power events V2

[Ingo Molnar <mingo@elte.hu>]

* Jean Pihet <jean.pihet@newoldbits.com> wrote:

> Ingo,
> 
> On Tue, Oct 26, 2010 at 9:10 AM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > * Thomas Renninger <trenn@suse.de> wrote:
> >
> >> Changes in V2:
> >>   - Introduce PWR_EVENT_EXIT instead of 0 to mark non-power state
> >>   - Use u32 instead of u64 for cpuid, state which is by far enough
> 
> ...
> 
> >>
> >> +#define PWR_EVENT_EXIT 0xFFFFFFFF
> >
> > Shouldnt this be part of the POWER_ enum? (and you can write -1 there)
> >
> >> +#ifndef _TRACE_POWER_ENUM_
> >> +#define _TRACE_POWER_ENUM_
> >> +enum {
> >> +     POWER_NONE = 0,
> >> +     POWER_CSTATE = 1,
> >> +     POWER_PSTATE = 2,
> >> +};
> >> +#endif
> >
> > Since we are cleaning up all these events, those enum definitions dont really look
> > logical. For example, what is 'POWER_NONE'? Can a CPU have 'no power'?
>
> The enum belongs to the deprecated API so I would rather not touch it.
> Keeping the deprecated code isolated will make it easier to remove
> later.

So what will replace it? We still have a state field.

Passing in platform specific codes is a step backwards.

> >> +TRACE_EVENT(machine_suspend,
> >> +
> >> +     TP_PROTO(unsigned int state),
> >> +
> >> +     TP_ARGS(state),
> >> +
> >> +     TP_STRUCT__entry(
> >> +             __field(        u32,            state           )
> >> +     ),
> >
> > Hm, this event is not used anywhere in the submitted patches. Where is the patch
> > that adds usage, and what are the possible values for 'state'?
>
> This will come as a separate patch, which fits all platforms. Cf.
> http://marc.info/?l=linux-omap&m=128620575300682&w=2.
> The state field is of type suspend_state_t, cf. include/linux/suspend.h

Ok, that's at least generic. Needs the review of Rafael, to determine whether this 
state value is all we want to know when we enter suspend.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html