[ARM] Improve reliability of APM-emulation suspend

The APM emulation can sometimes cause suspend to fail to work due to apparantly waiting for some process to acknowledge an event when it actually has already done so. We re-jig the event handling to work around this behaviour. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

[ARM] Improve reliability of APM-emulation suspend
The APM emulation can sometimes cause suspend to fail to work due to apparantly waiting for some process to acknowledge an event when it actually has already done so. We re-jig the event handling to work around this behaviour. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Russell King · Russell King
1 parent 0215ffb08c
Showing 1 changed file with 123 additions and 55 deletions Side-by-side Diff
arch/arm/kernel/apm.c
@@ -12,7 +12,6 @@
  */
 #include <linux/module.h>
 #include <linux/poll.h>
-#include <linux/timer.h>
 #include <linux/slab.h>
 #include <linux/proc_fs.h>
 #include <linux/miscdevice.h>
@@ -26,6 +25,7 @@
 #include <linux/init.h>
 #include <linux/completion.h>
 #include <linux/kthread.h>
+#include <linux/delay.h>
  
 #include <asm/apm.h> /* apm_power_info */
 #include <asm/system.h>
@@ -71,7 +71,8 @@
 #define SUSPEND_PENDING	1		/* suspend pending read */
 #define SUSPEND_READ	2		/* suspend read, pending ack */
 #define SUSPEND_ACKED	3		/* suspend acked */
-#define SUSPEND_DONE	4		/* suspend completed */
+#define SUSPEND_WAIT	4		/* waiting for suspend */
+#define SUSPEND_DONE	5		/* suspend completed */
  
 	struct apm_queue	queue;
 };
@@ -101,6 +102,7 @@
 static DEFINE_SPINLOCK(kapmd_queue_lock);
 static struct apm_queue kapmd_queue;
  
+static DECLARE_MUTEX(state_lock);
  
 static const char driver_version[] = "1.13";	/* no spaces */
  
  
  
  
  
  
  
  
  
  
  
  
@@ -148,38 +150,60 @@
 	q->events[q->event_head] = event;
 }
  
-static void queue_event_one_user(struct apm_user *as, apm_event_t event)
+static void queue_event(apm_event_t event)
 {
-	if (as->suser && as->writer) {
-		switch (event) {
-		case APM_SYS_SUSPEND:
-		case APM_USER_SUSPEND:
-			/*
-			 * If this user already has a suspend pending,
-			 * don't queue another one.
-			 */
-			if (as->suspend_state != SUSPEND_NONE)
-				return;
+	struct apm_user *as;
  
-			as->suspend_state = SUSPEND_PENDING;
-			suspends_pending++;
-			break;
-		}
+	down_read(&user_list_lock);
+	list_for_each_entry(as, &apm_user_list, list) {
+		if (as->reader)
+			queue_add_event(&as->queue, event);
 	}
-	queue_add_event(&as->queue, event);
+	up_read(&user_list_lock);
+	wake_up_interruptible(&apm_waitqueue);
 }
  
-static void queue_event(apm_event_t event, struct apm_user *sender)
+/*
+ * queue_suspend_event - queue an APM suspend event.
+ *
+ * Check that we're in a state where we can suspend.  If not,
+ * return -EBUSY.  Otherwise, queue an event to all "writer"
+ * users.  If there are no "writer" users, return '1' to
+ * indicate that we can immediately suspend.
+ */
+static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
 {
 	struct apm_user *as;
+	int ret = 1;
  
+	down(&state_lock);
 	down_read(&user_list_lock);
+
+	/*
+	 * If a thread is still processing, we can't suspend, so reject
+	 * the request.
+	 */
 	list_for_each_entry(as, &apm_user_list, list) {
-		if (as != sender && as->reader)
-			queue_event_one_user(as, event);
+		if (as != sender && as->reader && as->writer && as->suser &&
+		    as->suspend_state != SUSPEND_NONE) {
+			ret = -EBUSY;
+			goto out;
+		}
 	}
+
+	list_for_each_entry(as, &apm_user_list, list) {
+		if (as != sender && as->reader && as->writer && as->suser) {
+			as->suspend_state = SUSPEND_PENDING;
+			suspends_pending++;
+			queue_add_event(&as->queue, event);
+			ret = 0;
+		}
+	}
+ out:
 	up_read(&user_list_lock);
+	up(&state_lock);
 	wake_up_interruptible(&apm_waitqueue);
+	return ret;
 }
  
 static void apm_suspend(void)
  
  
  
@@ -191,17 +215,22 @@
 	 * Anyone on the APM queues will think we're still suspended.
 	 * Send a message so everyone knows we're now awake again.
 	 */
-	queue_event(APM_NORMAL_RESUME, NULL);
+	queue_event(APM_NORMAL_RESUME);
  
 	/*
 	 * Finally, wake up anyone who is sleeping on the suspend.
 	 */
+	down(&state_lock);
 	down_read(&user_list_lock);
 	list_for_each_entry(as, &apm_user_list, list) {
-		as->suspend_result = err;
-		as->suspend_state = SUSPEND_DONE;
+		if (as->suspend_state == SUSPEND_WAIT ||
+		    as->suspend_state == SUSPEND_ACKED) {
+	  		as->suspend_result = err;
+			as->suspend_state = SUSPEND_DONE;
+		}
 	}
 	up_read(&user_list_lock);
+	up(&state_lock);
  
 	wake_up(&apm_suspend_waitqueue);
 }
  
