Cleanup error handling for power management of sd

Cleanup error handling for power management of sd

[Oliver Neukum]

Errors happening while sd devices are going to sleep need
to interpreted for the generic layer. In particular errors
for unplugged devices and removed media must be ignored.
This hits mostly users of eSATA devices who unplug external
drives right before the system is supposed to go to sleep.

[PATCH 1/4] 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/4] 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/4] sd: deal with removal of medium to be synched

[Oliver Neukum]

sd_sync_cache() is called for purposes of power management.
Under those circumstances a medium removal is no error.
The data is lost under any circumstances, so the system may as
well sleep.

Signed-off-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 a344220..c39a97e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1278,8 +1278,16 @@ 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
+		 * 0x3a is medium not present
+		 * this is no error here
+		 */
+		if ((scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a))
+			return 0;
 	}
 
 	if (res)
-- 
1.7.7

[PATCH 4/4] scsi: translate errors for power management

[Oliver Neukum]

Synchronizing caches may fail. The reason for these
failures need to be translated to the generic layer
so that it nows whether to ignore a failure, retry later
or give up, aborting sleep.

Signed-off-by: Oliver Neukum <oneukum@suse.de>
---
 drivers/scsi/scsi_error.c |   33 +++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c         |    3 +--
 include/scsi/scsi_eh.h    |    1 +
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 4a6381c..13f7060 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2217,3 +2217,36 @@ void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq)
 	}
 }
 EXPORT_SYMBOL(scsi_build_sense_buffer);
+
+/**
+ * scsi_translate_errors_to_power_management - errors in a form driver core understands
+ * @res:	Result of scsi command
+ *
+ * Return value:
+ * 	error code driver core understands
+ **/
+int scsi_translate_errors_to_power_management(int res)
+{
+	int rv = -EIO;
+
+	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:
+	case DID_TIME_OUT:
+	case DID_ABORT:
+	case DID_RESET:
+	case DID_TRANSPORT_DISRUPTED:
+		return -EBUSY;
+	case DID_OK:
+		return 0;
+	}
+	return rv;
+}
+EXPORT_SYMBOL(scsi_translate_errors_to_power_management);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c39a97e..8597294 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1288,10 +1288,9 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 		 */
 		if ((scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a))
 			return 0;
+		return scsi_translate_errors_to_power_management(res);
 	}
 
-	if (res)
-		return -EIO;
 	return 0;
 }
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index 06a8790..bdf6d6c 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -59,6 +59,7 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 				   u64 * info_out);
 
 extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
+extern int scsi_translate_errors_to_power_management(int res);
 
 /*
  * Reset request from external source
-- 
1.7.7

Re: [PATCH 4/4] scsi: translate errors for power management

[James Bottomley]

On Thu, 2012-08-02 at 12:27 +0200, Oliver Neukum wrote:
> Synchronizing caches may fail. The reason for these
> failures need to be translated to the generic layer
> so that it nows whether to ignore a failure, retry later
> or give up, aborting sleep.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ---
>  drivers/scsi/scsi_error.c |   33 +++++++++++++++++++++++++++++++++
>  drivers/scsi/sd.c         |    3 +--
>  include/scsi/scsi_eh.h    |    1 +
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 4a6381c..13f7060 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -2217,3 +2217,36 @@ void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq)
>  	}
>  }
>  EXPORT_SYMBOL(scsi_build_sense_buffer);
> +
> +/**
> + * scsi_translate_errors_to_power_management - errors in a form driver core understands
> + * @res:	Result of scsi command
> + *
> + * Return value:
> + * 	error code driver core understands
> + **/
> +int scsi_translate_errors_to_power_management(int res)
> +{
> +	int rv = -EIO;

No need for a variable, I think

> +	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:
> +	case DID_TIME_OUT:
> +	case DID_ABORT:
> +	case DID_RESET:
> +	case DID_TRANSPORT_DISRUPTED:
> +		return -EBUSY;
> +	case DID_OK:
> +		return 0;
> +	}
> +	return rv;

this should be inside the switch as

default:
	return -EIO;

and no return statment outside of the switch.

Don't you still want to handle BUSY and QUEUE_FULL as -EBUSY?  Right at
the moment they'll fall through the default case and return -EIO.

James


> +}
> +EXPORT_SYMBOL(scsi_translate_errors_to_power_management);
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index c39a97e..8597294 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1288,10 +1288,9 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  		 */
>  		if ((scsi_sense_valid(&sshdr) && sshdr.asc == 0x3a))
>  			return 0;
> +		return scsi_translate_errors_to_power_management(res);
>  	}
>  
> -	if (res)
> -		return -EIO;
>  	return 0;
>  }
>  
> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
> index 06a8790..bdf6d6c 100644
> --- a/include/scsi/scsi_eh.h
> +++ b/include/scsi/scsi_eh.h
> @@ -59,6 +59,7 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
>  				   u64 * info_out);
>  
>  extern void scsi_build_sense_buffer(int desc, u8 *buf, u8 key, u8 asc, u8 ascq);
> +extern int scsi_translate_errors_to_power_management(int res);
>  
>  /*
>   * Reset request from external source

Re: [PATCH 4/4] scsi: translate errors for power management

[Oliver Neukum]

On Thursday 02 August 2012 11:43:44 James Bottomley wrote:
 
> this should be inside the switch as
> 
> default:
> 	return -EIO;
> 
> and no return statment outside of the switch.

OK

> Don't you still want to handle BUSY and QUEUE_FULL as -EBUSY?  Right at
> the moment they'll fall through the default case and return -EIO.

True. They should be handled.

	Regards
		Oliver