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

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

[Boaz Harrosh]

Thank you Christoph, Randy, Alen for your comments.
Here is ver 2 incorporating all your suggestions.

On Mon, Sep 10 2007 at 18:12 +0300, Christoph Hellwig <hch@infradead.org> wrote:
> 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.
> I think you can kill the old prefixes in the struct, they're saved
> per defintion.
Done

On Mon, Sep 10 2007 at 18:58 +0300, Alan Stern <stern@rowland.harvard.edu> wrote:
> A trivial problem is that quilt complains about patch 3, which leaves 
> extra whitespace at the end of line 591 in transport.c.
> 
Thanks

> 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?
> 
I hope you like my solution

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

> 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.
> 
Thanks, good catch

> 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.
> 
I was afraid of that at the beginning. But testing both with a Seagate
USB sata hard-disk and Sandisk 2Gb usb-stick. They both returned a short
read of 56 but other wise were happy. So I thought it is leftovers from
The time the sense buffer was driver allocated. Also the standard 
recommends a short-read behavior. But I have taken your advise to hart
and changed the code accordingly. Please check me out.

----------------------------------------------------------------  

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 ver2 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 copy_sense case set transfer size to the minimum
    size of sense_buffer and passed copy_sense. cmnd[4] is
    set accordingly.
  - REQUEST_SENSE is set into cmnd[0] so if copy_sense is
    not Zero passed cmnd can/should be NULL.
  - Also save/restore resid of faild command.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c8e351f..20a72aa 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -607,21 +607,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
  *    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)
+			     int cmnd_size, int timeout, unsigned copy_sense)
 {
 	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,24 +634,37 @@ 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,
 			    sizeof(scmd->sense_buffer));
-
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
 		scmd->request_buffer = &sgl;
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
+		scmd->request_bufflen = min_t(unsigned,
+		                       sizeof(scmd->sense_buffer), copy_sense);
 		scmd->use_sg = 1;
+		memset(scmd->cmnd, 0, 6);
+		scmd->cmnd[0] = REQUEST_SENSE;
+		scmd->cmnd[4] = scmd->request_bufflen;
+		/*
+		 * Zero the sense buffer.  The scsi spec mandates that any
+		 * untransferred sense data should be interpreted as being zero.
+		 */
+		memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
@@ -657,18 +673,11 @@ 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)
+	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
 
-	/*
-	 * Zero the sense buffer.  The scsi spec mandates that any
-	 * untransferred sense data should be interpreted as being zero.
-	 */
-	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
-
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -716,12 +725,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;
 }
@@ -737,10 +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};
-
-	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
+	return scsi_send_eh_cmnd(scmd, NULL, 0, SENSE_TIMEOUT, ~0);
 }
 
 /**
-- 
1.5.3.1

[PATCH ver2 2/5] scsi_error: 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 *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 structure.

   void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
	struct scsi_eh_save *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

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |  125 ++++++++++++++++++++++++++-------------------
 include/scsi/scsi_eh.h    |   23 ++++++++-
 2 files changed, 95 insertions(+), 53 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 20a72aa..a02400c 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -590,42 +590,23 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 }
 
 /**
- * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recory
  * @scmd:       SCSI command structure to hijack
- * @cmnd:       CDB to send
+ * @ses:        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
+ * @copy_sense: size of sense data to copy. or 0
  *
- * 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 != 0
+ * the command should be NULL and this functions sets up the cmnd and
+ * 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, unsigned copy_sense)
+void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
+			unsigned char *cmnd, int cmnd_size, unsigned 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 +615,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;
+	ses->cmd_len = scmd->cmd_len;
+	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+	ses->data_direction = scmd->sc_data_direction;
+	ses->bufflen = scmd->request_bufflen;
+	ses->buffer = scmd->request_buffer;
+	ses->use_sg = scmd->use_sg;
+	ses->resid = scmd->resid;
+	ses->result = scmd->result;
 
 	if (cmnd) {
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
@@ -650,9 +631,9 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	}
 
 	if (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
+		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
 			    sizeof(scmd->sense_buffer));
-		scmd->request_buffer = &sgl;
+		scmd->request_buffer = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->request_bufflen = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), copy_sense);
@@ -677,7 +658,58 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
+}
+EXPORT_SYMBOL(scsi_eh_prep_cmnd);
+
+/**
+ * scsi_eh_restore_cmnd  - Restore a scsi command info as part of error recory
+ * @scmd:       SCSI command structure to restore
+ * @ses:        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 *ses)
+{
+	/*
+	 * Restore original data
+	 */
+	scmd->cmd_len = ses->cmd_len;
+	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
+	scmd->sc_data_direction = ses->data_direction;
+	scmd->request_bufflen = ses->bufflen;
+	scmd->request_buffer = ses->buffer;
+	scmd->use_sg = ses->use_sg;
+	scmd->resid = ses->resid;
+	scmd->result = ses->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: size of sense data to copy or 0
+ *
+ * This function is used to send a scsi command down to a target device
+ * as part of the error recovery process. See also scsi_eh_prep_cmnd() above.
+ *
+ * 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, unsigned 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 ses;
+	int rtn;
+
+	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, copy_sense);
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -721,18 +753,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, &ses);
 	return rtn;
 }
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index c5c0f67..85b0447 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 {
+	int result;
+	enum dma_data_direction data_direction;
+	unsigned char cmd_len;
+	unsigned char cmnd[MAX_COMMAND_SIZE];
+
+	void *buffer;
+	unsigned bufflen;
+	unsigned short use_sg;
+	int resid;
+
+	struct scatterlist sense_sgl;
+};
+
+extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
+		struct scsi_eh_save *ses, unsigned char *cmnd,
+		int cmnd_size, unsigned copy_sense);
+
+extern void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
+		struct scsi_eh_save *ses);
+
 #endif /* _SCSI_SCSI_EH_H */
