Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Rafael J. Wysocki]

Hi,

Added some CCs.

On Thursday, May 12, 2011, Charles Hannum wrote:
> Short version: My laptop doesn't suspend when my Android phone is
> connected and has been “ejected”.
> 
> Long version:
> 
> Android phones connect as USB mass storage devices.  After the “Turn
> on USB storage” button has been clicked, there are a few different
> ways to detach the “disk”:
> 
> 1) pull the cable
> 2) click “Turn off USB storage”
> 3) “eject” the device
> 
> In cases 2 & 3, the USB device is still attached to the system, but
> will now return MEDIUM NOT PRESENT for many commands, including
> SYNCHRONIZE CACHE—basically it acts like any device with removable
> media.  However, the act of the “media” being removed does not
> invalidate sdkp->WCE; therefore sd_shutdown() and sd_suspend() still
> call sd_sync_cache(), which *fails* because it gets a MEDIUM NOT
> PRESENT sense code.  In the sd_suspend() case, this causes the entire
> suspend to fail, and the laptop rewakes immediately.
> 
> There are a few different ways to fix this; e.g. one could
> specifically test media_not_present() if a sense code is returned in
> sd_sync_cache().  However, the following patch seems simpler, and
> avoids calling sd_sync_cache() at all in this case.  sdkp->WCE will be
> reset when new medium is recognized and sd_read_cache_type() is
> called.  Note this code always gets called—it's in the same path as
> sd_read_capacity(), which has to be called for the device to be usable
> again; thus the patch is inherently safe.
> 
> Kernel tested: 2.6.38 (Ubuntu Natty)

Patch appended for completness.

I need someone from USB/SCSI camp to see if this approach makes sense.

Thanks,
Rafael


--- linux-2.6.38/drivers/scsi/sd.c.orig	2011-05-11 18:08:44.969497976 -0400
+++ linux-2.6.38/drivers/scsi/sd.c	2011-05-11 15:26:15.439610760 -0400
@@ -996,6 +996,7 @@
 	if (sdkp->device->removable) {
 		sdkp->media_present = 0;
 		sdkp->capacity = 0;
+		sdkp->WCE = 0;
 	}
 }
 

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[James Bottomley]

On Thu, 2011-05-12 at 22:03 +0200, Rafael J. Wysocki wrote:
> Hi,
> 
> Added some CCs.
> 
> On Thursday, May 12, 2011, Charles Hannum wrote:
> > Short version: My laptop doesn't suspend when my Android phone is
> > connected and has been “ejected”.
> > 
> > Long version:
> > 
> > Android phones connect as USB mass storage devices.  After the “Turn
> > on USB storage” button has been clicked, there are a few different
> > ways to detach the “disk”:
> > 
> > 1) pull the cable
> > 2) click “Turn off USB storage”
> > 3) “eject” the device
> > 
> > In cases 2 & 3, the USB device is still attached to the system, but
> > will now return MEDIUM NOT PRESENT for many commands, including
> > SYNCHRONIZE CACHE—basically it acts like any device with removable
> > media.  However, the act of the “media” being removed does not
> > invalidate sdkp->WCE; therefore sd_shutdown() and sd_suspend() still
> > call sd_sync_cache(), which *fails* because it gets a MEDIUM NOT
> > PRESENT sense code.  In the sd_suspend() case, this causes the entire
> > suspend to fail, and the laptop rewakes immediately.

So this was the subject of some debate when suspend of sd was first
introduced.  The synopsis of that debate is that by suspending, we're
about the power down the system, and anything in the cache will be lost,
possibly causing corruption, so failure to synchronise the cache
*should* abort suspend.

> > There are a few different ways to fix this; e.g. one could
> > specifically test media_not_present() if a sense code is returned in
> > sd_sync_cache(). 

Isn't this the best approach?  For removable medium, the onus is on the
user not to eject with the cache unsynced.  If the user ignores that, we
should recognise the problem and take a caveat emptor approach.

As a side note: having write through caches on removable media is a
really silly idea because of the above problem ...

>  However, the following patch seems simpler, and
> > avoids calling sd_sync_cache() at all in this case.  sdkp->WCE will be
> > reset when new medium is recognized and sd_read_cache_type() is
> > called.  Note this code always gets called—it's in the same path as
> > sd_read_capacity(), which has to be called for the device to be usable
> > again; thus the patch is inherently safe.
> > 
> > Kernel tested: 2.6.38 (Ubuntu Natty)
> 
> Patch appended for completness.
> 
> I need someone from USB/SCSI camp to see if this approach makes sense.

