On 06/02/2010 12:05 PM, FUJITA Tomonori wrote: > On Wed, 02 Jun 2010 11:54:25 +0300 > Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote: > >>> Some LLDs already use scsi_get_lba(). They don't know scsi command >>> from pc or fs so they can't use scsi_get_lba() now. Well, they can see >>> scsi_cmnd->request to know pc or fs but it's layer violation. They >>> shouldn't access to the block layer stuff. >>> >> >> right, so hide it all under an API that is implemented at the right >> level, and protect them. The driver should call a single simple >> API the magic will be done at the generic level >> >> if (blk_fs_rq(req)) >> return blk_rq_pos(req); >> else >> return scsi_somthing(); > > The first version that I posted yesterday is something like that. But > I don't think that it's clean. > I can't find it. I'd like to see > >>>> Only drivers/code that actually cares should call this member. >>>> >>>> It should be easy enough to search for __sector and change them >>>> to a function call. >>> >>> I don't think that calling such function in LLDs is a clean design. We >>> set request->__sector for fs requests. Why not for pc requests? Then >>> we can use blk_rq_pos() cleanly. >>> >> >> Because "fs requests" have a defined blk_rq_pos(). But "pc requests" >> don't necessarily have one. Pretending to invent one is stupid and >> dangerous. Better keep a -1 for all "pc requests". > > I don't see your logic, why using blk_rq_pos() for pc is dangerous? I already explained. because you must know the device type before you assume the presence of lba. > >> scsi LLDs can be pass-through for disks, as well as lots of other >> type of devices. They should not assume the role of a disk. The use >> of blk_rq_pos() at LLDs is probably wrong in the first place. Unless >> it is some kind of scsi emulation driver like scsi_debug. > > Not only scsi_debug. Have you seen how scsi_get_lba() is used? LLDs > sometime needs to know lba in scsi commands. > > How about calling this function in sd_prep_fn, sr_prep_fn instead of > scsi_setup_blk_pc_cmnd? Then it doesn't affect OSD. That's fine because these know that they are disk and ROM/RAM device types. Watch out with sr because it supports other command sets. there might be more too it. Boaz -- 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
