Re: [RFC][PATCH 0/7] PM: Implement autosleep and "wake locks", take2

["Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com>]

On 02/26/2012 02:31 AM, Rafael J. Wysocki wrote:

> 
> I think we can do something like in the updated patch [5/7] below.
> 
> It uses a special wakeup source object called "autosleep" to bump up the
> number of wakeup events in progress before acquiring autosleep_lock in
> pm_autosleep_set_state().  This way, either pm_autosleep_set_state() will
> acquire autosleep_lock before try_to_suspend(), in which case the latter
> will see the change of autosleep_state immediately (after autosleep_lock has
> been passed to it), or try_to_suspend() will get it first, but then
> pm_save_wakeup_count() or pm_suspend()/hibernate() will see the nonzero counter
> of wakeup events in progress and return error code (sooner or later).
> 
> The drawback is that writes to /sys/power/autosleep may interfere with
> the /sys/power/wakeup_count + /sys/power/state interface by interrupting
> transitions started by writing to /sys/power/state, for example (although
> I think that's highly unlikely).


Yes, but I think we can live with that.. It doesn't look like a big issue.

> 
> Additionally, I made pm_autosleep_lock() use mutex_trylock_interruptible()


You have used mutex_lock_interruptible() in the code below.. It wouldn't matter
as long as you have used some form of "interruptible" but I think
mutex_trylock_interruptible would be even better..

> to prevent operations on /sys/power/wakeup_count and/or /sys/power/state
> from failing the freezing of tasks started by try_to_suspend().
> 
> Thanks,
> Rafael
> 


The approach taken by the patch below looks good to me. I don't see any obvious
problems, except for the minor ones listed below.

> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Sleep: Implement opportunistic sleep
> 
> Introduce a mechanism by which the kernel can trigger global
> transitions to a sleep state chosen by user space if there are no
> active wakeup sources.
> 
> It consists of a new sysfs attribute, /sys/power/autosleep, that
> can be written one of the strings returned by reads from
> /sys/power/state, an ordered workqueue and a work item carrying out
> the "suspend" operations.  If a string representing the system's
> sleep state is written to /sys/power/autosleep, the work item
> triggering transitions to that state is queued up and it requeues
> itself after every execution until user space writes "off" to
> /sys/power/autosleep.
> 
> That work item enables the detection of wakeup events using the
> functions already defined in drivers/base/power/wakeup.c (with one
> small modification) and calls either pm_suspend(), or hibernate() to
> put the system into a sleep state.  If a wakeup event is reported
> while the transition is in progress, it will abort the transition and
> the "system suspend" work item will be queued up again.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Index: linux/kernel/power/main.c
> ===================================================================
> --- linux.orig/kernel/power/main.c
> +++ linux/kernel/power/main.c
> @@ -269,8 +269,7 @@ static ssize_t state_show(struct kobject
>  	return (s - buf);
>  }
> 
> -static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
> -			   const char *buf, size_t n)
> +static suspend_state_t decode_state(const char *buf, size_t n)
>  {
>  #ifdef CONFIG_SUSPEND
>  	suspend_state_t state = PM_SUSPEND_STANDBY;
> @@ -278,27 +277,48 @@ static ssize_t state_store(struct kobjec
>  #endif
>  	char *p;
>  	int len;
> -	int error = -EINVAL;
> 
>  	p = memchr(buf, '\n', n);
>  	len = p ? p - buf : n;
> 
> -	/* First, check if we are requested to hibernate */
> -	if (len == 4 && !strncmp(buf, "disk", len)) {
> -		error = hibernate();
> -		goto Exit;
> -	}
> +	/* Check hibernation first. */
> +	if (len == 4 && !strncmp(buf, "disk", len))
> +		return PM_SUSPEND_MAX;
> 
>  #ifdef CONFIG_SUSPEND
> -	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> -		if (*s && len == strlen(*s) && !strncmp(buf, *s, len)) {
> -			error = pm_suspend(state);
> -			break;
> -		}
> -	}
> +	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++)
> +		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> +			return state;
>  #endif
> 
> - Exit:
> +	return PM_SUSPEND_ON;
> +}
> +
> +static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t n)
> +{
> +	suspend_state_t state;
> +	int error;
> +
> +	error = pm_autosleep_lock();
> +	if (error)
> +		return error;
> +
> +	if (pm_autosleep_state() > PM_SUSPEND_ON) {
> +		error = -EBUSY;
> +		goto out;
> +	}
> +
> +	state = decode_state(buf, n);
> +	if (state < PM_SUSPEND_MAX)
> +		error = pm_suspend(state);
> +	else if (state > PM_SUSPEND_ON)
> +		error = hibernate();
> +	else
> +		error = -EINVAL;


By the way, the condition checks in the above if-else block look kinda
odd, considering what is done in other similar places, which are more
readable. It would be great if you could make them consistent.

> +
> + out:
> +	pm_autosleep_unlock();
>  	return error ? error : n;
>  }
> 
> @@ -339,7 +359,8 @@ static ssize_t wakeup_count_show(struct
>  {
>  	unsigned int val;
> 
> -	return pm_get_wakeup_count(&val) ? sprintf(buf, "%u\n", val) : -EINTR;
> +	return pm_get_wakeup_count(&val, true) ?
> +		sprintf(buf, "%u\n", val) : -EINTR;
>  }
> 
> +
> +static ssize_t autosleep_store(struct kobject *kobj,
> +			       struct kobj_attribute *attr,
> +			       const char *buf, size_t n)
> +{
> +	suspend_state_t state = decode_state(buf, n);
> +	int error;
> +
> +	if (state == PM_SUSPEND_ON && strncmp(buf, "off", 3)
> +	    && strncmp(buf, "off\n", 4))
> +		return -EINVAL;
> +


I am pretty sure you meant "if autosleep is already off, and the user
wrote "off" to /sys/power/autosleep, then return -EINVAL"

But strncmp() returns 0 if the strings match, and hence the code above
doesn't seem to do what you intended.

> +	error = pm_autosleep_set_state(state);
> +	return error ? error : n;
> +}
> +
> +power_attr(autosleep);
> +#endif /* CONFIG_PM_AUTOSLEEP */
>  #endif /* CONFIG_PM_SLEEP */
> 
>  #ifdef CONFIG_PM_TRACE


Regards,
Srivatsa S. Bhat