Ejected Nook (usb mass storage) prevents suspend

Ejected Nook (usb mass storage) prevents suspend

[Andy Lutomirski]

This is kernel 3.9.9-302.fc19.x86_64.

I plugged in a BN Nook (a usb mass storage device), used it, and
ejected it.  This makes suspend fail:

[50135.265514] PM: Entering freeze sleep
[50135.265517] Suspending console(s) (use no_console_suspend to debug)
[50135.287724] sd 7:0:0:0: [sdb] Synchronizing SCSI cache
[50135.290413] sd 7:0:0:0: [sdb]
[50135.290415] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
[50135.290418] sd 7:0:0:0: [sdb]
[50135.290422] Sense Key : Not Ready [current]
[50135.290424] sd 7:0:0:0: [sdb]
[50135.290429] Add. Sense: Medium not present
[50135.290448] dpm_run_callback(): scsi_bus_suspend+0x0/0x40 returns -5
[50135.290454] PM: Device 7:0:0:0 failed to suspend async: error -5
[50138.486917] PM: Some devices failed to suspend
[50138.525007] PM: resume of devices complete after 38.132 msecs
[50138.525315] pci_pm_runtime_suspend():
hcd_pci_runtime_suspend+0x0/0x50 returns -16
[50138.536357] PM: Finishing wakeup.

