On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Stefan Richter]

Commit 64a87b244b9297667ca80264aab849a36f494884 contains:
-------------------------- drivers/firewire/fw-sbp2.c --------------------------
index 2a99937..dda82f3 100644
@@ -1487,7 +1487,7 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
 	if (scsi_sg_count(cmd) && sbp2_map_scatterlist(orb, device, lu) < 0)
 		goto out;
 
-	memcpy(orb->request.command_block, cmd->cmnd, COMMAND_SIZE(*cmd->cmnd));
+	memcpy(orb->request.command_block, cmd->cmnd, cmd->cmd_len);
 
 	orb->base.callback = complete_command_orb;
 	orb->base.request_bus =


Should drivers/ieee1394/sbp2.c::sbp2_create_command_orb() be changed in
the same way?
-- 
Stefan Richter
-=====-==--- -=-= --=--
http://arcgraph.de/sr/

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Boaz Harrosh]

Stefan Richter wrote:
> Commit 64a87b244b9297667ca80264aab849a36f494884 contains:
> -------------------------- drivers/firewire/fw-sbp2.c --------------------------
> index 2a99937..dda82f3 100644
> @@ -1487,7 +1487,7 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done)
>  	if (scsi_sg_count(cmd) && sbp2_map_scatterlist(orb, device, lu) < 0)
>  		goto out;
>  
> -	memcpy(orb->request.command_block, cmd->cmnd, COMMAND_SIZE(*cmd->cmnd));
> +	memcpy(orb->request.command_block, cmd->cmnd, cmd->cmd_len);
>  
>  	orb->base.callback = complete_command_orb;
>  	orb->base.request_bus =
>
>
> Should drivers/ieee1394/sbp2.c::sbp2_create_command_orb() be changed in
> the same way?
>   
Sorry to have missed it. Yes it is best if it is changed. cmd->cmd_len 
is now guarantied to be set
properly at all cases. And some commands you want to support will not be 
set correctly by
COMMAND_SIZE(). do you want me to do it?

Boaz

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Stefan Richter]

From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: ieee1394: sbp2: use correct size of command descriptor block