@@ -227,8 +256,11 @@
 		if (copy_to_user(buf, &event, sizeof(event)))
 			break;
  
-		if (event == APM_SYS_SUSPEND || event == APM_USER_SUSPEND)
+		down(&state_lock);
+		if (as->suspend_state == SUSPEND_PENDING &&
+		    (event == APM_SYS_SUSPEND || event == APM_USER_SUSPEND))
 			as->suspend_state = SUSPEND_READ;
+		up(&state_lock);
  
 		buf += sizeof(event);
 		i -= sizeof(event);
  
@@ -270,9 +302,13 @@
  
 	switch (cmd) {
 	case APM_IOC_SUSPEND:
+		down(&state_lock);
+
 		as->suspend_result = -EINTR;
  
 		if (as->suspend_state == SUSPEND_READ) {
+			int pending;
+
 			/*
 			 * If we read a suspend command from /dev/apm_bios,
 			 * then the corresponding APM_IOC_SUSPEND ioctl is
  
  
  
  
  
  
  
  
@@ -280,47 +316,66 @@
 			 */
 			as->suspend_state = SUSPEND_ACKED;
 			suspends_pending--;
+			pending = suspends_pending == 0;
+			up(&state_lock);
+
+			/*
+			 * If there are no further acknowledges required,
+			 * suspend the system.
+			 */
+			if (pending)
+				apm_suspend();
+
+			/*
+			 * Wait for the suspend/resume to complete.  If there
+			 * are pending acknowledges, we wait here for them.
+			 *
+			 * Note: we need to ensure that the PM subsystem does
+			 * not kick us out of the wait when it suspends the
+			 * threads.
+			 */
+			flags = current->flags;
+			current->flags |= PF_NOFREEZE;
+
+			wait_event(apm_suspend_waitqueue,
+				   as->suspend_state == SUSPEND_DONE);
 		} else {
+			up(&state_lock);
+
 			/*
 			 * Otherwise it is a request to suspend the system.
 			 * Queue an event for all readers, and expect an
 			 * acknowledge from all writers who haven't already
 			 * acknowledged.
 			 */
-			queue_event(APM_USER_SUSPEND, as);
-		}
+			err = queue_suspend_event(APM_USER_SUSPEND, as);
+			if (err < 0)
+				break;
  
-		/*
-		 * If there are no further acknowledges required, suspend
-		 * the system.
-		 */
-		if (suspends_pending == 0)
-			apm_suspend();
+			if (err > 0)
+				apm_suspend();
  
-		/*
-		 * Wait for the suspend/resume to complete.  If there are
-		 * pending acknowledges, we wait here for them.
-		 *
-		 * Note that we need to ensure that the PM subsystem does
-		 * not kick us out of the wait when it suspends the threads.
-		 */
-		flags = current->flags;
-		current->flags |= PF_NOFREEZE;
+			/*
+			 * Wait for the suspend/resume to complete.  If there
+			 * are pending acknowledges, we wait here for them.
+			 *
+			 * Note: we need to ensure that the PM subsystem does
+			 * not kick us out of the wait when it suspends the
+			 * threads.
+			 */
+			flags = current->flags;
+			current->flags |= PF_NOFREEZE;
  
-		/*
-		 * Note: do not allow a thread which is acking the suspend
-		 * to escape until the resume is complete.
-		 */
-		if (as->suspend_state == SUSPEND_ACKED)
-			wait_event(apm_suspend_waitqueue,
-					 as->suspend_state == SUSPEND_DONE);
-		else
 			wait_event_interruptible(apm_suspend_waitqueue,
 					 as->suspend_state == SUSPEND_DONE);
+		}
  
 		current->flags = flags;
