USB not processing APM suspend event properly?

USB not processing APM suspend event properly?

[Thomas Hood]

The following syslog fragment shows what happens when
I suspended earlier today.  The USB subsystem disconnected
my USB mouse, but then tried (twice) to reconnect it again
while the apm driver was still processing the suspend.  The
attempts to reconnect failed.  I presume there is something
wrong with this picture.  Does this indicate a bug in
USB power management?

(The repeated "received system suspend notify" messages are
there because I have apm debug messages switch on, and I
am using a ThinkPad, which generates repeated suspend
events.)

Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: usb.c: USB disconnect on device 4
Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: hub.c: USB new device connect on
bus1/1, assigned device number 5
Dec  9 17:38:28 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:30 thanatos last message repeated 2 times
Dec  9 17:38:30 thanatos kernel: usb_control/bulk_msg: timeout
Dec  9 17:38:30 thanatos kernel: usb.c: USB device not accepting new
address=5 (error=-110)
Dec  9 17:38:31 thanatos kernel: hub.c: USB new device connect on
bus1/1, assigned device number 6
Dec  9 17:38:31 thanatos kernel: apm: setting state busy
Dec  9 17:38:31 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:33 thanatos last message repeated 2 times
Dec  9 17:38:34 thanatos kernel: usb_control/bulk_msg: timeout
Dec  9 17:38:34 thanatos kernel: usb.c: USB device not accepting new
address=6 (error=-110)
Dec  9 17:38:34 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:35 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:36 thanatos kernel: apm: setting state busy

USB not processing APM suspend event properly?

[Thomas Hood]

The following syslog fragment shows what happens when I
suspended a while ago.  The USB subsystem disconnected
my USB mouse, but then tried (twice) to reconnect it again
while the apm driver was still processing the suspend.  The
attempts to reconnect failed.  I presume there is something
wrong with this picture.  Does this indicate a bug in
USB power management?

(The repeated "received system suspend notify" messages are
there because I have apm debug messages switch on, and I
am using a ThinkPad, which generates repeated suspend
events.)

Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: usb.c: USB disconnect on device 4
Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:27 thanatos kernel: hub.c: USB new device connect on
bus1/1, assigned device number 5
Dec  9 17:38:28 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:30 thanatos last message repeated 2 times
Dec  9 17:38:30 thanatos kernel: usb_control/bulk_msg: timeout
Dec  9 17:38:30 thanatos kernel: usb.c: USB device not accepting new
address=5 (error=-110)
Dec  9 17:38:31 thanatos kernel: hub.c: USB new device connect on
bus1/1, assigned device number 6
Dec  9 17:38:31 thanatos kernel: apm: setting state busy
Dec  9 17:38:31 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:33 thanatos last message repeated 2 times
Dec  9 17:38:34 thanatos kernel: usb_control/bulk_msg: timeout
Dec  9 17:38:34 thanatos kernel: usb.c: USB device not accepting new
address=6 (error=-110)
Dec  9 17:38:34 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:35 thanatos kernel: apm: received system suspend notify
Dec  9 17:38:36 thanatos kernel: apm: setting state busy

Re: USB not processing APM suspend event properly?

[Alan Cox]

> my USB mouse, but then tried (twice) to reconnect it again
> while the apm driver was still processing the suspend.  The
> attempts to reconnect failed.  I presume there is something
> wrong with this picture.  Does this indicate a bug in
> USB power management?

More it indicates a bug in the APM code. Have a look at Russell King's
patches which actually issue the APM and user mode events in the right
order. There is a USB problem there too, but the USB problem can't be fixed
sanely until the APM order is fixed

Re: USB not processing APM suspend event properly?

[Russell King]

On Wed, Dec 12, 2001 at 10:41:33PM +0000, Alan Cox wrote:
> > my USB mouse, but then tried (twice) to reconnect it again
> > while the apm driver was still processing the suspend.  The
> > attempts to reconnect failed.  I presume there is something
> > wrong with this picture.  Does this indicate a bug in
> > USB power management?
> 
> More it indicates a bug in the APM code. Have a look at Russell King's
> patches which actually issue the APM and user mode events in the right
> order. There is a USB problem there too, but the USB problem can't be fixed
> sanely until the APM order is fixed