Boaz Harrosh wrote:
> cmd->cmd_len is now guarantied to be set properly at all cases.
> And some commands you want to support will not be set correctly
> by COMMAND_SIZE().

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/sbp2.c |   18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Index: linux/drivers/ieee1394/sbp2.c
===================================================================
--- linux.orig/drivers/ieee1394/sbp2.c
+++ linux/drivers/ieee1394/sbp2.c
@@ -1539,15 +1539,13 @@ static void sbp2_prep_command_orb_sg(str
 
 static void sbp2_create_command_orb(struct sbp2_lu *lu,
 				    struct sbp2_command_info *cmd,
-				    unchar *scsi_cmd,
-				    unsigned int scsi_use_sg,
-				    unsigned int scsi_request_bufflen,
-				    struct scatterlist *sg,
-				    enum dma_data_direction dma_dir)
+				    struct scsi_cmnd *SCpnt)
 {
 	struct sbp2_fwhost_info *hi = lu->hi;
 	struct sbp2_command_orb *orb = &cmd->command_orb;
 	u32 orb_direction;
+	unsigned int scsi_request_bufflen = scsi_bufflen(SCpnt);
+	enum dma_data_direction dma_dir = SCpnt->sc_data_direction;
 
 	/*
 	 * Set-up our command ORB.
@@ -1580,13 +1578,14 @@ static void sbp2_create_command_orb(stru
 		orb->data_descriptor_lo = 0x0;
 		orb->misc |= ORB_SET_DIRECTION(1);
 	} else
-		sbp2_prep_command_orb_sg(orb, hi, cmd, scsi_use_sg, sg,
+		sbp2_prep_command_orb_sg(orb, hi, cmd, scsi_sg_count(SCpnt),
+					 scsi_sglist(SCpnt),
 					 orb_direction, dma_dir);
 
 	sbp2util_cpu_to_be32_buffer(orb, sizeof(*orb));
 
 	memset(orb->cdb, 0, 12);
-	memcpy(orb->cdb, scsi_cmd, COMMAND_SIZE(*scsi_cmd));
+	memcpy(orb->cdb, SCpnt->cmnd, SCpnt->cmd_len);
 }
 
 static void sbp2_link_orb_command(struct sbp2_lu *lu,
@@ -1669,16 +1668,13 @@ static void sbp2_link_orb_command(struct
 static int sbp2_send_command(struct sbp2_lu *lu, struct scsi_cmnd *SCpnt,
 			     void (*done)(struct scsi_cmnd *))
 {
-	unchar *scsi_cmd = (unchar *)SCpnt->cmnd;
 	struct sbp2_command_info *cmd;
 
 	cmd = sbp2util_allocate_command_orb(lu, SCpnt, done);
 	if (!cmd)
 		return -EIO;
 
-	sbp2_create_command_orb(lu, cmd, scsi_cmd, scsi_sg_count(SCpnt),
-				scsi_bufflen(SCpnt), scsi_sglist(SCpnt),
-				SCpnt->sc_data_direction);
+	sbp2_create_command_orb(lu, cmd, SCpnt);
 	sbp2_link_orb_command(lu, cmd);
 
 	return 0;

-- 
Stefan Richter
-=====-==--- -=-= --=--
http://arcgraph.de/sr/


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Stefan Richter]

On  4 May, Stefan Richter wrote:
> Boaz Harrosh wrote:
>> cmd->cmd_len is now guarantied to be set properly at all cases.
>> And some commands you want to support will not be set correctly
>> by COMMAND_SIZE().
...
> --- linux.orig/drivers/ieee1394/sbp2.c
> +++ linux/drivers/ieee1394/sbp2.c
...
>  	memset(orb->cdb, 0, 12);
> -	memcpy(orb->cdb, scsi_cmd, COMMAND_SIZE(*scsi_cmd));
> +	memcpy(orb->cdb, SCpnt->cmnd, SCpnt->cmd_len);
...

Wait a minute...

drivers/ieee1394/sbp2.[ch] and drivers/firewire/fw-sbp2.c hardwire the
maximum size of the command descriptor block (command_block field in the
command block ORB as per SBP-2 clause 5.1.2) to 12 bytes.

We need to use BLK_MAX_CDB there, don't we?

(Besides, we should keep an eye on unit_characteristics.ORB_size as per
SBP-2 clause 7.4.8, but that's another story.)
-- 
Stefan Richter
-=====-==--- -=-= --=--
http://arcgraph.de/sr/



-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Boaz Harrosh]

Stefan Richter wrote:
> On  4 May, Stefan Richter wrote:
>> Boaz Harrosh wrote:
>>> cmd->cmd_len is now guarantied to be set properly at all cases.
>>> And some commands you want to support will not be set correctly
>>> by COMMAND_SIZE().
> ...
>> --- linux.orig/drivers/ieee1394/sbp2.c
>> +++ linux/drivers/ieee1394/sbp2.c
> ...
>>  	memset(orb->cdb, 0, 12);

if you are at it?
- 	memset(orb->cdb, 0, 12);
+ 	memset(orb->cdb, 0, sizeof(orb->cdb));

>> -	memcpy(orb->cdb, scsi_cmd, COMMAND_SIZE(*scsi_cmd));
>> +	memcpy(orb->cdb, SCpnt->cmnd, SCpnt->cmd_len);
> ...
> 
> Wait a minute...
> 
> drivers/ieee1394/sbp2.[ch] and drivers/firewire/fw-sbp2.c hardwire the
> maximum size of the command descriptor block (command_block field in the
> command block ORB as per SBP-2 clause 5.1.2) to 12 bytes.
> 
> We need to use BLK_MAX_CDB there, don't we?
> 
> (Besides, we should keep an eye on unit_characteristics.ORB_size as per
> SBP-2 clause 7.4.8, but that's another story.)

I'm not sure what you mean. But orb->cdb is 12 bytes, and setting max_cmd_len in
host template insures midlayer will never send a command bigger then that. So
->cmd_len will never be bigger then host->max_cmd_len. BLK_MAX_CDB should not
be used here.

But I don't see .max_cmd_len set in this file, is it set elsewhere?

Boaz

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Boaz Harrosh]

Boaz Harrosh wrote:
> Stefan Richter wrote:
>> On  4 May, Stefan Richter wrote:
>>> Boaz Harrosh wrote:
>>>> cmd->cmd_len is now guarantied to be set properly at all cases.
>>>> And some commands you want to support will not be set correctly
>>>> by COMMAND_SIZE().
>> ...
>>> --- linux.orig/drivers/ieee1394/sbp2.c
>>> +++ linux/drivers/ieee1394/sbp2.c
>> ...
>>>      memset(orb->cdb, 0, 12);
> 
> if you are at it?
> -     memset(orb->cdb, 0, 12);
> +     memset(orb->cdb, 0, sizeof(orb->cdb));
> 
>>> -    memcpy(orb->cdb, scsi_cmd, COMMAND_SIZE(*scsi_cmd));
>>> +    memcpy(orb->cdb, SCpnt->cmnd, SCpnt->cmd_len);
>> ...
>>
>> Wait a minute...
>>
>> drivers/ieee1394/sbp2.[ch] and drivers/firewire/fw-sbp2.c hardwire the
>> maximum size of the command descriptor block (command_block field in the
>> command block ORB as per SBP-2 clause 5.1.2) to 12 bytes.
>>
>> We need to use BLK_MAX_CDB there, don't we?
>>
>> (Besides, we should keep an eye on unit_characteristics.ORB_size as per
>> SBP-2 clause 7.4.8, but that's another story.)
> 
> I'm not sure what you mean. But orb->cdb is 12 bytes, and setting 
> max_cmd_len in
> host template insures midlayer will never send a command bigger then 
> that. So
> ->cmd_len will never be bigger then host->max_cmd_len. BLK_MAX_CDB 
> should not
> be used here.
> 
> But I don't see .max_cmd_len set in this file, is it set elsewhere?
> 
> Boaz

Oops sorry its fine not to set .max_cmd_len. If not set the midlayer will
assume 12. So the code you sent is fine as is.

Boaz

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Stefan Richter]

Boaz Harrosh wrote:
> its fine not to set .max_cmd_len. If not set the midlayer will
> assume 12. So the code you sent is fine as is.

Ah right, it's explained in scsi_host.h.  Thanks.
-- 
Stefan Richter
-=====-==--- -=-= --=--
http://arcgraph.de/sr/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Re: On patch "Let scsi_cmnd->cmnd use request->cmd buffer"

[Stefan Richter]

Boaz Harrosh wrote:
> Stefan Richter wrote:
>>> --- linux.orig/drivers/ieee1394/sbp2.c
>>> +++ linux/drivers/ieee1394/sbp2.c
>> ...
>>>      memset(orb->cdb, 0, 12);
> 
> if you are at it?
> -     memset(orb->cdb, 0, 12);
> +     memset(orb->cdb, 0, sizeof(orb->cdb));

Right, I'll do that.
-- 
Stefan Richter
-=====-==--- -=-= --=--
http://arcgraph.de/sr/

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone