cleanup error handling during suspend/resume of sd

cleanup error handling during suspend/resume of sd

[Oliver Neukum]

This series fixes an issue mostly seen with eSATA devices and
card readers. The errors don't arise on the SCSI level must
be processed and sorted for the generic code, so that issue
like recently disconnected devices don't impede system sleep.

Signed-off-by: Oliver Neukum <oneukum@suse.de>

[PATCH 1/3] sd: SCSI error codes must not be returned to generic code

[Oliver Neukum]

sd_start_stop_device leaks SCSI specific error codes
to the generic driver code

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 4df73e5..cef06f5 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2830,7 +2830,11 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 			sd_print_sense_hdr(sdkp, &sshdr);
 	}
 
-	return res;
+	/* SCSI error codes must not go to the generic layer */
+	if (res)
+		return -EIO;
+
+	return 0;
 }
 
 /*
-- 
1.7.7

[PATCH 2/3] sd: handle errors in sd_start_stop_device()

[Oliver Neukum]

Errors are not handled correctly in sd_start_stop_device().

First, if a device cannot be stopped because a medium has been
removed, this is not an error. This fixes a bug when suspending
the system right after pulling a medium out of a card reader.

Second, if the full system is to be suspended, a failure to stop
a device is not a good reason to abort a suspend, as that won't
improve matters.

Signed-of-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/scsi/sd.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cef06f5..a344220 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2828,6 +2828,10 @@ 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;
 	}
 
 	/* SCSI error codes must not go to the generic layer */
@@ -2884,6 +2888,10 @@ static int sd_suspend(struct device *dev, pm_message_t mesg)
 	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);
+
+		/* an error is not worth aborting a system sleep */
+		if (!(mesg.event & PM_EVENT_AUTO))
+			ret = 0;
 	}
 
 done:
-- 
1.7.7

[PATCH 3/3] sd: error handling synchronizing caches

[Oliver Neukum]

sd_sync_cache() can fail in several manners.
As the error return is passed to the generic layer,
it must be correctly processed.

Failures due to unplugged devices or removed media
should be ignored. In the other cases retryable and
fatal errors must be differentiated.

This fixes a problem with unplugging eSATA devices
before suspending the system.

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

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index a344220..49b0c52 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1278,8 +1278,32 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 	if (res) {
 		sd_print_result(sdkp, res);
+
 		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;
+		}
+	} else {
+		return 0;
 	}
 
 	if (res)
-- 
1.7.7

Re: [PATCH 3/3] sd: error handling synchronizing caches

[James Bottomley]

On Wed, 2012-08-01 at 16:44 +0200, Oliver Neukum wrote:
> sd_sync_cache() can fail in several manners.
> As the error return is passed to the generic layer,
> it must be correctly processed.
> 
> Failures due to unplugged devices or removed media
> should be ignored. In the other cases retryable and
> fatal errors must be differentiated.
> 
> This fixes a problem with unplugging eSATA devices
> before suspending the system.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/scsi/sd.c |   24 ++++++++++++++++++++++++
>  1 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a344220..49b0c52 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1278,8 +1278,32 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  
>  	if (res) {
>  		sd_print_result(sdkp, res);
> +
>  		if (driver_byte(res) & DRIVER_SENSE)
>  			sd_print_sense_hdr(sdkp, &sshdr);
> +			/* we need to evaluate the error return  */
> +			if ((scsi_sense_valid(&sshdr) &&

The indentation is completely wrong here ... it's implying that the
first if should have an opening brace.

> +				/* 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;

Shouldn't you return -EBUSY for at least QUEUE_FULL and  BUSY as well?

James


> +			default:
> +				return -EIO;
> +		}
> +	} else {
> +		return 0;
>  	}
>  
>  	if (res)

Re: [PATCH 3/3] sd: error handling synchronizing caches

[Mike Christie]

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index a344220..49b0c52 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1278,8 +1278,32 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  
>  	if (res) {
>  		sd_print_result(sdkp, res);
> +
>  		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;

Could we have all the host/scsi-ml to -EXYZ error conversions put in one
place like __scsi_error_from_host_byte()?