-- 
1.5.3.1

[PATCH ver2 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 |   46 +++-----------------------------------
 1 files changed, 4 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/storage/transport.c b/drivers/usb/storage/transport.c
index 323293a..c646750 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,25 +580,11 @@ 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 ses;
 
 		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, &ses, NULL, 0, US_SENSE_SIZE);
 
 		/* FIXME: we must do the protocol translation here */
 		if (us->subclass == US_SC_RBC || us->subclass == US_SC_SCSI)
@@ -606,36 +592,12 @@ void usb_stor_invoke_transport(struct scsi_cmnd *srb, struct us_data *us)
 		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, &ses);
 
 		if (test_bit(US_FLIDX_TIMED_OUT, &us->flags)) {
 			US_DEBUGP("-- auto-sense aborted\n");
-- 
1.5.3.1

[PATCH ver2 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       |   23 +++++++++--------------
 drivers/scsi/NCR5380.h       |    7 +++++++
 drivers/scsi/atari_NCR5380.c |   23 ++++++++---------------
 drivers/scsi/sun3_NCR5380.c  |   18 +++++++-----------
 4 files changed, 31 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index f8e449a..575c715 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,16 @@ 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->ses.cmd_len) {
+						scsi_eh_restore_cmnd(cmd, &hostdata->ses);
+						hostdata->ses.cmd_len = 0 ;
+					}
+
 					if ((cmd->cmnd[0] != REQUEST_SENSE) && (status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+						scsi_eh_prep_cmnd(cmd, &hostdata->ses, NULL, 0, ~0);
+
 						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..f9587e6 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 ses;
+#endif
 };
 
 #ifdef __KERNEL__
diff --git a/drivers/scsi/atari_NCR5380.c b/drivers/scsi/atari_NCR5380.c
index 03dbe60..b253d37 100644
--- a/drivers/scsi/atari_NCR5380.c
+++ b/drivers/scsi/atari_NCR5380.c
@@ -2235,24 +2235,17 @@ 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->ses.cmd_len) {
+						scsi_eh_restore_cmnd(cmd, &hostdata->ses);
+						hostdata->ses.cmd_len = 0 ;
+					}
+
 					if ((cmd->cmnd[0] != REQUEST_SENSE) &&
 					    (status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+						scsi_eh_prep_cmnd(cmd, &hostdata->ses, NULL, 0, ~0);
+
 						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..74081dd 100644
--- a/drivers/scsi/sun3_NCR5380.c
+++ b/drivers/scsi/sun3_NCR5380.c
@@ -2254,25 +2254,21 @@ 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->ses.cmd_len) {
+			scsi_eh_restore_cmnd(cmd, &hostdata->ses);
+			hostdata->ses.cmd_len = 0 ;
+		    }
+
 		    if ((cmd->cmnd[0] != REQUEST_SENSE) && 
 			(status_byte(cmd->SCp.Status) == CHECK_CONDITION)) {
+			scsi_eh_prep_cmnd(cmd, &hostdata->ses, NULL, 0, ~0);
 			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 ver2 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 |   16 +++-------------
 drivers/scsi/arm/fas216.h |    3 +++
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
index fb5f202..a715632 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->ses);
 	SCpnt->scsi_done(SCpnt);
 }
 
@@ -2103,23 +2104,12 @@ request_sense:
 	if (SCpnt->cmnd[0] == REQUEST_SENSE)
 		goto done;
 
+	scsi_eh_prep_cmnd(SCpnt, &info->ses, NULL, 0, ~0);
 	fas216_log_target(info, LOG_CONNECT, SCpnt->device->id,
 			  "requesting sense");
-	memset(SCpnt->cmnd, 0, sizeof (SCpnt->cmnd));
-	SCpnt->cmnd[0] = REQUEST_SENSE;
-	SCpnt->cmnd[1] = SCpnt->device->lun << 5;
-	SCpnt->cmnd[4] = sizeof(SCpnt->sense_buffer);
-	SCpnt->cmd_len = COMMAND_SIZE(SCpnt->cmnd[0]);
-	SCpnt->SCp.buffer = NULL;
-	SCpnt->SCp.buffers_residual = 0;
-	SCpnt->SCp.ptr = (char *)SCpnt->sense_buffer;
-	SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
-	SCpnt->SCp.phase = sizeof(SCpnt->sense_buffer);
+	init_SCp(SCpnt);
 	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..c39882e 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	*ses;		/* holds request sense restore info */
 	unsigned long		magic_end;
 } FAS216_Info;
 
-- 
1.5.3.1

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

[Matthew Dharm]

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

On Mon, Sep 10, 2007 at 10:35:25PM +0300, Boaz Harrosh wrote:
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 20a72aa..a02400c 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -590,42 +590,23 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>  }
>  
>  /**
> - * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
> + * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recory
>   * @scmd:       SCSI command structure to hijack
> - * @cmnd:       CDB to send
> + * @ses:        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
> + * @copy_sense: size of sense data to copy. or 0
>   *
> - * 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 != 0
> + * the command should be NULL and this functions sets up the cmnd and
> + * 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, unsigned copy_sense)
> +void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
> +			unsigned char *cmnd, int cmnd_size, unsigned copy_sense)

I'm not terribly keen on the naming of copy_sense, tho I see what you're
trying to do.  A better name and better comments about what it means when
!= 0 would fix this.

Matt

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

My mother not mind to die for stoppink Windows NT!  She is rememberink 
Stalin!
					-- Pitr
User Friendly, 9/6/1998

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

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

[Boaz Harrosh]

On Tue, Sep 11 2007 at 0:15 +0300, Matthew Dharm <mdharm-scsi@one-eyed-alien.net> wrote:
> On Mon, Sep 10, 2007 at 10:35:25PM +0300, Boaz Harrosh wrote:
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 20a72aa..a02400c 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -590,42 +590,23 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>>  }
>>  
>>  /**
>> - * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
>> + * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recory
>>   * @scmd:       SCSI command structure to hijack
>> - * @cmnd:       CDB to send
>> + * @ses:        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
>> + * @copy_sense: size of sense data to copy. or 0
>>   *
>> - * 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 != 0
>> + * the command should be NULL and this functions sets up the cmnd and
>> + * 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, unsigned copy_sense)
>> +void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
>> +			unsigned char *cmnd, int cmnd_size, unsigned copy_sense)
> 
> I'm not terribly keen on the naming of copy_sense, tho I see what you're
> trying to do.  A better name and better comments about what it means when
> != 0 would fix this.
> 
> Matt
> 
When you're right you're right. This is what happens when you try to
write comprehensible comments at 22:45 ;)

I will send first two patches as replays to the original patches

Thanks

[PATCH ver3 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 copy_sense case set transfer size to the minimum
    size of sense_buffer and passed @sense_bytes. cmnd[4] is
    set accordingly.
  - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
    not Zero passed cmnd can/should be NULL.
  - Also save/restore resid of faild command.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c8e351f..46d57f1 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -607,21 +607,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
  *    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)
+			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	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,24 +634,36 @@ 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 (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
-			    sizeof(scmd->sense_buffer));
+	if (cmnd) {
+		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
+		memcpy(scmd->cmnd, cmnd, cmnd_size);
+		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+	}
 
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
+	if (sense_bytes) {
+		scmd->request_bufflen = min_t(unsigned,
+		                       sizeof(scmd->sense_buffer), sense_bytes);
+		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
 		scmd->request_buffer = &sgl;
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->use_sg = 1;
+		memset(scmd->cmnd, 0, 6);
+		scmd->cmnd[0] = REQUEST_SENSE;
+		scmd->cmnd[4] = scmd->request_bufflen;
+		/*
+		 * Zero the sense buffer.  The scsi spec mandates that any
+		 * untransferred sense data should be interpreted as being zero.
+		 */
+		memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
@@ -657,18 +672,11 @@ 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)
+	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
 
-	/*
-	 * Zero the sense buffer.  The scsi spec mandates that any
-	 * untransferred sense data should be interpreted as being zero.
-	 */
-	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
-
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -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;
 }
@@ -737,10 +746,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};
-
-	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
+	return scsi_send_eh_cmnd(scmd, NULL, 0, SENSE_TIMEOUT, ~0);
 }
 
 /**
-- 
1.5.3.1

[PATCH ver3 2/5] scsi_error: 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 *sesci, unsigned char *cmnd,
	int cmnd_size, int sense_bytes)

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

   void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
	struct scsi_eh_save *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

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |  126 ++++++++++++++++++++++++++------------------
 include/scsi/scsi_eh.h    |   23 ++++++++-
 2 files changed, 96 insertions(+), 53 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 46d57f1..cd9fb34 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -590,42 +590,23 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 }
 
 /**
- * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recory
  * @scmd:       SCSI command structure to hijack
- * @cmnd:       CDB to send
+ * @ses:        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
+ * @sense_bytes: size of sense data to copy. or 0
  *
- * 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 @sense_bytes is 0 the command
+ * given must be one that does not transfer any data.  If @sense_bytes != 0
+ * the command should be NULL and this functions sets up the cmnd and
+ * command buffers to read @sense_bytes into @scmd->sense_buffer.
  **/
-static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
-			     int cmnd_size, int timeout, unsigned sense_bytes)
+void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
+			unsigned char *cmnd, int cmnd_size, unsigned sense_bytes)
 {
 	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 +615,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;
+	ses->cmd_len = scmd->cmd_len;
+	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+	ses->data_direction = scmd->sc_data_direction;
+	ses->bufflen = scmd->request_bufflen;
+	ses->buffer = scmd->request_buffer;
+	ses->use_sg = scmd->use_sg;
+	ses->resid = scmd->resid;
+	ses->result = scmd->result;
 
 	if (cmnd) {
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
@@ -652,8 +633,9 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	if (sense_bytes) {
 		scmd->request_bufflen = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), sense_bytes);
-		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
-		scmd->request_buffer = &sgl;
+		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
+		                                       scmd->request_bufflen);
+		scmd->request_buffer = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->use_sg = 1;
 		memset(scmd->cmnd, 0, 6);
@@ -676,7 +658,58 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
+}
+EXPORT_SYMBOL(scsi_eh_prep_cmnd);
+
+/**
+ * scsi_eh_restore_cmnd  - Restore a scsi command info as part of error recory
+ * @scmd:       SCSI command structure to restore
+ * @ses:        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 *ses)
+{
+	/*
+	 * Restore original data
+	 */
+	scmd->cmd_len = ses->cmd_len;
+	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
+	scmd->sc_data_direction = ses->data_direction;
+	scmd->request_bufflen = ses->bufflen;
+	scmd->request_buffer = ses->buffer;
+	scmd->use_sg = ses->use_sg;
+	scmd->resid = ses->resid;
+	scmd->result = ses->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
+ * @sense_bytes: size of sense data to copy or 0
+ *
+ * This function is used to send a scsi command down to a target device
+ * as part of the error recovery process. See also scsi_eh_prep_cmnd() above.
+ *
+ * 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, unsigned sense_bytes)
+{
+	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 ses;
+	int rtn;
+
+	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -720,18 +753,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, &ses);
 	return rtn;
 }
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index c5c0f67..44224ba 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 {
+	int result;
+	enum dma_data_direction data_direction;
+	unsigned char cmd_len;
+	unsigned char cmnd[MAX_COMMAND_SIZE];
+
+	void *buffer;
+	unsigned bufflen;
+	unsigned short use_sg;
+	int resid;
+
+	struct scatterlist sense_sgl;
+};
+
+extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
+		struct scsi_eh_save *ses, unsigned char *cmnd,
+		int cmnd_size, unsigned sense_bytes);
+
+extern void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
+		struct scsi_eh_save *ses);
+
 #endif /* _SCSI_SCSI_EH_H */
-- 
1.5.3.1

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

[Julian Calaby]

(added CCs - that's what you get for sending emails after 5.)

On 9/11/07, Boaz Harrosh <bharrosh@panasas.com> wrote:
>
>  static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
> -                            int cmnd_size, int timeout, int copy_sense)
> +                            int cmnd_size, int timeout, unsigned sense_bytes)

Shouldn't that be unsigned _int_?

Thanks,

--

Julian Calaby

Email: julian.calaby@gmail.com

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

[Benny Halevy]

On Sep 11, 2007, 11:11 +0300, "Julian Calaby" <julian.calaby@gmail.com> wrote:
> (added CCs - that's what you get for sending emails after 5.)
> 
> On 9/11/07, Boaz Harrosh <bharrosh@panasas.com> wrote:
>>  static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
>> -                            int cmnd_size, int timeout, int copy_sense)
>> +                            int cmnd_size, int timeout, unsigned sense_bytes)
> 
> Shouldn't that be unsigned _int_?

It could but first, "unsigned" and "unsigned int" are the same type
and second, this is consistent with *bufflen type which is "unsigned" all over
the place.

Benny

> 
> Thanks,
> 
> --
> 
> Julian Calaby
> 
> Email: julian.calaby@gmail.com
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Alan Stern]

On Tue, 11 Sep 2007, Boaz Harrosh wrote:

