* [RFC 0/4] varlen extended and vendor-specific cdbs
@ 2007-11-01 17:54 Boaz Harrosh
2007-11-01 18:00 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer Boaz Harrosh
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Boaz Harrosh @ 2007-11-01 17:54 UTC (permalink / raw)
To: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori,
linux-scsi
Cc: Pete Wyckoff, Benny Halevy
This is a proposal for adding support for variable-length, extended, and
vendor specific CDBs. It should now cover the entire range of the
SCSI standard.
This patchset extends my original submission (over a year old) in
that it starts with cleaning up scsi_cmnd, hence the first patch. The
other three are similar to the ones submitted before.
This effort is orthogonal to the bidi and scsi_data_buffer effort
and can be accepted now. The patchset, however, is presented in this RFC
on top of the scsi_data_buffer patches, as they sit in my tree. They
can easily be rebased to current scsi-misc. The iscsi patch is on top
of the iscsi branch of the iscsi-git-tree.
(Matthew, these patches will conflict with your scsi_cmnd cleanup patches
I promise to rebase them before submission)
A complete git-tree based on scsi-misc which includes a complete bidi
and varlen work can be fetched from:
- git://git.bhalevy.com/open-osd bidi branch (varlen branch is this work)
- http://www.bhalevy.com/git - has the gitweb GUI
[1/4] 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/4] block layer varlen-cdb
Unlike the scsi approach (see below), I did not want to unify
cmd[]/cmd_len and the *varlen_cdb and varlen_cdb_len
members of struct request. This is because unlike scsi, block
devices do not have a .max_cmd_len parameter to protect them
from unexpected large commands.
In the case varlen_cdb and varlen_cdb_len are used the cmd[]
buffer is ignored and the cmd_len will be set to zero.
[3/4] scsi: varlen extended and vendor-specific cdbs
Adds support for variable length, extended, and vendor-specific
cdbs at the scsi mid-layer.
[4/4] iscsi: extended cdb support
This is on top of the iscsi branch. In the URL above there
are the three missing patches for iscsi, that add support
for AHSs.
Boaz
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer 2007-11-01 17:54 [RFC 0/4] varlen extended and vendor-specific cdbs Boaz Harrosh @ 2007-11-01 18:00 ` Boaz Harrosh 2007-11-01 18:05 ` [PATCH 2/4] block layer varlen-cdb Boaz Harrosh ` (2 subsequent siblings) 3 siblings, 0 replies; 10+ messages in thread From: Boaz Harrosh @ 2007-11-01 18:00 UTC (permalink / raw) To: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi Cc: Pete Wyckoff, Benny Halevy - 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 (*)fixed-length here means commands that their size can be determined by their opcode and the CDB does not carry a length specifier, like 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/scsi/53c700.c | 6 +++--- drivers/scsi/ibmvscsi/ibmvscsi.c | 2 +- drivers/scsi/scsi_error.c | 14 ++++++++------ drivers/scsi/scsi_lib.c | 5 +++-- drivers/scsi/scsi_tgt_lib.c | 2 ++ drivers/usb/storage/isd200.c | 2 ++ include/scsi/scsi_cmnd.h | 10 +++++++--- include/scsi/scsi_eh.h | 6 ++++-- 10 files changed, 32 insertions(+), 19 deletions(-) diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c index 5596df6..151d02c 100644 --- a/drivers/firewire/fw-sbp2.c +++ b/drivers/firewire/fw-sbp2.c @@ -1212,7 +1212,7 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done) memset(orb->request.command_block, 0, sizeof(orb->request.command_block)); - 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 ffa3bf7..2d43c10 100644 --- a/drivers/s390/scsi/zfcp_dbf.c +++ b/drivers/s390/scsi/zfcp_dbf.c @@ -730,7 +730,7 @@ _zfcp_scsi_dbf_event_common(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/scsi/53c700.c b/drivers/scsi/53c700.c index 71ff3fb..85460f8 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 @@ -1003,7 +1003,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; @@ -1900,7 +1900,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/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c index 22d91ee..bf3c4a9 100644 --- a/drivers/scsi/ibmvscsi/ibmvscsi.c +++ b/drivers/scsi/ibmvscsi/ibmvscsi.c @@ -720,7 +720,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/scsi_error.c b/drivers/scsi/scsi_error.c index e7b87ea..a19da5b 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -593,7 +593,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 @@ -615,11 +615,13 @@ 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->result = scmd->result; + scmd->cmnd = ses->eh_cmnd; + memset(scmd->cmnd, 0, BLK_MAX_CDB); memset(&scmd->sdb, 0, sizeof(scmd->sdb)); if (sense_bytes) { @@ -630,14 +632,13 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses, scmd->sdb.sglist = &ses->sense_sgl; scmd->sc_data_direction = DMA_FROM_DEVICE; scmd->sdb.sg_count = 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]); } @@ -670,7 +671,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->result = ses->result; @@ -1689,7 +1690,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; + memset(scmd->cmnd, 0, BLK_MAX_CDB); 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 fe09ab2..0a940f4 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1149,6 +1149,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; } @@ -1186,8 +1188,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; @@ -1224,6 +1224,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); } EXPORT_SYMBOL(scsi_setup_fs_cmnd); diff --git a/drivers/scsi/scsi_tgt_lib.c b/drivers/scsi/scsi_tgt_lib.c index c2e776d..38e561f 100644 --- a/drivers/scsi/scsi_tgt_lib.c +++ b/drivers/scsi/scsi_tgt_lib.c @@ -108,6 +108,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/isd200.c b/drivers/usb/storage/isd200.c index 2d9a32b..50db42a 100644 --- a/drivers/usb/storage/isd200.c +++ b/drivers/usb/storage/isd200.c @@ -287,6 +287,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; }; @@ -445,6 +446,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 8b8759c..65f5627 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -6,12 +6,17 @@ #include <linux/types.h> #include <linux/timer.h> #include <linux/scatterlist.h> +#include <linux/blkdev.h> -struct request; struct scatterlist; struct Scsi_Host; struct scsi_device; +#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 { unsigned length; int resid; @@ -67,8 +72,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 2b226f6..b568467 100644 --- a/include/scsi/scsi_eh.h +++ b/include/scsi/scsi_eh.h @@ -68,12 +68,14 @@ extern int scsi_get_sense_info_fld(const u8 * sense_buffer, int sb_len, extern int scsi_reset_provider(struct scsi_device *, int); struct scsi_eh_save { + /* saved state */ 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; + /* new command support */ + unsigned char eh_cmnd[BLK_MAX_CDB]; struct scatterlist sense_sgl; }; -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] block layer varlen-cdb 2007-11-01 17:54 [RFC 0/4] varlen extended and vendor-specific cdbs Boaz Harrosh 2007-11-01 18:00 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer Boaz Harrosh @ 2007-11-01 18:05 ` Boaz Harrosh 2007-11-01 18:40 ` Matthew Wilcox 2007-11-01 18:07 ` [PATCH 3/4] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 2007-11-01 18:10 ` [PATCH 4/4] iscsi: extended cdb support Boaz Harrosh 3 siblings, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2007-11-01 18:05 UTC (permalink / raw) To: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi Cc: Pete Wyckoff, Benny Halevy - add varlen_cdb and varlen_cdb_len to hold a large user cdb if needed. They start as empty. Allocation of buffer must be done by user and held until request execution is done. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> --- block/ll_rw_blk.c | 2 ++ include/linux/blkdev.h | 7 ++++++- 2 files changed, 8 insertions(+), 1 deletions(-) diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c index b01dee3..df478f2 100644 --- a/block/ll_rw_blk.c +++ b/block/ll_rw_blk.c @@ -261,6 +261,8 @@ static void rq_init(struct request_queue *q, struct request *rq) rq->end_io_data = NULL; rq->completion_data = NULL; rq->next_rq = NULL; + rq->varlen_cdb_len = 0; + rq->varlen_cdb = NULL; } /** diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index bbf906a..990643d 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -287,8 +287,13 @@ struct request { /* * when request is used as a packet command carrier */ - unsigned int cmd_len; + unsigned short cmd_len; + unsigned short varlen_cdb_len; /* length of varlen_cdb buffer */ unsigned char cmd[BLK_MAX_CDB]; + unsigned char *varlen_cdb; /* an optional variable-length cdb. + * points to a user buffer that must be + * valid until end of request + */ unsigned int data_len; unsigned int sense_len; -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] block layer varlen-cdb 2007-11-01 18:05 ` [PATCH 2/4] block layer varlen-cdb Boaz Harrosh @ 2007-11-01 18:40 ` Matthew Wilcox 2007-11-02 6:32 ` Benny Halevy 0 siblings, 1 reply; 10+ messages in thread From: Matthew Wilcox @ 2007-11-01 18:40 UTC (permalink / raw) To: Boaz Harrosh Cc: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi, open-iscsi, Pete Wyckoff, Benny Halevy On Thu, Nov 01, 2007 at 08:05:06PM +0200, Boaz Harrosh wrote: > @@ -287,8 +287,13 @@ struct request { > /* > * when request is used as a packet command carrier > */ > - unsigned int cmd_len; > + unsigned short cmd_len; > + unsigned short varlen_cdb_len; /* length of varlen_cdb buffer */ > unsigned char cmd[BLK_MAX_CDB]; > + unsigned char *varlen_cdb; /* an optional variable-length cdb. > + * points to a user buffer that must be > + * valid until end of request > + */ > Try this instead: unsigned int cmd_len; - unsigned char cmd[BLK_MAX_CDB]; + unsigned char _cmd[BLK_MAX_CDB]; + unsigned char *cmd; Then initialise cmd to the address of _cmd. If you need to override it later (ie patch 3), you can. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] block layer varlen-cdb 2007-11-01 18:40 ` Matthew Wilcox @ 2007-11-02 6:32 ` Benny Halevy 2007-11-02 11:17 ` Matthew Wilcox 0 siblings, 1 reply; 10+ messages in thread From: Benny Halevy @ 2007-11-02 6:32 UTC (permalink / raw) To: Matthew Wilcox Cc: Boaz Harrosh, James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi, open-iscsi, Pete Wyckoff On Nov. 01, 2007, 20:40 +0200, Matthew Wilcox <matthew@wil.cx> wrote: > On Thu, Nov 01, 2007 at 08:05:06PM +0200, Boaz Harrosh wrote: >> @@ -287,8 +287,13 @@ struct request { >> /* >> * when request is used as a packet command carrier >> */ >> - unsigned int cmd_len; >> + unsigned short cmd_len; >> + unsigned short varlen_cdb_len; /* length of varlen_cdb buffer */ >> unsigned char cmd[BLK_MAX_CDB]; >> + unsigned char *varlen_cdb; /* an optional variable-length cdb. >> + * points to a user buffer that must be >> + * valid until end of request >> + */ >> > > Try this instead: > > unsigned int cmd_len; > - unsigned char cmd[BLK_MAX_CDB]; > + unsigned char _cmd[BLK_MAX_CDB]; > + unsigned char *cmd; > > Then initialise cmd to the address of _cmd. If you need to override it > later (ie patch 3), you can. > I agree this is probably the cleanest implementation but when Boaz and I initially discussed this approach he convinced me that LL block devices assume that req->cmd_len <= BLK_MAX_CDB and it is unsafe at the moment to expose them potentially larger commands. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] block layer varlen-cdb 2007-11-02 6:32 ` Benny Halevy @ 2007-11-02 11:17 ` Matthew Wilcox 2007-11-05 9:17 ` Boaz Harrosh 0 siblings, 1 reply; 10+ messages in thread From: Matthew Wilcox @ 2007-11-02 11:17 UTC (permalink / raw) To: Benny Halevy Cc: Boaz Harrosh, James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi, open-iscsi, Pete Wyckoff On Fri, Nov 02, 2007 at 08:32:12AM +0200, Benny Halevy wrote: > I agree this is probably the cleanest implementation but when Boaz and I > initially discussed this approach he convinced me that LL block devices assume > that req->cmd_len <= BLK_MAX_CDB and it is unsafe at the moment to expose them > potentially larger commands. We'll never submit a command to a low level driver that is longer than the max_cmd_len in the Scsi_Host. So if they've set it higher than they really can deal with, that's an easy bug to fix. -- Intel are signing my paycheques ... these opinions are still mine "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] block layer varlen-cdb 2007-11-02 11:17 ` Matthew Wilcox @ 2007-11-05 9:17 ` Boaz Harrosh 0 siblings, 0 replies; 10+ messages in thread From: Boaz Harrosh @ 2007-11-05 9:17 UTC (permalink / raw) To: Matthew Wilcox Cc: Benny Halevy, James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi, open-iscsi, Pete Wyckoff On Fri, Nov 02 2007 at 13:17 +0200, Matthew Wilcox <matthew@wil.cx> wrote: > On Fri, Nov 02, 2007 at 08:32:12AM +0200, Benny Halevy wrote: >> I agree this is probably the cleanest implementation but when Boaz and I >> initially discussed this approach he convinced me that LL block devices assume >> that req->cmd_len <= BLK_MAX_CDB and it is unsafe at the moment to expose them >> potentially larger commands. > > We'll never submit a command to a low level driver that is longer than > the max_cmd_len in the Scsi_Host. So if they've set it higher than they > really can deal with, that's an easy bug to fix. > This is true for scsi devices, and is what I did in patches 1/4 + 3/4, but for none-scsi, block devices, there is not such a ".max_cmd_len". There are no clients of large commands that are not scsi, so there is no use fixing any of that. The pointer at request is for the scsi case only. (Or can be used by new code for additional private command info) Boaz ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] scsi: varlen extended and vendor-specific cdbs 2007-11-01 17:54 [RFC 0/4] varlen extended and vendor-specific cdbs Boaz Harrosh 2007-11-01 18:00 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer Boaz Harrosh 2007-11-01 18:05 ` [PATCH 2/4] block layer varlen-cdb Boaz Harrosh @ 2007-11-01 18:07 ` Boaz Harrosh 2007-11-01 18:10 ` [PATCH 4/4] iscsi: extended cdb support Boaz Harrosh 3 siblings, 0 replies; 10+ messages in thread From: Boaz Harrosh @ 2007-11-01 18:07 UTC (permalink / raw) To: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi Cc: Pete Wyckoff, Benny Halevy 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 patches). - 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. - Add support for varlen_cdb in scsi_execute. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> Signed-off-by: Benny Halevy <bhalevy@panasas.com> --- block/scsi_ioctl.c | 4 ++-- drivers/scsi/constants.c | 10 +++------- drivers/scsi/scsi.c | 22 +++++++++++----------- drivers/scsi/scsi_lib.c | 25 +++++++++++++++++++++---- include/scsi/scsi.h | 40 +++++++++++++++++++++++++++++++++------- include/scsi/scsi_cmnd.h | 2 +- include/scsi/scsi_host.h | 8 +++----- 7 files changed, 74 insertions(+), 37 deletions(-) diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c index 91c7322..f08e196 100644 --- a/block/scsi_ioctl.c +++ b/block/scsi_ioctl.c @@ -33,13 +33,13 @@ #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 024553f..5edfe0f 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 e7dd171..6bbc053 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. */ @@ -467,6 +458,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)) { @@ -548,9 +540,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 0a940f4..c7e38f4 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -190,8 +190,18 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd, buffer, bufflen, __GFP_WAIT)) goto out; - req->cmd_len = COMMAND_SIZE(cmd[0]); - memcpy(req->cmd, cmd, req->cmd_len); + if (cmd[0] == VARIABLE_LENGTH_CMD) { + req->varlen_cdb_len = scsi_varlen_cdb_length(cmd); + req->varlen_cdb = (unsigned char *)cmd; + req->cmd_len = 0; + } else { + req->cmd_len = COMMAND_SIZE(cmd[0]); + memcpy(req->cmd, cmd, req->cmd_len); + if (req->cmd_len < MAX_COMMAND_SIZE) + memset(&req->cmd[req->cmd_len], 0, + MAX_COMMAND_SIZE - req->cmd_len); + } + req->sense = sense; req->sense_len = 0; req->retries = retries; @@ -441,7 +451,7 @@ static void scsi_init_cmd_errh(struct scsi_cmnd *cmd) scsi_set_resid(cmd, 0); memset(cmd->sense_buffer, 0, sizeof cmd->sense_buffer); 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) @@ -1188,7 +1198,14 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req) req->buffer = NULL; } - cmd->cmd_len = req->cmd_len; + if (req->varlen_cdb) { + cmd->cmnd = req->varlen_cdb; + cmd->cmd_len = req->varlen_cdb_len; + } else if (req->cmd_len) + cmd->cmd_len = req->cmd_len; + else + 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) diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 702fcfe..43c2ef1 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -11,13 +11,6 @@ #include <linux/types.h> /* - * 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. */ @@ -89,6 +82,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 @@ -116,6 +110,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 { + unsigned char opcode; /* opcode always == VARIABLE_LENGTH_CMD */ + unsigned char control; + unsigned char misc[5]; + unsigned char additional_cdb_length; /* total cdb length - 8 */ + unsigned char service_action[2]; + /* 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 65f5627..7f76413 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -68,7 +68,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 cb2bcab..35c231c 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -574,13 +574,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.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] iscsi: extended cdb support 2007-11-01 17:54 [RFC 0/4] varlen extended and vendor-specific cdbs Boaz Harrosh ` (2 preceding siblings ...) 2007-11-01 18:07 ` [PATCH 3/4] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh @ 2007-11-01 18:10 ` Boaz Harrosh 3 siblings, 0 replies; 10+ messages in thread From: Boaz Harrosh @ 2007-11-01 18:10 UTC (permalink / raw) To: James Bottomley, Jens Axboe, Mike Christie, FUJITA Tomonori, linux-scsi Cc: Pete Wyckoff, Benny Halevy Once varlen cdbs are supported by the block and scsi-ml layers we can apply this patch to support extended CDBs in iscsi. Signed-off-by: Boaz Harrosh <bharrosh@panasas.com> --- drivers/scsi/iscsi_tcp.h | 3 +- drivers/scsi/libiscsi.c | 56 ++++++++++++++++++++++++++++++++++++++++---- include/scsi/iscsi_proto.h | 6 +++- 3 files changed, 57 insertions(+), 8 deletions(-) diff --git a/drivers/scsi/iscsi_tcp.h b/drivers/scsi/iscsi_tcp.h index ec4e5cf..30077fa 100644 --- a/drivers/scsi/iscsi_tcp.h +++ b/drivers/scsi/iscsi_tcp.h @@ -23,6 +23,7 @@ #define ISCSI_TCP_H #include <scsi/libiscsi.h> +#include <scsi/scsi.h> /* Socket's Receive state machine */ #define IN_PROGRESS_WAIT_HEADER 0x0 @@ -49,7 +50,7 @@ #define XMSTATE_SOL_HDR_INIT 0x2000 #define ISCSI_SG_TABLESIZE SG_ALL -#define ISCSI_TCP_MAX_CMD_LEN 16 +#define ISCSI_TCP_MAX_CMD_LEN SCSI_MAX_VARLEN_CDB_SIZE struct crypto_hash; struct socket; diff --git a/drivers/scsi/libiscsi.c b/drivers/scsi/libiscsi.c index 6a747cc..e57963c 100644 --- a/drivers/scsi/libiscsi.c +++ b/drivers/scsi/libiscsi.c @@ -139,6 +139,45 @@ static int iscsi_add_hdr(struct iscsi_cmd_task *ctask, unsigned len) return 0; } +/* + * make an extended cdb AHS + */ +static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask) +{ + struct scsi_cmnd *cmd = ctask->sc; + unsigned rlen, pad_len; + unsigned short ahslength; + struct iscsi_ecdb_ahdr *ecdb_ahdr; + int rc; + + ecdb_ahdr = iscsi_next_hdr(ctask); + rlen = cmd->cmd_len - ISCSI_CDB_SIZE; + + BUG_ON(rlen > sizeof(ecdb_ahdr->ecdb)); + ahslength = rlen + sizeof(ecdb_ahdr->reserved); + + pad_len = iscsi_padding(rlen); + + rc = iscsi_add_hdr(ctask, sizeof(ecdb_ahdr->ahslength) + + sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len); + if (rc) + return rc; + + if (pad_len) + memset(&ecdb_ahdr->ecdb[rlen], 0, pad_len); + + ecdb_ahdr->ahslength = cpu_to_be16(ahslength); + ecdb_ahdr->ahstype = ISCSI_AHSTYPE_CDB; + ecdb_ahdr->reserved = 0; + memcpy(ecdb_ahdr->ecdb, cmd->cmnd + ISCSI_CDB_SIZE, rlen); + + debug_scsi("iscsi_prep_ecdb_ahs: varlen_cdb_len %d " + "rlen %d pad_len %d ahs_length %d iscsi_headers_size %u\n", + cmd->cmd_len, rlen, pad_len, ahslength, ctask->hdr_len); + + return 0; +} + /** * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu * @ctask: iscsi cmd task @@ -152,7 +191,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) struct iscsi_session *session = conn->session; struct iscsi_cmd *hdr = ctask->hdr; struct scsi_cmnd *sc = ctask->sc; - unsigned hdrlength; + unsigned hdrlength, cmd_len; int rc; ctask->hdr_len = 0; @@ -167,10 +206,17 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask) hdr->cmdsn = cpu_to_be32(session->cmdsn); session->cmdsn++; hdr->exp_statsn = cpu_to_be32(conn->exp_statsn); - memcpy(hdr->cdb, sc->cmnd, sc->cmd_len); - if (sc->cmd_len < MAX_COMMAND_SIZE) - memset(&hdr->cdb[sc->cmd_len], 0, - MAX_COMMAND_SIZE - sc->cmd_len); + cmd_len = sc->cmd_len; + if (cmd_len < ISCSI_CDB_SIZE) + memset(&hdr->cdb[cmd_len], 0, + ISCSI_CDB_SIZE - cmd_len); + else if (cmd_len > ISCSI_CDB_SIZE) { + rc = iscsi_prep_ecdb_ahs(ctask); + if (rc) + return rc; + cmd_len = ISCSI_CDB_SIZE; + } + memcpy(hdr->cdb, sc->cmnd, cmd_len); ctask->data_count = 0; ctask->imm_count = 0; diff --git a/include/scsi/iscsi_proto.h b/include/scsi/iscsi_proto.h index 4b2dce7..9dff9cc 100644 --- a/include/scsi/iscsi_proto.h +++ b/include/scsi/iscsi_proto.h @@ -110,6 +110,7 @@ struct iscsi_ahs_hdr { #define ISCSI_AHSTYPE_CDB 1 #define ISCSI_AHSTYPE_RLENGTH 2 +#define ISCSI_CDB_SIZE 16 /* iSCSI PDU Header */ struct iscsi_cmd { @@ -123,7 +124,7 @@ struct iscsi_cmd { __be32 data_length; __be32 cmdsn; __be32 exp_statsn; - uint8_t cdb[16]; /* SCSI Command Block */ + uint8_t cdb[ISCSI_CDB_SIZE]; /* SCSI Command Block */ /* Additional Data (Command Dependent) */ }; @@ -152,7 +153,8 @@ struct iscsi_ecdb_ahdr { __be16 ahslength; /* CDB length - 15, including reserved byte */ uint8_t ahstype; uint8_t reserved; - uint8_t ecdb[260 - 16]; /* 4-byte aligned extended CDB spillover */ + /* 4-byte aligned extended CDB spillover */ + uint8_t ecdb[260 - ISCSI_CDB_SIZE]; }; /* SCSI Response Header */ -- 1.5.3.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCHSET 0/3] Is it time for varlen extended and vendor-specific cdbs @ 2008-03-25 16:21 Boaz Harrosh 2008-04-13 16:30 ` [PATCHSET 0/4 ver2] " Boaz Harrosh 0 siblings, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2008-03-25 16:21 UTC (permalink / raw) To: James Bottomley, Jens Axboe, Christoph Hellwig, linux-scsi, Andrew Submitted is a patchset for adding support for variable-length, extended, and vendor specific CDBs. It should now cover the entire range of the SCSI standard. (and/or any other use of command packets in block devices) They are based on scsi-misc/scsi-rc-fixes/linux-block. They are aimed for the 2.6.26 merge window. Andrew please also include them in -mm for testing. Difference from last time, is at struct request. I did a smallish hack to save the extra pointer space. The pointer now shares it's space with the request->cmd, as they are mutual exclusive. The flag to switch between them is when cmd_len == 0 and varlen_cdb_len != 0 I've added 2 accessors to hide the mess. I think this approach should be safe with legacy drivers. This is on top of the size shrink to struct scsi_cmnd gained in the first patch. We save upto 12 bytes on 32 bit ARCHs So over all, this cleans things up, and add fixtures without *any extra cost*. [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] block layer varlen-cd Here I added an option to use a user supplied buffer for an arbitrary large command. Buffer must be kept valid until execution of request ends. The pointer to the buffer shares it's space with the fixed length command, so the size of struct request does not change. [3/3] scsi: varlen extended and vendor-specific cdbs Adds support for variable length, extended, and vendor-specific cdbs at the scsi mid-layer. Boaz ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHSET 0/4 ver2] Is it time for varlen extended and vendor-specific cdbs 2008-03-25 16:21 [PATCHSET 0/3] Is it time for varlen extended and vendor-specific cdbs Boaz Harrosh @ 2008-04-13 16:30 ` Boaz Harrosh 2008-04-13 16:37 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh 0 siblings, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2008-04-13 16:30 UTC (permalink / raw) To: James Bottomley, Jens Axboe, linux-scsi, Andrew Morton, Mike Yet another iteration with James's suggestions and the 3rd patch converted to the new block API. Also a small patch for iscsi_tcp.c --- Submitted is a patchset for adding support for variable-length, extended, and vendor specific CDBs. It should now cover the entire range of the SCSI standard, and/or any other use of command packets in block devices. They are based on scsi-misc. They are aimed for the 2.6.26 merge window. Andrew ! Please include them in -mm for testing. Atleast the first patch that no one had any comments and is the theoretically dangerous cleanup. Difference from last time, is at struct request. I did a smallish hack to save the extra pointer space. The pointer now shares it's space with the request->cmd, as they are mutual exclusive. The flag to switch between them is when cmd_len == 0 and varlen_cdb_len != 0 I've added 3 accessors to hide the mess. I think this approach should be safe with legacy drivers. This is on top of the size shrink to struct scsi_cmnd gained in the first patch. We save upto 12 bytes on 32 bit ARCHs So over all, this cleans things up, and add fixtures without *any extra cost*. [1/4] 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/4] block layer varlen-cd Here I added an option to use a user supplied buffer for an arbitrary large command. Buffer must be kept valid until execution of request ends. The pointer to the buffer shares it's space with the fixed length command, so the size of struct request does not change. [3/4] scsi: varlen extended and vendor-specific cdbs Adds support for variable length, extended, and vendor-specific cdbs at the scsi mid-layer. [4/4] iscsi_tcp: iscsi_tcp: Enable large commands just a one liner, Mike I need your Sign-off-by on this one. Boaz ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd buffer 2008-04-13 16:30 ` [PATCHSET 0/4 ver2] " Boaz Harrosh @ 2008-04-13 16:37 ` Boaz Harrosh 0 siblings, 0 replies; 10+ messages in thread From: Boaz Harrosh @ 2008-04-13 16:37 UTC (permalink / raw) To: James Bottomley, Jens Axboe, linux-scsi, Andrew Morton, Mike - 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 | 14 ++++++++------ drivers/scsi/scsi_lib.c | 5 +++-- drivers/scsi/scsi_tgt_lib.c | 2 ++ drivers/usb/storage/isd200.c | 2 ++ include/scsi/scsi_cmnd.h | 21 +++++++++++++++++++-- include/scsi/scsi_eh.h | 4 ++-- 16 files changed, 51 insertions(+), 27 deletions(-) diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c index 62b4e47..2260e56 100644 --- a/drivers/firewire/fw-sbp2.c +++ b/drivers/firewire/fw-sbp2.c @@ -1492,7 +1492,7 @@ static int sbp2_scsi_queuecommand(struct scsi_cmnd *cmd, scsi_done_fn_t done) memset(orb->request.command_block, 0, sizeof(orb->request.command_block)); - 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 85ba4cc..1bec0b2 100644 --- a/drivers/s390/scsi/zfcp_dbf.c +++ b/drivers/s390/scsi/zfcp_dbf.c @@ -1057,7 +1057,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 b7aa969..ea46881 100644 --- a/drivers/s390/scsi/zfcp_fsf.c +++ b/drivers/s390/scsi/zfcp_fsf.c @@ -4024,7 +4024,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 beecda9..a24c5e3 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 78d46a9..df9b865 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 68c0d09..b53420b 100644 --- a/drivers/scsi/qla1280.c +++ b/drivers/scsi/qla1280.c @@ -2863,7 +2863,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. */ @@ -3132,7 +3132,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 221f31e..8b5748a 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; @@ -1774,7 +1775,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; + memset(scmd->cmnd, 0, BLK_MAX_CDB); 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 67f412b..325270b 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1090,6 +1090,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; } @@ -1127,8 +1129,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; @@ -1165,6 +1165,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 a0f308b..8c92ef3 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/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..dea73e5 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 smaller 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] 10+ messages in thread
end of thread, other threads:[~2008-04-13 16:38 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2007-11-01 17:54 [RFC 0/4] varlen extended and vendor-specific cdbs Boaz Harrosh 2007-11-01 18:00 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd[] buffer Boaz Harrosh 2007-11-01 18:05 ` [PATCH 2/4] block layer varlen-cdb Boaz Harrosh 2007-11-01 18:40 ` Matthew Wilcox 2007-11-02 6:32 ` Benny Halevy 2007-11-02 11:17 ` Matthew Wilcox 2007-11-05 9:17 ` Boaz Harrosh 2007-11-01 18:07 ` [PATCH 3/4] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh 2007-11-01 18:10 ` [PATCH 4/4] iscsi: extended cdb support Boaz Harrosh -- strict thread matches above, loose matches on Subject: below -- 2008-03-25 16:21 [PATCHSET 0/3] Is it time for varlen extended and vendor-specific cdbs Boaz Harrosh 2008-04-13 16:30 ` [PATCHSET 0/4 ver2] " Boaz Harrosh 2008-04-13 16:37 ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
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).