Experimenting a bit, here's what happens.

 - Plug in device (so it's working).  Suspend works.
 - eject /dev/sdb.  Suspend fails.
 - Physically unplug the device.  Suspend works again.

This is actually a real issue -- my laptop can continue to power its
usb port while suspended, and the Nook is functional (and charges)
while plugged in but ejected, but I can't suspend my laptop so it can
charge my Nook.

Curiously, this issue seems to be Nook-specific.  I can't reproduce it
with a Corsair Flash Voyager.

--Andy

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Fri, 26 Jul 2013, Andy Lutomirski wrote:

> This is kernel 3.9.9-302.fc19.x86_64.
> 
> I plugged in a BN Nook (a usb mass storage device), used it, and
> ejected it.  This makes suspend fail:
> 
> [50135.265514] PM: Entering freeze sleep
> [50135.265517] Suspending console(s) (use no_console_suspend to debug)
> [50135.287724] sd 7:0:0:0: [sdb] Synchronizing SCSI cache
> [50135.290413] sd 7:0:0:0: [sdb]
> [50135.290415] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [50135.290418] sd 7:0:0:0: [sdb]
> [50135.290422] Sense Key : Not Ready [current]
> [50135.290424] sd 7:0:0:0: [sdb]
> [50135.290429] Add. Sense: Medium not present
> [50135.290448] dpm_run_callback(): scsi_bus_suspend+0x0/0x40 returns -5
> [50135.290454] PM: Device 7:0:0:0 failed to suspend async: error -5
> [50138.486917] PM: Some devices failed to suspend
> [50138.525007] PM: resume of devices complete after 38.132 msecs
> [50138.525315] pci_pm_runtime_suspend():
> hcd_pci_runtime_suspend+0x0/0x50 returns -16
> [50138.536357] PM: Finishing wakeup.

It looks like sd_sync_cache() should return immediately if no media is 
present or the media has been changed.

In addition, scsi_start_stop_device() should return immediately if no 
media is present.  Or at least, it shouldn't treat "medium not present" 
responses as errors.

Alan Stern

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Fri, 26 Jul 2013, Andy Lutomirski wrote:

> This is kernel 3.9.9-302.fc19.x86_64.
> 
> I plugged in a BN Nook (a usb mass storage device), used it, and
> ejected it.  This makes suspend fail:
> 
> [50135.265514] PM: Entering freeze sleep
> [50135.265517] Suspending console(s) (use no_console_suspend to debug)
> [50135.287724] sd 7:0:0:0: [sdb] Synchronizing SCSI cache
> [50135.290413] sd 7:0:0:0: [sdb]
> [50135.290415] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [50135.290418] sd 7:0:0:0: [sdb]
> [50135.290422] Sense Key : Not Ready [current]
> [50135.290424] sd 7:0:0:0: [sdb]
> [50135.290429] Add. Sense: Medium not present
> [50135.290448] dpm_run_callback(): scsi_bus_suspend+0x0/0x40 returns -5
> [50135.290454] PM: Device 7:0:0:0 failed to suspend async: error -5
> [50138.486917] PM: Some devices failed to suspend
> [50138.525007] PM: resume of devices complete after 38.132 msecs
> [50138.525315] pci_pm_runtime_suspend():
> hcd_pci_runtime_suspend+0x0/0x50 returns -16
> [50138.536357] PM: Finishing wakeup.

In addition to my earlier comment, the patch below should be applied.  
It will fix your immediate problem, although not in the best way.

Alan Stern



Index: usb-3.10/drivers/scsi/scsi_pm.c
===================================================================
--- usb-3.10.orig/drivers/scsi/scsi_pm.c
+++ usb-3.10/drivers/scsi/scsi_pm.c
@@ -48,8 +48,6 @@ static int scsi_dev_type_resume(struct d
 static int
 scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 {
-	int err = 0;

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Fri, 2013-07-26 at 16:31 -0400, Alan Stern wrote:

> In addition to my earlier comment, the patch below should be applied.  
> It will fix your immediate problem, although not in the best way.

Alan,

I think your diagnosis is correct, but not the fix.
This is run even in the runtime case. We might lose
data if the flush is not done.

	Regards
		Oliver

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Mon, 29 Jul 2013, Oliver Neukum wrote:

> On Fri, 2013-07-26 at 16:31 -0400, Alan Stern wrote:
> 
> > In addition to my earlier comment, the patch below should be applied.  
> > It will fix your immediate problem, although not in the best way.
> 
> Alan,
> 
> I think your diagnosis is correct, but not the fix.
> This is run even in the runtime case. We might lose
> data if the flush is not done.

Actually no, the scsi_bus_suspend_common() routine does not run during 
runtime PM.  It gets called only from scsi_bus_suspend(), 
scsi_bus_freeze(), and scsi_bus_poweroff(), which are all part of 
system sleep.

Alan Stern

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Mon, 2013-07-29 at 10:21 -0400, Alan Stern wrote:
> On Mon, 29 Jul 2013, Oliver Neukum wrote:
> 
> > On Fri, 2013-07-26 at 16:31 -0400, Alan Stern wrote:
> > 
> > > In addition to my earlier comment, the patch below should be applied.  
> > > It will fix your immediate problem, although not in the best way.
> > 
> > Alan,
> > 
> > I think your diagnosis is correct, but not the fix.
> > This is run even in the runtime case. We might lose
> > data if the flush is not done.
> 
> Actually no, the scsi_bus_suspend_common() routine does not run during 
> runtime PM.  It gets called only from scsi_bus_suspend(), 
> scsi_bus_freeze(), and scsi_bus_poweroff(), which are all part of 
> system sleep.

Well yes, but you handled only the system case that way. The runtime
case is still affected.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Tue, 30 Jul 2013, Oliver Neukum wrote:

> On Mon, 2013-07-29 at 10:21 -0400, Alan Stern wrote:
> > On Mon, 29 Jul 2013, Oliver Neukum wrote:
> > 
> > > On Fri, 2013-07-26 at 16:31 -0400, Alan Stern wrote:
> > > 
> > > > In addition to my earlier comment, the patch below should be applied.  
> > > > It will fix your immediate problem, although not in the best way.
> > > 
> > > Alan,
> > > 
> > > I think your diagnosis is correct, but not the fix.
> > > This is run even in the runtime case. We might lose
> > > data if the flush is not done.
> > 
> > Actually no, the scsi_bus_suspend_common() routine does not run during 
> > runtime PM.  It gets called only from scsi_bus_suspend(), 
> > scsi_bus_freeze(), and scsi_bus_poweroff(), which are all part of 
> > system sleep.
> 
> Well yes, but you handled only the system case that way. The runtime
> case is still affected.

That's why I said the patch would fix the immediate problem but it
wasn't the best solution.  You do agree that the patch is correct, as
far as it goes?

If you would like to handle the problems in sd.c concerning the
sync-cache and spin-down stuff, that will be fine with me.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Tue, 2013-07-30 at 13:00 -0400, Alan Stern wrote:

> That's why I said the patch would fix the immediate problem but it
> wasn't the best solution.  You do agree that the patch is correct, as
> far as it goes?

It will allow the system to sleep. But it seems to me that
a genuine error while flushing a drive's cache is one of the
few things actual worth aborting a system suspend for.

> If you would like to handle the problems in sd.c concerning the
> sync-cache and spin-down stuff, that will be fine with me.

Yes. The problem is not limited to USB.

	Regards
		Oliver

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 788 bytes --]

On Mon, 2013-07-29 at 10:21 -0400, Alan Stern wrote:
> On Mon, 29 Jul 2013, Oliver Neukum wrote:
> 
> > On Fri, 2013-07-26 at 16:31 -0400, Alan Stern wrote:
> > 
> > > In addition to my earlier comment, the patch below should be applied.  
> > > It will fix your immediate problem, although not in the best way.
> > 
> > Alan,
> > 
> > I think your diagnosis is correct, but not the fix.
> > This is run even in the runtime case. We might lose
> > data if the flush is not done.
> 
> Actually no, the scsi_bus_suspend_common() routine does not run during 
> runtime PM.  It gets called only from scsi_bus_suspend(), 
> scsi_bus_freeze(), and scsi_bus_poweroff(), which are all part of 
> system sleep.


These errors should be handled cleanly. How about this patch?

	Regards
		Oliver




[-- Attachment #2: 0001-sd-handle-errors-during-suspend.patch --]
[-- Type: text/x-patch, Size: 4347 bytes --]

>From 76a377d9894dc8945e9afecc7f9864e6dc3156b1 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.de>
Date: Wed, 31 Jul 2013 13:32:51 +0200
Subject: [PATCH] sd: handle errors during suspend

Errors during suspend must be handled according to reasons
Errors due to missing media or unplugged devices must be ignored.
The error returns must be modified so that the generic layer
understands them.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/scsi/scsi_pm.c |  3 ++-
 drivers/scsi/sd.c      | 61 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 4c5aabe..af4c050 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -54,7 +54,8 @@ scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 		/*
 		 * All the high-level SCSI drivers that implement runtime
 		 * PM treat runtime suspend, system suspend, and system
-		 * hibernate identically.
+		 * hibernate nearly identically. In all cases the requirements
+		 * for runtime suspension are stricter.
 		 */
 		if (pm_runtime_suspended(dev))
 			return 0;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 86fcf2c..76273f4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -105,7 +105,8 @@ static void sd_unlock_native_capacity(struct gendisk *disk);
 static int  sd_probe(struct device *);
 static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
-static int sd_suspend(struct device *);
+static int sd_suspend_system(struct device *);
+static int sd_suspend_runtime(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
 static int sd_done(struct scsi_cmnd *);
@@ -483,11 +484,11 @@ static struct class sd_disk_class = {
 };
 
 static const struct dev_pm_ops sd_pm_ops = {
-	.suspend		= sd_suspend,
+	.suspend		= sd_suspend_system,
 	.resume			= sd_resume,
-	.poweroff		= sd_suspend,
+	.poweroff		= sd_suspend_system,
 	.restore		= sd_resume,
-	.runtime_suspend	= sd_suspend,
+	.runtime_suspend	= sd_suspend_runtime,
 	.runtime_resume		= sd_resume,
 };
 
@@ -1455,12 +1456,31 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 	if (res) {
 		sd_print_result(sdkp, res);
-		if (driver_byte(res) & DRIVER_SENSE)
+
+		if (driver_byte(res) & DRIVER_SENSE) 
 			sd_print_sense_hdr(sdkp, &sshdr);
+		/* we need to evaluate the error return  */
+		if ((scsi_sense_valid(&sshdr) &&
+			/* 0x3a is medium not present */
+			sshdr.asc == 0x3a))
+				/* this is no error here */
+				return 0;
+
+		switch (host_byte(res)) {
+		/* ignore errors due to racing a disconnection */
+		case DID_BAD_TARGET:
+		case DID_NO_CONNECT:
+			return 0;
+		/* signal the upper layer it might try again */
+		case DID_BUS_BUSY:
+		case DID_IMM_RETRY:
+		case DID_REQUEUE:
+		case DID_SOFT_ERROR:
+			return -EBUSY;
+		default:
+			return -EIO;
+		}
 	}
-
-	if (res)
-		return -EIO;
 	return 0;
 }
 
@@ -3062,9 +3082,17 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 		sd_print_result(sdkp, res);
 		if (driver_byte(res) & DRIVER_SENSE)
 			sd_print_sense_hdr(sdkp, &sshdr);
+		if ((scsi_sense_valid(&sshdr) &&
+			/* 0x3a is medium not present */
+			sshdr.asc == 0x3a))
+			res = 0;
 	}
 
-	return res;
+	/* SCSI error codes must not go to the generic layer */
+	if (res)
+		return -EIO;
+
+	return 0;
 }
 
 /*
@@ -3096,7 +3124,7 @@ exit:
 	scsi_disk_put(sdkp);
 }
 
-static int sd_suspend(struct device *dev)
+static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
@@ -3113,7 +3141,10 @@ static int sd_suspend(struct device *dev)
 
 	if (sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
+		/* an error is not worth aborting a system sleep */
 		ret = sd_start_stop_device(sdkp, 0);
+		if (ignore_stop_errors)
+			ret = 0;
 	}
 
 done:
@@ -3121,6 +3152,16 @@ done:
 	return ret;
 }
 
+static int sd_suspend_system(struct device *dev)
+{
+	return sd_suspend_common(dev, true);
+}
+
+static int sd_suspend_runtime(struct device *dev)
+{
+        return sd_suspend_common(dev, false);
+}
+
 static int sd_resume(struct device *dev)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
-- 
1.8.3.1

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Wed, 31 Jul 2013, Oliver Neukum wrote:

> These errors should be handled cleanly. How about this patch?

> From 76a377d9894dc8945e9afecc7f9864e6dc3156b1 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.de>
> Date: Wed, 31 Jul 2013 13:32:51 +0200
> Subject: [PATCH] sd: handle errors during suspend
> 
> Errors during suspend must be handled according to reasons

You forgot to complete this sentence.

> Errors due to missing media or unplugged devices must be ignored.
> The error returns must be modified so that the generic layer
> understands them.

The patch contains a couple of trivial whitespace errors (space added
to the end of a line, or spaces used instead of a tab).

More importantly, if we already know that the medium is not present or
has been changed since it was last used, then there's no reason to call
sd_sync_cache() at all.

Alan Stern

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:

> More importantly, if we already know that the medium is not present or
> has been changed since it was last used, then there's no reason to call
> sd_sync_cache() at all.

Like this?

	Regards
		Oliver




[-- Attachment #2: 0001-sd-error-handling-during-flushing-caches.patch --]
[-- Type: text/x-patch, Size: 4958 bytes --]

>From 8c90d860652aa99e6e60d9b32bc3aa8d4db9efa5 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.de>
Date: Thu, 1 Aug 2013 10:08:20 +0200
Subject: [PATCH] sd: error handling during flushing caches

It makes no sense to flush the cache of a device without medium.
Errors during suspend must be handled according to their causes.
Errors due to missing media or unplugged devices must be ignored.
Errors due to devices being offlined must also be ignored.
The error returns must be modified so that the generic layer
understands them.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/scsi/scsi_pm.c |  3 ++-
 drivers/scsi/sd.c      | 69 ++++++++++++++++++++++++++++++++++++++++++--------
 2 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 4c5aabe..af4c050 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -54,7 +54,8 @@ scsi_bus_suspend_common(struct device *dev, int (*cb)(struct device *))
 		/*
 		 * All the high-level SCSI drivers that implement runtime
 		 * PM treat runtime suspend, system suspend, and system
-		 * hibernate identically.
+		 * hibernate nearly identically. In all cases the requirements
+		 * for runtime suspension are stricter.
 		 */
 		if (pm_runtime_suspended(dev))
 			return 0;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 86fcf2c..3c7f918 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -105,7 +105,8 @@ static void sd_unlock_native_capacity(struct gendisk *disk);
 static int  sd_probe(struct device *);
 static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
-static int sd_suspend(struct device *);
+static int sd_suspend_system(struct device *);
+static int sd_suspend_runtime(struct device *);
 static int sd_resume(struct device *);
 static void sd_rescan(struct device *);
 static int sd_done(struct scsi_cmnd *);
@@ -483,11 +484,11 @@ static struct class sd_disk_class = {
 };
 
 static const struct dev_pm_ops sd_pm_ops = {
-	.suspend		= sd_suspend,
+	.suspend		= sd_suspend_system,
 	.resume			= sd_resume,
-	.poweroff		= sd_suspend,
+	.poweroff		= sd_suspend_system,
 	.restore		= sd_resume,
-	.runtime_suspend	= sd_suspend,
+	.runtime_suspend	= sd_suspend_runtime,
 	.runtime_resume		= sd_resume,
 };
 
@@ -1437,6 +1438,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 	if (!scsi_device_online(sdp))
 		return -ENODEV;
 
+	if (!sdkp->media_present)
+		return 0;
 
 	for (retries = 3; retries > 0; --retries) {
 		unsigned char cmd[10] = { 0 };
@@ -1455,12 +1458,31 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 	if (res) {
 		sd_print_result(sdkp, res);
-		if (driver_byte(res) & DRIVER_SENSE)
+
+		if (driver_byte(res) & DRIVER_SENSE) 
 			sd_print_sense_hdr(sdkp, &sshdr);
+		/* we need to evaluate the error return  */
+		if ((scsi_sense_valid(&sshdr) &&
+			/* 0x3a is medium not present */
+			sshdr.asc == 0x3a))
+				/* this is no error here */
+				return 0;
+
+		switch (host_byte(res)) {
+		/* ignore errors due to racing a disconnection */
+		case DID_BAD_TARGET:
+		case DID_NO_CONNECT:
+			return 0;
+		/* signal the upper layer it might try again */
+		case DID_BUS_BUSY:
+		case DID_IMM_RETRY:
+		case DID_REQUEUE:
+		case DID_SOFT_ERROR:
+			return -EBUSY;
+		default:
+			return -EIO;
+		}
 	}
-
-	if (res)
-		return -EIO;
 	return 0;
 }
 
@@ -3062,9 +3084,17 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 		sd_print_result(sdkp, res);
 		if (driver_byte(res) & DRIVER_SENSE)
 			sd_print_sense_hdr(sdkp, &sshdr);
+		if ((scsi_sense_valid(&sshdr) &&
+			/* 0x3a is medium not present */
+			sshdr.asc == 0x3a))
+			res = 0;
 	}
 
