Re: [PATCH 1/8] PM: Add suspend block api.

Pavel Machek
2009-04-24T20:59:40Z

Hi!

> >> @@ -392,6 +393,9 @@ static void suspend_finish(void)
> >>
> >>
> >>  static const char * const pm_states[PM_SUSPEND_MAX] = {
> >> +#ifdef CONFIG_SUSPEND_BLOCK
> >> +     [PM_SUSPEND_ON]         = "on",
> >> +#endif
> >>       [PM_SUSPEND_STANDBY]    = "standby",
> >>       [PM_SUSPEND_MEM]        = "mem",
> >>  };
> >
> > This allows you to write "on" to the other interfaces where that makes
> > no sense.... right?
>
> state_store starts iterating at PM_SUSPEND_STANDBY, so don't think so.

Good.

> >> +bool suspend_is_blocked(void)
> >> +{
> >> +     if (WARN_ONCE(!enable_suspend_blockers, "ignoring suspend blockers\n"))
> >> +             return 0;
> >> +     return !!atomic_read(&suspend_block_count);
> >> +}
> >
> > Yes, if enable_suspend_blockers is one, we are in deep trouble.
>
> It just means that you compiled suspend blocker support into the
> kernel, but used an old interface to enter suspend. It warns if
> enable_suspend_blockers is 0 not 1.

I do not think WARN_ONCE() should be that easy to trigger. WARN is
very scary animal; better use normal printk?

> >> +/**
> >> + * suspend_blocker_init() - Initialize a suspend blocker
> >> + * @blocker: The suspend blocker to initialize.
> >> + * @name:    The name of the suspend blocker to show in
> >> + *           /proc/suspend_blockers
> >
> > I don't think suspend_blockers should go to /proc. They are not process-related.
>
> I moved that comment to the stats patch. It is still in procfs though,
> as it is most similar to other procfs entries. It is not allowed under
> the sysfs rules, and we do not want to enable debugfs on a production
> system since it exposes some unsafe interfaces.

You have to sort this out somehow. It does not belong to /proc.

Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html