[patchset 0/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[patchset 0/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Boaz Harrosh]

In motivation to abstract scsi_cmnd members and insulate
drivers/transports from scsi_cmnd internals. The last
place left was the REQUEST_SENSE sequence when done
synchronous, by drivers.

Also all these drivers would do a use_sg==0 command
invocation, preventing from doing cleanups on these
drivers/transports. So this patchset is left-overs
from Christoph's 2.6.18 cleanups.

In this patchset I have re-factored scsi_error to
make it possible for drivers to use an abstract
(easy to use, I hope) API for invoking a REQUEST_SENSE
command. The API is declared in scsi/scsi_eh.h

[patch 1/5] scsi_error: code cleanup before refactoring of scsi_send_eh_cmnd()
  Move some code around and add new fixtures here. So next patch is
  left a Mechanical breakup of code.

[patch 2/5] scsi_error: Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE
  Here new API is introduced in scsi/scsi_eh.h and mechanical move of code
  to the new functions.

[PATCH 3/5] usb: transport.c use scsi_eh API in REQUEST_SENSE execution
  Use above API for drivers/usb/storage/transport.c. Now that last
  User of use_sg==0, we can do the USB storage cleanups.

[PATCH 4/5] NCR5380: Use scsi_eh API for REQUEST_SENSE invocation
  All NCR5380 family of drivers used to send a REQUEST_SENSE
  command.

[PATCH 5/5] arm: fas216 Use scsi_eh API for REQUEST_SENSE invocation
	drivers/scsi/arm/fas216.c need change also.

Please Test as much as possible, and report any problems/differences.

Boaz Harrosh
 

[PATCH 1/5] scsi_error: code cleanup before refactoring of scsi_send_eh_cmnd()

[Boaz Harrosh]

  - regrouped variables for easier reviewing of next patch
  - Support of cmnd==NULL in call to scsi_send_eh_cmnd()
  - In the REQUEST_SENSE case set cmnd[4] to the size of
    sense_buffer. It was previously set by caller but needed
    to be in sync with sense_buffer size.
  - Also save/restore resid of faild command.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c8e351f..0bcfbe5 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -611,17 +611,20 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 {
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
-	int old_result = scmd->result;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	unsigned long flags;
-	struct scatterlist sgl;
+
+	unsigned char old_cmd_len;
 	unsigned char old_cmnd[MAX_COMMAND_SIZE];
 	enum dma_data_direction old_data_direction;
-	unsigned short old_use_sg;
-	unsigned char old_cmd_len;
 	unsigned old_bufflen;
 	void *old_buffer;
+	unsigned short old_use_sg;
+	int old_resid;
+	int old_result;
+
+	struct scatterlist sgl;
 	int rtn;
 
 	/*
@@ -631,15 +634,20 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	 * we will need to restore these values prior to running the actual
 	 * command.
 	 */
-	old_buffer = scmd->request_buffer;
-	old_bufflen = scmd->request_bufflen;
+	old_cmd_len = scmd->cmd_len;
 	memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
 	old_data_direction = scmd->sc_data_direction;
-	old_cmd_len = scmd->cmd_len;
+	old_bufflen = scmd->request_bufflen;
+	old_buffer = scmd->request_buffer;
 	old_use_sg = scmd->use_sg;
+	old_resid = scmd->resid;
+	old_result = scmd->result;
 
-	memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
-	memcpy(scmd->cmnd, cmnd, cmnd_size);
+	if (cmnd) {
+		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
+		memcpy(scmd->cmnd, cmnd, cmnd_size);
+		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+	}
 
 	if (copy_sense) {
 		sg_init_one(&sgl, scmd->sense_buffer,
@@ -649,6 +657,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 		scmd->request_bufflen = sgl.length;
 		scmd->request_buffer = &sgl;
 		scmd->use_sg = 1;
+		scmd->cmnd[4] = sgl.length;
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
@@ -657,7 +666,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	}
 
 	scmd->underflow = 0;
-	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
 	if (sdev->scsi_level <= SCSI_2)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
@@ -716,12 +724,13 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	/*
 	 * Restore original data
 	 */
-	scmd->request_buffer = old_buffer;
-	scmd->request_bufflen = old_bufflen;
+	scmd->cmd_len = old_cmd_len;
 	memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
 	scmd->sc_data_direction = old_data_direction;
-	scmd->cmd_len = old_cmd_len;
+	scmd->request_bufflen = old_bufflen;
+	scmd->request_buffer = old_buffer;
 	scmd->use_sg = old_use_sg;
+	scmd->resid = old_resid;
 	scmd->result = old_result;
 	return rtn;
 }
@@ -738,7 +747,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 static int scsi_request_sense(struct scsi_cmnd *scmd)
 {
 	static unsigned char generic_sense[6] =
-		{REQUEST_SENSE, 0, 0, 0, 252, 0};
+		{REQUEST_SENSE, 0, 0, 0, 0, 0};
 
 	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
 }
-- 
1.5.3.1

[PATCH 2/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Boaz Harrosh]

 - Drivers/transports that want to send a synchronous REQUEST_SENSE command
   as part of their .queuecommand sequence, have 2 new API's that facilitate
   in doing so and abstract them from scsi-ml internals.

   void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
	struct scsi_eh_save_cmnd_info *sesci, unsigned char *cmnd,
	int cmnd_size, int copy_sense) -

   Will hijack a command and prepare it for request sense if needed.
   And will save any later needed info into a scsi_eh_save_cmnd_info
   structure.

   void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
	struct scsi_eh_save_cmnd_info *sesci);

   Will undo any changes done to a command by above function. Making
   it ready for completion.

 - Re-factor scsi_send_eh_cmnd to use above APIs

Sign-off-by Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |  133 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_eh.h    |   23 ++++++++-
 2 files changed, 102 insertions(+), 54 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 0bcfbe5..0327e20 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -590,42 +590,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 }
 
 /**
- * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * scsi_prep_eh_cmnd  - Save a scsi command info as part of error recory
  * @scmd:       SCSI command structure to hijack
- * @cmnd:       CDB to send
+ * @sesci       structure to save restore information
+ * @cmnd:       CDB to send. Can be NULL if no new cmnd is needed
  * @cmnd_size:  size in bytes of @cmnd
- * @timeout:    timeout for this request
  * @copy_sense: request sense data if set to 1
  *
- * This function is used to send a scsi command down to a target device
- * as part of the error recovery process.  If @copy_sense is 0 the command
- * sent must be one that does not transfer any data.  If @copy_sense is 1
- * the command must be REQUEST_SENSE and this functions copies out the
- * sense buffer it got into @scmd->sense_buffer.
- *
- * Return value:
- *    SUCCESS or FAILED or NEEDS_RETRY
+ * This function is used to save a scsi command information before re-execution
+ * as part of an error recovery process.  If @copy_sense is 0 the command
+ * given must be one that does not transfer any data.  If @copy_sense is 1
+ * the command must be REQUEST_SENSE and this functions sets up the
+ * command buffers to be read into @scmd->sense_buffer.
  **/
-static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
-			     int cmnd_size, int timeout, int copy_sense)
+void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
+	struct scsi_eh_save_cmnd_info *sesci, unsigned char *cmnd,
+	int cmnd_size, int copy_sense)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct Scsi_Host *shost = sdev->host;
-	DECLARE_COMPLETION_ONSTACK(done);
-	unsigned long timeleft;
-	unsigned long flags;
-
-	unsigned char old_cmd_len;
-	unsigned char old_cmnd[MAX_COMMAND_SIZE];
-	enum dma_data_direction old_data_direction;
-	unsigned old_bufflen;
-	void *old_buffer;
-	unsigned short old_use_sg;
-	int old_resid;
-	int old_result;
-
-	struct scatterlist sgl;
-	int rtn;
 
 	/*
 	 * We need saved copies of a number of fields - this is because
@@ -634,14 +616,14 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	 * we will need to restore these values prior to running the actual
 	 * command.
 	 */