-	return res;
+	/* SCSI error codes must not go to the generic layer */
+	if (res)
+		return -EIO;
+
+	return 0;
 }
 
 /*
@@ -3096,7 +3126,7 @@ exit:
 	scsi_disk_put(sdkp);
 }
 
-static int sd_suspend(struct device *dev)
+static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
@@ -3107,13 +3137,20 @@ static int sd_suspend(struct device *dev)
 	if (sdkp->WCE) {
 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
 		ret = sd_sync_cache(sdkp);
-		if (ret)
+		if (ret) {
+			/* ignore OFFLINE device */
+			if (ret == -ENODEV)
+				ret = 0;
 			goto done;
+		}
 	}
 
 	if (sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
+		/* an error is not worth aborting a system sleep */
 		ret = sd_start_stop_device(sdkp, 0);
+		if (ignore_stop_errors)
+			ret = 0;
 	}
 
 done:
@@ -3121,6 +3158,16 @@ done:
 	return ret;
 }
 
+static int sd_suspend_system(struct device *dev)
+{
+	return sd_suspend_common(dev, true);
+}
+
+static int sd_suspend_runtime(struct device *dev)
+{
+	return sd_suspend_common(dev, false);
+}
+
 static int sd_resume(struct device *dev)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
-- 
1.8.3.1

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Thu, 1 Aug 2013, Oliver Neukum wrote:

> On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
> 
> > More importantly, if we already know that the medium is not present or
> > has been changed since it was last used, then there's no reason to call
> > sd_sync_cache() at all.
> 
> Like this?

Yes, I like this a lot better, except I would put the test for 
!sdkp->media_present in sd_suspend_common() -- no need to print the 
"Synchronizing SCSI Cache" message if you're not going to go through 
with it.

What do the SCSI people think?

Alan Stern

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Thu, 2013-08-01 at 11:53 -0400, Alan Stern wrote:
> On Thu, 1 Aug 2013, Oliver Neukum wrote:
> 
> > On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
> > 
> > > More importantly, if we already know that the medium is not present or
> > > has been changed since it was last used, then there's no reason to call
> > > sd_sync_cache() at all.
> > 
> > Like this?
> 
> Yes, I like this a lot better, except I would put the test for 
> !sdkp->media_present in sd_suspend_common() -- no need to print the

But your observation that it makes no sense while no medium is present
is valid whatever be the reason for wanting to flush.

> "Synchronizing SCSI Cache" message if you're not going to go through 
> with it.
> 
> What do the SCSI people think?

I don't know. I would like Andy to test before I ask.

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Fri, 2 Aug 2013, Oliver Neukum wrote:

> On Thu, 2013-08-01 at 11:53 -0400, Alan Stern wrote:
> > On Thu, 1 Aug 2013, Oliver Neukum wrote:
> > 
> > > On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
> > > 
> > > > More importantly, if we already know that the medium is not present or
> > > > has been changed since it was last used, then there's no reason to call
> > > > sd_sync_cache() at all.
> > > 
> > > Like this?
> > 
> > Yes, I like this a lot better, except I would put the test for 
> > !sdkp->media_present in sd_suspend_common() -- no need to print the
> 
> But your observation that it makes no sense while no medium is present
> is valid whatever be the reason for wanting to flush.

So do the test in both places.

Alan Stern

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Fri, 2013-08-02 at 09:54 -0400, Alan Stern wrote:
> On Fri, 2 Aug 2013, Oliver Neukum wrote:
> 
> > On Thu, 2013-08-01 at 11:53 -0400, Alan Stern wrote:
> > > On Thu, 1 Aug 2013, Oliver Neukum wrote:
> > > 
> > > > On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
> > > > 
> > > > > More importantly, if we already know that the medium is not present or
> > > > > has been changed since it was last used, then there's no reason to call
> > > > > sd_sync_cache() at all.
> > > > 
> > > > Like this?
> > > 
> > > Yes, I like this a lot better, except I would put the test for 
> > > !sdkp->media_present in sd_suspend_common() -- no need to print the
> > 
> > But your observation that it makes no sense while no medium is present
> > is valid whatever be the reason for wanting to flush.
> 
> So do the test in both places.

Code duplication for what reasons?

	Regards
		Oliver



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Alan Stern]

On Fri, 2 Aug 2013, Oliver Neukum wrote:

> On Fri, 2013-08-02 at 09:54 -0400, Alan Stern wrote:
> > On Fri, 2 Aug 2013, Oliver Neukum wrote:
> > 
> > > On Thu, 2013-08-01 at 11:53 -0400, Alan Stern wrote:
> > > > On Thu, 1 Aug 2013, Oliver Neukum wrote:
> > > > 
> > > > > On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
> > > > > 
> > > > > > More importantly, if we already know that the medium is not present or
> > > > > > has been changed since it was last used, then there's no reason to call
> > > > > > sd_sync_cache() at all.
> > > > > 
> > > > > Like this?
> > > > 
> > > > Yes, I like this a lot better, except I would put the test for 
> > > > !sdkp->media_present in sd_suspend_common() -- no need to print the
> > > 
> > > But your observation that it makes no sense while no medium is present
> > > is valid whatever be the reason for wanting to flush.
> > 
> > So do the test in both places.
> 
> Code duplication for what reasons?

Like I said before, to avoid printing a misleading line in the kernel 
log.

