| 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: <201202150022.26072.rjw@sisk.pl>
Date: Wed, 15 Feb 2012 00:22:25 +0100
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: Arve Hjønnevåg <arve@...roid.com>
Cc: 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>,
John Stultz <john.stultz@...aro.org>,
Brian Swetland <swetland@...gle.com>,
Neil Brown <neilb@...e.de>,
Alan Stern <stern@...land.harvard.edu>
Subject: Re: [RFC][PATCH 0/8] PM: Implement autosleep and "wake locks"
On Tuesday, February 14, 2012, Arve Hjønnevåg wrote:
> On Mon, Feb 6, 2012 at 5:00 PM, Rafael J. Wysocki <rjw@...k.pl> wrote:
> ...
> > All in all, it's not as much code as I thought it would be and it seems to be
> > relatively simple (which rises the question why the Android people didn't
> > even _try_ to do something like this instead of slapping the "real" wakelocks
> > onto the kernel FWIW). IMHO it doesn't add anything really new to the kernel,
> > except for the user space interfaces that should be maintainable. At least I
> > think I should be able to maintain them. :-)
> >
>
> Replacing a working solution with an untested one takes time.
Sure, that's pretty obvious. :-)
> That said, I have recently tried replacing all our kernel wake-locks with a
> thin wrapper around wake-sources. This appears to mostly work,
Good!
> but the wake-source timeout feature has some bugs or incompatible apis. An
> init api would also be useful for embedding wake-sources in other data
> structures without adding another memory allocation. Your patch to
> move the spinlock init to wakeup_source_add still require the struct
> to be zero initialized and the name set manually.
That should be easy to fix. What about the appended patch?
> I needed to use two wake-sources per wake-lock since calling
> __pm_stay_awake after __pm_wakeup_event on a wake-source does not
> cancel the timeout. Unless there is a reason to keep this behavior I
> would like __pm_stay_awake to cancel any active timeout.
That actually is a bug. At least it's not consistent with
__pm_wakeup_event() that will replace the existing timeout with a new
one.
I'll post a patch to fix that in the next couple of days, stay tuned. :-)
> Destroying a wake-source also has some problems. If you call
> wakeup_source_destroy it will spin forever if the wake-source is
> active without a timeout. And, if you call __pm_relax then
> wakeup_source_destroy it could free the wake-source memory while the
> timer function is still running.
This also is a bug that needs fixing anyway.
> It also looks as if the wake_source can be immediately deactivated if
> you call __pm_wakeup_event at the same time as the previous timeout expired.
Yes, there is a race window if the timer function has already started.
It looks like I wanted to make it too simple. :-) Will fix.
Thanks,
Rafael
Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
---
drivers/base/power/wakeup.c | 44 +++++++++++++++++++++++++++++++++++++-------
include/linux/pm_wakeup.h | 9 +++++++++
2 files changed, 46 insertions(+), 7 deletions(-)
Index: linux/drivers/base/power/wakeup.c
===================================================================
--- linux.orig/drivers/base/power/wakeup.c
+++ linux/drivers/base/power/wakeup.c
@@ -53,6 +53,28 @@ static void pm_wakeup_timer_fn(unsigned
static LIST_HEAD(wakeup_sources);
/**
+ * wakeup_source_init - Initialize a struct wakeup_source object.
+ * @ws: Wakeup source to initialize.
+ * @name: Name of the new wakeup source.
+ */
+int wakeup_source_init(struct wakeup_source *ws, const char *name)
+{
+ int ret = 0;
+
+ if (!ws)
+ return -EINVAL;
+
+ memset(ws, 0, sizeof(*ws));
+ if (name) {
+ ws->name = kstrdup(name, GFP_KERNEL);
+ if (!ws->name)
+ ret = -ENOMEM;
+ }
+ return ret;
+}
+EXPORT_SYMBOL_GPL(wakeup_source_init);
+
+/**
* wakeup_source_create - Create a struct wakeup_source object.
* @name: Name of the new wakeup source.
*/
@@ -60,22 +82,20 @@ struct wakeup_source *wakeup_source_crea
{
struct wakeup_source *ws;
- ws = kzalloc(sizeof(*ws), GFP_KERNEL);
+ ws = kmalloc(sizeof(*ws), GFP_KERNEL);
if (!ws)
return NULL;
- if (name)
- ws->name = kstrdup(name, GFP_KERNEL);
-
+ wakeup_source_init(ws, name);
return ws;
}
EXPORT_SYMBOL_GPL(wakeup_source_create);
/**
- * wakeup_source_destroy - Destroy a struct wakeup_source object.
- * @ws: Wakeup source to destroy.
+ * wakeup_source_drop - Prepare a struct wakeup_source object for destruction.
+ * @ws: Wakeup source to prepare for destruction.
*/
-void wakeup_source_destroy(struct wakeup_source *ws)
+void wakeup_source_drop(struct wakeup_source *ws)
{
if (!ws)
return;
@@ -91,6 +111,16 @@ void wakeup_source_destroy(struct wakeup
spin_unlock_irq(&ws->lock);
kfree(ws->name);
+}
+EXPORT_SYMBOL_GPL(wakeup_source_drop);
+
+/**
+ * wakeup_source_destroy - Destroy a struct wakeup_source object.
+ * @ws: Wakeup source to destroy.
+ */
+void wakeup_source_destroy(struct wakeup_source *ws)
+{
+ wakeup_source_drop(ws);
kfree(ws);
}
EXPORT_SYMBOL_GPL(wakeup_source_destroy);
Index: linux/include/linux/pm_wakeup.h
===================================================================
--- linux.orig/include/linux/pm_wakeup.h
+++ linux/include/linux/pm_wakeup.h
@@ -73,7 +73,9 @@ static inline bool device_may_wakeup(str
}
/* drivers/base/power/wakeup.c */
+extern int wakeup_source_init(struct wakeup_source *ws, const char *name);
extern struct wakeup_source *wakeup_source_create(const char *name);
+extern void wakeup_source_drop(struct wakeup_source *ws);
extern void wakeup_source_destroy(struct wakeup_source *ws);
extern void wakeup_source_add(struct wakeup_source *ws);
extern void wakeup_source_remove(struct wakeup_source *ws);
@@ -103,11 +105,18 @@ static inline bool device_can_wakeup(str
return dev->power.can_wakeup;
}
+static inline int wakeup_source_init(struct wakeup_source *ws, const char *name)
+{
+ return -ENOSYS;
+}
+
static inline struct wakeup_source *wakeup_source_create(const char *name)
{
return NULL;
}
+static inline void wakeup_source_drop(struct wakeup_source *ws) {}
+
static inline void wakeup_source_destroy(struct wakeup_source *ws) {}
static inline void wakeup_source_add(struct wakeup_source *ws) {}
--
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