Re: [PATCH] use one-element sg list in scsi_send_eh_cmnd

Re: [PATCH] use one-element sg list in scsi_send_eh_cmnd

[Rolf Eike Beer]

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

Christoph Hellwig wrote:
> 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 <hch@lst.de>
>
> 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
> @@ -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"?
> + * @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

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

[PATCH] use one-element sg list in scsi_send_eh_cmnd

[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 <hch@lst.de>

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
@@ -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);
 	}
 
 

Re: [PATCH] use one-element sg list in scsi_send_eh_cmnd

[Luben Tuikov]

--- Christoph Hellwig <hch@lst.de> wrote:

> 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

> Test on my mptsas card, but this should get testing with as many
> drivers as possible.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 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
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] use one-element sg list in scsi_send_eh_cmnd

[Christoph Hellwig]

On Sat, Nov 04, 2006 at 12:15:58PM -0800, Luben Tuikov wrote:
> --- Christoph Hellwig <hch@lst.de> wrote:
> 
> > 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)

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

Re: [PATCH] use one-element sg list in scsi_send_eh_cmnd

[Stefan Richter]

Rolf Eike Beer wrote on 2006-11-03:
> Christoph Hellwig wrote:
>> + * @cmnd_size:  size if bytes of @cmnd
>                          ^
> "in"?

"size of @cmnd, in bytes"?
-- 
Stefan Richter
-=====-=-==- =-== --==-
http://arcgraph.de/sr/