Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer
On 08/23/2014 03:09 PM, Douglas Gilbert wrote: For inclusion in 3.17 only. May want to check if blk-mq work in lk 3.16 and earlier breaks the bsg driver's capability to send SCSI cdbs that are longer than 16 bytes. I think 3.16 and earlier are OK. The problem was caused by scsi_mq_prep_fn(), which is new in 3.17. Thanks for testing. Tony -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer
On 14-08-22 03:53 PM, Tony Battersby wrote: This patch fixes code such as the following with scsi-mq enabled: rq = blk_get_request(...); blk_rq_set_block_pc(rq); rq-cmd = my_cmd_buffer; /* separate CDB buffer */ blk_execute_rq_nowait(...); Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for large CDBs only). Without this patch, scsi_mq_prep_fn() will set rq-cmd back to rq-__cmd, causing the wrong CDB to be sent to the device. Signed-off-by: Tony Battersby to...@cybernetics.com Tested-by: Douglas Gilbert dgilb...@interlog.com Patch tested on lk 3.17-rc1 with the sg driver using the READ(32) command. For inclusion in 3.17 only. May want to check if blk-mq work in lk 3.16 and earlier breaks the bsg driver's capability to send SCSI cdbs that are longer than 16 bytes. diff -urpN linux-3.17.0-rc1-a/block/blk-core.c linux-3.17.0-rc1-b/block/blk-core.c --- linux-3.17.0-rc1-a/block/blk-core.c 2014-08-16 12:40:26.0 -0400 +++ linux-3.17.0-rc1-b/block/blk-core.c 2014-08-22 14:03:33.0 -0400 @@ -1252,7 +1252,6 @@ void blk_rq_set_block_pc(struct request rq-__sector = (sector_t) -1; rq-bio = rq-biotail = NULL; memset(rq-__cmd, 0, sizeof(rq-__cmd)); - rq-cmd = rq-__cmd; } EXPORT_SYMBOL(blk_rq_set_block_pc); diff -urpN linux-3.17.0-rc1-a/block/blk-mq.c linux-3.17.0-rc1-b/block/blk-mq.c --- linux-3.17.0-rc1-a/block/blk-mq.c 2014-08-16 12:40:26.0 -0400 +++ linux-3.17.0-rc1-b/block/blk-mq.c 2014-08-22 14:02:32.0 -0400 @@ -172,6 +172,8 @@ static void blk_mq_rq_ctx_init(struct re /* tag was already set */ rq-errors = 0; + rq-cmd = rq-__cmd; + rq-extra_len = 0; rq-sense_len = 0; rq-resid_len = 0; diff -urpN linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c --- linux-3.17.0-rc1-a/drivers/scsi/scsi_lib.c 2014-08-16 12:40:26.0 -0400 +++ linux-3.17.0-rc1-b/drivers/scsi/scsi_lib.c 2014-08-22 14:02:32.0 -0400 @@ -1808,7 +1808,6 @@ static int scsi_mq_prep_fn(struct reques cmd-tag = req-tag; - req-cmd = req-__cmd; cmd-cmnd = req-cmd; cmd-prot_op = SCSI_PROT_NORMAL; -- -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer
On 14-08-22 03:53 PM, Tony Battersby wrote: This patch fixes code such as the following with scsi-mq enabled: rq = blk_get_request(...); blk_rq_set_block_pc(rq); rq-cmd = my_cmd_buffer; /* separate CDB buffer */ blk_execute_rq_nowait(...); Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for large CDBs only). Without this patch, scsi_mq_prep_fn() will set rq-cmd back to rq-__cmd, causing the wrong CDB to be sent to the device. Still looking at this one. 'sg_write_same --32' is my tool of choice. The target is scsi_debug which needs dif=2 (because mkp read the draft and only allows WRITE SAME(32) when the protection_level=2). Turned on command tracing and this is what I saw: sd 7:0:0:0: scsi_debug: cmd 9e 10 00 00 00 00 00 00 00 00 00 00 00 20 00 00 sd 7:0:0:0: scsi_debug: cmd 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 08 24 6b d3 00 88 ff ff 20 00 c7 e8 00 00 00 00 So the WS(32) command did get through, it is the second command that arrived on its tail that is a worry. So yes, IMO it is broken. Now the 16 byte cdb length in the sg driver introduced in lk 3.17-rc1 was copied directly from the bsg driver which has had that capability for some time. Since your patch touches two files in drivers/block I'm wondering the bsg driver's 16 byte cdb length capability is broken in lk 3.16 ? Doug Gilbert -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH][SCSI] scsi-mq: fix requests that use a separate CDB buffer
On 14-08-22 08:52 PM, Douglas Gilbert wrote: On 14-08-22 03:53 PM, Tony Battersby wrote: This patch fixes code such as the following with scsi-mq enabled: rq = blk_get_request(...); blk_rq_set_block_pc(rq); rq-cmd = my_cmd_buffer; /* separate CDB buffer */ blk_execute_rq_nowait(...); Code like this appears in e.g. sg_start_req() in drivers/scsi/sg.c (for large CDBs only). Without this patch, scsi_mq_prep_fn() will set rq-cmd back to rq-__cmd, causing the wrong CDB to be sent to the device. Still looking at this one. 'sg_write_same --32' is my tool of choice. The target is scsi_debug which needs dif=2 (because mkp read the draft and only allows WRITE SAME(32) when the protection_level=2). Turned on command tracing and this is what I saw: sd 7:0:0:0: scsi_debug: cmd 9e 10 00 00 00 00 00 00 00 00 00 00 00 20 00 00 sd 7:0:0:0: scsi_debug: cmd 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 08 24 6b d3 00 88 ff ff 20 00 c7 e8 00 00 00 00 So the WS(32) command did get through, it is the second command that arrived on its tail that is a worry. So yes, IMO it is broken. Got that wrong, that is a READ CAPACITY(16) that got through properly (issued by sg_write_same) followed by a 32 byte command of random bytes instead of the requested WS(32), the first 6 bytes of which will cause it to be parsed as a TEST UNIT READY. Now the 16 byte cdb length in the sg driver introduced in lk 3.17-rc1 was copied directly from the bsg driver which has had that capability for some time. Since your patch touches two files in drivers/block I'm wondering the bsg driver's 16 byte cdb length capability is broken in lk 3.16 ? Doug Gilbert -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