-	old_cmd_len = scmd->cmd_len;
-	memcpy(old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
-	old_data_direction = scmd->sc_data_direction;
-	old_bufflen = scmd->request_bufflen;
-	old_buffer = scmd->request_buffer;
-	old_use_sg = scmd->use_sg;
-	old_resid = scmd->resid;
-	old_result = scmd->result;
+	sesci->old_cmd_len = scmd->cmd_len;
+	memcpy(sesci->old_cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+	sesci->old_data_direction = scmd->sc_data_direction;
+	sesci->old_bufflen = scmd->request_bufflen;
+	sesci->old_buffer = scmd->request_buffer;
+	sesci->old_use_sg = scmd->use_sg;
+	sesci->old_resid = scmd->resid;
+	sesci->old_result = scmd->result;
 
 	if (cmnd) {
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
@@ -650,14 +632,15 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	}
 
 	if (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
+		struct scatterlist* sgl = &sesci->sense_sgl;
+		sg_init_one(sgl, scmd->sense_buffer,
 			    sizeof(scmd->sense_buffer));
 
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
-		scmd->request_buffer = &sgl;
+		scmd->request_bufflen = sgl->length;
+		scmd->request_buffer = sgl;
 		scmd->use_sg = 1;
-		scmd->cmnd[4] = sgl.length;
+		scmd->cmnd[4] = sgl->length;
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
@@ -676,7 +659,62 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	 * untransferred sense data should be interpreted as being zero.
 	 */
 	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
+}
+EXPORT_SYMBOL(scsi_eh_prep_cmnd);
 
+/**
+ * scsi_restore_eh_cmnd  - Restore a scsi command info as part of error recory
+ * @scmd:       SCSI command structure to restore
+ * @sesci	saved information from a coresponding call to scsi_prep_eh_cmnd
+ *
+ * Undo any damage done by above scsi_prep_eh_cmnd().
+ **/
+void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
+	struct scsi_eh_save_cmnd_info *sesci)
+{
+	/*
+	 * Restore original data
+	 */
+	scmd->cmd_len = sesci->old_cmd_len;
+	memcpy(scmd->cmnd, sesci->old_cmnd, sizeof(scmd->cmnd));
+	scmd->sc_data_direction = sesci->old_data_direction;
+	scmd->request_bufflen = sesci->old_bufflen;
+	scmd->request_buffer = sesci->old_buffer;
+	scmd->use_sg = sesci->old_use_sg;
+	scmd->resid = sesci->old_resid;
+	scmd->result = sesci->old_result;
+}
+EXPORT_SYMBOL(scsi_eh_restore_cmnd);
+
+/**
+ * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * @scmd:       SCSI command structure to hijack
+ * @cmnd:       CDB to send
+ * @cmnd_size:  size in bytes of @cmnd
+ * @timeout:    timeout for this request
+ * @copy_sense: request sense data if set to 1
+ *
+ * This function is used to send a scsi command down to a target device
+ * as part of the error recovery process.  If @copy_sense is 0 the command
+ * sent must be one that does not transfer any data.  If @copy_sense is 1
+ * the command must be REQUEST_SENSE and this functions copies out the
+ * sense buffer it got into @scmd->sense_buffer.
+ *
+ * Return value:
+ *    SUCCESS or FAILED or NEEDS_RETRY
+ **/
+static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
+			     int cmnd_size, int timeout, int copy_sense)
+{
+	struct scsi_device *sdev = scmd->device;
+	struct Scsi_Host *shost = sdev->host;
+	DECLARE_COMPLETION_ONSTACK(done);
+	unsigned long timeleft;
+	unsigned long flags;
+	struct scsi_eh_save_cmnd_info sesci;
+	int rtn;
+
+	scsi_eh_prep_cmnd(scmd, &sesci, cmnd, cmnd_size, copy_sense);
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -720,18 +758,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 		rtn = FAILED;
 	}
 
-
-	/*
-	 * Restore original data
-	 */
-	scmd->cmd_len = old_cmd_len;
-	memcpy(scmd->cmnd, old_cmnd, sizeof(scmd->cmnd));
-	scmd->sc_data_direction = old_data_direction;
-	scmd->request_bufflen = old_bufflen;
-	scmd->request_buffer = old_buffer;
-	scmd->use_sg = old_use_sg;
-	scmd->resid = old_resid;
-	scmd->result = old_result;
+	scsi_eh_restore_cmnd(scmd, &sesci);
 	return rtn;
 }
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index c5c0f67..07982b0 100644
--- a/include/scsi/scsi_eh.h
+++ b/include/scsi/scsi_eh.h
@@ -1,7 +1,7 @@
 #ifndef _SCSI_SCSI_EH_H
 #define _SCSI_SCSI_EH_H
 
-struct scsi_cmnd;
+#include <scsi/scsi_cmnd.h>
 struct scsi_device;
 struct Scsi_Host;
 
@@ -65,4 +65,25 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len,
 
 extern int scsi_reset_provider(struct scsi_device *, int);
 
