On Thu, 13 Oct 2011, Barry Song wrote: > >> @@ -2412,6 +2412,8 @@ static int do_scsi_command(struct fsg_dev *fsg) > >> break; > >> > >> case READ_6: > >> + if (unlikely(!fsg->curlun)) > >> + goto unknown_cmnd; > >> i = fsg->cmnd[4]; > >> fsg->data_size_from_cmnd = (i == 0 ? 256 : i) << fsg->curlun->blkbits; > > > > Instead of adding all theses checks, it would be better to add a new > > flag: fsg->data_size_is_in_blocks. Skip doing the bit shifts here and > > set the flag instead. Then have check_command() do the test and the > > bit shift if the flag is set. > > with this new flag, we actually need to add codes to set it to 0 for > non-READ/WRITE commands and set it to 1 for READ/WRITE commands. Not really. All you need to do is set the flag back to 0 in check_command() -- and then only in the case where it was 1 originally. > and another issue is we have to add codes to check this flag in check_command(). > it doesn't seem to save many codes. Checking the LUN value and doing the bit shifts in a single place is better than doing them repeatedly in many different command handlers. > >> @@ -2642,6 +2654,10 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh) > >> if (fsg->data_size == 0) > >> fsg->data_dir = DATA_DIR_NONE; > >> fsg->lun = cbw->Lun; > >> + if (fsg->lun >= 0 && fsg->lun < fsg->nluns) > >> + fsg->curlun = &fsg->luns[fsg->lun]; > >> + else > >> + fsg->curlun = NULL; > >> fsg->tag = cbw->Tag; > >> return 0; > >> } > > > > Instead of adding this here, it should be in get_next_command(). Then > > it would be obvious that you missed the !transport_is_bbb() case, which > > needs a similar change (and an assignment to fsg->lun). In fact, this > > enw code can go outside the big "if" statement. > > for f_mass_storage, received_cbw() should be a better place for this > since received_cbw() will always be called in get_next_command(). > and in the last some lines of received_cbw(), context is updating > common->lun according to command. I don't know what you're talking about. common->lun gets set three lines before the end of received_cbw() and is otherwise untouched in that subroutine. Regardless, there's nothing wrong with putting the new code in get_next_command(). And in file_storage.c it's definitely better to do it that way, so why not use the same code in both drivers? > for file_storage, update curlen in received_cbw() will miss > !transport_is_bbb() case. Exactly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