>   - regrouped variables for easier reviewing of next patch
>   - Support of cmnd==NULL in call to scsi_send_eh_cmnd()
>   - In the copy_sense case set transfer size to the minimum
>     size of sense_buffer and passed @sense_bytes. cmnd[4] is
>     set accordingly.
>   - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
>     not Zero passed cmnd can/should be NULL.
>   - Also save/restore resid of faild command.

The "if (sense_bytes)" block doesn't set scmd->cmd_len.  Yes, I know 
that value gets changed by the usb-storage driver, but nevertheless it 
should be set here to 6.

Apart from that, this looks fine to me.

Alan Stern

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

[Boaz Harrosh]

On Tue, Sep 11 2007 at 18:41 +0300, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 11 Sep 2007, Boaz Harrosh wrote:
> 
>>   - regrouped variables for easier reviewing of next patch
>>   - Support of cmnd==NULL in call to scsi_send_eh_cmnd()
>>   - In the copy_sense case set transfer size to the minimum
>>     size of sense_buffer and passed @sense_bytes. cmnd[4] is
>>     set accordingly.
>>   - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
>>     not Zero passed cmnd can/should be NULL.
>>   - Also save/restore resid of faild command.
> 
> The "if (sense_bytes)" block doesn't set scmd->cmd_len.  Yes, I know 
> that value gets changed by the usb-storage driver, but nevertheless it 
> should be set here to 6.
> 
> Apart from that, this looks fine to me.
> 
> Alan Stern
> 
Thanks a million. The 22:45 effect again.
Will patch ver4 of this patch as reply to
first one.

Boaz

Re: [PATCH ver4 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 copy_sense case set transfer size to the minimum
    size of sense_buffer and passed @sense_bytes. cmnd[4] is
    set accordingly.
  - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
    not Zero passed cmnd can/should be NULL.
  - Also save/restore resid of faild command.

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c8e351f..11c928d 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -607,21 +607,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
  *    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)
+			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	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,24 +634,37 @@ 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 (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
-			    sizeof(scmd->sense_buffer));
+	if (cmnd) {
+		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
+		memcpy(scmd->cmnd, cmnd, cmnd_size);
+		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+	}
 
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
+	if (sense_bytes) {
+		scmd->request_bufflen = min_t(unsigned,
+		                       sizeof(scmd->sense_buffer), sense_bytes);
+		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
 		scmd->request_buffer = &sgl;
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->use_sg = 1;
+		memset(scmd->cmnd, 0, 6);
+		scmd->cmnd[0] = REQUEST_SENSE;
+		scmd->cmnd[4] = scmd->request_bufflen;
+		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+		/*
+		 * Zero the sense buffer.  The scsi spec mandates that any
+		 * untransferred sense data should be interpreted as being zero.
+		 */
+		memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
@@ -657,18 +673,11 @@ 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)
+	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
 
-	/*
-	 * Zero the sense buffer.  The scsi spec mandates that any
-	 * untransferred sense data should be interpreted as being zero.
-	 */
-	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
-
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -716,12 +725,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;
 }