+struct scsi_eh_save_cmnd_info {
+	int old_result;
+	enum dma_data_direction old_data_direction;
+	unsigned char old_cmd_len;
+	unsigned char old_cmnd[MAX_COMMAND_SIZE];
+	
+	void* old_buffer;
+	unsigned old_bufflen;
+	unsigned short old_use_sg;
+	int old_resid;
+
+	struct scatterlist sense_sgl;
+};
+
+extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
+	struct scsi_eh_save_cmnd_info *sesci, unsigned char *cmnd,
+	int cmnd_size, int copy_sense);
+
+extern void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
+	struct scsi_eh_save_cmnd_info *sesci);
+
 #endif /* _SCSI_SCSI_EH_H */
-- 
1.5.3.1

Re: [PATCH 2/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Christoph Hellwig]

> +void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
> +	struct scsi_eh_save_cmnd_info *sesci, unsigned char *cmnd,
> +	int cmnd_size, int copy_sense)

I think just struct "struct scsi_eh_save *save" is descriptive enough and
almost fits on a line as well..  Also continuation of the prototype
is indented by two tabs normally.

> +struct scsi_eh_save_cmnd_info {
> +	int old_result;
> +	enum dma_data_direction old_data_direction;
> +	unsigned char old_cmd_len;
> +	unsigned char old_cmnd[MAX_COMMAND_SIZE];
> +	
> +	void* old_buffer;

	void *old_buffer;

> +	unsigned old_bufflen;
> +	unsigned short old_use_sg;
> +	int old_resid;
> +
> +	struct scatterlist sense_sgl;
> +};

I think you can kill the old prefixes in the struct, they're saved
per defintion.

Except for these cosmetic details the patch looks fine to me,
thanks a lot!

Re: [PATCH 2/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Randy Dunlap]

On Mon, 10 Sep 2007 17:23:52 +0300 Boaz Harrosh wrote:


Hi Boaz,

Just a small nit:  several of the kernel-doc lines need ':' added.
See below.

> Sign-off-by Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/scsi_error.c |  133 +++++++++++++++++++++++++++------------------
>  include/scsi/scsi_eh.h    |   23 ++++++++-
>  2 files changed, 102 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 0bcfbe5..0327e20 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -590,42 +590,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>  }
>  
>  /**
> - * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
> + * scsi_prep_eh_cmnd  - Save a scsi command info as part of error recory
>   * @scmd:       SCSI command structure to hijack
> - * @cmnd:       CDB to send
> + * @sesci       structure to save restore information

 * @sesci:

> + * @cmnd:       CDB to send. Can be NULL if no new cmnd is needed
>   * @cmnd_size:  size in bytes of @cmnd
> - * @timeout:    timeout for this request
>   * @copy_sense: request sense data if set to 1
>   *
>   **/
> -static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
> -			     int cmnd_size, int timeout, int copy_sense)
> +void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
> +	struct scsi_eh_save_cmnd_info *sesci, unsigned char *cmnd,
> +	int cmnd_size, int copy_sense)
>  {

> @@ -676,7 +659,62 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
>  
> +/**
> + * scsi_restore_eh_cmnd  - Restore a scsi command info as part of error recory
> + * @scmd:       SCSI command structure to restore
> + * @sesci	saved information from a coresponding call to scsi_prep_eh_cmnd

 * @sesci:

> + *
> + * Undo any damage done by above scsi_prep_eh_cmnd().
> + **/
> +void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
> +	struct scsi_eh_save_cmnd_info *sesci)
> +{
> +}
> +EXPORT_SYMBOL(scsi_eh_restore_cmnd);

Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[PATCH 3/5] usb: transport.c use scsi_eh API in REQUEST_SENSE execution

[Boaz Harrosh]

  - Use new scsi_eh_prep/restor_cmnd() for synchronous
    REQUEST_SENSE invocation.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/usb/storage/transport.c |   51 ++++++--------------------------------
 1 files changed, 8 insertions(+), 43 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 323293a..92dec34 100644
--- a/drivers/usb/storage/transport.c
+++ b/drivers/usb/storage/transport.c
@@ -50,7 +50,7 @@
 #include <linux/slab.h>
 
 #include <scsi/scsi.h>
-#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_eh.h>
 #include <scsi/scsi_device.h>
 
 #include "usb.h"
