Skip to contentSkip to conversation

Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

PATCH In review Boaz Harrosh 6 participants 11 messages Sep 10, 2007

Subsystems (5)
lore
Share & tools
lore
#0 lore raw

cover Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

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

#1 lore raw

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

- 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.

---
 drivers/scsi/scsi_error.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)
drivers/scsi/scsi_error.c +23 -14
index c8e351f..0bcfbe5 100644
@@ -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);
 }
Signature · 1 line
1.5.3.1
#2 lore raw

PATCH 2/5 Refactoring scsi_error to facilitate in synchronous REQUEST_SENSE

In reply to 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 <[email protected]>

---
 drivers/scsi/scsi_error.c |  133 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_eh.h    |   23 ++++++++-
 2 files changed, 102 insertions(+), 54 deletions(-)
drivers/scsi/scsi_error.c +80 -53
index 0bcfbe5..0327e20 100644
@@ -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;
 }
 
include/scsi/scsi_eh.h +22 -1
index c5c0f67..07982b0 100644
@@ -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 */
Signature · 1 line
1.5.3.1
#5 lore raw

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

In reply to Boaz Harrosh

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

---
 drivers/usb/storage/transport.c |   51 ++++++--------------------------------
 1 files changed, 8 insertions(+), 43 deletions(-)
drivers/usb/storage/transport.c +8 -43
index 323293a..92dec34 100644
@@ -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");
Signature · 1 line
1.5.3.1
#6 lore raw

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

In reply to Boaz Harrosh

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

---
 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(-)
drivers/scsi/NCR5380.c +11 -14
index f8e449a..1ca3727 100644
@@ -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 *)
drivers/scsi/NCR5380.h +7 -0
index bccf13f..73855fc 100644
@@ -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__
drivers/scsi/atari_NCR5380.c +10 -15
index 03dbe60..a4bdd4c 100644
@@ -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);
drivers/scsi/sun3_NCR5380.c +9 -11
index 98e3fe1..7a5287e 100644
@@ -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);
Signature · 1 line
1.5.3.1
#7 lore raw

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

In reply to Boaz Harrosh

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

---
 drivers/scsi/arm/fas216.c |    5 ++---
 drivers/scsi/arm/fas216.h |    3 +++
 2 files changed, 5 insertions(+), 3 deletions(-)
drivers/scsi/arm/fas216.c +2 -3
index fb5f202..d80702b 100644
@@ -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;
 
drivers/scsi/arm/fas216.h +3 -0
index 00e5f05..8449416 100644
@@ -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;
 
Signature · 1 line
1.5.3.1
#3 lore raw
In reply to Boaz Harrosh

drivers/scsi/scsi_error.c static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd) · line 608

Quoted 3 lines
drivers/scsi/scsi_error.c +3 -0
+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.

include/scsi/scsi_eh.h extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len, · line 74

Quoted 7 lines
include/scsi/scsi_eh.h +7 -0
+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;

include/scsi/scsi_eh.h extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len, · line 80

Quoted 6 lines
include/scsi/scsi_eh.h +6 -0
+	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!

#4 lore raw
In reply to Boaz Harrosh

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.

Quoted 11 lines

Sign-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/scsi_error.c |  133 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_eh.h    |   23 ++++++++-
 2 files changed, 102 insertions(+), 54 deletions(-)
drivers/scsi/scsi_error.c +0 -0
index 0bcfbe5..0327e20 100644

drivers/scsi/scsi_error.c static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd) · line 595

Quoted 8 lines
drivers/scsi/scsi_error.c +2 -2
@@ -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:

Quoted 14 lines
drivers/scsi/scsi_error.c +4 -3
+ * @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)
 {
 

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 663

Quoted 5 lines
drivers/scsi/scsi_error.c +4 -0
@@ -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:

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 687

Quoted 8 lines
drivers/scsi/scsi_error.c +8 -0
+ *
+ * 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 ***

maintainer
#8 lore raw
In reply to Boaz Harrosh
Quoted 10 lines · 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.

Quoted 4 lines

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

#9 lore raw
In reply to Boaz Harrosh
Quoted 5 lines · 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.

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

Signature · 7 lines
Matthew Dharm                              Home: [email protected] 
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
#10 lore raw
Quoted 7 lines · Matthew Dharm
Quoted 5 lines · 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.

Quoted 6 lines

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.

Quoted 9 lines

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