I don't really think so, because it's pretending the device cache has
flipped to write through.  It's certainly possible to envisage removable
media where the cache is in the housing and we still need to preserve
the idea of it being write back.

Instinct tells me the correct set of fixes is to add a sync cache from
release (so we automatically sync on last close, which is usually when
an ordered remove happens), keep the one on shutdown, just in case the
system goes down with stuff still mounted and print a nasty message on
suspend for a write back device that's been removed.

I also think we shouldn't abort the suspend if the disk doesn't respond
correctly to start/stop ... the power is going to be disconnected
anyway, so it's no issue if the disk spins for a second or so longer.

The problem this is going to cause is double sync on shutdown (once when
final unmount closes the device and once on shutdown) ... do people
agree that's a price worth paying?

Something like this?

James

---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e567302..b5c485a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -408,6 +408,41 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
 	mutex_unlock(&sd_ref_mutex);
 }
 
+static int sd_sync_cache(struct scsi_disk *sdkp)
+{
+	int retries, res;
+	struct scsi_device *sdp = sdkp->device;
+	struct scsi_sense_hdr sshdr;
+
+	if (!scsi_device_online(sdp))
+		return -ENODEV;
+
+
+	for (retries = 3; retries > 0; --retries) {
+		unsigned char cmd[10] = { 0 };
+
+		cmd[0] = SYNCHRONIZE_CACHE;
+		/*
+		 * Leave the rest of the command zero to indicate
+		 * flush everything.
+		 */
+		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
+				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL);
+		if (res == 0)
+			break;
+	}
+
+	if (res) {
+		sd_print_result(sdkp, res);
+		if (driver_byte(res) & DRIVER_SENSE)
+			sd_print_sense_hdr(sdkp, &sshdr);
+	}
+
+	if (res)
+		return -EIO;
+	return 0;
+}
+
 static void sd_prot_op(struct scsi_cmnd *scmd, unsigned int dif)
 {
 	unsigned int prot_op = SCSI_PROT_NORMAL;
@@ -897,6 +932,11 @@ static int sd_release(struct gendisk *disk, fmode_t mode)
 			scsi_set_medium_removal(sdev, SCSI_REMOVAL_ALLOW);
 	}
 
+	if (sdkp->WCE) {
+		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
+		sd_sync_cache(sdkp);
+	}
+
 	/*
 	 * XXX and what if there are packets in flight and this close()
 	 * XXX is followed by a "rmmod sd_mod"?
@@ -1093,41 +1133,6 @@ out:
 	return retval;
 }
 
-static int sd_sync_cache(struct scsi_disk *sdkp)
-{
-	int retries, res;
-	struct scsi_device *sdp = sdkp->device;
-	struct scsi_sense_hdr sshdr;
-
-	if (!scsi_device_online(sdp))
-		return -ENODEV;
-
-
-	for (retries = 3; retries > 0; --retries) {
-		unsigned char cmd[10] = { 0 };
-
-		cmd[0] = SYNCHRONIZE_CACHE;
-		/*
-		 * Leave the rest of the command zero to indicate
-		 * flush everything.
-		 */
-		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL);
-		if (res == 0)
-			break;
-	}
-
-	if (res) {
-		sd_print_result(sdkp, res);
-		if (driver_byte(res) & DRIVER_SENSE)
-			sd_print_sense_hdr(sdkp, &sshdr);
-	}
-
-	if (res)
-		return -EIO;
-	return 0;
-}
-
 static void sd_rescan(struct device *dev)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
@@ -2627,15 +2632,19 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 		return 0;	/* this can happen */
 
 	if (sdkp->WCE) {
-		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
-		ret = sd_sync_cache(sdkp);
-		if (ret)
-			goto done;
+		if (sdkp->media_present) {
+			sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
+			ret = sd_sync_cache(sdkp);
+			if (ret)
+				goto done;
+		} else {
+			sd_printk(KERN_NOTICE, sdkp, "Disk already ejected, not synchronizing SCSI cache\n");
+		}	
 	}
 
 	if ((mesg.event & PM_EVENT_SLEEP) && sdkp->device->manage_start_stop) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
