Skip to contentSkip to conversation

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

DISCUSSION Stefan Richter 3 participants 5 messages May 10, 2007 6 messages not in the archive SCSI

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stefan Richter

author
#5 lore raw
Quoted 7 lines · Kristian Høgsberg

The SCSI layer only passes sg requests down, so drop the use_sg == 0, request_bufflen != 0 case.

Signed-off-by Kristian Hoegsberg <[email protected]>
---
 drivers/firewire/fw-sbp2.c |   43 +------------------------------------------
 1 files changed, 1 insertions(+), 42 deletions(-)

[...]

Quoted 13 lines
(quoted diff) +0 -9
@@ -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?
-		 */

Review static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done) · line 1042

Quoted 10 lines
(quoted diff) +1 -5
-		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?

Signature · 7 lines
Stefan Richter
-=====-=-=== -=-= -=-=-
http://arcgraph.de/sr/
-
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
#6 lore raw
Quoted 4 lines · Stefan Richter
Quoted 2 lines

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/

author
#7 lore raw
Quoted 6 lines · Christoph Hellwig
Quoted 1 line · Stefan Richter

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

Signature · 3 lines
Stefan Richter
-=====-=-=== -=-= -=-=-
http://arcgraph.de/sr/
#8 lore raw
In reply to Stefan Richter
Quoted 32 lines · Stefan Richter
Quoted 7 lines · Kristian Høgsberg

The SCSI layer only passes sg requests down, so drop the use_sg == 0, request_bufflen != 0 case.

Signed-off-by Kristian Hoegsberg <[email protected]>
---
 drivers/firewire/fw-sbp2.c |   43 +------------------------------------------
 1 files changed, 1 insertions(+), 42 deletions(-)

[...]

Quoted 21 lines
(quoted diff) +1 -14
@@ -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;
-	}
Quoted 4 lines

Review static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done) · line 1042

Quoted 2 lines
(quoted diff) +0 -0
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#10 lore raw

Re: [PATCH 6/9] Make scsi_host_template::proc_name const char * instead of char *.

Quoted 24 lines · Kristian Høgsberg
Signed-off-by Kristian Hoegsberg <[email protected]>
---
 drivers/scsi/scsi_debug.c |    2 +-
 include/scsi/scsi_host.h  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/scsi_debug.c +1 -1
index 3e2930b..843f9c1 100644
@@ -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;
include/scsi/scsi_host.h +0 -0
index 7f1f411..0dfb5c2 100644

include/scsi/scsi_host.h struct scsi_host_template { · line 349

Quoted 8 lines
include/scsi/scsi_host.h +1 -1
@@ -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.

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

From: =?utf-8?q?Kristian_H=C3=B8gsberg?= <[email protected]>
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 <[email protected]>
---
 drivers/scsi/scsi_debug.c |    2 +-
 include/scsi/scsi_host.h  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/scsi/scsi_debug.c +1 -1
index 3e2930b..843f9c1 100644
@@ -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;
include/scsi/scsi_host.h +1 -1
index 7f1f411..0dfb5c2 100644
@@ -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 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/