On Mon, 14 Apr 2008 15:22:21 +0300 Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > On Mon, Apr 14 2008 at 15:08 +0300, FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote: > > On Mon, 14 Apr 2008 14:28:45 +0300 > > Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > > > >> On Mon, Apr 14 2008 at 14:04 +0300, FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote: > >>> On Mon, 14 Apr 2008 12:49:49 +0300 > >>> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > >>> > >>>> On Sun, Apr 13 2008 at 19:50 +0300, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > >>>>> On Sun, Apr 13 2008 at 19:17 +0300, FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote: > >>>>>> On Sun, 13 Apr 2008 12:13:18 +0300 > >>>>>> Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > >>>>>> > >>>>>> 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. > >>>>>> -- > >>>>> Sorry TOMO, I was sending the ver2 patchset and only saw your mail in > >>>>> the middle, so anyway you have the latest I have now. > >>>>> > >>>>> If it's ok with you I will squash your patches onto mine and add your > >>>>> sign-off-by. There is no use putting code in the tree that will be changed > >>>>> immediately after. > >>>>> > >>>>> Please note that I'm a bit afraid to put code that has both length as one > >>>>> if you are more confident then me, I will take your word for it. > >>>>> > >>>>> Thanks for helping out, as you can see I did it very safe, but with your > >>>>> help maybe it can finally go in. Thanks++ > >>>>> > >>>>> Boaz > >>>>> -- > >>>> Maybe you mean something like below. I changed cdb => cmd and I use one > >>>> cmd_len. I think that "cdb" was a good name. Note that we have BLK_MAX_CDB > >>>> right there next to it. But if you don't like it then I don't mind to > >>>> change the name, I think it was James idea. > >>> I don't. I've been talking mainly about how to represent the length of > >>> command. > >>> > >>> About 'cdb' name, I thought that there is a better name than > >>> BLK_MAX_CDB. But it's up to Jens. > >>> > >>> > >>>> But please note!!! > >>>> I think having one cmd_len is DANGEROUS. And it forces a full code audit > >>>> to be sure, has with my approach we are much more safe. There, I said it. > >>> Could you be more specific? So far, you have not explained how it can > >>> be dangerous. > >> There is a pending patch by Pete, and our main goal, is to push OSD commands > >> through bsg. All over the kernel there are cooked up char-devices, like in > >> gdth, that do vendor specific management commands. All these, and future, > >> special-command can go from user-mode through bsg in a common way. Via support > >> of vendor specific and extended commands. All the new scsi defined commands > >> that are bigger then 16 are not going to be sent through sg, but through > >> bsg right? So we do want to use this facility from kernel and from user-land > >> but we need some protection. In scsi LLDs we have .max_cmnd but block devices > >> don't have that. So either you need to restrict what can be done, or play it safe > >> like I did. > > > > If we creates bsg devices for block devices, of course, we need to a > > mechanism to govern the length of commands. > > > > But why do you want to create bsg devices for block devices? If block > > devices want large commands for some reasons, their bsg hook functions > > need to handle the length of commands anyway. There is no reason that > > the block layer has two values to represent one item, the length of > > command. > > > > If we want to govern the command length in a common place, then I > > think that it would be better to add the limit to request queue. > > > > And again, what are your block devices? What do they want bsg for? > > -- > > I don't have any block devices personally. I just thought that the diff > between sg a bsg is that bsg will support any block device, so I guess > I was wrong, bsg is a user-to-scsi bridge just like sg is. I thought > bsg registers on every request_queue, how does bsg filter which devices > to export and which not? bsg is not a user-to-scsi bridge just like sg is. You can create bsg devices for any request queue but we don't create bsg devices for every request queue in kernel. When you create a bsg device, you need your hook funciton to perform a request via the bsg device. The hook function needs to handle large commands if necessary. If you are interested in how bsg works for non scsi devices, see scsi_transport_sas.c. It creates bsg devices for sas objects to handle SMP requests. > What about the union, don't we care about the space saved? I think that the approach to use a cmd pointer is clearer. But it's up to Jens. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