@@ -580,62 +580,27 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 	/* Now, if we need to do the auto-sense, let's do it */
 	if (need_auto_sense) {
 		int temp_result;
-		void* old_request_buffer;
-		unsigned short old_sg;
-		unsigned old_request_bufflen;
-		unsigned char old_sc_data_direction;
-		unsigned char old_cmd_len;
-		unsigned char old_cmnd[MAX_COMMAND_SIZE];
-		int old_resid;
+		struct scsi_eh_save_cmnd_info sesci;
+		static unsigned char generic_sense[6] =
+			{REQUEST_SENSE, 0, 0, 0, 0, 0};
 
 		US_DEBUGP("Issuing auto-REQUEST_SENSE\n");
 
-		/* save the old command */
-		memcpy(old_cmnd, srb->cmnd, MAX_COMMAND_SIZE);
-		old_cmd_len = srb->cmd_len;
-
-		/* set the command and the LUN */
-		memset(srb->cmnd, 0, MAX_COMMAND_SIZE);
-		srb->cmnd[0] = REQUEST_SENSE;
-		srb->cmnd[1] = old_cmnd[1] & 0xE0;
-		srb->cmnd[4] = 18;
-
+		scsi_eh_prep_cmnd(srb, &sesci, generic_sense,
+			sizeof(generic_sense), 1);
+		
 		/* FIXME: we must do the protocol translation here */
 		if (us->subclass == US_SC_RBC || us->subclass == US_SC_SCSI)
 			srb->cmd_len = 6;
 		else
 			srb->cmd_len = 12;
 
-		/* set the transfer direction */
-		old_sc_data_direction = srb->sc_data_direction;
-		srb->sc_data_direction = DMA_FROM_DEVICE;
-
-		/* use the new buffer we have */
-		old_request_buffer = srb->request_buffer;
-		srb->request_buffer = us->sensebuf;
-
-		/* set the buffer length for transfer */
-		old_request_bufflen = srb->request_bufflen;
-		srb->request_bufflen = US_SENSE_SIZE;
-
-		/* set up for no scatter-gather use */
-		old_sg = srb->use_sg;
-		srb->use_sg = 0;
-
 		/* issue the auto-sense command */
-		old_resid = srb->resid;
 		srb->resid = 0;
 		temp_result = us->transport(us->srb, us);
 
 		/* let's clean up right away */
-		memcpy(srb->sense_buffer, us->sensebuf, US_SENSE_SIZE);
-		srb->resid = old_resid;
-		srb->request_buffer = old_request_buffer;
-		srb->request_bufflen = old_request_bufflen;
-		srb->use_sg = old_sg;
-		srb->sc_data_direction = old_sc_data_direction;
-		srb->cmd_len = old_cmd_len;
-		memcpy(srb->cmnd, old_cmnd, MAX_COMMAND_SIZE);
+		scsi_eh_restore_cmnd(srb, &sesci);
 
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 			US_DEBUGP("-- auto-sense aborted\n");
-- 
1.5.3.1

[PATCH 4/5] NCR5380: Use scsi_eh API for REQUEST_SENSE invocation

[Boaz Harrosh]

  - Use new scsi_eh_prep/restor_cmnd() for synchronous
    REQUEST_SENSE invocation.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/NCR5380.c       |   25 +++++++++++--------------
 drivers/scsi/NCR5380.h       |    7 +++++++
 drivers/scsi/atari_NCR5380.c |   25 ++++++++++---------------
 drivers/scsi/sun3_NCR5380.c  |   20 +++++++++-----------
 4 files changed, 37 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index f8e449a..1ca3727 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1542,9 +1542,7 @@ part2:
 	hostdata->connected = cmd;
 	hostdata->busy[cmd->device->id] |= (1 << cmd->device->lun);
 
-	if (cmd->SCp.ptr != (char *)cmd->sense_buffer) {
-		initialize_SCp(cmd);
-	}
+	initialize_SCp(cmd);
 
 	return 0;
 
@@ -2280,19 +2278,18 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) {
 						cmd->result = (cmd->result & 0x00ffff) | (DID_ERROR << 16);
 
 #ifdef AUTOSENSE
+					if ((cmd->cmnd[0] == REQUEST_SENSE) &&
+						hostdata->sesci.old_cmd_len) {
+						scsi_eh_restore_cmnd(cmd, &hostdata->sesci);
+						hostdata->sesci.old_cmd_len = 0 ;
+					}
+
 					if ((cmd->cmnd[0] != REQUEST_SENSE) && (status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+						unsigned char cmnd[6] = {REQUEST_SENSE ,0 ,0 ,0 ,0 ,0};
+						scsi_eh_prep_cmnd(cmd, &hostdata->sesci, cmnd,
+						                                     sizeof(cmnd), 1);
+
 						dprintk(NDEBUG_AUTOSENSE, ("scsi%d : performing request sense\n", instance->host_no));
-						cmd->cmnd[0] = REQUEST_SENSE;
-						cmd->cmnd[1] &= 0xe0;
-						cmd->cmnd[2] = 0;
-						cmd->cmnd[3] = 0;
-						cmd->cmnd[4] = sizeof(cmd->sense_buffer);
-						cmd->cmnd[5] = 0;
-
-						cmd->SCp.buffer = NULL;
-						cmd->SCp.buffers_residual = 0;
-						cmd->SCp.ptr = (char *) cmd->sense_buffer;
-						cmd->SCp.this_residual = sizeof(cmd->sense_buffer);
 
 						LIST(cmd, hostdata->issue_queue);
 						cmd->host_scribble = (unsigned char *)
diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index bccf13f..73855fc 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -30,6 +30,10 @@
 
 #include <linux/interrupt.h>
 
+#ifdef AUTOSENSE
+#include <scsi/scsi_eh.h>
+#endif
+
 #define NCR5380_PUBLIC_RELEASE 7
 #define NCR53C400_PUBLIC_RELEASE 2
 
@@ -281,6 +285,9 @@ struct NCR5380_hostdata {
 	unsigned pendingr;
 	unsigned pendingw;
 #endif
+#ifdef AUTOSENSE
+	struct scsi_eh_save_cmnd_info sesci;
+#endif
 };
 
 #ifdef __KERNEL__
diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 03dbe60..a4bdd4c 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2235,24 +2235,19 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance)
 						cmd->result = (cmd->result & 0x00ffff) | (DID_ERROR << 16);
 
 #ifdef AUTOSENSE
+					if ((cmd->cmnd[0] == REQUEST_SENSE) &&
+						hostdata->sesci.old_cmd_len) {
+						scsi_eh_restore_cmnd(cmd, &hostdata->sesci);
+						hostdata->sesci.old_cmd_len = 0 ;
+					}
+
 					if ((cmd->cmnd[0] != REQUEST_SENSE) &&
 					    (status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+						unsigned char cmnd[6] = {REQUEST_SENSE ,0 ,0 ,0 ,0 ,0};
+						scsi_eh_prep_cmnd(cmd, &hostdata->sesci, cmnd,
+						                                     sizeof(cmnd), 1);
+
 						ASEN_PRINTK("scsi%d: performing request sense\n", HOSTNO);
-						cmd->cmnd[0] = REQUEST_SENSE;
-						cmd->cmnd[1] &= 0xe0;
-						cmd->cmnd[2] = 0;
-						cmd->cmnd[3] = 0;
-						cmd->cmnd[4] = sizeof(cmd->sense_buffer);
-						cmd->cmnd[5] = 0;
-						cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
-
-						cmd->use_sg = 0;
-						/* this is initialized from initialize_SCp
-						cmd->SCp.buffer = NULL;
-						cmd->SCp.buffers_residual = 0;
-						*/
-						cmd->request_buffer = (char *) cmd->sense_buffer;
-						cmd->request_bufflen = sizeof(cmd->sense_buffer);
 
 						local_irq_save(flags);
 						LIST(cmd,hostdata->issue_queue);
diff --git a/drivers/scsi/sun3_NCR5380.c b/drivers/scsi/sun3_NCR5380.c
index 98e3fe1..7a5287e 100644
--- a/drivers/scsi/sun3_NCR5380.c
+++ b/drivers/scsi/sun3_NCR5380.c
@@ -2254,25 +2254,23 @@ static void NCR5380_information_transfer (struct Scsi_Host *instance)
 			cmd->result = (cmd->result & 0x00ffff) | (DID_ERROR << 16);
 		    
 #ifdef AUTOSENSE
+		    if ((cmd->cmnd[0] == REQUEST_SENSE) &&
+			                        hostdata->sesci.old_cmd_len) {
+			scsi_eh_restore_cmnd(cmd, &hostdata->sesci);
+			hostdata->sesci.old_cmd_len = 0 ;
+		    }
+
 		    if ((cmd->cmnd[0] != REQUEST_SENSE) && 
 			(status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+			unsigned char cmnd[6] = {REQUEST_SENSE ,0 ,0 ,0 ,0 ,0};
+			scsi_eh_prep_cmnd(cmd, &hostdata->sesci, cmnd,
+			                                       sizeof(cmnd), 1);
 			ASEN_PRINTK("scsi%d: performing request sense\n",
 				    HOSTNO);
-			cmd->cmnd[0] = REQUEST_SENSE;
-			cmd->cmnd[1] &= 0xe0;
-			cmd->cmnd[2] = 0;
-			cmd->cmnd[3] = 0;
-			cmd->cmnd[4] = sizeof(cmd->sense_buffer);
-			cmd->cmnd[5] = 0;
-			cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]);
-
-			cmd->use_sg = 0;
 			/* this is initialized from initialize_SCp 
 			cmd->SCp.buffer = NULL;
 			cmd->SCp.buffers_residual = 0;
 			*/
-			cmd->request_buffer = (char *) cmd->sense_buffer;
-			cmd->request_bufflen = sizeof(cmd->sense_buffer);
 
 			local_irq_save(flags);
 			LIST(cmd,hostdata->issue_queue);
-- 
1.5.3.1

[PATCH 5/5] arm: fas216 Use scsi_eh API for REQUEST_SENSE invocation

[Boaz Harrosh]

  - Use new scsi_eh_prep/restor_cmnd() for synchronous
    REQUEST_SENSE invocation.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/arm/fas216.c |    5 ++---
 drivers/scsi/arm/fas216.h |    3 +++
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
index fb5f202..d80702b 100644
--- a/drivers/scsi/arm/fas216.c
+++ b/drivers/scsi/arm/fas216.c
@@ -2018,6 +2018,7 @@ static void fas216_rq_sns_done(FAS216_Info *info, struct scsi_cmnd *SCpnt,
 	 * the upper layers to process.  This would have been set
 	 * correctly by fas216_std_done.
 	 */
+	scsi_eh_restore_cmnd(SCpnt, &info->sesci);
 	SCpnt->scsi_done(SCpnt);
 }
 
@@ -2103,6 +2104,7 @@ request_sense:
 	if (SCpnt->cmnd[0] == REQUEST_SENSE)
 		goto done;
 
+	scsi_eh_prep_cmnd(SCpnt, &info->sesci, NULL, 0, 1);
 	fas216_log_target(info, LOG_CONNECT, SCpnt->device->id,
 			  "requesting sense");
 	memset(SCpnt->cmnd, 0, sizeof (SCpnt->cmnd));
@@ -2117,9 +2119,6 @@ request_sense:
 	SCpnt->SCp.phase = sizeof(SCpnt->sense_buffer);
 	SCpnt->SCp.Message = 0;
 	SCpnt->SCp.Status = 0;
-	SCpnt->request_bufflen = sizeof(SCpnt->sense_buffer);
-	SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	SCpnt->use_sg = 0;
 	SCpnt->tag = 0;
 	SCpnt->host_scribble = (void *)fas216_rq_sns_done;
 
diff --git a/drivers/scsi/arm/fas216.h b/drivers/scsi/arm/fas216.h
index 00e5f05..8449416 100644
--- a/drivers/scsi/arm/fas216.h
+++ b/drivers/scsi/arm/fas216.h
@@ -16,6 +16,8 @@
 #define NO_IRQ 255
 #endif
 
+#include <scsi/scsi_eh.h>
+
 #include "queue.h"
 #include "msgqueue.h"
 
@@ -311,6 +313,7 @@ typedef struct {
 
 	/* miscellaneous */
 	int			internal_done;		/* flag to indicate request done */
+	struct scsi_eh_save_cmnd_info *sesci;		/* holds request sense restore info */
 	unsigned long		magic_end;
 } FAS216_Info;
 
-- 
1.5.3.1

Re: [patchset 0/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Alan Stern]

On Mon, 10 Sep 2007, Boaz Harrosh wrote:

> In motivation to abstract scsi_cmnd members and insulate
> drivers/transports from scsi_cmnd internals. The last
> place left was the REQUEST_SENSE sequence when done
> synchronous, by drivers.
> 
> Also all these drivers would do a use_sg==0 command
> invocation, preventing from doing cleanups on these
> drivers/transports. So this patchset is left-overs
> from Christoph's 2.6.18 cleanups.
> 
> In this patchset I have re-factored scsi_error to
> make it possible for drivers to use an abstract
> (easy to use, I hope) API for invoking a REQUEST_SENSE
> command. The API is declared in scsi/scsi_eh.h

This is a good idea, but the implementation has a few problems.

A trivial problem is that quilt complains about patch 3, which leaves 
extra whitespace at the end of line 591 in transport.c.

More seriously, why make the caller define a static generic_sense 
array?  Why not put the array in scsi_eh_prep_cmnd(), where it can be 
used whenever copy_sense is nonzero?

The line initializing the sense buffer to zero should be inside the 
copy_sense conditional block.

The code setting the LUN bits in scmd[1] is wrong.  It should be like 
the code in scsi_dispatch_cmd(); i.e., those bits should not be set if 
the scsi_level is SCSI_UNKNOWN.

Finally, a really serious problem.  The code sets the buffer length for
the REQUEST SENSE command to be the length of scmd->sense_buffer, which
is 96.  But many USB devices won't work properly unless the requesed
sense data length is 18.  The appropriate adjustment must be added to 
transport.c in patch 3.

Alan Stern

Re: [patchset 0/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[Matthew Dharm]

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

On Mon, Sep 10, 2007 at 05:11:23PM +0300, Boaz Harrosh wrote:
> 
> In motivation to abstract scsi_cmnd members and insulate
> drivers/transports from scsi_cmnd internals. The last
> place left was the REQUEST_SENSE sequence when done
> synchronous, by drivers.

This probably isn't serious, but I noticed one thing (beyond what Alan's
analysis noted)...

I've always assumed that the scatterlist structs passed to an HCD were,
themselves, allocated from DMA-able memory.  That is, not just the transfer
buffers themselves, but the struct scatterlist also.

In this implementation, the struct scatterlist used for the single-element
transfer of the request sense buffer is part of the
struct scsi_eh_save_cmnd_info, which is allocated on the stack (for at
least usb-storage).  And, stack isn't DMA-able on all arches.

It is not a problem for usb-storage, since the struct scatterlists are
processed in code into a series of URBs, so nobody actually does DMA the
scatterlist structures.  However, I don't know enough about the other HCDs
to be certain about them.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Now payink attention, please.  This is mouse.  Click-click. Easy to 
use, da? Now you try...
					-- Pitr to Miranda
User Friendly, 10/11/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patchset 0/5] Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

[James Bottomley]

On Mon, 2007-09-10 at 10:03 -0700, Matthew Dharm wrote:
> On Mon, Sep 10, 2007 at 05:11:23PM +0300, Boaz Harrosh wrote:
> > 
> > In motivation to abstract scsi_cmnd members and insulate
> > drivers/transports from scsi_cmnd internals. The last
> > place left was the REQUEST_SENSE sequence when done
> > synchronous, by drivers.
> 
> This probably isn't serious, but I noticed one thing (beyond what Alan's
> analysis noted)...
> 
> I've always assumed that the scatterlist structs passed to an HCD were,
> themselves, allocated from DMA-able memory.  That is, not just the transfer
> buffers themselves, but the struct scatterlist also.

They are, but this is an artifact of the command allocation.  Commands
(as in the 6, 10 12 or 16 bytes of command sequence) are designed to be
DMAable, meaning that everything that makes up a struct scsi_cmnd is
automatically DMAable.  However, the design is for the dma_mapped
scatterlist to be converted to a form the underlying HBA can use.  I
don't currently know of any HBA whose descriptor format matches those of
struct scatterlist, so I don't think there'll be any impact to putting
the scatterlist in memory that might not be DMAable by the HBA.

> In this implementation, the struct scatterlist used for the single-element
> transfer of the request sense buffer is part of the
> struct scsi_eh_save_cmnd_info, which is allocated on the stack (for at
> least usb-storage).  And, stack isn't DMA-able on all arches.
> 
> It is not a problem for usb-storage, since the struct scatterlists are
> processed in code into a series of URBs, so nobody actually does DMA the
> scatterlist structures.  However, I don't know enough about the other HCDs
> to be certain about them.

James