| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4F4CAB45.4000903@linux.vnet.ibm.com>
Date: Tue, 28 Feb 2012 15:54:05 +0530
From: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
To: "Rafael J. Wysocki" <rjw@...k.pl>
CC: John Stultz <john.stultz@...aro.org>,
Linux PM list <linux-pm@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Magnus Damm <magnus.damm@...il.com>, markgross@...gnar.org,
Matthew Garrett <mjg@...hat.com>,
Greg KH <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Brian Swetland <swetland@...gle.com>,
Neil Brown <neilb@...e.de>,
Alan Stern <stern@...land.harvard.edu>,
Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [RFC][PATCH 0/7] PM: Implement autosleep and "wake locks", take2
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@...k.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@...k.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists