* [PATCH 3/3] scsi support variable length commands
@ 2008-04-30 8:13 Boaz Harrosh
2008-04-30 8:19 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Boaz Harrosh @ 2008-04-30 8:13 UTC (permalink / raw)
To: James Bottomley, Mike Christie, linux-scsi; +Cc: Matthew Dharm, matthieu castet
Now that large command support is in mainline this can go in.
Same old patches, with a few conflicts fixed. They are based on yesterdays
linux-block/for-linus + scsi-misc.
[PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer
new hunk here from new usb/storage/cypress_atacb.c
[PATCH 2/3] scsi: varlen extended and vendor-specific cdbs
[PATCH 3/3] iscsi_tcp: Enable any size command
Boaz
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer 2008-04-30 8:13 [PATCH 3/3] scsi support variable length commands Boaz Harrosh @ 2008-04-30 8:19 ` Boaz Harrosh 2008-04-30 8:27 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 2008-04-30 8:30 ` [PATCH 3/3] iscsi_tcp: Enable any size command Boaz Harrosh 2 siblings, 0 replies; 12+ messages in thread From: Boaz Harrosh @ 2008-04-30 8:19 UTC (permalink / raw) To: James Bottomley, Mike Christie, linux-scsi Cc: Matthew Dharm, matthieu castet, FUJITA Tomonori - struct scsi_cmnd had a 16 bytes command buffer of its own. This is an unnecessary duplication and copy of request's cmd. It is probably left overs from the time that scsi_cmnd could function without a request attached. So clean that up. - Once above is done, few places, apart from scsi-ml, needed adjustments due to changing the data type of scsi_cmnd->cmnd. - Lots of drivers still use MAX_COMMAND_SIZE. So I have left that #define but equate it to BLK_MAX_CDB. The way I see it and is reflected in the patch below is. MAX_COMMAND_SIZE - means: The longest fixed-length (*) SCSI CDB as per the SCSI standard and is not related to the implementation. BLK_MAX_CDB. - The allocated space at the request level - I have audit all ISA drivers and made sure none use ->cmnd in a DMA Operation. Same audit was done by Andi Kleen. (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, (unlike the VARIABLE_LENGTH_CMD(0x7f) command). This is actually not exactly true and the SCSI standard also defines extended commands and vendor specific commands that can be bigger than 16 bytes. The kernel will support these using the same infrastructure used for VARLEN CDB's. So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml supports without specifying a cmd_len by ULD's Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> --- drivers/firewire/fw-sbp2.c | 2 +- drivers/s390/scsi/zfcp_dbf.c | 2 +- drivers/s390/scsi/zfcp_fsf.c | 2 +- drivers/scsi/53c700.c | 6 +++--- drivers/scsi/a100u2w.c | 2 +- drivers/scsi/gdth.c | 2 +- drivers/scsi/hptiop.c | 6 +++--- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- drivers/scsi/initio.c | 2 +- drivers/scsi/qla1280.c | 4 ++-- drivers/scsi/scsi_error.c | 15 ++++++++------- drivers/scsi/scsi_lib.c | 5 +++-- drivers/scsi/scsi_tgt_lib.c | 2 ++ drivers/usb/storage/cypress_atacb.c | 2 +- drivers/usb/storage/isd200.c | 2 ++ include/scsi/scsi_cmnd.h | 21 +++++++++++++++++++-- include/scsi/scsi_eh.h | 4 ++-- 17 files changed, 52 insertions(+), 29 deletions(-) diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c index 2a99937..dda82f3 100644 --- a/drivers/firewire/fw-sbp2.c +++ b/drivers/firewire/fw-sbp2.c @@ -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 = diff --git a/drivers/s390/scsi/zfcp_dbf.c b/drivers/s390/scsi/zfcp_dbf.c index 37b85c6..c8bad67 100644 --- a/drivers/s390/scsi/zfcp_dbf.c +++ b/drivers/s390/scsi/zfcp_dbf.c @@ -1055,7 +1055,7 @@ static void zfcp_scsi_dbf_event(const char *tag, const char *tag2, int level, rec->scsi_result = scsi_cmnd->result; rec->scsi_cmnd = (unsigned long)scsi_cmnd; rec->scsi_serial = scsi_cmnd->serial_number; - memcpy(rec->scsi_opcode, &scsi_cmnd->cmnd, + memcpy(rec->scsi_opcode, scsi_cmnd->cmnd, min((int)scsi_cmnd->cmd_len, ZFCP_DBF_SCSI_OPCODE)); rec->scsi_retries = scsi_cmnd->retries; diff --git a/drivers/s390/scsi/zfcp_fsf.c b/drivers/s390/scsi/zfcp_fsf.c index 9af2330..b2ea4ea 100644 --- a/drivers/s390/scsi/zfcp_fsf.c +++ b/drivers/s390/scsi/zfcp_fsf.c @@ -4014,7 +4014,7 @@ zfcp_fsf_send_fcp_command_task_handler(struct zfcp_fsf_req *fsf_req) ZFCP_LOG_TRACE("scpnt->result =0x%x, command was:\n", scpnt->result); ZFCP_HEX_DUMP(ZFCP_LOG_LEVEL_TRACE, - (void *) &scpnt->cmnd, scpnt->cmd_len); + scpnt->cmnd, scpnt->cmd_len); ZFCP_LOG_TRACE("%i bytes sense data provided by FCP\n", fcp_rsp_iu->fcp_sns_len); diff --git a/drivers/scsi/53c700.c b/drivers/scsi/53c700.c index f4c4fe9..f5a9add 100644 --- a/drivers/scsi/53c700.c +++ b/drivers/scsi/53c700.c @@ -599,7 +599,7 @@ NCR_700_scsi_done(struct NCR_700_Host_Parameters *hostdata, (struct NCR_700_command_slot *)SCp->host_scribble; dma_unmap_single(hostdata->dev, slot->pCmd, - sizeof(SCp->cmnd), DMA_TO_DEVICE); + MAX_COMMAND_SIZE, DMA_TO_DEVICE); if (slot->flags == NCR_700_FLAG_AUTOSENSE) { char *cmnd = NCR_700_get_sense_cmnd(SCp->device); #ifdef NCR_700_DEBUG @@ -1004,7 +1004,7 @@ process_script_interrupt(__u32 dsps, __u32 dsp, struct scsi_cmnd *SCp, * here */ NCR_700_unmap(hostdata, SCp, slot); dma_unmap_single(hostdata->dev, slot->pCmd, - sizeof(SCp->cmnd), + MAX_COMMAND_SIZE, DMA_TO_DEVICE); cmnd[0] = REQUEST_SENSE; @@ -1901,7 +1901,7 @@ NCR_700_queuecommand(struct scsi_cmnd *SCp, void (*done)(struct scsi_cmnd *)) } slot->resume_offset = 0; slot->pCmd = dma_map_single(hostdata->dev, SCp->cmnd, - sizeof(SCp->cmnd), DMA_TO_DEVICE); + MAX_COMMAND_SIZE, DMA_TO_DEVICE); NCR_700_start_command(SCp); return 0; } diff --git a/drivers/scsi/a100u2w.c b/drivers/scsi/a100u2w.c index 792b2e8..ced3eeb 100644 --- a/drivers/scsi/a100u2w.c +++ b/drivers/scsi/a100u2w.c @@ -895,7 +895,7 @@ static void inia100_build_scb(struct orc_host * host, struct orc_scb * scb, stru } else { scb->tag_msg = 0; /* No tag support */ } - memcpy(&scb->cdb[0], &cmd->cmnd, scb->cdb_len); + memcpy(scb->cdb, cmd->cmnd, scb->cdb_len); } /** diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c index c6d6e7c..8e2e964 100644 --- a/drivers/scsi/gdth.c +++ b/drivers/scsi/gdth.c @@ -465,7 +465,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd, scp->request = (struct request *)&wait; scp->timeout_per_command = timeout*HZ; scp->cmd_len = 12; - memcpy(scp->cmnd, cmnd, 12); + scp->cmnd = cmnd; cmndinfo.priority = IOCTL_PRI; cmndinfo.internal_cmd_str = gdtcmd; cmndinfo.internal_command = 1; diff --git a/drivers/scsi/hptiop.c b/drivers/scsi/hptiop.c index 5b7be1e..aaa48e0 100644 --- a/drivers/scsi/hptiop.c +++ b/drivers/scsi/hptiop.c @@ -763,9 +763,9 @@ static int hptiop_queuecommand(struct scsi_cmnd *scp, scp, host->host_no, scp->device->channel, scp->device->id, scp->device->lun, - *((u32 *)&scp->cmnd), - *((u32 *)&scp->cmnd + 1), - *((u32 *)&scp->cmnd + 2), + ((u32 *)scp->cmnd)[0], + ((u32 *)scp->cmnd)[1], + ((u32 *)scp->cmnd)[2], _req->index, _req->req_virt); scp->result = 0; diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index 4a922c5..dd8c716 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -738,7 +738,7 @@ static int ibmvscsi_queuecommand(struct scsi_cmnd *cmnd, srp_cmd = &evt_struct->iu.srp.cmd; memset(srp_cmd, 0x00, SRP_MAX_IU_LEN); srp_cmd->opcode = SRP_CMD; - memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(cmnd->cmnd)); + memcpy(srp_cmd->cdb, cmnd->cmnd, sizeof(srp_cmd->cdb)); srp_cmd->lun = ((u64) lun) << 48; if (!map_data_for_srp_cmd(cmnd, evt_struct, srp_cmd, hostdata->dev)) { diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c index dbae3fd..e3f7397 100644 --- a/drivers/scsi/initio.c +++ b/drivers/scsi/initio.c @@ -2590,7 +2590,7 @@ static void initio_build_scb(struct initio_host * host, struct scsi_ctrl_blk * c cblk->hastat = 0; cblk->tastat = 0; /* Command the command */ - memcpy(&cblk->cdb[0], &cmnd->cmnd, cmnd->cmd_len); + memcpy(cblk->cdb, cmnd->cmnd, cmnd->cmd_len); /* Set up tags */ if (cmnd->device->tagged_supported) { /* Tag Support */ diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c index 09ab3ea..fa06093 100644 --- a/drivers/scsi/qla1280.c +++ b/drivers/scsi/qla1280.c @@ -2858,7 +2858,7 @@ qla1280_64bit_start_scsi(struct scsi_qla_host *ha, struct srb * sp) /* Load SCSI command packet. */ pkt->cdb_len = cpu_to_le16(CMD_CDBLEN(cmd)); - memcpy(pkt->scsi_cdb, &(CMD_CDBP(cmd)), CMD_CDBLEN(cmd)); + memcpy(pkt->scsi_cdb, CMD_CDBP(cmd), CMD_CDBLEN(cmd)); /* dprintk(1, "Build packet for command[0]=0x%x\n",pkt->scsi_cdb[0]); */ /* Set transfer direction. */ @@ -3127,7 +3127,7 @@ qla1280_32bit_start_scsi(struct scsi_qla_host *ha, struct srb * sp) /* Load SCSI command packet. */ pkt->cdb_len = cpu_to_le16(CMD_CDBLEN(cmd)); - memcpy(pkt->scsi_cdb, &(CMD_CDBP(cmd)), CMD_CDBLEN(cmd)); + memcpy(pkt->scsi_cdb, CMD_CDBP(cmd), CMD_CDBLEN(cmd)); /*dprintk(1, "Build packet for command[0]=0x%x\n",pkt->scsi_cdb[0]); */ /* Set transfer direction. */ diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 1eaba6c..eaf5a8a 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -626,7 +626,7 @@ static void scsi_abort_eh_cmnd(struct scsi_cmnd *scmd) * @scmd: SCSI command structure to hijack * @ses: structure to save restore information * @cmnd: CDB to send. Can be NULL if no new cmnd is needed - * @cmnd_size: size in bytes of @cmnd + * @cmnd_size: size in bytes of @cmnd (must be <= BLK_MAX_CDB) * @sense_bytes: size of sense data to copy. or 0 (if != 0 @cmnd is ignored) * * This function is used to save a scsi command information before re-execution @@ -648,12 +648,14 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses, * command. */ ses->cmd_len = scmd->cmd_len; - memcpy(ses->cmnd, scmd->cmnd, sizeof(scmd->cmnd)); + ses->cmnd = scmd->cmnd; ses->data_direction = scmd->sc_data_direction; ses->sdb = scmd->sdb; ses->next_rq = scmd->request->next_rq; ses->result = scmd->result; + scmd->cmnd = ses->eh_cmnd; + memset(scmd->cmnd, 0, BLK_MAX_CDB); memset(&scmd->sdb, 0, sizeof(scmd->sdb)); scmd->request->next_rq = NULL; @@ -665,14 +667,13 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses, scmd->sdb.table.sgl = &ses->sense_sgl; scmd->sc_data_direction = DMA_FROM_DEVICE; scmd->sdb.table.nents = 1; - memset(scmd->cmnd, 0, sizeof(scmd->cmnd)); scmd->cmnd[0] = REQUEST_SENSE; scmd->cmnd[4] = scmd->sdb.length; scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]); } else { scmd->sc_data_direction = DMA_NONE; if (cmnd) { - memset(scmd->cmnd, 0, sizeof(scmd->cmnd)); + BUG_ON(cmnd_size > BLK_MAX_CDB); memcpy(scmd->cmnd, cmnd, cmnd_size); scmd->cmd_len = COMMAND_SIZE(scmd->cmnd[0]); } @@ -705,7 +706,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses) * Restore original data */ scmd->cmd_len = ses->cmd_len; - memcpy(scmd->cmnd, ses->cmnd, sizeof(scmd->cmnd)); + scmd->cmnd = ses->cmnd; scmd->sc_data_direction = ses->data_direction; scmd->sdb = ses->sdb; scmd->request->next_rq = ses->next_rq; @@ -1775,8 +1776,8 @@ scsi_reset_provider(struct scsi_device *dev, int flag) scmd->request = &req; memset(&scmd->eh_timeout, 0, sizeof(scmd->eh_timeout)); - memset(&scmd->cmnd, '\0', sizeof(scmd->cmnd)); - + scmd->cmnd = req.cmd; + scmd->scsi_done = scsi_reset_provider_done_command; memset(&scmd->sdb, 0, sizeof(scmd->sdb)); diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index d545ad1..a709849 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1094,6 +1094,8 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev, cmd->tag = req->tag; cmd->request = req; + cmd->cmnd = req->cmd; + return cmd; } @@ -1131,8 +1133,6 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) req->buffer = NULL; } - BUILD_BUG_ON(sizeof(req->cmd) > sizeof(cmd->cmnd)); - memcpy(cmd->cmnd, req->cmd, sizeof(cmd->cmnd)); cmd->cmd_len = req->cmd_len; if (!req->data_len) cmd->sc_data_direction = DMA_NONE; @@ -1169,6 +1169,7 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req) if (unlikely(!cmd)) return BLKPREP_DEFER; + memset(cmd->cmnd, 0, BLK_MAX_CDB); return scsi_init_io(cmd, GFP_ATOMIC); } EXPORT_SYMBOL(scsi_setup_fs_cmnd); diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c index ee8496a..257e097 100644 --- a/drivers/scsi/scsi_tgt_lib.c +++ b/drivers/scsi/scsi_tgt_lib.c @@ -107,6 +107,8 @@ struct scsi_cmnd *scsi_host_get_command(struct Scsi_Host *shost, cmd->jiffies_at_alloc = jiffies; cmd->request = rq; + cmd->cmnd = rq->cmd; + rq->special = cmd; rq->cmd_type = REQ_TYPE_SPECIAL; rq->cmd_flags |= REQ_TYPE_BLOCK_PC; diff --git a/drivers/usb/storage/cypress_atacb.c b/drivers/usb/storage/cypress_atacb.c index d88824b..898e67d 100644 --- a/drivers/usb/storage/cypress_atacb.c +++ b/drivers/usb/storage/cypress_atacb.c @@ -46,7 +46,7 @@ void cypress_atacb_passthrough(struct scsi_cmnd *srb, struct us_data *us) } memcpy(save_cmnd, srb->cmnd, sizeof(save_cmnd)); - memset(srb->cmnd, 0, sizeof(srb->cmnd)); + memset(srb->cmnd, 0, MAX_COMMAND_SIZE); /* check if we support the command */ if (save_cmnd[1] >> 5) /* MULTIPLE_COUNT */ diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c index 971d13d..3addcd8 100644 --- a/drivers/usb/storage/isd200.c +++ b/drivers/usb/storage/isd200.c @@ -292,6 +292,7 @@ struct isd200_info { /* maximum number of LUNs supported */ unsigned char MaxLUNs; + unsigned char cmnd[BLK_MAX_CDB]; struct scsi_cmnd srb; struct scatterlist sg; }; @@ -450,6 +451,7 @@ static int isd200_action( struct us_data *us, int action, memset(&ata, 0, sizeof(ata)); memset(&srb_dev, 0, sizeof(srb_dev)); + srb->cmnd = info->cmnd; srb->device = &srb_dev; ++srb->serial_number; diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 8d20e60..7ed883c 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -7,10 +7,28 @@ #include <linux/types.h> #include <linux/timer.h> #include <linux/scatterlist.h> +#include <linux/blkdev.h> struct Scsi_Host; struct scsi_device; +/* + * MAX_COMMAND_SIZE is: + * The longest fixed-length SCSI CDB as per the SCSI standard. + * fixed-length means: commands that their size can be determined + * by their opcode and the CDB does not carry a length specifier, (unlike + * the VARIABLE_LENGTH_CMD(0x7f) command). This is actually not exactly + * true and the SCSI standard also defines extended commands and + * vendor specific commands that can be bigger than 16 bytes. The kernel + * will support these using the same infrastructure used for VARLEN CDB's. + * So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml + * supports without specifying a cmd_len by ULD's + */ +#define MAX_COMMAND_SIZE 16 +#if (MAX_COMMAND_SIZE > BLK_MAX_CDB) +# error MAX_COMMAND_SIZE can not be bigger than BLK_MAX_CDB +#endif + struct scsi_data_buffer { struct sg_table table; unsigned length; @@ -64,8 +82,7 @@ struct scsi_cmnd { enum dma_data_direction sc_data_direction; /* These elements define the operation we are about to perform */ -#define MAX_COMMAND_SIZE 16 - unsigned char cmnd[MAX_COMMAND_SIZE]; + unsigned char *cmnd; struct timer_list eh_timeout; /* Used to time out the command. */ diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h index d3a133b..2a9add2 100644 --- a/include/scsi/scsi_eh.h +++ b/include/scsi/scsi_eh.h @@ -75,11 +75,11 @@ struct scsi_eh_save { int result; enum dma_data_direction data_direction; unsigned char cmd_len; - unsigned char cmnd[MAX_COMMAND_SIZE]; + unsigned char *cmnd; struct scsi_data_buffer sdb; struct request *next_rq; - /* new command support */ + unsigned char eh_cmnd[BLK_MAX_CDB]; struct scatterlist sense_sgl; }; -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs 2008-04-30 8:13 [PATCH 3/3] scsi support variable length commands Boaz Harrosh 2008-04-30 8:19 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh @ 2008-04-30 8:27 ` Boaz Harrosh 2008-04-30 8:30 ` [PATCH 3/3] iscsi_tcp: Enable any size command Boaz Harrosh 2 siblings, 0 replies; 12+ messages in thread From: Boaz Harrosh @ 2008-04-30 8:27 UTC (permalink / raw) To: James Bottomley, Mike Christie, linux-scsi; +Cc: FUJITA Tomonori Add support for variable-length, extended, and vendor specific CDBs to scsi-ml. It is now possible for initiators and ULD's to issue these types of commands. LLDs need not change much. All they need is to raise the .max_cmd_len to the longest command they support (see iscsi patch). - clean-up some code paths that did not expect commands to be larger than 16, and change cmd_len members' type to short as char is not enough. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- block/scsi_ioctl.c | 5 ++--- drivers/scsi/constants.c | 10 +++------- drivers/scsi/scsi.c | 15 ++++----------- drivers/scsi/scsi_lib.c | 2 +- include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- include/scsi/scsi_cmnd.h | 2 +- include/scsi/scsi_host.h | 8 +++----- 7 files changed, 47 insertions(+), 35 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index ffa3720..78199c0 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -33,13 +33,12 @@ #include <scsi/scsi_cmnd.h> /* Command group 3 is reserved and should never be used. */ -const unsigned char scsi_command_size[8] = +const unsigned char scsi_command_size_tbl[8] = { 6, 10, 10, 12, 16, 12, 10, 10 }; - -EXPORT_SYMBOL(scsi_command_size); +EXPORT_SYMBOL(scsi_command_size_tbl); #include <scsi/sg.h> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c index 403a7f2..9785d73 100644 --- a/drivers/scsi/constants.c +++ b/drivers/scsi/constants.c @@ -28,7 +28,6 @@ #define SERVICE_ACTION_OUT_12 0xa9 #define SERVICE_ACTION_IN_16 0x9e #define SERVICE_ACTION_OUT_16 0x9f -#define VARIABLE_LENGTH_CMD 0x7f @@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) cdb0 = cdbp[0]; switch(cdb0) { case VARIABLE_LENGTH_CMD: - len = cdbp[7] + 8; + len = scsi_varlen_cdb_length(cdbp); if (len < 10) { printk("short variable length command, " "len=%d ext_len=%d", len, cdb_len); @@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) cdb0 = cdbp[0]; switch(cdb0) { case VARIABLE_LENGTH_CMD: - len = cdbp[7] + 8; + len = scsi_varlen_cdb_length(cdbp); if (len < 10) { printk("short opcode=0x%x command, len=%d " "ext_len=%d", cdb0, len, cdb_len); @@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb) int k, len; print_opcode_name(cdb, 0); - if (VARIABLE_LENGTH_CMD == cdb[0]) - len = cdb[7] + 8; - else - len = COMMAND_SIZE(cdb[0]); + len = scsi_command_size(cdb); /* print out all bytes in cdb */ for (k = 0; k < len; ++k) printk(" %02x", cdb[k]); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index dc36321..b4f997a 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd); #define MIN_RESET_PERIOD (15*HZ) /* - * Macro to determine the size of SCSI command. This macro takes vendor - * unique commands into account. SCSI commands in groups 6 and 7 are - * vendor unique and we will depend upon the command length being - * supplied correctly in cmd_len. - */ -#define CDB_SIZE(cmd) (((((cmd)->cmnd[0] >> 5) & 7) < 6) ? \ - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) - -/* * Note - the initial logging level can be set here to log events at boot time. * After the system is up, you may enable logging via the /proc interface. */ @@ -709,9 +700,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) * Before we queue this command, check if the command * length exceeds what the host adapter can handle. */ - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { + if (cmd->cmd_len > cmd->device->host->max_cmd_len) { SCSI_LOG_MLQUEUE(3, - printk("queuecommand : command too long.\n")); + printk("queuecommand : command too long. " + "cdb_size=%d host->max_cmd_len=%d\n", + cmd->cmd_len, cmd->device->host->max_cmd_len)); cmd->result = (DID_ABORT << 16); scsi_done(cmd); diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index a709849..a82d2fe 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -445,7 +445,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) scsi_set_resid(cmd, 0); memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); if (cmd->cmd_len == 0) - cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]); + cmd->cmd_len = scsi_command_size(cmd->cmnd); } void scsi_device_unbusy(struct scsi_device *sdev) diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 1f74bcd..32742c4 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -30,13 +30,6 @@ #endif /* - * SCSI command lengths - */ - -extern const unsigned char scsi_command_size[8]; -#define COMMAND_SIZE(opcode) scsi_command_size[((opcode) >> 5) & 7] - -/* * Special value for scanning to specify scanning or rescanning of all * possible channels, (target) ids, or luns on a given shost. */ @@ -109,6 +102,7 @@ extern const unsigned char scsi_command_size[8]; #define MODE_SENSE_10 0x5a #define PERSISTENT_RESERVE_IN 0x5e #define PERSISTENT_RESERVE_OUT 0x5f +#define VARIABLE_LENGTH_CMD 0x7f #define REPORT_LUNS 0xa0 #define MAINTENANCE_IN 0xa3 #define MOVE_MEDIUM 0xa5 @@ -136,6 +130,38 @@ extern const unsigned char scsi_command_size[8]; #define ATA_12 0xa1 /* 12-byte pass-thru */ /* + * SCSI command lengths + */ + +#define SCSI_MAX_VARLEN_CDB_SIZE 260 + +/* defined in T10 SCSI Primary Commands-2 (SPC2) */ +struct scsi_varlen_cdb_hdr { + u8 opcode; /* opcode always == VARIABLE_LENGTH_CMD */ + u8 control; + u8 misc[5]; + u8 additional_cdb_length; /* total cdb length - 8 */ + __be16 service_action; + /* service specific data follows */ +}; + +static inline unsigned +scsi_varlen_cdb_length(const void *hdr) +{ + return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; +} + +extern const unsigned char scsi_command_size_tbl[8]; +#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] + +static inline unsigned +scsi_command_size(const unsigned char *cmnd) +{ + return (cmnd[0] == VARIABLE_LENGTH_CMD) ? + scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]); +} + +/* * SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft * T10/1561-D Revision 4 Draft dated 7th November 2002. */ diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 7ed883c..3e46dfa 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -78,7 +78,7 @@ struct scsi_cmnd { int allowed; int timeout_per_command; - unsigned char cmd_len; + unsigned short cmd_len; enum dma_data_direction sc_data_direction; /* These elements define the operation we are about to perform */ diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index d967d6d..1834fdf 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -573,13 +573,11 @@ struct Scsi_Host { /* * The maximum length of SCSI commands that this host can accept. * Probably 12 for most host adapters, but could be 16 for others. + * or 260 if the driver supports variable length cdbs. * For drivers that don't set this field, a value of 12 is - * assumed. I am leaving this as a number rather than a bit - * because you never know what subsequent SCSI standards might do - * (i.e. could there be a 20 byte or a 24-byte command a few years - * down the road?). + * assumed. */ - unsigned char max_cmd_len; + unsigned short max_cmd_len; int this_id; int can_queue; -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] iscsi_tcp: Enable any size command 2008-04-30 8:13 [PATCH 3/3] scsi support variable length commands Boaz Harrosh 2008-04-30 8:19 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh 2008-04-30 8:27 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh @ 2008-04-30 8:30 ` Boaz Harrosh 2008-05-12 15:47 ` James Bottomley 2 siblings, 1 reply; 12+ messages in thread From: Boaz Harrosh @ 2008-04-30 8:30 UTC (permalink / raw) To: James Bottomley, Mike Christie, linux-scsi; +Cc: FUJITA Tomonori Let through upto the largest command of 260 defined by the scsi standard. iscsi core supports this already. Now that the scsi-ml supports it we can start using large commands. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> --- drivers/scsi/iscsi_tcp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 72b9b2a..826c97c 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { .host_template = &iscsi_sht, .conndata_size = sizeof(struct iscsi_conn), .max_conn = 1, - .max_cmd_len = 16, + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, /* session management */ .create_session = iscsi_tcp_session_create, .destroy_session = iscsi_tcp_session_destroy, -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iscsi_tcp: Enable any size command 2008-04-30 8:30 ` [PATCH 3/3] iscsi_tcp: Enable any size command Boaz Harrosh @ 2008-05-12 15:47 ` James Bottomley 2008-05-13 8:52 ` Boaz Harrosh 0 siblings, 1 reply; 12+ messages in thread From: James Bottomley @ 2008-05-12 15:47 UTC (permalink / raw) To: Boaz Harrosh; +Cc: Mike Christie, linux-scsi, FUJITA Tomonori On Wed, 2008-04-30 at 11:30 +0300, Boaz Harrosh wrote: > Let through upto the largest command of 260 defined by the scsi standard. > iscsi core supports this already. Now that the scsi-ml supports it we can > start using large commands. > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> > Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> > --- > drivers/scsi/iscsi_tcp.c | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > index 72b9b2a..826c97c 100644 > --- a/drivers/scsi/iscsi_tcp.c > +++ b/drivers/scsi/iscsi_tcp.c > @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { > .host_template = &iscsi_sht, > .conndata_size = sizeof(struct iscsi_conn), > .max_conn = 1, > - .max_cmd_len = 16, > + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, > /* session management */ > .create_session = iscsi_tcp_session_create, > .destroy_session = iscsi_tcp_session_destroy, OK, this isn't quite right. The escb definition in iscsi.h is: struct iscsi_ecdb_ahdr { __be16 ahslength; /* CDB length - 15, including reserved byte */ uint8_t ahstype; uint8_t reserved; /* 4-byte aligned extended CDB spillover */ uint8_t ecdb[260 - ISCSI_CDB_SIZE]; }; Either that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE or we need to hard code 260 in the max_cmd_len. Since SCSI_MAX_VARLEN_CDB_SIZE is really a useless constant (nothing depends on it), and internal packets in iscsi depend on this, it probably makes the most sense for this to be an iscsi local constant. The value (260) also looks a bit bogus, isn't 262 the maximum possible size for a 0x7f variable length command? The iSCSI maxiumum is far higher than this (but no protocol sends anything above the 0x7f maximum currently). James James ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iscsi_tcp: Enable any size command 2008-05-12 15:47 ` James Bottomley @ 2008-05-13 8:52 ` Boaz Harrosh 2008-05-13 14:24 ` James Bottomley 0 siblings, 1 reply; 12+ messages in thread From: Boaz Harrosh @ 2008-05-13 8:52 UTC (permalink / raw) To: James Bottomley; +Cc: Mike Christie, linux-scsi, FUJITA Tomonori James Bottomley wrote: > On Wed, 2008-04-30 at 11:30 +0300, Boaz Harrosh wrote: >> Let through upto the largest command of 260 defined by the scsi standard. >> iscsi core supports this already. Now that the scsi-ml supports it we can >> start using large commands. >> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> >> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> >> --- >> drivers/scsi/iscsi_tcp.c | 2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c >> index 72b9b2a..826c97c 100644 >> --- a/drivers/scsi/iscsi_tcp.c >> +++ b/drivers/scsi/iscsi_tcp.c >> @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { >> .host_template = &iscsi_sht, >> .conndata_size = sizeof(struct iscsi_conn), >> .max_conn = 1, >> - .max_cmd_len = 16, >> + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, >> /* session management */ >> .create_session = iscsi_tcp_session_create, >> .destroy_session = iscsi_tcp_session_destroy, > > OK, this isn't quite right. The escb definition in iscsi.h is: > struct iscsi_ecdb_ahdr { > __be16 ahslength; /* CDB length - 15, including reserved byte */ > uint8_t ahstype; > uint8_t reserved; > /* 4-byte aligned extended CDB spillover */ > uint8_t ecdb[260 - ISCSI_CDB_SIZE]; > }; > > Either that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE or we need to > hard code 260 in the max_cmd_len. > Yes that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE. The reason it is not is because that code is much older than the definition of SCSI_MAX_VARLEN_CDB_SIZE. > Since SCSI_MAX_VARLEN_CDB_SIZE is really a useless constant (nothing > depends on it), and internal packets in iscsi depend on this, it > probably makes the most sense for this to be an iscsi local constant. > As you said below, this is not an iscsi limitation it is a scsi limitation. Logically it belongs to scsi.h near the varlen definitions. If you prefer hard coded constants I don't mind, just that from the school I came from they would fail me if I did that, even for a single user. > The value (260) also looks a bit bogus, isn't 262 the maximum possible > size for a 0x7f variable length command? The iSCSI maxiumum is far > higher than this (but no protocol sends anything above the 0x7f maximum > currently). > 260 comes from the scsi standard. The 8th byte of a scsi varlen header is a one byte length specifier. (see struct scsi_varlen_cdb_hdr in scsi.h) Now the standard says that the header must be 4 bytes aligned so the maximum that can be written in that byte is 252, plus the constant 8. > James > > Please choose the one you want. Mike I will need your ACK on one of these. --- From: Boaz Harrosh <bharrosh@panasas.com> Date: Sun, 13 Apr 2008 17:52:42 +0300 Subject: [PATCH] iscsi_tcp: Enable any size command Let through upto the largest command of 260 defined by the scsi standard. iscsi core supports this already. Now that the scsi-ml supports it we can start using large commands. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> --- drivers/scsi/iscsi_tcp.c | 2 +- include/scsi/iscsi_proto.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 72b9b2a..826c97c 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { .host_template = &iscsi_sht, .conndata_size = sizeof(struct iscsi_conn), .max_conn = 1, - .max_cmd_len = 16, + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, /* session management */ .create_session = iscsi_tcp_session_create, .destroy_session = iscsi_tcp_session_destroy, diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h index e0593bf..f2a2c11 100644 --- a/include/scsi/iscsi_proto.h +++ b/include/scsi/iscsi_proto.h @@ -22,6 +22,7 @@ #define ISCSI_PROTO_H #include <linux/types.h> +#include <scsi/scsi.h> #define ISCSI_DRAFT20_VERSION 0x00 @@ -156,7 +157,7 @@ struct iscsi_ecdb_ahdr { uint8_t ahstype; uint8_t reserved; /* 4-byte aligned extended CDB spillover */ - uint8_t ecdb[260 - ISCSI_CDB_SIZE]; + uint8_t ecdb[SCSI_MAX_VARLEN_CDB_SIZE - ISCSI_CDB_SIZE]; }; /* SCSI Response Header */ --- From: Boaz Harrosh <bharrosh@panasas.com> Date: Sun, 13 Apr 2008 17:52:42 +0300 Subject: [PATCH] iscsi_tcp: Enable any size command Let through upto the largest command of 260 defined by the scsi standard. iscsi core supports this already. Now that the scsi-ml supports it we can start using large commands. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> --- drivers/scsi/iscsi_tcp.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c index 72b9b2a..073dea7 100644 --- a/drivers/scsi/iscsi_tcp.c +++ b/drivers/scsi/iscsi_tcp.c @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { .host_template = &iscsi_sht, .conndata_size = sizeof(struct iscsi_conn), .max_conn = 1, - .max_cmd_len = 16, + .max_cmd_len = 260, /* session management */ .create_session = iscsi_tcp_session_create, .destroy_session = iscsi_tcp_session_destroy, ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iscsi_tcp: Enable any size command 2008-05-13 8:52 ` Boaz Harrosh @ 2008-05-13 14:24 ` James Bottomley 2008-05-13 16:01 ` Boaz Harrosh 0 siblings, 1 reply; 12+ messages in thread From: James Bottomley @ 2008-05-13 14:24 UTC (permalink / raw) To: Boaz Harrosh; +Cc: Mike Christie, linux-scsi, FUJITA Tomonori On Tue, 2008-05-13 at 11:52 +0300, Boaz Harrosh wrote: > James Bottomley wrote: > > On Wed, 2008-04-30 at 11:30 +0300, Boaz Harrosh wrote: > >> Let through upto the largest command of 260 defined by the scsi standard. > >> iscsi core supports this already. Now that the scsi-ml supports it we can > >> start using large commands. > >> > >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> > >> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> > >> --- > >> drivers/scsi/iscsi_tcp.c | 2 +- > >> 1 files changed, 1 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c > >> index 72b9b2a..826c97c 100644 > >> --- a/drivers/scsi/iscsi_tcp.c > >> +++ b/drivers/scsi/iscsi_tcp.c > >> @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { > >> .host_template = &iscsi_sht, > >> .conndata_size = sizeof(struct iscsi_conn), > >> .max_conn = 1, > >> - .max_cmd_len = 16, > >> + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, > >> /* session management */ > >> .create_session = iscsi_tcp_session_create, > >> .destroy_session = iscsi_tcp_session_destroy, > > > > OK, this isn't quite right. The escb definition in iscsi.h is: > > struct iscsi_ecdb_ahdr { > > __be16 ahslength; /* CDB length - 15, including reserved byte */ > > uint8_t ahstype; > > uint8_t reserved; > > /* 4-byte aligned extended CDB spillover */ > > uint8_t ecdb[260 - ISCSI_CDB_SIZE]; > > }; > > > > Either that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE or we need to > > hard code 260 in the max_cmd_len. > > > > Yes that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE. The reason it > is not is because that code is much older than the definition of > SCSI_MAX_VARLEN_CDB_SIZE. > > > Since SCSI_MAX_VARLEN_CDB_SIZE is really a useless constant (nothing > > depends on it), and internal packets in iscsi depend on this, it > > probably makes the most sense for this to be an iscsi local constant. > > > > As you said below, this is not an iscsi limitation it is a scsi > limitation. Logically it belongs to scsi.h near the varlen definitions. > If you prefer hard coded constants I don't mind, just that from the school > I came from they would fail me if I did that, even for a single user. > > > The value (260) also looks a bit bogus, isn't 262 the maximum possible > > size for a 0x7f variable length command? The iSCSI maxiumum is far > > higher than this (but no protocol sends anything above the 0x7f maximum > > currently). > > > > 260 comes from the scsi standard. The 8th byte of a scsi varlen header > is a one byte length specifier. (see struct scsi_varlen_cdb_hdr in scsi.h) > Now the standard says that the header must be 4 bytes aligned so the > maximum that can be written in that byte is 252, plus the constant 8. I don't think it can be alignment issues otherwise six byte commands like READ_6/WRITE_6 would be illegal. I don't think there are any alignment requirements per se. However, it does look like the definition section of SAM-3:3.1.15 does say "... or a variable length of between 12 and 260 bytes" with no reason given, so that will do. James ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] iscsi_tcp: Enable any size command 2008-05-13 14:24 ` James Bottomley @ 2008-05-13 16:01 ` Boaz Harrosh 0 siblings, 0 replies; 12+ messages in thread From: Boaz Harrosh @ 2008-05-13 16:01 UTC (permalink / raw) To: James Bottomley; +Cc: Mike Christie, linux-scsi, FUJITA Tomonori James Bottomley wrote: > On Tue, 2008-05-13 at 11:52 +0300, Boaz Harrosh wrote: >> James Bottomley wrote: >>> On Wed, 2008-04-30 at 11:30 +0300, Boaz Harrosh wrote: >>>> Let through upto the largest command of 260 defined by the scsi standard. >>>> iscsi core supports this already. Now that the scsi-ml supports it we can >>>> start using large commands. >>>> >>>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> >>>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu> >>>> --- >>>> drivers/scsi/iscsi_tcp.c | 2 +- >>>> 1 files changed, 1 insertions(+), 1 deletions(-) >>>> >>>> diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c >>>> index 72b9b2a..826c97c 100644 >>>> --- a/drivers/scsi/iscsi_tcp.c >>>> +++ b/drivers/scsi/iscsi_tcp.c >>>> @@ -1978,7 +1978,7 @@ static struct iscsi_transport iscsi_tcp_transport = { >>>> .host_template = &iscsi_sht, >>>> .conndata_size = sizeof(struct iscsi_conn), >>>> .max_conn = 1, >>>> - .max_cmd_len = 16, >>>> + .max_cmd_len = SCSI_MAX_VARLEN_CDB_SIZE, >>>> /* session management */ >>>> .create_session = iscsi_tcp_session_create, >>>> .destroy_session = iscsi_tcp_session_destroy, >>> OK, this isn't quite right. The escb definition in iscsi.h is: >>> struct iscsi_ecdb_ahdr { >>> __be16 ahslength; /* CDB length - 15, including reserved byte */ >>> uint8_t ahstype; >>> uint8_t reserved; >>> /* 4-byte aligned extended CDB spillover */ >>> uint8_t ecdb[260 - ISCSI_CDB_SIZE]; >>> }; >>> >>> Either that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE or we need to >>> hard code 260 in the max_cmd_len. >>> >> Yes that 260 needs to become SCSI_MAX_VARLEN_CDB_SIZE. The reason it >> is not is because that code is much older than the definition of >> SCSI_MAX_VARLEN_CDB_SIZE. >> >>> Since SCSI_MAX_VARLEN_CDB_SIZE is really a useless constant (nothing >>> depends on it), and internal packets in iscsi depend on this, it >>> probably makes the most sense for this to be an iscsi local constant. >>> >> As you said below, this is not an iscsi limitation it is a scsi >> limitation. Logically it belongs to scsi.h near the varlen definitions. >> If you prefer hard coded constants I don't mind, just that from the school >> I came from they would fail me if I did that, even for a single user. >> >>> The value (260) also looks a bit bogus, isn't 262 the maximum possible >>> size for a 0x7f variable length command? The iSCSI maxiumum is far >>> higher than this (but no protocol sends anything above the 0x7f maximum >>> currently). >>> >> 260 comes from the scsi standard. The 8th byte of a scsi varlen header >> is a one byte length specifier. (see struct scsi_varlen_cdb_hdr in scsi.h) >> Now the standard says that the header must be 4 bytes aligned so the >> maximum that can be written in that byte is 252, plus the constant 8. > > I don't think it can be alignment issues otherwise six byte commands > like READ_6/WRITE_6 would be illegal. I don't think there are any > alignment requirements per se. However, it does look like the > definition section of SAM-3:3.1.15 does say "... or a variable length of > between 12 and 260 bytes" with no reason given, so that will do. > It's only for varlen. >From SCSI_Primary_Commands-3-spc3r23 section 4.3.3 The variable length CDB formats: "The ADDITIONAL CDB LENGTH field specifies the number of additional CDB bytes. This value in the ADDITIONAL CDB LENGTH field shall be a multiple of 4" > James > Thanks Boaz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3 ver2] block layer extended-cdb support @ 2008-04-13 16:17 FUJITA Tomonori 2008-04-14 10:50 ` [PATCH 0/4] add large command support to the block layer FUJITA Tomonori 0 siblings, 1 reply; 12+ messages in thread From: FUJITA Tomonori @ 2008-04-13 16:17 UTC (permalink / raw) To: bharrosh Cc: fujita.tomonori, jens.axboe, James.Bottomley, hch, linux-scsi, akpm On Sun, 13 Apr 2008 12:13:18 +0300 Boaz Harrosh <bharrosh@panasas.com> wrote: > On Sat, Apr 12 2008 at 8:52 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > > On Sun, 06 Apr 2008 12:35:04 +0300 > > Boaz Harrosh <bharrosh@panasas.com> wrote: > > > >> On Fri, Apr 04 2008 at 14:46 +0300, Jens Axboe <jens.axboe@oracle.com> wrote: > >>> On Thu, Apr 03 2008, Boaz Harrosh wrote: > >>>> static void req_bio_endio(struct request *rq, struct bio *bio, > >>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > >>>> index 6f79d40..2f87c9d 100644 > >>>> --- a/include/linux/blkdev.h > >>>> +++ b/include/linux/blkdev.h > >>>> @@ -213,8 +213,15 @@ struct request { > >>>> /* > >>>> * when request is used as a packet command carrier > >>>> */ > >>>> - unsigned int cmd_len; > >>>> - unsigned char cmd[BLK_MAX_CDB]; > >>>> + unsigned short cmd_len; > >>>> + unsigned short ext_cdb_len; /* length of ext_cdb buffer */ > >>>> + union { > >>>> + unsigned char cmd[BLK_MAX_CDB]; > >>>> + unsigned char *ext_cdb;/* an optional extended cdb. > >>>> + * points to a user buffer that must > >>>> + * be valid until end of request > >>>> + */ > >>>> + }; > >>> Why not just something ala > >>> > >>> unsigned short cmd_len; > >>> unsigned char __cmd[BLK_MAX_CDB]; > >>> unsigned char *cmd; > >>> > >>> and then have rq_init() do > >>> > >>> rq->cmd = rq->__cmd; > >>> > >>> and just have a function for setting up a larger ->cmd and adjusting > >>> ->cmd_len in the process? > >>> > >>> Then rq_set_cdb() would be > >>> > >>> static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len) > >>> { > >>> rq->cmd = cdb; > >>> rq->cmd_len = cdb_len; > >>> } > >>> > >>> and rq_get_cdb() plus rq_get_cdb_len() could just go away. > >>> > >> Because this way it is dangerous if large commands are issued to legacy > >> drivers. In scsi-land we have .cmd_len at host template that will govern if > >> we are allow to issue larger commands to the driver. In block devices we do > >> not have such a facility, and the danger is if such commands are issued through > >> bsg or other means, even by malicious code. What you say is the ideal and it > >> is what I've done for scsi, but for block devices we can not do that yet. > >> With the way I did it here, Legacy drivers will see zero length command and > >> will do the right thing, from what I've seen. > > > > What are exactly block devices? ub and ide? > > > > bsg are created only for scsi devices (and scsi objects like sas host) > > now. Are there other means to send commands except for ioctl? > > I'm not 100% sure either way, so I would like to be safe. Any way, there > is the size issue, this way we add *nothing* at all, so it looks preferable. > The final outcome will be the same both ways. I think that a clean design is an important issue than the sizeof of struct request. > I would like if you reconsider the ugliness issue. I admit that at first I > personally disliked it, but now that I look at it, I think it is cleaner, > coding style, this way. Because the union points out the exclusiveness of > the two systems, the striate way give the notion of two separate systems. That's a ugly hack for me. Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len? For me, as Jens proposed, having only cmd_len is the right way. And 'cdb' name is not appropriate for the block layer, I think. I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/4] add large command support to the block layer 2008-04-13 16:17 [PATCH 2/3 ver2] block layer extended-cdb support FUJITA Tomonori @ 2008-04-14 10:50 ` FUJITA Tomonori 2008-04-15 12:24 ` [PATCH 0/3] scsi: variable-length CDBs support Boaz Harrosh 0 siblings, 1 reply; 12+ messages in thread From: FUJITA Tomonori @ 2008-04-14 10:50 UTC (permalink / raw) To: linux-scsi Cc: bharrosh, Jens Axboe, Bartlomiej Zolnierkiewicz, James Bottomley, Alasdair G Kergon, Geert Uytterhoeven, FUJITA Tomonori This patchset adds large command support to the block layer. We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array. In short, rq_init() does rq->cmd = rq->__cmd; So we can access to rq->cmd and rq->cmd_len as before. Boaz argues that having single cmd_len is dangerous: > Because this way it is dangerous if large commands are issued to legacy > drivers. In scsi-land we have .cmd_len at host template that will govern if > we are allow to issue larger commands to the driver. In block devices we do > not have such a facility, and the danger is if such commands are issued through > bsg or other means, even by malicious code. What you say is the ideal and it I don't see how it is dangerous. We don't send large commands to block devices via bsg. We can send large commands to block devices via only SG_IO. SG_IO (block/scsi_ioctl.c) forbids us sending commands large than 16 bytes, BLK_MAX_CDB. So how can we send large commands to block devices? The first, second, and third patches are safe cleanups (a preparation for changing rq->cmd from the static array to a pointer). They don't conflict with Boaz's patchset. Even if we choose Boaz's approach now, we'll move to the approach to have one command length in struct request in the long term. So they can be applied now. The forth patch adds large command support. It's a substitute for Boaz's second patch. It's pretty simple except for one hack for ide-io. It's necessary since the ide subsystem allocates struct request without using blk_get_request. In the long term, it would better to convert it to use blk_get_request properly. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/3] scsi: variable-length CDBs support 2008-04-14 10:50 ` [PATCH 0/4] add large command support to the block layer FUJITA Tomonori @ 2008-04-15 12:24 ` Boaz Harrosh 2008-04-15 12:34 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 0 siblings, 1 reply; 12+ messages in thread From: Boaz Harrosh @ 2008-04-15 12:24 UTC (permalink / raw) To: FUJITA Tomonori Cc: linux-scsi, Jens Axboe, Bartlomiej Zolnierkiewicz, James Bottomley, Alasdair G Kergon, Geert Uytterhoeven Based on TOMO's block patches for supporting large commands These patches will enable support for variable-length, extended, and vendor specific CDBs. It should now cover the entire range of the SCSI standard. These patches are independent of TOMO's patches to the block layer, because the block layer API for drivers did not change. So all I did was make sure nothing will break when large commands start coming. They are based on scsi-misc and could go in as is. I have tested my OSD setup with scsi-misc + for-2.6.26 branch from Jens + these patches and it all works fine. So I'm happy. [1/3] Let scsi_cmnd->cmnd use request->cmd buffer Here I let scsi_cmnd->cmnd point to the space allocated by request->cmd, instead of copying the data. The scsi_cmnd->cmd_len is guaranteed to contain the right length of the command. I have tried to go over every single place in the kernel that uses scsi_cmnd->cmnd and make sure it looks sane. Surprisingly to me, that was not at all bad. I hope I did not miss anything. I've tested on an x86_64 machine booting from a sata disk and ran the iscsi regression tests as well as my bidi and varlen tests on top of the complete patchset and all tests passed. [2/3] scsi: varlen extended and vendor-specific cdbs Adds support for variable length, extended, and vendor-specific cdbs at the scsi mid-layer. [3/3] iscsi_tcp: Enable large commands One liner for iscsi_tcp. Mike please ACK this patch Boaz ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs 2008-04-15 12:24 ` [PATCH 0/3] scsi: variable-length CDBs support Boaz Harrosh @ 2008-04-15 12:34 ` Boaz Harrosh 2008-04-16 2:09 ` FUJITA Tomonori 0 siblings, 1 reply; 12+ messages in thread From: Boaz Harrosh @ 2008-04-15 12:34 UTC (permalink / raw) To: FUJITA Tomonori, James Bottomley Cc: linux-scsi, Jens Axboe, Bartlomiej Zolnierkiewicz, Alasdair G Kergon, Geert Uytterhoeven Add support for variable-length, extended, and vendor specific CDBs to scsi-ml. It is now possible for initiators and ULD's to issue these types of commands. LLDs need not change much. All they need is to raise the .max_cmd_len to the longest command they support (see iscsi patch). - clean-up some code paths that did not expect commands to be larger than 16, and change cmd_len members' type to short as char is not enough. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- block/scsi_ioctl.c | 5 ++--- drivers/scsi/constants.c | 10 +++------- drivers/scsi/scsi.c | 22 +++++++++++----------- drivers/scsi/scsi_lib.c | 8 ++++++-- include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- include/scsi/scsi_cmnd.h | 2 +- include/scsi/scsi_host.h | 8 +++----- 7 files changed, 59 insertions(+), 36 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index a2c3a93..aaf07e4 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -33,13 +33,12 @@ #include <scsi/scsi_cmnd.h> /* Command group 3 is reserved and should never be used. */ -const unsigned char scsi_command_size[8] = +const unsigned char scsi_command_size_tbl[8] = { 6, 10, 10, 12, 16, 12, 10, 10 }; - -EXPORT_SYMBOL(scsi_command_size); +EXPORT_SYMBOL(scsi_command_size_tbl); #include <scsi/sg.h> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c index 403a7f2..9785d73 100644 --- a/drivers/scsi/constants.c +++ b/drivers/scsi/constants.c @@ -28,7 +28,6 @@ #define SERVICE_ACTION_OUT_12 0xa9 #define SERVICE_ACTION_IN_16 0x9e #define SERVICE_ACTION_OUT_16 0x9f -#define VARIABLE_LENGTH_CMD 0x7f @@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) cdb0 = cdbp[0]; switch(cdb0) { case VARIABLE_LENGTH_CMD: - len = cdbp[7] + 8; + len = scsi_varlen_cdb_length(cdbp); if (len < 10) { printk("short variable length command, " "len=%d ext_len=%d", len, cdb_len); @@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) cdb0 = cdbp[0]; switch(cdb0) { case VARIABLE_LENGTH_CMD: - len = cdbp[7] + 8; + len = scsi_varlen_cdb_length(cdbp); if (len < 10) { printk("short opcode=0x%x command, len=%d " "ext_len=%d", cdb0, len, cdb_len); @@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb) int k, len; print_opcode_name(cdb, 0); - if (VARIABLE_LENGTH_CMD == cdb[0]) - len = cdb[7] + 8; - else - len = COMMAND_SIZE(cdb[0]); + len = scsi_command_size(cdb); /* print out all bytes in cdb */ for (k = 0; k < len; ++k) printk(" %02x", cdb[k]); diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index f6980bd..3dabecb 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd); #define MIN_RESET_PERIOD (15*HZ) /* - * Macro to determine the size of SCSI command. This macro takes vendor - * unique commands into account. SCSI commands in groups 6 and 7 are - * vendor unique and we will depend upon the command length being - * supplied correctly in cmd_len. - */ -#define CDB_SIZE(cmd) (((((cmd)->cmnd[0] >> 5) & 7) < 6) ? \ - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) - -/* * Note - the initial logging level can be set here to log events at boot time. * After the system is up, you may enable logging via the /proc interface. */ @@ -620,6 +611,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) unsigned long flags = 0; unsigned long timeout; int rtn = 0; + unsigned cmd_len; /* check if the device is still usable */ if (unlikely(cmd->device->sdev_state == SDEV_DEL)) { @@ -701,9 +693,17 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) * Before we queue this command, check if the command * length exceeds what the host adapter can handle. */ - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { + cmd_len = cmd->cmd_len; + if (!cmd_len) { + BUG_ON(cmd->cmnd[0] == VARIABLE_LENGTH_CMD); + cmd_len = COMMAND_SIZE((cmd)->cmnd[0]); + } + + if (cmd_len > cmd->device->host->max_cmd_len) { SCSI_LOG_MLQUEUE(3, - printk("queuecommand : command too long.\n")); + printk("queuecommand : command too long. " + "cdb_size=%d host->max_cmd_len=%d\n", + cmd->cmd_len, cmd->device->host->max_cmd_len)); cmd->result = (DID_ABORT << 16); scsi_done(cmd); diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 325270b..e621505 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -195,6 +195,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, req->cmd_len = COMMAND_SIZE(cmd[0]); memcpy(req->cmd, cmd, req->cmd_len); + req->sense = sense; req->sense_len = 0; req->retries = retries; @@ -445,7 +446,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) scsi_set_resid(cmd, 0); memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); if (cmd->cmd_len == 0) - cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]); + cmd->cmd_len = scsi_command_size(cmd->cmnd); } void scsi_device_unbusy(struct scsi_device *sdev) @@ -1130,13 +1131,16 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) } cmd->cmd_len = req->cmd_len; + if (!cmd->cmd_len) + cmd->cmd_len = scsi_command_size(cmd->cmnd); + if (!req->data_len) cmd->sc_data_direction = DMA_NONE; else if (rq_data_dir(req) == WRITE) cmd->sc_data_direction = DMA_TO_DEVICE; else cmd->sc_data_direction = DMA_FROM_DEVICE; - + cmd->transfersize = req->data_len; cmd->allowed = req->retries; cmd->timeout_per_command = req->timeout; diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 1f74bcd..32742c4 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -30,13 +30,6 @@ #endif /* - * SCSI command lengths - */ - -extern const unsigned char scsi_command_size[8]; -#define COMMAND_SIZE(opcode) scsi_command_size[((opcode) >> 5) & 7] - -/* * Special value for scanning to specify scanning or rescanning of all * possible channels, (target) ids, or luns on a given shost. */ @@ -109,6 +102,7 @@ extern const unsigned char scsi_command_size[8]; #define MODE_SENSE_10 0x5a #define PERSISTENT_RESERVE_IN 0x5e #define PERSISTENT_RESERVE_OUT 0x5f +#define VARIABLE_LENGTH_CMD 0x7f #define REPORT_LUNS 0xa0 #define MAINTENANCE_IN 0xa3 #define MOVE_MEDIUM 0xa5 @@ -136,6 +130,38 @@ extern const unsigned char scsi_command_size[8]; #define ATA_12 0xa1 /* 12-byte pass-thru */ /* + * SCSI command lengths + */ + +#define SCSI_MAX_VARLEN_CDB_SIZE 260 + +/* defined in T10 SCSI Primary Commands-2 (SPC2) */ +struct scsi_varlen_cdb_hdr { + u8 opcode; /* opcode always == VARIABLE_LENGTH_CMD */ + u8 control; + u8 misc[5]; + u8 additional_cdb_length; /* total cdb length - 8 */ + __be16 service_action; + /* service specific data follows */ +}; + +static inline unsigned +scsi_varlen_cdb_length(const void *hdr) +{ + return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; +} + +extern const unsigned char scsi_command_size_tbl[8]; +#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] + +static inline unsigned +scsi_command_size(const unsigned char *cmnd) +{ + return (cmnd[0] == VARIABLE_LENGTH_CMD) ? + scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]); +} + +/* * SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft * T10/1561-D Revision 4 Draft dated 7th November 2002. */ diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index dea73e5..f4dacb1 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -78,7 +78,7 @@ struct scsi_cmnd { int allowed; int timeout_per_command; - unsigned char cmd_len; + unsigned short cmd_len; enum dma_data_direction sc_data_direction; /* These elements define the operation we are about to perform */ diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index 4913286..31f1bfd 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -573,13 +573,11 @@ struct Scsi_Host { /* * The maximum length of SCSI commands that this host can accept. * Probably 12 for most host adapters, but could be 16 for others. + * or 260 if the driver supports variable length cdbs. * For drivers that don't set this field, a value of 12 is - * assumed. I am leaving this as a number rather than a bit - * because you never know what subsequent SCSI standards might do - * (i.e. could there be a 20 byte or a 24-byte command a few years - * down the road?). + * assumed. */ - unsigned char max_cmd_len; + unsigned short max_cmd_len; int this_id; int can_queue; -- 1.5.3.3 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs 2008-04-15 12:34 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh @ 2008-04-16 2:09 ` FUJITA Tomonori 2008-04-16 6:40 ` Boaz Harrosh 0 siblings, 1 reply; 12+ messages in thread From: FUJITA Tomonori @ 2008-04-16 2:09 UTC (permalink / raw) To: bharrosh Cc: fujita.tomonori, James.Bottomley, linux-scsi, jens.axboe, bzolnier, agk, Geert.Uytterhoeven On Tue, 15 Apr 2008 15:34:47 +0300 Boaz Harrosh <bharrosh@panasas.com> wrote: > > Add support for variable-length, extended, and vendor specific > CDBs to scsi-ml. It is now possible for initiators and ULD's > to issue these types of commands. LLDs need not change much. > All they need is to raise the .max_cmd_len to the longest command > they support (see iscsi patch). > > - clean-up some code paths that did not expect commands to be > larger than 16, and change cmd_len members' type to short as > char is not enough. > > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> > Signed-off-by: Benny Halevy <bhalevy@panasas.com> > --- > block/scsi_ioctl.c | 5 ++--- > drivers/scsi/constants.c | 10 +++------- > drivers/scsi/scsi.c | 22 +++++++++++----------- > drivers/scsi/scsi_lib.c | 8 ++++++-- > include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- > include/scsi/scsi_cmnd.h | 2 +- > include/scsi/scsi_host.h | 8 +++----- > 7 files changed, 59 insertions(+), 36 deletions(-) > > diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > index a2c3a93..aaf07e4 100644 > --- a/block/scsi_ioctl.c > +++ b/block/scsi_ioctl.c > @@ -33,13 +33,12 @@ > #include <scsi/scsi_cmnd.h> > > /* Command group 3 is reserved and should never be used. */ > -const unsigned char scsi_command_size[8] = > +const unsigned char scsi_command_size_tbl[8] = > { > 6, 10, 10, 12, > 16, 12, 10, 10 > }; > - > -EXPORT_SYMBOL(scsi_command_size); > +EXPORT_SYMBOL(scsi_command_size_tbl); > > #include <scsi/sg.h> > > diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c > index 403a7f2..9785d73 100644 > --- a/drivers/scsi/constants.c > +++ b/drivers/scsi/constants.c > @@ -28,7 +28,6 @@ > #define SERVICE_ACTION_OUT_12 0xa9 > #define SERVICE_ACTION_IN_16 0x9e > #define SERVICE_ACTION_OUT_16 0x9f > -#define VARIABLE_LENGTH_CMD 0x7f > > > > @@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) > cdb0 = cdbp[0]; > switch(cdb0) { > case VARIABLE_LENGTH_CMD: > - len = cdbp[7] + 8; > + len = scsi_varlen_cdb_length(cdbp); > if (len < 10) { > printk("short variable length command, " > "len=%d ext_len=%d", len, cdb_len); > @@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) > cdb0 = cdbp[0]; > switch(cdb0) { > case VARIABLE_LENGTH_CMD: > - len = cdbp[7] + 8; > + len = scsi_varlen_cdb_length(cdbp); > if (len < 10) { > printk("short opcode=0x%x command, len=%d " > "ext_len=%d", cdb0, len, cdb_len); > @@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb) > int k, len; > > print_opcode_name(cdb, 0); > - if (VARIABLE_LENGTH_CMD == cdb[0]) > - len = cdb[7] + 8; > - else > - len = COMMAND_SIZE(cdb[0]); > + len = scsi_command_size(cdb); > /* print out all bytes in cdb */ > for (k = 0; k < len; ++k) > printk(" %02x", cdb[k]); > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index f6980bd..3dabecb 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd); > #define MIN_RESET_PERIOD (15*HZ) > > /* > - * Macro to determine the size of SCSI command. This macro takes vendor > - * unique commands into account. SCSI commands in groups 6 and 7 are > - * vendor unique and we will depend upon the command length being > - * supplied correctly in cmd_len. > - */ > -#define CDB_SIZE(cmd) (((((cmd)->cmnd[0] >> 5) & 7) < 6) ? \ > - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) > - > -/* > * Note - the initial logging level can be set here to log events at boot time. > * After the system is up, you may enable logging via the /proc interface. > */ > @@ -620,6 +611,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) > unsigned long flags = 0; > unsigned long timeout; > int rtn = 0; > + unsigned cmd_len; > > /* check if the device is still usable */ > if (unlikely(cmd->device->sdev_state == SDEV_DEL)) { > @@ -701,9 +693,17 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) > * Before we queue this command, check if the command > * length exceeds what the host adapter can handle. > */ > - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { > + cmd_len = cmd->cmd_len; > + if (!cmd_len) { > + BUG_ON(cmd->cmnd[0] == VARIABLE_LENGTH_CMD); > + cmd_len = COMMAND_SIZE((cmd)->cmnd[0]); > + } Hmm, how can cmd->cmd_len be zero here? > + if (cmd_len > cmd->device->host->max_cmd_len) { > SCSI_LOG_MLQUEUE(3, > - printk("queuecommand : command too long.\n")); > + printk("queuecommand : command too long. " > + "cdb_size=%d host->max_cmd_len=%d\n", > + cmd->cmd_len, cmd->device->host->max_cmd_len)); > cmd->result = (DID_ABORT << 16); Why can't we just do: if (scsi_command_size(cmd->cmnd) > cmd->device->host->max_cmd_len) { > scsi_done(cmd); > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > index 325270b..e621505 100644 > --- a/drivers/scsi/scsi_lib.c > +++ b/drivers/scsi/scsi_lib.c > @@ -195,6 +195,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > > req->cmd_len = COMMAND_SIZE(cmd[0]); > memcpy(req->cmd, cmd, req->cmd_len); > + Please don't put a new line at a place unrelated with this patch. > req->sense = sense; > req->sense_len = 0; > req->retries = retries; > @@ -445,7 +446,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) > scsi_set_resid(cmd, 0); > memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); > if (cmd->cmd_len == 0) > - cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]); > + cmd->cmd_len = scsi_command_size(cmd->cmnd); > } > > void scsi_device_unbusy(struct scsi_device *sdev) > @@ -1130,13 +1131,16 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > } > > cmd->cmd_len = req->cmd_len; > + if (!cmd->cmd_len) > + cmd->cmd_len = scsi_command_size(cmd->cmnd); > + how can cmd->cmd_len be zero here? SG_IO path sets up req->cmd_len properly for PC commands. > if (!req->data_len) > cmd->sc_data_direction = DMA_NONE; > else if (rq_data_dir(req) == WRITE) > cmd->sc_data_direction = DMA_TO_DEVICE; > else > cmd->sc_data_direction = DMA_FROM_DEVICE; > - > + > cmd->transfersize = req->data_len; > cmd->allowed = req->retries; > cmd->timeout_per_command = req->timeout; > diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h > index 1f74bcd..32742c4 100644 > --- a/include/scsi/scsi.h > +++ b/include/scsi/scsi.h > @@ -30,13 +30,6 @@ > #endif > > /* > - * SCSI command lengths > - */ > - > -extern const unsigned char scsi_command_size[8]; > -#define COMMAND_SIZE(opcode) scsi_command_size[((opcode) >> 5) & 7] > - > -/* > * Special value for scanning to specify scanning or rescanning of all > * possible channels, (target) ids, or luns on a given shost. > */ > @@ -109,6 +102,7 @@ extern const unsigned char scsi_command_size[8]; > #define MODE_SENSE_10 0x5a > #define PERSISTENT_RESERVE_IN 0x5e > #define PERSISTENT_RESERVE_OUT 0x5f > +#define VARIABLE_LENGTH_CMD 0x7f > #define REPORT_LUNS 0xa0 > #define MAINTENANCE_IN 0xa3 > #define MOVE_MEDIUM 0xa5 > @@ -136,6 +130,38 @@ extern const unsigned char scsi_command_size[8]; > #define ATA_12 0xa1 /* 12-byte pass-thru */ > > /* > + * SCSI command lengths > + */ > + > +#define SCSI_MAX_VARLEN_CDB_SIZE 260 > + > +/* defined in T10 SCSI Primary Commands-2 (SPC2) */ > +struct scsi_varlen_cdb_hdr { > + u8 opcode; /* opcode always == VARIABLE_LENGTH_CMD */ > + u8 control; > + u8 misc[5]; > + u8 additional_cdb_length; /* total cdb length - 8 */ > + __be16 service_action; > + /* service specific data follows */ > +}; > + > +static inline unsigned > +scsi_varlen_cdb_length(const void *hdr) > +{ > + return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; > +} > + > +extern const unsigned char scsi_command_size_tbl[8]; > +#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] > + > +static inline unsigned > +scsi_command_size(const unsigned char *cmnd) > +{ > + return (cmnd[0] == VARIABLE_LENGTH_CMD) ? > + scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]); > +} > + > +/* > * SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft > * T10/1561-D Revision 4 Draft dated 7th November 2002. > */ > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index dea73e5..f4dacb1 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -78,7 +78,7 @@ struct scsi_cmnd { > int allowed; > int timeout_per_command; > > - unsigned char cmd_len; > + unsigned short cmd_len; > enum dma_data_direction sc_data_direction; > > /* These elements define the operation we are about to perform */ > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > index 4913286..31f1bfd 100644 > --- a/include/scsi/scsi_host.h > +++ b/include/scsi/scsi_host.h > @@ -573,13 +573,11 @@ struct Scsi_Host { > /* > * The maximum length of SCSI commands that this host can accept. > * Probably 12 for most host adapters, but could be 16 for others. > + * or 260 if the driver supports variable length cdbs. > * For drivers that don't set this field, a value of 12 is > - * assumed. I am leaving this as a number rather than a bit > - * because you never know what subsequent SCSI standards might do > - * (i.e. could there be a 20 byte or a 24-byte command a few years > - * down the road?). > + * assumed. > */ > - unsigned char max_cmd_len; > + unsigned short max_cmd_len; > > int this_id; > int can_queue; > -- > 1.5.3.3 > > > -- > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs 2008-04-16 2:09 ` FUJITA Tomonori @ 2008-04-16 6:40 ` Boaz Harrosh 2008-04-17 4:01 ` FUJITA Tomonori 0 siblings, 1 reply; 12+ messages in thread From: Boaz Harrosh @ 2008-04-16 6:40 UTC (permalink / raw) To: FUJITA Tomonori Cc: James.Bottomley, linux-scsi, jens.axboe, bzolnier, agk, Geert.Uytterhoeven On Wed, Apr 16 2008 at 5:09 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > On Tue, 15 Apr 2008 15:34:47 +0300 > Boaz Harrosh <bharrosh@panasas.com> wrote: > >> Add support for variable-length, extended, and vendor specific >> CDBs to scsi-ml. It is now possible for initiators and ULD's >> to issue these types of commands. LLDs need not change much. >> All they need is to raise the .max_cmd_len to the longest command >> they support (see iscsi patch). >> >> - clean-up some code paths that did not expect commands to be >> larger than 16, and change cmd_len members' type to short as >> char is not enough. >> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> >> --- >> block/scsi_ioctl.c | 5 ++--- >> drivers/scsi/constants.c | 10 +++------- >> drivers/scsi/scsi.c | 22 +++++++++++----------- >> drivers/scsi/scsi_lib.c | 8 ++++++-- >> include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- >> include/scsi/scsi_cmnd.h | 2 +- >> include/scsi/scsi_host.h | 8 +++----- >> 7 files changed, 59 insertions(+), 36 deletions(-) >> >> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c >> index a2c3a93..aaf07e4 100644 >> --- a/block/scsi_ioctl.c >> +++ b/block/scsi_ioctl.c >> @@ -33,13 +33,12 @@ >> #include <scsi/scsi_cmnd.h> >> >> /* Command group 3 is reserved and should never be used. */ >> -const unsigned char scsi_command_size[8] = >> +const unsigned char scsi_command_size_tbl[8] = >> { >> 6, 10, 10, 12, >> 16, 12, 10, 10 >> }; >> - >> -EXPORT_SYMBOL(scsi_command_size); >> +EXPORT_SYMBOL(scsi_command_size_tbl); >> >> #include <scsi/sg.h> >> >> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c >> index 403a7f2..9785d73 100644 >> --- a/drivers/scsi/constants.c >> +++ b/drivers/scsi/constants.c >> @@ -28,7 +28,6 @@ >> #define SERVICE_ACTION_OUT_12 0xa9 >> #define SERVICE_ACTION_IN_16 0x9e >> #define SERVICE_ACTION_OUT_16 0x9f >> -#define VARIABLE_LENGTH_CMD 0x7f >> >> >> >> @@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) >> cdb0 = cdbp[0]; >> switch(cdb0) { >> case VARIABLE_LENGTH_CMD: >> - len = cdbp[7] + 8; >> + len = scsi_varlen_cdb_length(cdbp); >> if (len < 10) { >> printk("short variable length command, " >> "len=%d ext_len=%d", len, cdb_len); >> @@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) >> cdb0 = cdbp[0]; >> switch(cdb0) { >> case VARIABLE_LENGTH_CMD: >> - len = cdbp[7] + 8; >> + len = scsi_varlen_cdb_length(cdbp); >> if (len < 10) { >> printk("short opcode=0x%x command, len=%d " >> "ext_len=%d", cdb0, len, cdb_len); >> @@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb) >> int k, len; >> >> print_opcode_name(cdb, 0); >> - if (VARIABLE_LENGTH_CMD == cdb[0]) >> - len = cdb[7] + 8; >> - else >> - len = COMMAND_SIZE(cdb[0]); >> + len = scsi_command_size(cdb); >> /* print out all bytes in cdb */ >> for (k = 0; k < len; ++k) >> printk(" %02x", cdb[k]); >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c >> index f6980bd..3dabecb 100644 >> --- a/drivers/scsi/scsi.c >> +++ b/drivers/scsi/scsi.c >> @@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd); >> #define MIN_RESET_PERIOD (15*HZ) >> >> /* >> - * Macro to determine the size of SCSI command. This macro takes vendor >> - * unique commands into account. SCSI commands in groups 6 and 7 are >> - * vendor unique and we will depend upon the command length being >> - * supplied correctly in cmd_len. >> - */ >> -#define CDB_SIZE(cmd) (((((cmd)->cmnd[0] >> 5) & 7) < 6) ? \ >> - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) >> - >> -/* >> * Note - the initial logging level can be set here to log events at boot time. >> * After the system is up, you may enable logging via the /proc interface. >> */ >> @@ -620,6 +611,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) >> unsigned long flags = 0; >> unsigned long timeout; >> int rtn = 0; >> + unsigned cmd_len; >> >> /* check if the device is still usable */ >> if (unlikely(cmd->device->sdev_state == SDEV_DEL)) { >> @@ -701,9 +693,17 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) >> * Before we queue this command, check if the command >> * length exceeds what the host adapter can handle. >> */ >> - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { >> + cmd_len = cmd->cmd_len; >> + if (!cmd_len) { >> + BUG_ON(cmd->cmnd[0] == VARIABLE_LENGTH_CMD); >> + cmd_len = COMMAND_SIZE((cmd)->cmnd[0]); >> + } > > Hmm, how can cmd->cmd_len be zero here? > With FS_PC commands this is zero here. Because ULD prepare the command only after prep_fn. > >> + if (cmd_len > cmd->device->host->max_cmd_len) { >> SCSI_LOG_MLQUEUE(3, >> - printk("queuecommand : command too long.\n")); >> + printk("queuecommand : command too long. " >> + "cdb_size=%d host->max_cmd_len=%d\n", >> + cmd->cmd_len, cmd->device->host->max_cmd_len)); >> cmd->result = (DID_ABORT << 16); > > Why can't we just do: > > if (scsi_command_size(cmd->cmnd) > cmd->device->host->max_cmd_len) { > Because we can't. If ULD gave us length then we only use that, and we do not assume we know anything about the command. scsi_command_size() only knows about scsi commands and not all scsi commands at that, not all versions and dialects of scsi. The commit log of this patch sayes: "support for variable-length, extended, and vendor specific" This code here enables that. > >> scsi_done(cmd); >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c >> index 325270b..e621505 100644 >> --- a/drivers/scsi/scsi_lib.c >> +++ b/drivers/scsi/scsi_lib.c >> @@ -195,6 +195,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, >> >> req->cmd_len = COMMAND_SIZE(cmd[0]); >> memcpy(req->cmd, cmd, req->cmd_len); >> + > > Please don't put a new line at a place unrelated with this patch. > Ooops left over from the rebasing sorry. > >> req->sense = sense; >> req->sense_len = 0; >> req->retries = retries; >> @@ -445,7 +446,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) >> scsi_set_resid(cmd, 0); >> memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); >> if (cmd->cmd_len == 0) >> - cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]); >> + cmd->cmd_len = scsi_command_size(cmd->cmnd); >> } >> >> void scsi_device_unbusy(struct scsi_device *sdev) >> @@ -1130,13 +1131,16 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) >> } >> >> cmd->cmd_len = req->cmd_len; >> + if (!cmd->cmd_len) >> + cmd->cmd_len = scsi_command_size(cmd->cmnd); >> + > > how can cmd->cmd_len be zero here? SG_IO path sets up req->cmd_len > properly for PC commands. > It is either that or BUG_ON(), I would say that a simple thing like that I would let the Initiator get lazy if it wants to. > >> if (!req->data_len) >> cmd->sc_data_direction = DMA_NONE; >> else if (rq_data_dir(req) == WRITE) >> cmd->sc_data_direction = DMA_TO_DEVICE; >> else >> cmd->sc_data_direction = DMA_FROM_DEVICE; >> - >> + >> cmd->transfersize = req->data_len; >> cmd->allowed = req->retries; >> cmd->timeout_per_command = req->timeout; >> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h >> index 1f74bcd..32742c4 100644 >> --- a/include/scsi/scsi.h >> +++ b/include/scsi/scsi.h >> @@ -30,13 +30,6 @@ >> #endif >> >> /* >> - * SCSI command lengths >> - */ >> - >> -extern const unsigned char scsi_command_size[8]; >> -#define COMMAND_SIZE(opcode) scsi_command_size[((opcode) >> 5) & 7] >> - >> -/* >> * Special value for scanning to specify scanning or rescanning of all >> * possible channels, (target) ids, or luns on a given shost. >> */ >> @@ -109,6 +102,7 @@ extern const unsigned char scsi_command_size[8]; >> #define MODE_SENSE_10 0x5a >> #define PERSISTENT_RESERVE_IN 0x5e >> #define PERSISTENT_RESERVE_OUT 0x5f >> +#define VARIABLE_LENGTH_CMD 0x7f >> #define REPORT_LUNS 0xa0 >> #define MAINTENANCE_IN 0xa3 >> #define MOVE_MEDIUM 0xa5 >> @@ -136,6 +130,38 @@ extern const unsigned char scsi_command_size[8]; >> #define ATA_12 0xa1 /* 12-byte pass-thru */ >> >> /* >> + * SCSI command lengths >> + */ >> + >> +#define SCSI_MAX_VARLEN_CDB_SIZE 260 >> + >> +/* defined in T10 SCSI Primary Commands-2 (SPC2) */ >> +struct scsi_varlen_cdb_hdr { >> + u8 opcode; /* opcode always == VARIABLE_LENGTH_CMD */ >> + u8 control; >> + u8 misc[5]; >> + u8 additional_cdb_length; /* total cdb length - 8 */ >> + __be16 service_action; >> + /* service specific data follows */ >> +}; >> + >> +static inline unsigned >> +scsi_varlen_cdb_length(const void *hdr) >> +{ >> + return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; >> +} >> + >> +extern const unsigned char scsi_command_size_tbl[8]; >> +#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] >> + >> +static inline unsigned >> +scsi_command_size(const unsigned char *cmnd) >> +{ >> + return (cmnd[0] == VARIABLE_LENGTH_CMD) ? >> + scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]); >> +} >> + >> +/* >> * SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft >> * T10/1561-D Revision 4 Draft dated 7th November 2002. >> */ >> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h >> index dea73e5..f4dacb1 100644 >> --- a/include/scsi/scsi_cmnd.h >> +++ b/include/scsi/scsi_cmnd.h >> @@ -78,7 +78,7 @@ struct scsi_cmnd { >> int allowed; >> int timeout_per_command; >> >> - unsigned char cmd_len; >> + unsigned short cmd_len; >> enum dma_data_direction sc_data_direction; >> >> /* These elements define the operation we are about to perform */ >> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h >> index 4913286..31f1bfd 100644 >> --- a/include/scsi/scsi_host.h >> +++ b/include/scsi/scsi_host.h >> @@ -573,13 +573,11 @@ struct Scsi_Host { >> /* >> * The maximum length of SCSI commands that this host can accept. >> * Probably 12 for most host adapters, but could be 16 for others. >> + * or 260 if the driver supports variable length cdbs. >> * For drivers that don't set this field, a value of 12 is >> - * assumed. I am leaving this as a number rather than a bit >> - * because you never know what subsequent SCSI standards might do >> - * (i.e. could there be a 20 byte or a 24-byte command a few years >> - * down the road?). >> + * assumed. >> */ >> - unsigned char max_cmd_len; >> + unsigned short max_cmd_len; >> >> int this_id; >> int can_queue; >> -- >> 1.5.3.3 >> >> >> -- As a reply to this mail I'll send a patch without the extra space. Boaz ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs 2008-04-16 6:40 ` Boaz Harrosh @ 2008-04-17 4:01 ` FUJITA Tomonori 0 siblings, 0 replies; 12+ messages in thread From: FUJITA Tomonori @ 2008-04-17 4:01 UTC (permalink / raw) To: bharrosh Cc: fujita.tomonori, James.Bottomley, linux-scsi, jens.axboe, bzolnier, agk, Geert.Uytterhoeven On Wed, 16 Apr 2008 09:40:17 +0300 Boaz Harrosh <bharrosh@panasas.com> wrote: > On Wed, Apr 16 2008 at 5:09 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote: > > On Tue, 15 Apr 2008 15:34:47 +0300 > > Boaz Harrosh <bharrosh@panasas.com> wrote: > > > >> Add support for variable-length, extended, and vendor specific > >> CDBs to scsi-ml. It is now possible for initiators and ULD's > >> to issue these types of commands. LLDs need not change much. > >> All they need is to raise the .max_cmd_len to the longest command > >> they support (see iscsi patch). > >> > >> - clean-up some code paths that did not expect commands to be > >> larger than 16, and change cmd_len members' type to short as > >> char is not enough. > >> > >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> > >> Signed-off-by: Benny Halevy <bhalevy@panasas.com> > >> --- > >> block/scsi_ioctl.c | 5 ++--- > >> drivers/scsi/constants.c | 10 +++------- > >> drivers/scsi/scsi.c | 22 +++++++++++----------- > >> drivers/scsi/scsi_lib.c | 8 ++++++-- > >> include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- > >> include/scsi/scsi_cmnd.h | 2 +- > >> include/scsi/scsi_host.h | 8 +++----- > >> 7 files changed, 59 insertions(+), 36 deletions(-) > >> > >> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c > >> index a2c3a93..aaf07e4 100644 > >> --- a/block/scsi_ioctl.c > >> +++ b/block/scsi_ioctl.c > >> @@ -33,13 +33,12 @@ > >> #include <scsi/scsi_cmnd.h> > >> > >> /* Command group 3 is reserved and should never be used. */ > >> -const unsigned char scsi_command_size[8] = > >> +const unsigned char scsi_command_size_tbl[8] = > >> { > >> 6, 10, 10, 12, > >> 16, 12, 10, 10 > >> }; > >> - > >> -EXPORT_SYMBOL(scsi_command_size); > >> +EXPORT_SYMBOL(scsi_command_size_tbl); > >> > >> #include <scsi/sg.h> > >> > >> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c > >> index 403a7f2..9785d73 100644 > >> --- a/drivers/scsi/constants.c > >> +++ b/drivers/scsi/constants.c > >> @@ -28,7 +28,6 @@ > >> #define SERVICE_ACTION_OUT_12 0xa9 > >> #define SERVICE_ACTION_IN_16 0x9e > >> #define SERVICE_ACTION_OUT_16 0x9f > >> -#define VARIABLE_LENGTH_CMD 0x7f > >> > >> > >> > >> @@ -210,7 +209,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) > >> cdb0 = cdbp[0]; > >> switch(cdb0) { > >> case VARIABLE_LENGTH_CMD: > >> - len = cdbp[7] + 8; > >> + len = scsi_varlen_cdb_length(cdbp); > >> if (len < 10) { > >> printk("short variable length command, " > >> "len=%d ext_len=%d", len, cdb_len); > >> @@ -300,7 +299,7 @@ static void print_opcode_name(unsigned char * cdbp, int cdb_len) > >> cdb0 = cdbp[0]; > >> switch(cdb0) { > >> case VARIABLE_LENGTH_CMD: > >> - len = cdbp[7] + 8; > >> + len = scsi_varlen_cdb_length(cdbp); > >> if (len < 10) { > >> printk("short opcode=0x%x command, len=%d " > >> "ext_len=%d", cdb0, len, cdb_len); > >> @@ -335,10 +334,7 @@ void __scsi_print_command(unsigned char *cdb) > >> int k, len; > >> > >> print_opcode_name(cdb, 0); > >> - if (VARIABLE_LENGTH_CMD == cdb[0]) > >> - len = cdb[7] + 8; > >> - else > >> - len = COMMAND_SIZE(cdb[0]); > >> + len = scsi_command_size(cdb); > >> /* print out all bytes in cdb */ > >> for (k = 0; k < len; ++k) > >> printk(" %02x", cdb[k]); > >> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > >> index f6980bd..3dabecb 100644 > >> --- a/drivers/scsi/scsi.c > >> +++ b/drivers/scsi/scsi.c > >> @@ -79,15 +79,6 @@ static void scsi_done(struct scsi_cmnd *cmd); > >> #define MIN_RESET_PERIOD (15*HZ) > >> > >> /* > >> - * Macro to determine the size of SCSI command. This macro takes vendor > >> - * unique commands into account. SCSI commands in groups 6 and 7 are > >> - * vendor unique and we will depend upon the command length being > >> - * supplied correctly in cmd_len. > >> - */ > >> -#define CDB_SIZE(cmd) (((((cmd)->cmnd[0] >> 5) & 7) < 6) ? \ > >> - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) > >> - > >> -/* > >> * Note - the initial logging level can be set here to log events at boot time. > >> * After the system is up, you may enable logging via the /proc interface. > >> */ > >> @@ -620,6 +611,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) > >> unsigned long flags = 0; > >> unsigned long timeout; > >> int rtn = 0; > >> + unsigned cmd_len; > >> > >> /* check if the device is still usable */ > >> if (unlikely(cmd->device->sdev_state == SDEV_DEL)) { > >> @@ -701,9 +693,17 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd) > >> * Before we queue this command, check if the command > >> * length exceeds what the host adapter can handle. > >> */ > >> - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { > >> + cmd_len = cmd->cmd_len; > >> + if (!cmd_len) { > >> + BUG_ON(cmd->cmnd[0] == VARIABLE_LENGTH_CMD); > >> + cmd_len = COMMAND_SIZE((cmd)->cmnd[0]); > >> + } > > > > Hmm, how can cmd->cmd_len be zero here? > > > > With FS_PC commands this is zero here. Because ULD prepare the command > only after prep_fn. I don't think so. See what scsi_init_cmd_errh does. It's called before scsi_dispatch_cmd. > > > >> + if (cmd_len > cmd->device->host->max_cmd_len) { > >> SCSI_LOG_MLQUEUE(3, > >> - printk("queuecommand : command too long.\n")); > >> + printk("queuecommand : command too long. " > >> + "cdb_size=%d host->max_cmd_len=%d\n", > >> + cmd->cmd_len, cmd->device->host->max_cmd_len)); > >> cmd->result = (DID_ABORT << 16); > > > > Why can't we just do: > > > > if (scsi_command_size(cmd->cmnd) > cmd->device->host->max_cmd_len) { > > > > Because we can't. If ULD gave us length then we only use that, and we do > not assume we know anything about the command. scsi_command_size() only > knows about scsi commands and not all scsi commands at that, not all > versions and dialects of scsi. The commit log of this patch sayes: > "support for variable-length, extended, and vendor specific" > This code here enables that. Again, look at scsi_init_cmd_errh. > > > >> scsi_done(cmd); > >> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c > >> index 325270b..e621505 100644 > >> --- a/drivers/scsi/scsi_lib.c > >> +++ b/drivers/scsi/scsi_lib.c > >> @@ -195,6 +195,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, > >> > >> req->cmd_len = COMMAND_SIZE(cmd[0]); > >> memcpy(req->cmd, cmd, req->cmd_len); > >> + > > > > Please don't put a new line at a place unrelated with this patch. > > > Ooops left over from the rebasing sorry. > > > >> req->sense = sense; > >> req->sense_len = 0; > >> req->retries = retries; > >> @@ -445,7 +446,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) > >> scsi_set_resid(cmd, 0); > >> memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE); > >> if (cmd->cmd_len == 0) > >> - cmd->cmd_len = COMMAND_SIZE(cmd->cmnd[0]); > >> + cmd->cmd_len = scsi_command_size(cmd->cmnd); > >> } > >> > >> void scsi_device_unbusy(struct scsi_device *sdev) > >> @@ -1130,13 +1131,16 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) > >> } > >> > >> cmd->cmd_len = req->cmd_len; > >> + if (!cmd->cmd_len) > >> + cmd->cmd_len = scsi_command_size(cmd->cmnd); > >> + > > > > how can cmd->cmd_len be zero here? SG_IO path sets up req->cmd_len > > properly for PC commands. > > > > It is either that or BUG_ON(), I would say that a simple thing like that > I would let the Initiator get lazy if it wants to. Please do only what the patch describes. This patch is expected to add large command support. The above code is not related with that at all. I don't think we need the above code. But if you still think we need the above code, send a separate patch. > > > >> if (!req->data_len) > >> cmd->sc_data_direction = DMA_NONE; > >> else if (rq_data_dir(req) == WRITE) > >> cmd->sc_data_direction = DMA_TO_DEVICE; > >> else > >> cmd->sc_data_direction = DMA_FROM_DEVICE; > >> - > >> + > >> cmd->transfersize = req->data_len; > >> cmd->allowed = req->retries; > >> cmd->timeout_per_command = req->timeout; > >> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h > >> index 1f74bcd..32742c4 100644 > >> --- a/include/scsi/scsi.h > >> +++ b/include/scsi/scsi.h > >> @@ -30,13 +30,6 @@ > >> #endif > >> > >> /* > >> - * SCSI command lengths > >> - */ > >> - > >> -extern const unsigned char scsi_command_size[8]; > >> -#define COMMAND_SIZE(opcode) scsi_command_size[((opcode) >> 5) & 7] > >> - > >> -/* > >> * Special value for scanning to specify scanning or rescanning of all > >> * possible channels, (target) ids, or luns on a given shost. > >> */ > >> @@ -109,6 +102,7 @@ extern const unsigned char scsi_command_size[8]; > >> #define MODE_SENSE_10 0x5a > >> #define PERSISTENT_RESERVE_IN 0x5e > >> #define PERSISTENT_RESERVE_OUT 0x5f > >> +#define VARIABLE_LENGTH_CMD 0x7f > >> #define REPORT_LUNS 0xa0 > >> #define MAINTENANCE_IN 0xa3 > >> #define MOVE_MEDIUM 0xa5 > >> @@ -136,6 +130,38 @@ extern const unsigned char scsi_command_size[8]; > >> #define ATA_12 0xa1 /* 12-byte pass-thru */ > >> > >> /* > >> + * SCSI command lengths > >> + */ > >> + > >> +#define SCSI_MAX_VARLEN_CDB_SIZE 260 > >> + > >> +/* defined in T10 SCSI Primary Commands-2 (SPC2) */ > >> +struct scsi_varlen_cdb_hdr { > >> + u8 opcode; /* opcode always == VARIABLE_LENGTH_CMD */ > >> + u8 control; > >> + u8 misc[5]; > >> + u8 additional_cdb_length; /* total cdb length - 8 */ > >> + __be16 service_action; > >> + /* service specific data follows */ > >> +}; > >> + > >> +static inline unsigned > >> +scsi_varlen_cdb_length(const void *hdr) > >> +{ > >> + return ((struct scsi_varlen_cdb_hdr *)hdr)->additional_cdb_length + 8; > >> +} > >> + > >> +extern const unsigned char scsi_command_size_tbl[8]; > >> +#define COMMAND_SIZE(opcode) scsi_command_size_tbl[((opcode) >> 5) & 7] > >> + > >> +static inline unsigned > >> +scsi_command_size(const unsigned char *cmnd) > >> +{ > >> + return (cmnd[0] == VARIABLE_LENGTH_CMD) ? > >> + scsi_varlen_cdb_length(cmnd) : COMMAND_SIZE(cmnd[0]); > >> +} > >> + > >> +/* > >> * SCSI Architecture Model (SAM) Status codes. Taken from SAM-3 draft > >> * T10/1561-D Revision 4 Draft dated 7th November 2002. > >> */ > >> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > >> index dea73e5..f4dacb1 100644 > >> --- a/include/scsi/scsi_cmnd.h > >> +++ b/include/scsi/scsi_cmnd.h > >> @@ -78,7 +78,7 @@ struct scsi_cmnd { > >> int allowed; > >> int timeout_per_command; > >> > >> - unsigned char cmd_len; > >> + unsigned short cmd_len; > >> enum dma_data_direction sc_data_direction; > >> > >> /* These elements define the operation we are about to perform */ > >> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h > >> index 4913286..31f1bfd 100644 > >> --- a/include/scsi/scsi_host.h > >> +++ b/include/scsi/scsi_host.h > >> @@ -573,13 +573,11 @@ struct Scsi_Host { > >> /* > >> * The maximum length of SCSI commands that this host can accept. > >> * Probably 12 for most host adapters, but could be 16 for others. > >> + * or 260 if the driver supports variable length cdbs. > >> * For drivers that don't set this field, a value of 12 is > >> - * assumed. I am leaving this as a number rather than a bit > >> - * because you never know what subsequent SCSI standards might do > >> - * (i.e. could there be a 20 byte or a 24-byte command a few years > >> - * down the road?). > >> + * assumed. > >> */ > >> - unsigned char max_cmd_len; > >> + unsigned short max_cmd_len; > >> > >> int this_id; > >> int can_queue; > >> -- > >> 1.5.3.3 > >> > >> > >> -- > > As a reply to this mail I'll send a patch without the extra space. > > Boaz > > -- > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2008-05-13 16:07 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-04-30 8:13 [PATCH 3/3] scsi support variable length commands Boaz Harrosh 2008-04-30 8:19 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh 2008-04-30 8:27 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 2008-04-30 8:30 ` [PATCH 3/3] iscsi_tcp: Enable any size command Boaz Harrosh 2008-05-12 15:47 ` James Bottomley 2008-05-13 8:52 ` Boaz Harrosh 2008-05-13 14:24 ` James Bottomley 2008-05-13 16:01 ` Boaz Harrosh -- strict thread matches above, loose matches on Subject: below -- 2008-04-13 16:17 [PATCH 2/3 ver2] block layer extended-cdb support FUJITA Tomonori 2008-04-14 10:50 ` [PATCH 0/4] add large command support to the block layer FUJITA Tomonori 2008-04-15 12:24 ` [PATCH 0/3] scsi: variable-length CDBs support Boaz Harrosh 2008-04-15 12:34 ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 2008-04-16 2:09 ` FUJITA Tomonori 2008-04-16 6:40 ` Boaz Harrosh 2008-04-17 4:01 ` FUJITA Tomonori
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).