And for those who have an interest, here's the patch...  It might be
outdated a little now though.  It came out of a problem I had on my
laptop, where the machine drove itself into total battery exhaustion
because Linux wouldn't let the APM bios suspend/hibernate the machine.

This was caused by the PCMCIA network interface being off, and the apmd
scripts trying to fuser -k and unmount an in-use NFS partition, which
in turn wanted to generate NFS traffic to the server over the shut down
PCMCIA network interface...

--------------------- original mail follows --------------------

Basically, I've changed the order that things happen on suspend, such that
we always pass the event to apmd, and any other listeners on /dev/apm_bios.
Once everyone has replied (subject to the rules of course), it then sends
the PM_SUSPEND to the devices.

The original code sent PM_SUSPEND on receiving the original request, and
then multiple times each time a listener on /dev/apm_bios replied.  Not
good if apmd wants to unmount NFS, and your NFS mounts require traffic
over your PCMCIA ether card!


--- orig/arch/i386/kernel/apm.c	Wed Nov  7 14:46:35 2001
+++ linux/arch/i386/kernel/apm.c	Wed Nov  7 15:01:22 2001
@@ -1133,40 +1133,21 @@
 #endif
 }
 
-static int send_event(apm_event_t event)
-{
-	switch (event) {
-	case APM_SYS_SUSPEND:
-	case APM_CRITICAL_SUSPEND:
-	case APM_USER_SUSPEND:
-		/* map all suspends to ACPI D3 */
-		if (pm_send_all(PM_SUSPEND, (void *)3)) {
-			if (event == APM_CRITICAL_SUSPEND) {
-				printk(KERN_CRIT
-					"apm: Critical suspend was vetoed, "
-					"expect armageddon\n" );
-				return 0;
-			}
-			if (apm_info.connection_version > 0x100)
-				apm_set_power_state(APM_STATE_REJECT);
-			return 0;
-		}
-		break;
-	case APM_NORMAL_RESUME:
-	case APM_CRITICAL_RESUME:
-		/* map all resumes to ACPI D0 */
-		(void) pm_send_all(PM_RESUME, (void *)0);
-		break;
-	}
-
-	return 1;
-}
-
-static int suspend(void)
+static int suspend(int vetoable)
 {
 	int		err;
 	struct apm_user	*as;
 
+	if (pm_send_all(PM_SUSPEND, (void *)3)) {
+		if (!vetoable) {
+			printk(KERN_CRIT
+			       "apm: suspend was vetoed, expect armageddon\n");
+		} else if (apm_info.connection_version > 0x100) {
+			apm_set_power_state(APM_STATE_REJECT);
+			return -EIO;
+		}
+	}
+
 	get_time_diff();
 	cli();
 	err = apm_set_power_state(APM_STATE_SUSPEND);
@@ -1176,12 +1157,15 @@
 		err = APM_SUCCESS;
 	if (err != APM_SUCCESS)
 		apm_error("suspend", err);
-	send_event(APM_NORMAL_RESUME);
 	sti();
+
+	pm_send_all(PM_RESUME, (void *)0);
+
 	queue_event(APM_NORMAL_RESUME, NULL);
+	err = (err == APM_SUCCESS) ? 0 : -EIO;
 	for (as = user_list; as != NULL; as = as->next) {
 		as->suspend_wait = 0;
-		as->suspend_result = ((err == APM_SUCCESS) ? 0 : -EIO);
+		as->suspend_result = err;
 	}
 	ignore_normal_resume = 1;
 	wake_up_interruptible(&apm_suspend_waitqueue);
@@ -1241,11 +1225,9 @@
 		switch (event) {
 		case APM_SYS_STANDBY:
 		case APM_USER_STANDBY:
-			if (send_event(event)) {
-				queue_event(event, NULL);
-				if (standbys_pending <= 0)
-					standby();
-			}
+			queue_event(event, NULL);
+			if (standbys_pending <= 0)
+				standby();
 			break;
 
 		case APM_USER_SUSPEND:
@@ -1270,12 +1252,10 @@
 			 */
 			if (waiting_for_resume)
 				return;
-			if (send_event(event)) {
-				queue_event(event, NULL);
-				waiting_for_resume = 1;
-				if (suspends_pending <= 0)
-					(void) suspend();
-			}
+			queue_event(event, NULL);
+			waiting_for_resume = 1;
+			if (suspends_pending <= 0)
+				(void) suspend(1);
 			break;
 
 		case APM_NORMAL_RESUME:
@@ -1287,7 +1267,7 @@
 			if ((event != APM_NORMAL_RESUME)
 			    || (ignore_normal_resume == 0)) {
 				set_time();
-				send_event(event);
+				pm_send_all(PM_RESUME, (void *)0);
 				queue_event(event, NULL);
 			}
 			break;
@@ -1295,7 +1275,6 @@
 		case APM_CAPABILITY_CHANGE:
 		case APM_LOW_BATTERY:
 		case APM_POWER_STATUS_CHANGE:
-			send_event(event);
 			queue_event(event, NULL);
 			break;
 
@@ -1304,12 +1283,11 @@
 			break;
 
 		case APM_CRITICAL_SUSPEND:
-			send_event(event);
 			/*
 			 * We can only hope it worked - we are not allowed
 			 * to reject a critical suspend.
 			 */
-			(void) suspend();
+			(void) suspend(0);
 			break;
 		}
 	}
