Re: [PATCH 2/3] block layer varlen-cdb
On Tue, Feb 12 2008 at 19:54 +0200, Boaz Harrosh <[EMAIL PROTECTED]> wrote: > On Tue, Feb 12 2008 at 19:48 +0200, Christoph Hellwig <[EMAIL PROTECTED]> > wrote: >> On Sun, Feb 10, 2008 at 09:09:41PM +0200, Boaz Harrosh wrote: >>> - 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. >>> - Since there can be either a fix_length command up to 16 bytes >>> or a variable_length, larger then 16 bytes, commands but never >>> both, we hold the two types in a union to save space. The >>> presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb >>> mode. >> this one I'm a bit confused by, why can't we just set the length >> of the variable length command in cmd_len aswell, and if cmd_len > >> the length of the cmd array it's a variable length command? >> >> Note that this is both to keep the logic simpler and not to grow >> struct request further. Especially for the rather rare case >> of a bidi command. > > Because this will be dangerous for the Legacy block devices. > Unlike scsi drivers block drivers do not have a .max_cmnd_len > and upper layer will not check to make sure that the device supports > the longer command. If such a command goes through, lets say bsg > the drivers do blindly memcpy(,,rq->cmd_len) and will crash. > Better safe then sorry, at no cost. > > Boaz > > - PS: the struct request does not grow. Because before cmd_len was unsigned but now both cmd_len and varlen_cdb_len are short so it is the same as before. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] block layer varlen-cdb
On Tue, Feb 12 2008 at 19:48 +0200, Christoph Hellwig <[EMAIL PROTECTED]> wrote: > On Sun, Feb 10, 2008 at 09:09:41PM +0200, Boaz Harrosh wrote: >> - 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. >> - Since there can be either a fix_length command up to 16 bytes >> or a variable_length, larger then 16 bytes, commands but never >> both, we hold the two types in a union to save space. The >> presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb >> mode. > > this one I'm a bit confused by, why can't we just set the length > of the variable length command in cmd_len aswell, and if cmd_len > > the length of the cmd array it's a variable length command? > > Note that this is both to keep the logic simpler and not to grow > struct request further. Especially for the rather rare case > of a bidi command. Because this will be dangerous for the Legacy block devices. Unlike scsi drivers block drivers do not have a .max_cmnd_len and upper layer will not check to make sure that the device supports the longer command. If such a command goes through, lets say bsg the drivers do blindly memcpy(,,rq->cmd_len) and will crash. Better safe then sorry, at no cost. Boaz - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] block layer varlen-cdb
On Sun, Feb 10, 2008 at 09:09:41PM +0200, Boaz Harrosh wrote: > - 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. > - Since there can be either a fix_length command up to 16 bytes > or a variable_length, larger then 16 bytes, commands but never > both, we hold the two types in a union to save space. The > presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb > mode. this one I'm a bit confused by, why can't we just set the length of the variable length command in cmd_len aswell, and if cmd_len > the length of the cmd array it's a variable length command? Note that this is both to keep the logic simpler and not to grow struct request further. Especially for the rather rare case of a bidi command. - To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] block layer varlen-cdb
- 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.
- Since there can be either a fix_length command up to 16 bytes
or a variable_length, larger then 16 bytes, commands but never
both, we hold the two types in a union to save space. The
presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb
mode.
- Please use added rq_{set,get}_varlen_cdb() to set every thing
up in a way that will not confuse drivers that do not support
varlen_cdb's.
- Note that this patch does not add any size to struct request
since the unsigned cmd_len is split here to 2 ushorts, which
is more then enough.
Signed-off-by: Boaz Harrosh <[EMAIL PROTECTED]>
---
block/blk-core.c |2 ++
include/linux/blkdev.h | 27 +--
2 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 4afb39c..1c5cfa7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -124,6 +124,8 @@ 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;
}
static void req_bio_endio(struct request *rq, struct bio *bio,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 90392a9..a8a6c20 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -211,8 +211,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 varlen_cdb_len; /* length of varlen_cdb buffer */
+ union {
+ 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;
@@ -478,6 +485,22 @@ enum {
#define rq_is_sync(rq) (rq_data_dir((rq)) == READ || (rq)->cmd_flags &
REQ_RW_SYNC)
#define rq_is_meta(rq) ((rq)->cmd_flags & REQ_RW_META)
+static inline void rq_set_varlen_cdb(struct request *rq, u8 *cdb, short
cdb_len)
+{
+ rq->cmd_len = 0; /* Make sure legacy drivers don't get confused */
+ rq->varlen_cdb_len = cdb_len;
+ rq->varlen_cdb = cdb;
+}
+
+/* If ! a varlen_cdb than return NULL */
+static inline u8 *rq_get_varlen_cdb(struct request *rq)
+{
+ if (!rq->cmd_len && rq->varlen_cdb_len)
+ return rq->varlen_cdb;
+ else
+ return NULL;
+}
+
static inline int blk_queue_full(struct request_queue *q, int rw)
{
if (rw == READ)
--
1.5.3.3
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
