Skip to contentSkip to conversation

use one-element sg list in scsi_send_eh_cmnd

PATCH In review Christoph Hellwig 4 participants 5 messages Nov 4, 2006 – Nov 6, 2006 SCSI

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 1/1 use one-element sg list in scsi_send_eh_cmnd

scsi_send_eh_cmnd is the last user of non-sg commands currently. This patch switches it to a one-element SG list. Also updates the kerneldoc comment for scsi_send_eh_cmnd to reflect reality while we're at it.

Test on my mptsas card, but this should get testing with as many drivers as possible.

Signed-off-by Christoph Hellwig <[email protected]>
drivers/scsi/scsi_error.c +24 -9
Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
@@ -453,9 +453,18 @@
 }
 
 /**
- * scsi_send_eh_cmnd  - send a cmd to a device as part of error recovery.
- * @scmd:	SCSI Cmd to send.
- * @timeout:	Timeout for cmd.
+ * 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 if 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

@@ -469,6 +478,7 @@
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	unsigned long flags;
+	struct scatterlist sgl;
 	unsigned char old_cmnd[MAX_COMMAND_SIZE];
 	enum dma_data_direction old_data_direction;
 	unsigned short old_use_sg;

@@ -500,19 +510,24 @@
 		if (shost->hostt->unchecked_isa_dma)
 			gfp_mask |= __GFP_DMA;
 
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = 252;
-		scmd->request_buffer = kzalloc(scmd->request_bufflen, gfp_mask);
-		if (!scmd->request_buffer)
+		sgl.page = alloc_page(gfp_mask);
+		if (!sgl.page)
 			return FAILED;
+		sgl.offset = 0;
+		sgl.length = 252;
+
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
+		scmd->request_bufflen = sgl.length;
+		scmd->request_buffer = &sgl;
+		scmd->use_sg = 1;
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
+		scmd->use_sg = 0;
 	}
 
 	scmd->underflow = 0;
-	scmd->use_sg = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
 	if (sdev->scsi_level <= SCSI_2)

@@ -583,7 +598,7 @@
 			memcpy(scmd->sense_buffer, scmd->request_buffer,
 			       sizeof(scmd->sense_buffer));
 		}
-		kfree(scmd->request_buffer);
+		__free_page(sgl.page);
 	}
#1 lore raw
Quoted 4 lines · --- Christoph Hellwig

scsi_send_eh_cmnd is the last user of non-sg commands currently. This patch switches it to a one-element SG list. Also updates the kerneldoc comment for scsi_send_eh_cmnd to reflect reality while we're at it.

So now "use_sg" means "always yes".

In this case I'd suggest you rename "use_sg" to something like "num_sg" or "num_scatter" or "num_scatter_elements" or something to this effect.

This will unambiguate its meaning and use.

Luben

Quoted 84 lines

Test on my mptsas card, but this should get testing with as many drivers as possible.

Signed-off-by Christoph Hellwig <[email protected]>

Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================

drivers/scsi/scsi_error.c +24 -10
@@ -453,9 +453,18 @@
 }
 
 /**
- * scsi_send_eh_cmnd  - send a cmd to a device as part of error recovery.
- * @scmd:	SCSI Cmd to send.
- * @timeout:	Timeout for cmd.
+ * 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 if 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

@@ -469,6 +478,7 @@
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
 	unsigned long flags;
+	struct scatterlist sgl;
 	unsigned char old_cmnd[MAX_COMMAND_SIZE];
 	enum dma_data_direction old_data_direction;
 	unsigned short old_use_sg;

@@ -500,19 +510,24 @@
 		if (shost->hostt->unchecked_isa_dma)
 			gfp_mask |= __GFP_DMA;
 
-		scmd->sc_data_direction = DMA_FROM_DEVICE;
-		scmd->request_bufflen = 252;
-		scmd->request_buffer = kzalloc(scmd->request_bufflen, gfp_mask);
-		if (!scmd->request_buffer)
+		sgl.page = alloc_page(gfp_mask);
+		if (!sgl.page)
 			return FAILED;
+		sgl.offset = 0;
+		sgl.length = 252;
+
+		scmd->sc_data_direction = DMA_FROM_DEVICE;
+		scmd->request_bufflen = sgl.length;
+		scmd->request_buffer = &sgl;
+		scmd->use_sg = 1;
 	} else {
 		scmd->request_buffer = NULL;
 		scmd->request_bufflen = 0;
 		scmd->sc_data_direction = DMA_NONE;
+		scmd->use_sg = 0;
 	}
 
 	scmd->underflow = 0;
-	scmd->use_sg = 0;
 	scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]);
 
 	if (sdev->scsi_level <= SCSI_2)

@@ -583,7 +598,7 @@
 			memcpy(scmd->sense_buffer, scmd->request_buffer,
 			       sizeof(scmd->sense_buffer));
 		}
-		kfree(scmd->request_buffer);
+		__free_page(sgl.page);
 	}
 
 
-

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#2 lore raw
Quoted 8 lines · Luben Tuikov
Quoted 4 lines · --- Christoph Hellwig

scsi_send_eh_cmnd is the last user of non-sg commands currently. This patch switches it to a one-element SG list. Also updates the kerneldoc comment for scsi_send_eh_cmnd to reflect reality while we're at it.

So now "use_sg" means "always yes".

Yes. (Except for the no data transfer case of course)

Quoted 5 lines

In this case I'd suggest you rename "use_sg" to something like "num_sg" or "num_scatter" or "num_scatter_elements" or something to this effect.

This will unambiguate its meaning and use.

That's entirely correct and I plan to fix it. The fix will happen as part of introducing support for bidirectional commands, as we will need to use_sg/num_scatter elements in that case anyway.

#3 lore raw
In reply to Christoph Hellwig
Quoted 17 lines · Christoph Hellwig

scsi_send_eh_cmnd is the last user of non-sg commands currently. This patch switches it to a one-element SG list. Also updates the kerneldoc comment for scsi_send_eh_cmnd to reflect reality while we're at it.

Test on my mptsas card, but this should get testing with as many drivers as possible.

Signed-off-by Christoph Hellwig <[email protected]>

Index: scsi-misc-2.6/drivers/scsi/scsi_error.c
===================================================================
--- scsi-misc-2.6.orig/drivers/scsi/scsi_error.c 2006-10-30
17:00:23.000000000 +0100 +++
scsi-misc-2.6/drivers/scsi/scsi_error.c 2006-11-04 17:30:33.000000000 +0100

drivers/scsi/scsi_error.c line 459

Quoted 10 lines
drivers/scsi/scsi_error.c +4 -3
@@ -453,9 +453,18 @@
 }
 
 /**
- * scsi_send_eh_cmnd  - send a cmd to a device as part of error recovery.
- * @scmd:	SCSI Cmd to send.
- * @timeout:	Timeout for cmd.
+ * 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 if bytes of @cmnd

                         ^

"in"?

drivers/scsi/scsi_error.c line 470

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

Eike

#4 lore raw

Rolf Eike Beer wrote on 2006-11-03:

Quoted 4 lines
Quoted 1 line · Christoph Hellwig

+ * @cmnd_size: size if bytes of @cmnd

                         ^

"in"?

"size of @cmnd, in bytes"?

Signature · 3 lines
Stefan Richter
-=====-=-==- =-== --==-
http://arcgraph.de/sr/