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

Arve Hjønnevåg
2009-05-05T03:48:09Z

2009/5/2 Rafael J. Wysocki <rjw@sisk.pl>:
> [--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;
>         ...
> }

I don't think that is much better, but I changed the first patch to
use a loop anyway. The loop is then removed (as before) in the patch
that adds the stats.

>
> [--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. :-)

You have been making other changes to those files which made it a moving target.

> 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?

The stats patch also used it to track wakeup events, but I have added
another call for this to get rid of the sysdev.

--
Arve Hjønnevåg