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

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday 30 April 2009, Arve Hjønnevåg wrote:
> 2009/4/29 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 15 April 2009, Arve Hjønnevåg wrote:
> >> diff --git a/include/linux/suspend_block.h b/include/linux/suspend_block.h
> >> new file mode 100755
> >> index 0000000..7820c60
> >> --- /dev/null
> >> +++ b/include/linux/suspend_block.h
> >
> > suspend_blockers.h perhaps?
> 
> suspend_blocker.h?

Fine by me.

[--snip--]
> >> +config SUSPEND_BLOCK
> >> +     bool "Suspend block"
> >> +     depends on PM
> >> +     select RTC_LIB
> >
> > depends on RTC_LIB
> >
> > select doesn't really work and I don't think it ever will.
> 
> Nothing depends on RTC_LIB, it is only selected.

If nothing in your code needs anything depending on RTC_LIB, why select it?

[--snip--]
> >
> >> +
> >> +retry:
> >> +     if (suspend_is_blocked()) {
> >> +             if (debug_mask & DEBUG_SUSPEND)
> >> +                     pr_info("suspend: abort suspend\n");
> >> +             goto abort;
> >
> > If that were in a loop you could just use 'break' here.
> 
> do you think this is better:
> while(1) {
>         if (exception1) {
>                 ...
>                 break;
>         }
>         ...
>         if (exception2) {
>                 ...
>                 continue;
>         }
>         break;
> }

I rather thought of

for (;;) {
         if (exception1) {
                 ...
                 break;
         }
         ...
         if (!exception2)
                 break;
         ...
}

[--snip--]
> >> +static int suspend_block_suspend(struct sys_device *dev, pm_message_t state)
> >> +{
> >> +     int ret = suspend_is_blocked() ? -EAGAIN : 0;
> >> +     if (debug_mask & DEBUG_SUSPEND)
> >> +             pr_info("suspend_block_suspend return %d\n", ret);
> >> +     return ret;
> >> +}
> >> +
> >> +static struct sysdev_class suspend_block_sysclass = {
> >> +     .name = "suspend_block",
> >> +     .suspend = suspend_block_suspend,
> >> +};
> >> +static struct sys_device suspend_block_sysdev = {
> >> +     .cls = &suspend_block_sysclass,
> >> +};
> >> +
> >
> > Hmm.  Perhaps add the suspend_is_blocked() check at the beginning of
> > sysdev_suspend() instead of this?  Surely you don't want to suspend
> > any sysdevs with any suspend blockers active, right?
> 
> You could make the same argument for any device. Using a sysdev makes
> the patch easier to apply.

You've lost me here. :-)

AFAICT, the only purpose of the sysdev class above is to abort suspend if
suspend_is_blocked() returns 'true'.  If that is correct, then IMO you could
achieve the same goal by calling suspend_is_blocked() directly from
sysdev_suspend(), right before check_wakeup_irqs().  Or am I missing anything?

Thanks,
Rafael