@@ -737,10 +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};
-
-	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
+	return scsi_send_eh_cmnd(scmd, NULL, 0, SENSE_TIMEOUT, ~0);
 }
 
 /**
-- 
1.5.3.1

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

[Russell King]

On Mon, Sep 10, 2007 at 10:39:11PM +0300, Boaz Harrosh wrote:
> 
>   - 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 |   16 +++-------------
>  drivers/scsi/arm/fas216.h |    3 +++
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
> index fb5f202..a715632 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->ses);
>  	SCpnt->scsi_done(SCpnt);
>  }
>  
> @@ -2103,23 +2104,12 @@ request_sense:
>  	if (SCpnt->cmnd[0] == REQUEST_SENSE)
>  		goto done;
>  
> +	scsi_eh_prep_cmnd(SCpnt, &info->ses, NULL, 0, ~0);
>  	fas216_log_target(info, LOG_CONNECT, SCpnt->device->id,
>  			  "requesting sense");
> -	memset(SCpnt->cmnd, 0, sizeof (SCpnt->cmnd));
> -	SCpnt->cmnd[0] = REQUEST_SENSE;
> -	SCpnt->cmnd[1] = SCpnt->device->lun << 5;
> -	SCpnt->cmnd[4] = sizeof(SCpnt->sense_buffer);
> -	SCpnt->cmd_len = COMMAND_SIZE(SCpnt->cmnd[0]);
> -	SCpnt->SCp.buffer = NULL;
> -	SCpnt->SCp.buffers_residual = 0;
> -	SCpnt->SCp.ptr = (char *)SCpnt->sense_buffer;
> -	SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
> -	SCpnt->SCp.phase = sizeof(SCpnt->sense_buffer);
> +	init_SCp(SCpnt);
>  	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;

So where do we end up setting up the request sense command?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

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

[Benny Halevy]

Russell King wrote:
> On Mon, Sep 10, 2007 at 10:39:11PM +0300, Boaz Harrosh wrote:
>   
>>   - 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 |   16 +++-------------
>>  drivers/scsi/arm/fas216.h |    3 +++
>>  2 files changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/scsi/arm/fas216.c b/drivers/scsi/arm/fas216.c
>> index fb5f202..a715632 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->ses);
>>  	SCpnt->scsi_done(SCpnt);
>>  }
>>  
>> @@ -2103,23 +2104,12 @@ request_sense:
>>  	if (SCpnt->cmnd[0] == REQUEST_SENSE)
>>  		goto done;
>>  
>> +	scsi_eh_prep_cmnd(SCpnt, &info->ses, NULL, 0, ~0);
>>  	fas216_log_target(info, LOG_CONNECT, SCpnt->device->id,
>>  			  "requesting sense");
>> -	memset(SCpnt->cmnd, 0, sizeof (SCpnt->cmnd));
>> -	SCpnt->cmnd[0] = REQUEST_SENSE;
>> -	SCpnt->cmnd[1] = SCpnt->device->lun << 5;
>> -	SCpnt->cmnd[4] = sizeof(SCpnt->sense_buffer);
>> -	SCpnt->cmd_len = COMMAND_SIZE(SCpnt->cmnd[0]);
>> -	SCpnt->SCp.buffer = NULL;
>> -	SCpnt->SCp.buffers_residual = 0;
>> -	SCpnt->SCp.ptr = (char *)SCpnt->sense_buffer;
>> -	SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
>> -	SCpnt->SCp.phase = sizeof(SCpnt->sense_buffer);
>> +	init_SCp(SCpnt);
>>  	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;
>>     
>
> So where do we end up setting up the request sense command?
>
>   
In scsi_eh_prep_cmnd(), when (sense_bytes != 0), called here:

+	scsi_eh_prep_cmnd(SCpnt, &info->ses, NULL, 0, ~0);

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

[James Bottomley]

On Tue, 2007-09-11 at 20:39 +0300, Boaz Harrosh wrote:
> - regrouped variables for easier reviewing of next patch
>   - Support of cmnd==NULL in call to scsi_send_eh_cmnd()
>   - In the copy_sense case set transfer size to the minimum
>     size of sense_buffer and passed @sense_bytes. cmnd[4] is
>     set accordingly.
>   - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
>     not Zero passed cmnd can/should be NULL.
>   - Also save/restore resid of faild command.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/scsi_error.c |   69 ++++++++++++++++++++++++--------------------
>  1 files changed, 38 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index c8e351f..11c928d 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -607,21 +607,24 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
>   *    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)
> +			     int cmnd_size, int timeout, unsigned sense_bytes)

You've altered the signature and function of the routine here, this
needs to be documented by updating the docbook description above the
function.  (I know you do it later, but each changeset should really be
logically complete).

>  {
>  	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,24 +634,37 @@ 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 (copy_sense) {
> -		sg_init_one(&sgl, scmd->sense_buffer,
> -			    sizeof(scmd->sense_buffer));
> +	if (cmnd) {
> +		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
> +		memcpy(scmd->cmnd, cmnd, cmnd_size);
> +		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> +	}
>  
> -		scmd->sc_data_direction = DMA_FROM_DEVICE;
> -		scmd->request_bufflen = sgl.length;
> +	if (sense_bytes) {
> +		scmd->request_bufflen = min_t(unsigned,
> +		                       sizeof(scmd->sense_buffer), sense_bytes);
> +		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
>  		scmd->request_buffer = &sgl;
> +		scmd->sc_data_direction = DMA_FROM_DEVICE;
>  		scmd->use_sg = 1;
> +		memset(scmd->cmnd, 0, 6);

This will lead to subtle bugs: some devices (notably ATAPI) have a fixed
command slot they will copy this into.  You have potentially trailing
bytes of junk that this could pick up ... we have ATAPI devices known to
fall over if they see trailing junk in the command.  Just consolidate
the scmd->cmnd memsets to

memset(scmd->cmnd, 0, sizeof(scmd->cmnd);

outside of the ifs

> +		scmd->cmnd[0] = REQUEST_SENSE;
> +		scmd->cmnd[4] = scmd->request_bufflen;
> +		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
> +		/*
> +		 * Zero the sense buffer.  The scsi spec mandates that any
> +		 * untransferred sense data should be interpreted as being zero.
> +		 */
> +		memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));
>  	} else {
>  		scmd->request_buffer = NULL;
>  		scmd->request_bufflen = 0;
> @@ -657,18 +673,11 @@ 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)
> +	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
>  		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
>  			(sdev->lun << 5 & 0xe0);
>  
> -	/*
> -	 * Zero the sense buffer.  The scsi spec mandates that any
> -	 * untransferred sense data should be interpreted as being zero.
> -	 */
> -	memset(scmd->sense_buffer, 0, sizeof(scmd->sense_buffer));

Moving this to the sense_bytes specific piece of the if also introduces
a subtle bug:  If the driver doesn't do auto request sense and the
command completes with CHECK CONDITION, the sense checking routine will
potentially pick up stale sense data here

> -
>  	shost->eh_action = &done;
>  
>  	spin_lock_irqsave(shost->host_lock, flags);
> @@ -716,12 +725,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;
>  }
> @@ -737,10 +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};
> -
> -	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
> +	return scsi_send_eh_cmnd(scmd, NULL, 0, SENSE_TIMEOUT, ~0);
>  }
>  
>  /**

Other than the three comments, this looks OK.

James

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

[James Bottomley]

On Tue, 2007-09-11 at 11:04 +0300, Boaz Harrosh wrote:
> - 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 *sesci, unsigned char *cmnd,
> 	int cmnd_size, int sense_bytes)
> 
>    Will hijack a command and prepare it for request sense if needed.
>    And will save any later needed info into a scsi_eh_save structure.
> 
>    void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
> 	struct scsi_eh_save *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
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

This one looks OK, but it will likely not apply when you fix the first
patch, so could you rebase and resend this as well.

Thanks,

James

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

[Boaz Harrosh]

I will send a ver4 of these patches as reply to base patches
Meanwhile I have some comments below.

Thanks for the review.

On Wed, Oct 03 2007 at 17:55 +0200, James Bottomley <James.Bottomley@SteelEye.com> wrote:
> 
> You've altered the signature and function of the routine here, this
> needs to be documented by updating the docbook description above the
> function.  (I know you do it later, but each changeset should really be
> logically complete).
> 
Done, thanks

>> +		memset(scmd->cmnd, 0, 6);
> 
> This will lead to subtle bugs: some devices (notably ATAPI) have a fixed
> command slot they will copy this into.  You have potentially trailing
> bytes of junk that this could pick up ... we have ATAPI devices known to
> fall over if they see trailing junk in the command.  Just consolidate
> the scmd->cmnd memsets to
> 
> memset(scmd->cmnd, 0, sizeof(scmd->cmnd);
> 
> outside of the ifs
> 
I wanted to have a fixture where if @cmnd and @sense_bytes are Zero than
scsi_cmnd->cmnd is not touched at all. So I fixed it by doing if (@cmnd)
only in the second arm of the if and fixed the above to what you said.
(See patch that follows)

> 
> Moving this to the sense_bytes specific piece of the if also introduces
> a subtle bug:  If the driver doesn't do auto request sense and the
> command completes with CHECK CONDITION, the sense checking routine will
> potentially pick up stale sense data here
> 
Good call thanks, done.

> 
> Other than the three comments, this looks OK.
> 
> James
> 
> 

[PATCH ver5 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 @sense_bytes case set transfer size to the minimum
    size of sense_buffer and passed @sense_bytes. cmnd[4] is
    set accordingly.
  - REQUEST_SENSE is set into cmnd[0] so if @sense_bytes is
    not Zero passed @cmnd should be NULL.
  - Also save/restore resid of failed command.
  - Adjust caller

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

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index f49feb9..4d53501 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -592,36 +592,39 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 /**
  * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
  * @scmd:       SCSI command structure to hijack
- * @cmnd:       CDB to send
+ * @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
+ * @sense_bytes: size of sense data to copy. or 0 (if != 0 @cmnd is ignored)
  *
  * 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.
+ * as part of the error recovery process.  If @sense_bytes is 0 the command
+ * sent must be one that does not transfer any data.  If @sense_bytes != 0
+ * @cmnd is ignored and this functions sets up a REQUEST_SENSE command
+ * and cmnd buffers to read @sense_bytes 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)
+			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	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,35 +634,41 @@ 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 (copy_sense) {
-		sg_init_one(&sgl, scmd->sense_buffer,
-			    sizeof(scmd->sense_buffer));
-
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = sgl.length;
+	if (sense_bytes) {
+		scmd->request_bufflen = min_t(unsigned,
+		                       sizeof(scmd->sense_buffer), sense_bytes);
+		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
 		scmd->request_buffer = &sgl;
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->use_sg = 1;
+		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
+		scmd->cmnd[0] = REQUEST_SENSE;
+		scmd->cmnd[4] = scmd->request_bufflen;
+		scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
 		scmd->use_sg = 0;
+		if (cmnd) {
+			memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
+			memcpy(scmd->cmnd, cmnd, cmnd_size);
+			scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
+		}
 	}
 
 	scmd->underflow = 0;
-	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
-	if (sdev->scsi_level <= SCSI_2)
+	if (sdev->scsi_level <= SCSI_2 && sdev->scsi_level != SCSI_UNKNOWN)
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
 
@@ -716,12 +725,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;
 }
@@ -737,10 +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};
-
-	return scsi_send_eh_cmnd(scmd, generic_sense, 6, SENSE_TIMEOUT, 1);
+	return scsi_send_eh_cmnd(scmd, NULL, 0, SENSE_TIMEOUT, ~0);
 }
 
 /**
-- 
1.5.3.1

[PATCH ver5 2/5] scsi_error: 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 *sesci, unsigned char *cmnd,
	int cmnd_size, int sense_bytes)

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

   void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
	struct scsi_eh_save *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

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi_error.c |  114 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_eh.h    |   23 +++++++++-
 2 files changed, 90 insertions(+), 47 deletions(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 4d53501..d29f846 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -590,42 +590,23 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd)
 }
 
 /**
- * scsi_send_eh_cmnd  - submit a scsi command as part of error recory
+ * scsi_eh_prep_cmnd  - Save a scsi command info as part of error recory
  * @scmd:       SCSI command structure to hijack
+ * @ses:        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
  * @sense_bytes: size of sense data to copy. or 0 (if != 0 @cmnd is ignored)
  *
- * This function is used to send a scsi command down to a target device
+ * This function is used to save a scsi command information before re-execution
  * as part of the error recovery process.  If @sense_bytes is 0 the command
  * sent must be one that does not transfer any data.  If @sense_bytes != 0
  * @cmnd is ignored and this functions sets up a REQUEST_SENSE command
  * and cmnd buffers to read @sense_bytes 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, unsigned sense_bytes)
+void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
+			unsigned char *cmnd, int cmnd_size, unsigned sense_bytes)
 {
 	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,20 +615,21 @@ 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;
+	ses->cmd_len = scmd->cmd_len;
+	memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd));
+	ses->data_direction = scmd->sc_data_direction;
+	ses->bufflen = scmd->request_bufflen;
+	ses->buffer = scmd->request_buffer;
+	ses->use_sg = scmd->use_sg;
+	ses->resid = scmd->resid;
+	ses->result = scmd->result;
 
 	if (sense_bytes) {
 		scmd->request_bufflen = min_t(unsigned,
 		                       sizeof(scmd->sense_buffer), sense_bytes);
-		sg_init_one(&sgl, scmd->sense_buffer, scmd->request_bufflen);
-		scmd->request_buffer = &sgl;
+		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
+		                                       scmd->request_bufflen);
+		scmd->request_buffer = &ses->sense_sgl;
 		scmd->sc_data_direction = DMA_FROM_DEVICE;
 		scmd->use_sg = 1;
 		memset(scmd->cmnd, 0, sizeof(scmd->cmnd));
@@ -677,7 +659,58 @@ 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_eh_restore_cmnd  - Restore a scsi command info as part of error recory
+ * @scmd:       SCSI command structure to restore
+ * @ses:        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 *ses)
+{
+	/*
+	 * Restore original data
+	 */
+	scmd->cmd_len = ses->cmd_len;
+	memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd));
+	scmd->sc_data_direction = ses->data_direction;
+	scmd->request_bufflen = ses->bufflen;
+	scmd->request_buffer = ses->buffer;
+	scmd->use_sg = ses->use_sg;
+	scmd->resid = ses->resid;
+	scmd->result = ses->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
+ * @sense_bytes: size of sense data to copy or 0
+ *
+ * This function is used to send a scsi command down to a target device
+ * as part of the error recovery process. See also scsi_eh_prep_cmnd() above.
+ *
+ * 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, unsigned sense_bytes)
+{
+	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 ses;
+	int rtn;
+
+	scsi_eh_prep_cmnd(scmd, &ses, cmnd, cmnd_size, sense_bytes);
 	shost->eh_action = &done;
 
 	spin_lock_irqsave(shost->host_lock, flags);
@@ -721,18 +754,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, &ses);
 	return rtn;
 }
 
diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
index c5c0f67..44224ba 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 {
+	int result;
+	enum dma_data_direction data_direction;
+	unsigned char cmd_len;
+	unsigned char cmnd[MAX_COMMAND_SIZE];
+
+	void *buffer;
+	unsigned bufflen;
+	unsigned short use_sg;
+	int resid;
+
+	struct scatterlist sense_sgl;
+};
+
+extern void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd,
+		struct scsi_eh_save *ses, unsigned char *cmnd,
+		int cmnd_size, unsigned sense_bytes);
+
+extern void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd,
+		struct scsi_eh_save *ses);
+
 #endif /* _SCSI_SCSI_EH_H */
-- 
1.5.3.1