use_sg != 0 assumption (was Re: [PATCH 5/9] Drop single buffer request support.)

use_sg != 0 assumption (was Re: [PATCH 5/9] Drop single buffer request support.)

[Stefan Richter]

Kristian Høgsberg wrote:
> The SCSI layer only passes sg requests down, so drop the
> use_sg == 0, request_bufflen != 0 case.
> 
> Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
> ---
>  drivers/firewire/fw-sbp2.c |   43 +------------------------------------------
>  1 files changed, 1 insertions(+), 42 deletions(-)

[...]

> @@ -1063,21 +1035,8 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
>  		orb->request.misc |=
>  			COMMAND_ORB_DIRECTION(SBP2_DIRECTION_TO_MEDIA);
>  
> -	if (cmd->use_sg) {
> -		if (sbp2_command_orb_map_scatterlist(orb) < 0)
> -			goto fail_map_payload;
> -	} else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
> -		/*
> -		 * FIXME: Need to split this into a sg list... but
> -		 * could we get the scsi or blk layer to do that by
> -		 * reporting our max supported block size?
> -		 */
> -		fw_error("command > 64k\n");
> +	if (cmd->use_sg && sbp2_command_orb_map_scatterlist(orb) < 0)
>  		goto fail_map_payload;
> -	} else if (cmd->request_bufflen > 0) {
> -		if (sbp2_command_orb_map_buffer(orb) < 0)
> -			goto fail_map_payload;
> -	}
>  
>  	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
>  

Would a BUG_ON(cmd->use_sg == 0); be in order?
-- 
Stefan Richter
-=====-=-=== -=-= -=-=-
http://arcgraph.de/sr/
-
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: use_sg != 0 assumption (was Re: [PATCH 5/9] Drop single buffer request support.)

[Christoph Hellwig]

On Thu, May 10, 2007 at 12:38:31PM +0200, Stefan Richter wrote:
> >  	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
> >  
> 
> Would a BUG_ON(cmd->use_sg == 0); be in order?

I don't think we should put this into every driver.  If you want to
be sure this doesn't happen anymore put it just in front of the queuecommand
call.

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: use_sg != 0 assumption (was Re: [PATCH 5/9] Drop single buffer request support.)

[Stefan Richter]

Christoph Hellwig wrote:
> On Thu, May 10, 2007 at 12:38:31PM +0200, Stefan Richter wrote:
>> Would a BUG_ON(cmd->use_sg == 0); be in order?
> 
> I don't think we should put this into every driver.  If you want to
> be sure this doesn't happen anymore put it just in front of the queuecommand
> call.

Thanks for the pointer.  I'm just noticing that Jens Axboe has a patch
which puts BUG_ON(!cmd->use_sg); into scsi_lib.c's sg table allocation
path.  (The "Chaining sg lists for bio IO commands v3" patchset from a
few hours ago, part 11/13 "SCSI: support for allocating large
scatterlists".)
-- 
Stefan Richter
-=====-=-=== -=-= -=-=-
http://arcgraph.de/sr/

Re: use_sg != 0 assumption (was Re: [PATCH 5/9] Drop single buffer request support.)

[Kristian Høgsberg]

Stefan Richter wrote:
> Kristian Høgsberg wrote:
>> The SCSI layer only passes sg requests down, so drop the
>> use_sg == 0, request_bufflen != 0 case.
>>
>> Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
>> ---
>>  drivers/firewire/fw-sbp2.c |   43 +------------------------------------------
>>  1 files changed, 1 insertions(+), 42 deletions(-)
> 
> [...]
> 
>> @@ -1063,21 +1035,8 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
>>  		orb->request.misc |=
>>  			COMMAND_ORB_DIRECTION(SBP2_DIRECTION_TO_MEDIA);
>>  
>> -	if (cmd->use_sg) {
>> -		if (sbp2_command_orb_map_scatterlist(orb) < 0)
>> -			goto fail_map_payload;
>> -	} else if (cmd->request_bufflen > SBP2_MAX_SG_ELEMENT_LENGTH) {
>> -		/*
>> -		 * FIXME: Need to split this into a sg list... but
>> -		 * could we get the scsi or blk layer to do that by
>> -		 * reporting our max supported block size?
>> -		 */
>> -		fw_error("command > 64k\n");
>> +	if (cmd->use_sg && sbp2_command_orb_map_scatterlist(orb) < 0)
>>  		goto fail_map_payload;
>> -	} else if (cmd->request_bufflen > 0) {
>> -		if (sbp2_command_orb_map_buffer(orb) < 0)
>> -			goto fail_map_payload;
>> -	}
>>  
>>  	fw_memcpy_to_be32(&orb->request, &orb->request, sizeof orb->request);
>>  
> 
> Would a BUG_ON(cmd->use_sg == 0); be in order?

That's a good idea, though it needs to be

   BUG_ON(cmd->usg_sg == 0 && cmd->request_bufflen > 0);

since commands with no payload have use_sg == 0 and request_bufflen == 0.

Kristian


-
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 6/9] Make scsi_host_template::proc_name const char * instead of char *.

[Stefan Richter]

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

Kristian Høgsberg wrote:
> Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
> ---
>  drivers/scsi/scsi_debug.c |    2 +-
>  include/scsi/scsi_host.h  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 3e2930b..843f9c1 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -2876,7 +2876,7 @@ static int __init scsi_debug_init(void)
>  
>  	init_all_queued();
>  
> -	sdebug_driver_template.proc_name = (char *)sdebug_proc_name;
> +	sdebug_driver_template.proc_name = sdebug_proc_name;
>  
>  	host_to_add = scsi_debug_add_host;
>          scsi_debug_add_host = 0;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 7f1f411..0dfb5c2 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -343,7 +343,7 @@ struct scsi_host_template {
>  	/*
>  	 * Name of proc directory
>  	 */
> -	char *proc_name;
> +	const char *proc_name;
>  
>  	/*
>  	 * Used to store the procfs directory if a driver implements the

This has to go via James B and LSML.
-- 
Stefan Richter
-=====-=-=== -=-= -=-=-
http://arcgraph.de/sr/

[-- Attachment #2: Make-scsi_host_template-proc_name-const-char-instead-of-char.patch --]
[-- Type: text/plain, Size: 1167 bytes --]

From: =?utf-8?q?Kristian_H=C3=B8gsberg?= <krh@redhat.com>
Subject: [PATCH 6/9] Make scsi_host_template::proc_name const char * instead of char *.
Date: Wed,  9 May 2007 19:23:12 -0400

Signed-off-by: Kristian Hoegsberg <krh@redhat.com>
---
 drivers/scsi/scsi_debug.c |    2 +-
 include/scsi/scsi_host.h  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 3e2930b..843f9c1 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -2876,7 +2876,7 @@ static int __init scsi_debug_init(void)
 
 	init_all_queued();
 
-	sdebug_driver_template.proc_name = (char *)sdebug_proc_name;
+	sdebug_driver_template.proc_name = sdebug_proc_name;
 
 	host_to_add = scsi_debug_add_host;
         scsi_debug_add_host = 0;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 7f1f411..0dfb5c2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -343,7 +343,7 @@ struct scsi_host_template {
 	/*
 	 * Name of proc directory
 	 */
-	char *proc_name;
+	const char *proc_name;
 
 	/*
 	 * Used to store the procfs directory if a driver implements the


[-- Attachment #3: Type: text/plain, Size: 286 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

[-- Attachment #4: Type: text/plain, Size: 0 bytes --]