-		ret = sd_start_stop_device(sdkp, 0);
+		sd_start_stop_device(sdkp, 0); /* ignore return code */
 	}
 
 done:

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Charles Hannum]

On Thu, May 12, 2011 at 16:36, James Bottomley
<James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Thu, 2011-05-12 at 22:03 +0200, Rafael J. Wysocki wrote:
>> Hi,
>>
>> Added some CCs.
>>
>> On Thursday, May 12, 2011, Charles Hannum wrote:
>> > Short version: My laptop doesn't suspend when my Android phone is
>> > connected and has been “ejected”.
>> >
>> > Long version:
>> >
>> > Android phones connect as USB mass storage devices.  After the “Turn
>> > on USB storage” button has been clicked, there are a few different
>> > ways to detach the “disk”:
>> >
>> > 1) pull the cable
>> > 2) click “Turn off USB storage”
>> > 3) “eject” the device
>> >
>> > In cases 2 & 3, the USB device is still attached to the system, but
>> > will now return MEDIUM NOT PRESENT for many commands, including
>> > SYNCHRONIZE CACHE—basically it acts like any device with removable
>> > media.  However, the act of the “media” being removed does not
>> > invalidate sdkp->WCE; therefore sd_shutdown() and sd_suspend() still
>> > call sd_sync_cache(), which *fails* because it gets a MEDIUM NOT
>> > PRESENT sense code.  In the sd_suspend() case, this causes the entire
>> > suspend to fail, and the laptop rewakes immediately.
>
> So this was the subject of some debate when suspend of sd was first
> introduced.  The synopsis of that debate is that by suspending, we're
> about the power down the system, and anything in the cache will be lost,
> possibly causing corruption, so failure to synchronise the cache
> *should* abort suspend.

If the device has been “ejected”, there is by definition no data in
the cache that needs to be synchronized—or, if there is, it's because
we failed to do a SYNCHRONIZE CACHE in the eject path.  At this point
the device believes there's no medium present, and the driver even
agrees, so it's perfectly reasonable for the device to return MEDIUM
NOT PRESENT.  The problem here is a generic failure of the suspend
code in the case of removable media that has been removed—there is a
piece of state left behind in the driver that causes it to misbehave.

Nothing I have mentioned (other than enumerating the cable-pulling
case), nor the change I proposed, has anything whatsoever to do with
the user inappropriately yanking an active device.

>> > There are a few different ways to fix this; e.g. one could
>> > specifically test media_not_present() if a sense code is returned in
>> > sd_sync_cache().
>
> Isn't this the best approach?  For removable medium, the onus is on the
> user not to eject with the cache unsynced.  If the user ignores that, we
> should recognise the problem and take a caveat emptor approach.

I was attempting to make the case where the user behaved completely
appropriately work right, not worrying about the other cases.  It
seems a bit silly to even call sd_sync_cache() if we already know the
medium has been removed; furthermore, it means the behavior is
different between when the device is first plugged in (WCE is still 0)
and when it's been used and then ejected (WCE remains 1 until a new
medium is inserted); this inconsistency seems poor.

> As a side note: having write through caches on removable media is a
> really silly idea because of the above problem ...

In the Android case, I see no reason why it's a problem, and the
device acts appropriately under the circumstances.  And given tray
locking, I'm not sure why it's a problem even for spinning media.  But
regardless, these are real devices, and that ship has long since
sailed.
--
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: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Alan Stern]

On Thu, 12 May 2011, James Bottomley wrote:

> > I need someone from USB/SCSI camp to see if this approach makes sense.
> 
> I don't really think so, because it's pretending the device cache has
> flipped to write through.  It's certainly possible to envisage removable
> media where the cache is in the housing and we still need to preserve
> the idea of it being write back.

I don't follow your argument here.  What difference does it make what 
kind of cache the drive has, if the media is gone?

> Instinct tells me the correct set of fixes is to add a sync cache from
> release (so we automatically sync on last close, which is usually when
> an ordered remove happens),

That certainly makes sense.  Is there any reason why this isn't done 
already?

>  keep the one on shutdown, just in case the
> system goes down with stuff still mounted and print a nasty message on
> suspend for a write back device that's been removed.

