[PATCH 1/2] scsi: Handle Unit Attention when issuing SCSI command

[PATCH 1/2] scsi: Handle Unit Attention when issuing SCSI command

[Gabriel Krisman Bertazi]

Usually, re-sending the SCSI command is enough to recover from a Unit
Attention (UA).  This adds a generic retry code to the SCSI command path
in case of an UA, before giving up and returning the error condition to
the caller.

I added the UA verification into scsi_execute instead of
scsi_execute_req because there are at least a few callers that invoke
scsi_execute directly and would benefit from the internal UA retry.
Also, I didn't use scsi_normalize_sense to not duplicate functionality
with scsi_execute_req_flags.  Instead, scsi_execute uses a small helper
function that verifies only the UA condition directly from the raw sense
buffer.  If this design is not OK, I can refactor to use
scsi_normalize_sense.

This prevents us from duplicating the retry code in at least a few
places.  In particular, it fixes an issue found in some IBM enclosures,
in which the device may return an Unit Attention during probe, breaking
the bind with the ses module:

scsi 1:0:7:0: Failed to get diagnostic page 0x8000002
scsi 1:0:7:0: Failed to bind enclosure -19

Finally, should we have a NORETRY_UA flag to allow callers to disable
this mechanism?

Link: https://patchwork.kernel.org/patch/9336763/
Suggested-by: Brian King <brking@linux.vnet.ibm.com>
Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
---
 drivers/scsi/scsi_lib.c    | 47 +++++++++++++++++++++++++++++++++++++++++-----
 include/scsi/scsi_common.h |  9 +++++++++
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c71344aebdbb..a4af411de2a4 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -47,6 +47,9 @@ struct kmem_cache *scsi_sdb_cache;
  */
 #define SCSI_QUEUE_DELAY	3
 
+/* Maximum number of retries when a scsi command triggers an Unit Attention. */
+#define UNIT_ATTENTION_RETRIES 5
+
 static void
 scsi_set_blocked(struct scsi_cmnd *cmd, int reason)
 {
@@ -164,7 +167,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 	__scsi_queue_insert(cmd, reason, 1);
 }
 /**
- * scsi_execute - insert request and wait for the result
+ * __scsi_execute - insert request and wait for the result
  * @sdev:	scsi device
  * @cmd:	scsi command
  * @data_direction: data direction
@@ -179,10 +182,10 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
  * returns the req->errors value which is the scsi_cmnd result
  * field.
  */
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
-		 int data_direction, void *buffer, unsigned bufflen,
-		 unsigned char *sense, int timeout, int retries, u64 flags,
-		 int *resid)
+int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+		   int data_direction, void *buffer, unsigned bufflen,
+		   unsigned char *sense, int timeout, int retries, u64 flags,
+		   int *resid)
 {
 	struct request *req;
 	int write = (data_direction == DMA_TO_DEVICE);
@@ -227,6 +230,40 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 
 	return ret;
 }
+
+int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+		 int data_direction, void *buffer, unsigned bufflen,
+		 unsigned char *sense, int timeout, int retries, u64 flags,
+		 int *resid)
+{
+	int result;
+	int retry = UNIT_ATTENTION_RETRIES;
+	bool priv_sense = false;
+
+	if (!sense) {
+		sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
+		priv_sense = true;
+		if (!sense)
+			return DRIVER_ERROR << 24;
+	}
+
+	while (retry--) {
+		result = __scsi_execute(sdev, cmd, data_direction, buffer,
+					bufflen, sense, timeout, retries,
+					flags, resid);
+
+		if (!scsi_sense_unit_attention(sense))
+			break;
+
+		if (retry)
+			memset(sense, 0, SCSI_SENSE_BUFFERSIZE);
+	}
+
+	if (priv_sense)
+		kfree(sense);
+
+	return result;
+}
 EXPORT_SYMBOL(scsi_execute);
 
 int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned char *cmd,
diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
index 20bf7eaef05a..747b632d5b57 100644
--- a/include/scsi/scsi_common.h
+++ b/include/scsi/scsi_common.h
@@ -58,6 +58,15 @@ static inline bool scsi_sense_valid(const struct scsi_sense_hdr *sshdr)
 	return (sshdr->response_code & 0x70) == 0x70;
 }
 
+static inline bool scsi_sense_unit_attention(const char *sense)
+{
+	int resp = sense[0] & 0x7f;
+
+	return ((resp & 0x70) &&
+		((resp >= 0x72 && (sense[1] & 0xf) == UNIT_ATTENTION) ||
+		 (resp < 0x72 && (sense[2] & 0xf) == UNIT_ATTENTION)));
+}
+
 extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
 				 struct scsi_sense_hdr *sshdr);
 
-- 
2.7.4

[PATCH 2/2] sr: Drop custom handling of unit attention

[Gabriel Krisman Bertazi]

This is no longer necessary, since we handle it in scsi_execute.

Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
---
 drivers/scsi/sr_ioctl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 03054c0e7689..f7724e52fd0f 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -179,8 +179,7 @@ static int sr_play_trkind(struct cdrom_device_info *cdi,
 }
 
 /* We do our own retries because we want to know what the specific
-   error code is.  Normally the UNIT_ATTENTION code will automatically
-   clear after one error */
+   error code is. */
 
 int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 {
@@ -220,8 +219,6 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
 			if (!cgc->quiet)
 				sr_printk(KERN_INFO, cd,
 					  "disc change detected.\n");
-			if (retries++ < 10)
-				goto retry;
 			err = -ENOMEDIUM;
 			break;
 		case NOT_READY:	/* This happens if there is no disc in drive */
-- 
2.7.4

Re: [PATCH 1/2] scsi: Handle Unit Attention when issuing SCSI command

[James Bottomley]

On Wed, 2016-10-12 at 02:25 -0300, Gabriel Krisman Bertazi wrote:
> Usually, re-sending the SCSI command is enough to recover from a Unit
> Attention (UA).  This adds a generic retry code to the SCSI command 
> path in case of an UA, before giving up and returning the error 
> condition to the caller.
> 
> I added the UA verification into scsi_execute instead of
> scsi_execute_req because there are at least a few callers that invoke
> scsi_execute directly and would benefit from the internal UA retry.
> Also, I didn't use scsi_normalize_sense to not duplicate 
> functionality with scsi_execute_req_flags.  Instead, scsi_execute 
> uses a small helper function that verifies only the UA condition 
> directly from the raw sense buffer.  If this design is not OK, I can 
> refactor to use scsi_normalize_sense.
> 
> This prevents us from duplicating the retry code in at least a few
> places.  In particular, it fixes an issue found in some IBM 
> enclosures, in which the device may return an Unit Attention during 
> probe, breaking the bind with the ses module:
> 
> scsi 1:0:7:0: Failed to get diagnostic page 0x8000002
> scsi 1:0:7:0: Failed to bind enclosure -19
> 
> Finally, should we have a NORETRY_UA flag to allow callers to disable
> this mechanism?

I think not: let's use retries for this.  Allowing no retries would be
the signal that you want the UA condition returned.

> Link: https://patchwork.kernel.org/patch/9336763/
> Suggested-by: Brian King <brking@linux.vnet.ibm.com>
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Signed-off-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>
> ---
>  drivers/scsi/scsi_lib.c    | 47
> +++++++++++++++++++++++++++++++++++++++++-----
>  include/scsi/scsi_common.h |  9 +++++++++
>  2 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index c71344aebdbb..a4af411de2a4 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -47,6 +47,9 @@ struct kmem_cache *scsi_sdb_cache;
>   */
>  #define SCSI_QUEUE_DELAY	3
>  
> +/* Maximum number of retries when a scsi command triggers an Unit
> Attention. */
> +#define UNIT_ATTENTION_RETRIES 5

We would also tick down the passed in retries so you don't need this
artificial setting.

>  static void
>  scsi_set_blocked(struct scsi_cmnd *cmd, int reason)
>  {
> @@ -164,7 +167,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int
> reason)
>  	__scsi_queue_insert(cmd, reason, 1);
>  }
>  /**
> - * scsi_execute - insert request and wait for the result
> + * __scsi_execute - insert request and wait for the result
>   * @sdev:	scsi device
>   * @cmd:	scsi command
>   * @data_direction: data direction
> @@ -179,10 +182,10 @@ void scsi_queue_insert(struct scsi_cmnd *cmd,
> int reason)
>   * returns the req->errors value which is the scsi_cmnd result
>   * field.
>   */
> -int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> -		 int data_direction, void *buffer, unsigned bufflen,
> -		 unsigned char *sense, int timeout, int retries, u64
> flags,
> -		 int *resid)
> +int __scsi_execute(struct scsi_device *sdev, const unsigned char
> *cmd,
> +		   int data_direction, void *buffer, unsigned
> bufflen,
> +		   unsigned char *sense, int timeout, int retries,
> u64 flags,
> +		   int *resid)
>  {
>  	struct request *req;
>  	int write = (data_direction == DMA_TO_DEVICE);
> @@ -227,6 +230,40 @@ int scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
>  
>  	return ret;
>  }
> +
> +int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
> +		 int data_direction, void *buffer, unsigned bufflen,
> +		 unsigned char *sense, int timeout, int retries, u64
> flags,
> +		 int *resid)
> +{
> +	int result;
> +	int retry = UNIT_ATTENTION_RETRIES;
> +	bool priv_sense = false;
> +
> +	if (!sense) {
> +		sense = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
> +		priv_sense = true;
> +		if (!sense)
> +			return DRIVER_ERROR << 24;
> +	}
> +
> +	while (retry--) {
> +		result = __scsi_execute(sdev, cmd, data_direction,
> buffer,
> +					bufflen, sense, timeout,
> retries,
> +					flags, resid);

OK, so really this isn't what you want, because blk_execute_req may
have used several of your retries, so you now get a maximum possible
set of retries at UNIT_ATTENTION_RETRIES*retries.  You need to start
from the returned req->retries, which probably means this loop needs to
be inside __scsi_execute.

James


> +
> +		if (!scsi_sense_unit_attention(sense))
> +			break;
> +
> +		if (retry)
> +			memset(sense, 0, SCSI_SENSE_BUFFERSIZE);
> +	}
> +
> +	if (priv_sense)
> +		kfree(sense);
> +
> +	return result;
> +}
>  EXPORT_SYMBOL(scsi_execute);
>  
>  int scsi_execute_req_flags(struct scsi_device *sdev, const unsigned
> char *cmd,
> diff --git a/include/scsi/scsi_common.h b/include/scsi/scsi_common.h
> index 20bf7eaef05a..747b632d5b57 100644
> --- a/include/scsi/scsi_common.h
> +++ b/include/scsi/scsi_common.h
> @@ -58,6 +58,15 @@ static inline bool scsi_sense_valid(const struct
> scsi_sense_hdr *sshdr)
>  	return (sshdr->response_code & 0x70) == 0x70;
>  }
>  
> +static inline bool scsi_sense_unit_attention(const char *sense)
> +{
> +	int resp = sense[0] & 0x7f;
> +
> +	return ((resp & 0x70) &&
> +		((resp >= 0x72 && (sense[1] & 0xf) ==
> UNIT_ATTENTION) ||
> +		 (resp < 0x72 && (sense[2] & 0xf) ==
> UNIT_ATTENTION)));
> +}
> +
>  extern bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
>  				 struct scsi_sense_hdr *sshdr);
>