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

Rafael J. Wysocki
2009-04-29T21:24:17Z

On Friday 24 April 2009, Pavel Machek wrote:
> 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?

Yes, printk() please.

> > >> +/**
> > >> + * 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.

Agreed.

Thanks,
Rafael