There's no need for a nasty message unless the cache is still dirty.  
Your suggested patch doesn't check for that -- in fact, I don't think
the driver even knows whether the cache is dirty.  (Not that this 
matters, seeing as how your patch doesn't print any nasty messages.)

> I also think we shouldn't abort the suspend if the disk doesn't respond
> correctly to start/stop ... the power is going to be disconnected
> anyway, so it's no issue if the disk spins for a second or so longer.

That's a good idea.  On several occasions Linus has mentioned that 
almost nothing should stop a system suspend.  It's even questionable 
whether a SYNC failure should stop a suspend ... but that's a separate 
matter.

What happens if the medium was recently removed, meaning that 
sdkp->media_present hasn't yet had a chance to become 0?  The patch 
needs to handle that case as well, perhaps by adding another check 
after sd_sync_cache() returns.

> The problem this is going to cause is double sync on shutdown (once when
> final unmount closes the device and once on shutdown) ... do people
> agree that's a price worth paying?

I don't think the price will be high.  The second sync will have
nothing to do, because the first sync will have cleaned out the cache.

Alan Stern

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[James Bottomley]

On Thu, 2011-05-12 at 17:32 -0400, Alan Stern wrote:
> On Thu, 12 May 2011, James Bottomley wrote:
> 
> > > I need someone from USB/SCSI camp to see if this approach makes sense.
> > 
> > I don't really think so, because it's pretending the device cache has
> > flipped to write through.  It's certainly possible to envisage removable
> > media where the cache is in the housing and we still need to preserve
> > the idea of it being write back.
> 
> I don't follow your argument here.  What difference does it make what 
> kind of cache the drive has, if the media is gone?

It depends what the drive is caching and what's removable.  If it's some
type of hybrid, and the cache isn't mapped to the media for instance.

The point really is that if the cache is in the housing, setting it to
write through when it's not is not a correct reflection of reality.

> > Instinct tells me the correct set of fixes is to add a sync cache from
> > release (so we automatically sync on last close, which is usually when
> > an ordered remove happens),
> 
> That certainly makes sense.  Is there any reason why this isn't done 
> already?

None I can think of.  When the caching logic was first done, I don't
think removable media were even considered, so it was all about shutdown
behaviour.

> >  keep the one on shutdown, just in case the
> > system goes down with stuff still mounted and print a nasty message on
> > suspend for a write back device that's been removed.
> 
> There's no need for a nasty message unless the cache is still dirty.  
> Your suggested patch doesn't check for that -- in fact, I don't think
> the driver even knows whether the cache is dirty.  (Not that this 
> matters, seeing as how your patch doesn't print any nasty messages.)

right: We don't currently track dirty state.  We could, I suppose, but I
don't think it's worth it.

> > I also think we shouldn't abort the suspend if the disk doesn't respond
> > correctly to start/stop ... the power is going to be disconnected
> > anyway, so it's no issue if the disk spins for a second or so longer.
> 
> That's a good idea.  On several occasions Linus has mentioned that 
> almost nothing should stop a system suspend.  It's even questionable 
> whether a SYNC failure should stop a suspend ... but that's a separate 
> matter.

Heh, if you want to open that old can of worms again, be my guest ...

> What happens if the medium was recently removed, meaning that 
> sdkp->media_present hasn't yet had a chance to become 0?  The patch 
> needs to handle that case as well, perhaps by adding another check 
> after sd_sync_cache() returns.

or actually, just checking the value when it looks at the return code.

> > The problem this is going to cause is double sync on shutdown (once when
> > final unmount closes the device and once on shutdown) ... do people
> > agree that's a price worth paying?
> 
> I don't think the price will be high.  The second sync will have
> nothing to do, because the first sync will have cleaned out the cache.

Heh, I've got a long inbox full of complaints about the two capacity
messages we used to print on boot ...

James

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Charles Hannum]

On Thu, May 12, 2011 at 17:43, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> It depends what the drive is caching and what's removable.  If it's some
> type of hybrid, and the cache isn't mapped to the media for instance.

Except that the SCSI spec says the cache affected by SYNCHRONIZE CACHE
is related to the medium.

> The point really is that if the cache is in the housing, setting it to
> write through when it's not is not a correct reflection of reality.

Since this pretty clearly violates the intent of the spec, I have to
ask whether you are aware of any such device.  I've never seen one.

Also, the only actual behavioral difference between my proposed patch
and your proposed one is to print out a message; you still weren't
calling sd_sync_cache() if the medium was not present.  I'm not
beholden to a specific patch—but printing out an extra message, which
seems to imply that something is wrong, is decidely uncool in the
original case I referenced (my Android phone, after “Turn on USB
storage” and then “/usr/bin/eject”), where nothing is wrong other than
the driver trying to SYNCHRONIZE CACHE to a medium that it already
knows is not present.

> or actually, just checking the value when it looks at the return code.

That was what I did first, but then I realized two things:

1) The pre-use (before clicking “Turn on USB storage”) state has
WCE==0 because the mode page code hasn't been called at that point.
There is no good reason the pre-use and post-eject behavior should be
different, given that the device itself is in *exactly* the same state
at these points.

