linux-scsi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHSET 0/3] varlen extended and vendor-specific cdbs
@ 2007-12-18 13:33 Boaz Harrosh
  2007-12-18 13:38 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Boaz Harrosh @ 2007-12-18 13:33 UTC (permalink / raw)
  To: James Bottomley, Jens Axboe, Mike Christie, linux-scsi
  Cc: Benny Halevy, Matthew Wilcox

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.

They are based on scsi-misc + the submitted bidi patches.

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. I can easily go back to the separate pointer,
but I figured a little hack is better then a bloat.

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 with out
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.

I will send a patch to iscsi once the pending iscsi patchset gets
committed into scsi-misc.

Boaz



^ permalink raw reply	[flat|nested] 7+ 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; 7+ 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] 7+ messages in thread
* [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
  0 siblings, 1 reply; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2008-04-30  8:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-12-18 13:33 [PATCHSET 0/3] varlen extended and vendor-specific cdbs Boaz Harrosh
2007-12-18 13:38 ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
2007-12-18 13:42 ` [PATCH 2/2] block layer varlen-cdb Boaz Harrosh
2007-12-18 13:45 ` [PATCH 3/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh
2007-12-20 19:39 ` [PATCHSET 0/3] " 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:30     ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
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

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).