+
+		down(&state_lock);
 		err = as->suspend_result;
 		as->suspend_state = SUSPEND_NONE;
+		up(&state_lock);
 		break;
 	}
  
@@ -330,6 +385,8 @@
 static int apm_release(struct inode * inode, struct file * filp)
 {
 	struct apm_user *as = filp->private_data;
+	int pending = 0;
+
 	filp->private_data = NULL;
  
 	down_write(&user_list_lock);
  
  
@@ -342,11 +399,14 @@
 	 * need to balance suspends_pending, which means the
 	 * possibility of sleeping.
 	 */
+	down(&state_lock);
 	if (as->suspend_state != SUSPEND_NONE) {
 		suspends_pending -= 1;
-		if (suspends_pending == 0)
-			apm_suspend();
+		pending = suspends_pending == 0;
 	}
+	up(&state_lock);
+	if (pending)
+		apm_suspend();
  
 	kfree(as);
 	return 0;
@@ -470,6 +530,7 @@
 {
 	do {
 		apm_event_t event;
+		int ret;
  
 		wait_event_interruptible(kapmd_wait,
 				!queue_empty(&kapmd_queue) || kthread_should_stop());
  
@@ -489,13 +550,20 @@
  
 		case APM_LOW_BATTERY:
 		case APM_POWER_STATUS_CHANGE:
-			queue_event(event, NULL);
+			queue_event(event);
 			break;
  
 		case APM_USER_SUSPEND:
 		case APM_SYS_SUSPEND:
-			queue_event(event, NULL);
-			if (suspends_pending == 0)
+			ret = queue_suspend_event(event, NULL);
+			if (ret < 0) {
+				/*
+				 * We were busy.  Try again in 50ms.
+				 */
+				queue_add_event(&kapmd_queue, event);
+				msleep(50);
+			}
+			if (ret > 0)
 				apm_suspend();
 			break;
...	...	@@ -12,7 +12,6 @@
12	12	*/
13	13	#include <linux/module.h>
14	14	#include <linux/poll.h>
15		-#include <linux/timer.h>
16	15	#include <linux/slab.h>
17	16	#include <linux/proc_fs.h>
18	17	#include <linux/miscdevice.h>
...	...	@@ -26,6 +25,7 @@
26	25	#include <linux/init.h>
27	26	#include <linux/completion.h>
28	27	#include <linux/kthread.h>
	28	+#include <linux/delay.h>
29	29
30	30	#include <asm/apm.h> /* apm_power_info */
31	31	#include <asm/system.h>
...	...	@@ -71,7 +71,8 @@
71	71	#define SUSPEND_PENDING 1 /* suspend pending read */
72	72	#define SUSPEND_READ 2 /* suspend read, pending ack */
73	73	#define SUSPEND_ACKED 3 /* suspend acked */
74		-#define SUSPEND_DONE 4 /* suspend completed */
	74	+#define SUSPEND_WAIT 4 /* waiting for suspend */
	75	+#define SUSPEND_DONE 5 /* suspend completed */
75	76
76	77	struct apm_queue queue;
77	78	};
...	...	@@ -101,6 +102,7 @@
101	102	static DEFINE_SPINLOCK(kapmd_queue_lock);
102	103	static struct apm_queue kapmd_queue;
103	104
	105	+static DECLARE_MUTEX(state_lock);
104	106
105	107	static const char driver_version[] = "1.13"; /* no spaces */
106	108
107	109
108	110
109	111
110	112
111	113
112	114
113	115
114	116
115	117
116	118
117	119
...	...	@@ -148,38 +150,60 @@
148	150	q->events[q->event_head] = event;
149	151	}
150	152
151		-static void queue_event_one_user(struct apm_user *as, apm_event_t event)
	153	+static void queue_event(apm_event_t event)
152	154	{
153		- if (as->suser && as->writer) {
154		- switch (event) {
155		- case APM_SYS_SUSPEND:
156		- case APM_USER_SUSPEND:
157		- /*
158		- * If this user already has a suspend pending,
159		- * don't queue another one.
160		- */
161		- if (as->suspend_state != SUSPEND_NONE)
162		- return;
	155	+ struct apm_user *as;
163	156
164		- as->suspend_state = SUSPEND_PENDING;
165		- suspends_pending++;
166		- break;
167		- }
	157	+ down_read(&user_list_lock);
	158	+ list_for_each_entry(as, &apm_user_list, list) {
	159	+ if (as->reader)
	160	+ queue_add_event(&as->queue, event);
168	161	}
169		- queue_add_event(&as->queue, event);
	162	+ up_read(&user_list_lock);
	163	+ wake_up_interruptible(&apm_waitqueue);
170	164	}
171	165
172		-static void queue_event(apm_event_t event, struct apm_user *sender)
	166	+/*
	167	+ * queue_suspend_event - queue an APM suspend event.
	168	+ *
	169	+ * Check that we're in a state where we can suspend. If not,
	170	+ * return -EBUSY. Otherwise, queue an event to all "writer"
	171	+ * users. If there are no "writer" users, return '1' to
	172	+ * indicate that we can immediately suspend.
	173	+ */
	174	+static int queue_suspend_event(apm_event_t event, struct apm_user *sender)
173	175	{
174	176	struct apm_user *as;
	177	+ int ret = 1;
175	178
	179	+ down(&state_lock);
176	180	down_read(&user_list_lock);
	181	+
	182	+ /*
	183	+ * If a thread is still processing, we can't suspend, so reject
	184	+ * the request.
	185	+ */
177	186	list_for_each_entry(as, &apm_user_list, list) {
178		- if (as != sender && as->reader)
179		- queue_event_one_user(as, event);
	187	+ if (as != sender && as->reader && as->writer && as->suser &&
	188	+ as->suspend_state != SUSPEND_NONE) {
	189	+ ret = -EBUSY;
	190	+ goto out;
	191	+ }
180	192	}
	193	+
	194	+ list_for_each_entry(as, &apm_user_list, list) {
	195	+ if (as != sender && as->reader && as->writer && as->suser) {
	196	+ as->suspend_state = SUSPEND_PENDING;
	197	+ suspends_pending++;
	198	+ queue_add_event(&as->queue, event);
	199	+ ret = 0;
	200	+ }
	201	+ }
	202	+ out:
181	203	up_read(&user_list_lock);
	204	+ up(&state_lock);
182	205	wake_up_interruptible(&apm_waitqueue);
	206	+ return ret;
183	207	}
184	208
185	209	static void apm_suspend(void)
186	210
187	211
188	212
...	...	@@ -191,17 +215,22 @@
191	215	* Anyone on the APM queues will think we're still suspended.
192	216	* Send a message so everyone knows we're now awake again.
193	217	*/
194		- queue_event(APM_NORMAL_RESUME, NULL);
	218	+ queue_event(APM_NORMAL_RESUME);
195	219
196	220	/*
197	221	* Finally, wake up anyone who is sleeping on the suspend.
198	222	*/
	223	+ down(&state_lock);
199	224	down_read(&user_list_lock);
200	225	list_for_each_entry(as, &apm_user_list, list) {
201		- as->suspend_result = err;
202		- as->suspend_state = SUSPEND_DONE;
	226	+ if (as->suspend_state == SUSPEND_WAIT \|\|
	227	+ as->suspend_state == SUSPEND_ACKED) {
	228	+ as->suspend_result = err;
	229	+ as->suspend_state = SUSPEND_DONE;
	230	+ }
203	231	}
204	232	up_read(&user_list_lock);
	233	+ up(&state_lock);
205	234
206	235	wake_up(&apm_suspend_waitqueue);
207	236	}
208	237
...	...	@@ -227,8 +256,11 @@
227	256	if (copy_to_user(buf, &event, sizeof(event)))
228	257	break;
229	258
230		- if (event == APM_SYS_SUSPEND \|\| event == APM_USER_SUSPEND)
	259	+ down(&state_lock);
	260	+ if (as->suspend_state == SUSPEND_PENDING &&
	261	+ (event == APM_SYS_SUSPEND \|\| event == APM_USER_SUSPEND))
231	262	as->suspend_state = SUSPEND_READ;
	263	+ up(&state_lock);
232	264
233	265	buf += sizeof(event);
234	266	i -= sizeof(event);
235	267
...	...	@@ -270,9 +302,13 @@
270	302
271	303	switch (cmd) {
272	304	case APM_IOC_SUSPEND:
	305	+ down(&state_lock);
	306	+
273	307	as->suspend_result = -EINTR;
274	308
275	309	if (as->suspend_state == SUSPEND_READ) {
	310	+ int pending;
	311	+
276	312	/*
277	313	* If we read a suspend command from /dev/apm_bios,
278	314	* then the corresponding APM_IOC_SUSPEND ioctl is
279	315
280	316
281	317
282	318
283	319
284	320
285	321
286	322
...	...	@@ -280,47 +316,66 @@
280	316	*/
281	317	as->suspend_state = SUSPEND_ACKED;
282	318	suspends_pending--;
	319	+ pending = suspends_pending == 0;
	320	+ up(&state_lock);
	321	+
	322	+ /*
	323	+ * If there are no further acknowledges required,
	324	+ * suspend the system.
	325	+ */
	326	+ if (pending)
	327	+ apm_suspend();
	328	+
	329	+ /*
	330	+ * Wait for the suspend/resume to complete. If there
	331	+ * are pending acknowledges, we wait here for them.
	332	+ *
	333	+ * Note: we need to ensure that the PM subsystem does
	334	+ * not kick us out of the wait when it suspends the
	335	+ * threads.
	336	+ */
	337	+ flags = current->flags;
	338	+ current->flags \|= PF_NOFREEZE;
	339	+
	340	+ wait_event(apm_suspend_waitqueue,
	341	+ as->suspend_state == SUSPEND_DONE);
283	342	} else {
	343	+ up(&state_lock);
	344	+
284	345	/*
285	346	* Otherwise it is a request to suspend the system.
286	347	* Queue an event for all readers, and expect an
287	348	* acknowledge from all writers who haven't already
288	349	* acknowledged.
289	350	*/
290		- queue_event(APM_USER_SUSPEND, as);
291		- }
	351	+ err = queue_suspend_event(APM_USER_SUSPEND, as);
	352	+ if (err < 0)
	353	+ break;
292	354
293		- /*
294		- * If there are no further acknowledges required, suspend
295		- * the system.
296		- */
297		- if (suspends_pending == 0)
298		- apm_suspend();
	355	+ if (err > 0)
	356	+ apm_suspend();
299	357
300		- /*
301		- * Wait for the suspend/resume to complete. If there are
302		- * pending acknowledges, we wait here for them.
303		- *
304		- * Note that we need to ensure that the PM subsystem does
305		- * not kick us out of the wait when it suspends the threads.
306		- */
307		- flags = current->flags;
308		- current->flags \|= PF_NOFREEZE;
	358	+ /*
	359	+ * Wait for the suspend/resume to complete. If there
	360	+ * are pending acknowledges, we wait here for them.
	361	+ *
	362	+ * Note: we need to ensure that the PM subsystem does
	363	+ * not kick us out of the wait when it suspends the
	364	+ * threads.
	365	+ */
	366	+ flags = current->flags;
	367	+ current->flags \|= PF_NOFREEZE;
309	368
310		- /*
311		- * Note: do not allow a thread which is acking the suspend
312		- * to escape until the resume is complete.
313		- */
314		- if (as->suspend_state == SUSPEND_ACKED)
315		- wait_event(apm_suspend_waitqueue,
316		- as->suspend_state == SUSPEND_DONE);
317		- else
318	369	wait_event_interruptible(apm_suspend_waitqueue,
319	370	as->suspend_state == SUSPEND_DONE);
	371	+ }
320	372
321	373	current->flags = flags;
	374	+
	375	+ down(&state_lock);
322	376	err = as->suspend_result;
323	377	as->suspend_state = SUSPEND_NONE;
	378	+ up(&state_lock);
324	379	break;
325	380	}
326	381
...	...	@@ -330,6 +385,8 @@
330	385	static int apm_release(struct inode * inode, struct file * filp)
331	386	{
332	387	struct apm_user *as = filp->private_data;
	388	+ int pending = 0;
	389	+
333	390	filp->private_data = NULL;
334	391
335	392	down_write(&user_list_lock);
336	393
337	394
...	...	@@ -342,11 +399,14 @@
342	399	* need to balance suspends_pending, which means the
343	400	* possibility of sleeping.
344	401	*/
	402	+ down(&state_lock);
345	403	if (as->suspend_state != SUSPEND_NONE) {
346	404	suspends_pending -= 1;
347		- if (suspends_pending == 0)
348		- apm_suspend();
	405	+ pending = suspends_pending == 0;
349	406	}
	407	+ up(&state_lock);
	408	+ if (pending)
	409	+ apm_suspend();
350	410
351	411	kfree(as);
352	412	return 0;
...	...	@@ -470,6 +530,7 @@
470	530	{
471	531	do {
472	532	apm_event_t event;
	533	+ int ret;
473	534
474	535	wait_event_interruptible(kapmd_wait,
475	536	!queue_empty(&kapmd_queue) \|\| kthread_should_stop());
476	537
...	...	@@ -489,13 +550,20 @@
489	550
490	551	case APM_LOW_BATTERY:
491	552	case APM_POWER_STATUS_CHANGE:
492		- queue_event(event, NULL);
	553	+ queue_event(event);
493	554	break;
494	555
495	556	case APM_USER_SUSPEND:
496	557	case APM_SYS_SUSPEND:
497		- queue_event(event, NULL);
498		- if (suspends_pending == 0)
	558	+ ret = queue_suspend_event(event, NULL);
	559	+ if (ret < 0) {
	560	+ /*
	561	+ * We were busy. Try again in 50ms.
	562	+ */
	563	+ queue_add_event(&kapmd_queue, event);
	564	+ msleep(50);
	565	+ }
	566	+ if (ret > 0)
499	567	apm_suspend();
500	568	break;
501	569