@@ -1479,9 +1457,7 @@
 			as->standbys_read--;
 			as->standbys_pending--;
 			standbys_pending--;
-		} else if (!send_event(APM_USER_STANDBY))
-			return -EAGAIN;
-		else
+		} else
 			queue_event(APM_USER_STANDBY, as);
 		if (standbys_pending <= 0)
 			standby();
@@ -1491,13 +1467,10 @@
 			as->suspends_read--;
 			as->suspends_pending--;
 			suspends_pending--;
-		} else if (!send_event(APM_USER_SUSPEND))
-			return -EAGAIN;
-		else
+		} else
 			queue_event(APM_USER_SUSPEND, as);
 		if (suspends_pending <= 0) {
-			if (suspend() != APM_SUCCESS)
-				return -EIO;
+			return suspend(1);
 		} else {
 			as->suspend_wait = 1;
 			wait_event_interruptible(apm_suspend_waitqueue,
@@ -1528,7 +1501,7 @@
 	if (as->suspends_pending > 0) {
 		suspends_pending -= as->suspends_pending;
 		if (suspends_pending <= 0)
-			(void) suspend();
+			(void) suspend(1);
 	}
 	if (user_list == as)
 		user_list = as->next;


--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: USB not processing APM suspend event properly?

[Thomas Hood]

> This was caused by the PCMCIA network interface being
> off, and the apmd scripts trying to fuser -k and
> unmount an in-use NFS partition, which in turn wanted
> to generate NFS traffic to the server over the shut
> down PCMCIA network interface...

So your problem was that PCMCIA got shut down too early.

> Basically, I've changed the order that things happen on
> suspend, such that we always pass the event to apmd, and
> any other listeners on /dev/apm_bios.  Once everyone has
> replied (subject to the rules of course), it then sends
> the PM_SUSPEND to the devices.

I suppose that the reason why it is presently coded as
it is is that the drivers can veto the suspend whereas
the "listeners" can't, and the apm driver wants to tell
the listeners about the event only if it isn't vetoed.
With your change, the apm driver really should ignore
a veto from the drivers since it has already told the
listeners about the event.  Therefore, while your patch
is an improvement, it still isn't the whole solution.

One possible solution is for the apm driver first to _ask_
the device drivers whether a suspend is allowed; then
to tell the listeners; then to tell the device drivers
to suspend.  This solution could be implemented by adding
the "ask" function to each driver with pm support.  The
driver's opportunity to veto the suspend would be when
it is asked, not when it is told to suspend.

> The original code sent PM_SUSPEND on receiving the original
> request, and then multiple times each time a listener on
> /dev/apm_bios replied.  Not good if apmd wants to unmount
> NFS, and your NFS mounts require traffic over your PCMCIA
> ether card!

Actually, it's not true that the original code sends PM_SUSPEND
each time a listener on /dev/apm_bios replies.  The code 
fragment is:
                if (as->suspends_read > 0) {
                        as->suspends_read--;
                        as->suspends_pending--;
                        suspends_pending--;
                } else if (!send_event(APM_USER_SUSPEND))
                        return -EAGAIN;
                else
                        queue_event(APM_USER_SUSPEND, as);
This only calls send_event() if the ioctl(suspend) is NOT a
reply to a notification that was earlier read from the queue.

Re: USB not processing APM suspend event properly?

[Russell King]

On Wed, Dec 12, 2001 at 06:40:26PM -0500, Thomas Hood wrote:
> I suppose that the reason why it is presently coded as
> it is is that the drivers can veto the suspend whereas
> the "listeners" can't, and the apm driver wants to tell
> the listeners about the event only if it isn't vetoed.
> With your change, the apm driver really should ignore
> a veto from the drivers since it has already told the
> listeners about the event.  Therefore, while your patch
> is an improvement, it still isn't the whole solution.

I think it does perform acceptable behaviour however - if the drivers
(in the unlikely event) refuse the event after we've told user space,
we tell user space we resumed.

> One possible solution is for the apm driver first to _ask_
> the device drivers whether a suspend is allowed; then
> to tell the listeners; then to tell the device drivers
> to suspend.

What if the condition of the suspend gets changed by the act of paging
in memory, or some other change that occured in user space?  It's a
chicken and egg problem. 8(

> Actually, it's not true that the original code sends PM_SUSPEND
> each time a listener on /dev/apm_bios replies.  The code 
> fragment is:
>                 if (as->suspends_read > 0) {
>                         as->suspends_read--;
>                         as->suspends_pending--;
>                         suspends_pending--;
>                 } else if (!send_event(APM_USER_SUSPEND))
>                         return -EAGAIN;
>                 else
>                         queue_event(APM_USER_SUSPEND, as);
> This only calls send_event() if the ioctl(suspend) is NOT a
> reply to a notification that was earlier read from the queue.

It is a minor point, but an unwanted one that just obfuscates the operation
of the APM system - only the first PM SUSPEND event causes a change.

Sending 'n' PM_SUSPEND events because you have 'n' listeners on
/dev/apm_bios seems to be a waste of time when it could be handled
more cleanly.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: USB not processing APM suspend event properly?

[Thomas Hood]

> Sending 'n' PM_SUSPEND events because you have 'n' listeners
> on /dev/apm_bios seems to be a waste of time when it could be
> handled more cleanly.

But do you agree that the present code does NOT do this?

The present code does not send 'n' events---only one.

Re: USB not processing APM suspend event properly?

[Russell King]

On Wed, Dec 12, 2001 at 08:03:48PM -0500, Thomas Hood wrote:
> But do you agree that the present code does NOT do this?
> 
> The present code does not send 'n' events---only one.

Ok, thinking about this obfuscated code more, it would appear so.  It
would also appear that when the suspend request comes from the APM bios,
the ioctl() method will not call send_event() at all - instead it comes
from check_events().

However, as I said previously, this is a minor issue.  I'd rather the
major problem was fixed.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: USB not processing APM suspend event properly?

[Thomas Hood]

On Thu, 2001-12-13 at 05:36, Russell King wrote:
> On Wed, Dec 12, 2001 at 08:03:48PM -0500, Thomas Hood wrote:
> > But do you agree that the present code does NOT do this?
>
> Ok, thinking about this obfuscated code more, it would appear so.  It
> would also appear that when the suspend request comes from the APM bios,
> the ioctl() method will not call send_event() at all - instead it comes
> from check_events().

Yes.

> However, as I said previously, this is a minor issue.  I'd rather the
> major problem was fixed.

I agree entirely.  I think that this change should be made.
The question is 'When?'.  Is this too big a change to make
in 2.4?

Thomas