If you prefer, just get rid of the log message.  Or make it KERN_DEBUG 
instead of KERN_NOTICE, along with the "Stopping disk" message.  Those 
two things might get pretty annoying when people start using 
block-layer runtime PM.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Andy Lutomirski]

What's the status of this?  Do I still need to test it?

I won't have access to the Nook that triggered it the first time for a
couple weeks, but I could try to find another device like that.

--Andy

On Fri, Aug 2, 2013 at 7:18 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote:
> On Fri, 2 Aug 2013, Oliver Neukum wrote:
>
>> On Fri, 2013-08-02 at 09:54 -0400, Alan Stern wrote:
>> > On Fri, 2 Aug 2013, Oliver Neukum wrote:
>> >
>> > > On Thu, 2013-08-01 at 11:53 -0400, Alan Stern wrote:
>> > > > On Thu, 1 Aug 2013, Oliver Neukum wrote:
>> > > >
>> > > > > On Wed, 2013-07-31 at 11:13 -0400, Alan Stern wrote:
>> > > > >
>> > > > > > More importantly, if we already know that the medium is not present or
>> > > > > > has been changed since it was last used, then there's no reason to call
>> > > > > > sd_sync_cache() at all.
>> > > > >
>> > > > > Like this?
>> > > >
>> > > > Yes, I like this a lot better, except I would put the test for
>> > > > !sdkp->media_present in sd_suspend_common() -- no need to print the
>> > >
>> > > But your observation that it makes no sense while no medium is present
>> > > is valid whatever be the reason for wanting to flush.
>> >
>> > So do the test in both places.
>>
>> Code duplication for what reasons?
>
> Like I said before, to avoid printing a misleading line in the kernel
> log.
>
> If you prefer, just get rid of the log message.  Or make it KERN_DEBUG
> instead of KERN_NOTICE, along with the "Stopping disk" message.  Those
> two things might get pretty annoying when people start using
> block-layer runtime PM.
>
> Alan Stern
>



-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Fri, 2013-09-13 at 06:37 -0700, Andy Lutomirski wrote:
> What's the status of this?  Do I still need to test it?
> 
> I won't have access to the Nook that triggered it the first time for a
> couple weeks, but I could try to find another device like that.

It works for me.
I'll push upstream in a form that Alan likes.

	Regards
		Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Ejected Nook (usb mass storage) prevents suspend

[Oliver Neukum]

On Fri, 2013-07-26 at 09:54 -0700, Andy Lutomirski wrote:
> This is kernel 3.9.9-302.fc19.x86_64.
> 
> I plugged in a BN Nook (a usb mass storage device), used it, and
> ejected it.  This makes suspend fail:
> 
> [50135.265514] PM: Entering freeze sleep
> [50135.265517] Suspending console(s) (use no_console_suspend to debug)
> [50135.287724] sd 7:0:0:0: [sdb] Synchronizing SCSI cache
> [50135.290413] sd 7:0:0:0: [sdb]
> [50135.290415] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
> [50135.290418] sd 7:0:0:0: [sdb]
> [50135.290422] Sense Key : Not Ready [current]
> [50135.290424] sd 7:0:0:0: [sdb]
> [50135.290429] Add. Sense: Medium not present
> [50135.290448] dpm_run_callback(): scsi_bus_suspend+0x0/0x40 returns -5
> [50135.290454] PM: Device 7:0:0:0 failed to suspend async: error -5
> [50138.486917] PM: Some devices failed to suspend
> [50138.525007] PM: resume of devices complete after 38.132 msecs
> [50138.525315] pci_pm_runtime_suspend():
> hcd_pci_runtime_suspend+0x0/0x50 returns -16
> [50138.536357] PM: Finishing wakeup.
> 
> Experimenting a bit, here's what happens.
> 
>  - Plug in device (so it's working).  Suspend works.
>  - eject /dev/sdb.  Suspend fails.
>  - Physically unplug the device.  Suspend works again.

Hi,

this looks like a logical error in sd_suspend(). It should
ignore errors due to missing media (or offlined devices)

	Regards
		Oliver