2) I definitely do not want a spurious message that could be
interpreted as an error, since this is perfectly normal and acceptable
use.

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Oliver Neukum]

Am Donnerstag, 12. Mai 2011, 22:36:03 schrieb James Bottomley:

> Instinct tells me the correct set of fixes is to add a sync cache from
> release (so we automatically sync on last close, which is usually when
> an ordered remove happens), keep the one on shutdown, just in case the
> system goes down with stuff still mounted and print a nasty message on
> suspend for a write back device that's been removed.
> 
> I also think we shouldn't abort the suspend if the disk doesn't respond
> correctly to start/stop ... the power is going to be disconnected
> anyway, so it's no issue if the disk spins for a second or so longer.
> 
> The problem this is going to cause is double sync on shutdown (once when
> final unmount closes the device and once on shutdown) ... do people
> agree that's a price worth paying?
> 
> Something like this?

Hi,

it seems to me that the error codes should really be evaluated a bit better.
How about this?

	Regards
		Oliver

>From 2ab03ae46d6dc126a51ee9cd38206fbdc8bf6a34 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oliver@neukum.org>
Date: Mon, 25 Oct 2010 12:25:26 +0200
Subject: [PATCH 2/5] SCSI:sd:error handling in sd_sync_cache()

The SCSI commands sd_sync_cache() uses may fail.
These errors needs to be evaluated to return correct
error codes. This fixes system suspension while a device
removal is being handled.

Signed-off-by: Oliver NEukum <oneukum@suse.de>
---
 drivers/scsi/sd.c |   28 ++++++++++++++++++++++++----
 1 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fcba7d2..5ed75bf 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1104,11 +1104,31 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 		sd_print_result(sdkp, res);
 		if (driver_byte(res) & DRIVER_SENSE)
 			sd_print_sense_hdr(sdkp, &sshdr);
-	}
 
-	if (res)
-		return -EIO;
-	return 0;
+		/* 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;
+		}
+	} else {
+		return 0;
+	}
 }
 
 static void sd_rescan(struct device *dev)
-- 
1.7.1

Re: [PATCH] scsi/sd: fix suspend with USB-connected Android phone (one line)

[Hannes Reinecke]

On 05/14/2011 09:11 PM, Oliver Neukum wrote:
> Am Donnerstag, 12. Mai 2011, 22:36:03 schrieb James Bottomley:
>
>> Instinct tells me the correct set of fixes is to add a sync cache from
>> release (so we automatically sync on last close, which is usually when
>> an ordered remove happens), keep the one on shutdown, just in case the
>> system goes down with stuff still mounted and print a nasty message on
>> suspend for a write back device that's been removed.
>>
>> I also think we shouldn't abort the suspend if the disk doesn't respond
>> correctly to start/stop ... the power is going to be disconnected
>> anyway, so it's no issue if the disk spins for a second or so longer.
>>
>> The problem this is going to cause is double sync on shutdown (once when
>> final unmount closes the device and once on shutdown) ... do people
>> agree that's a price worth paying?
>>
>> Something like this?
>
> Hi,
>
> it seems to me that the error codes should really be evaluated a bit better.
> How about this?
>
> 	Regards
> 		Oliver
>
>  From 2ab03ae46d6dc126a51ee9cd38206fbdc8bf6a34 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum<oliver@neukum.org>
> Date: Mon, 25 Oct 2010 12:25:26 +0200
> Subject: [PATCH 2/5] SCSI:sd:error handling in sd_sync_cache()
>
> The SCSI commands sd_sync_cache() uses may fail.
> These errors needs to be evaluated to return correct
> error codes. This fixes system suspension while a device
> removal is being handled.
>
> Signed-off-by: Oliver NEukum<oneukum@suse.de>
Yep, this makes sense.

Acked-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)