2011/10/13 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>: > 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. i don't see too much benefit we can get from adding a new flag. might it is clearer. if you prefer that way, we can take. > >> >> @@ -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? ok > >> for file_storage, update curlen in received_cbw() will miss >> !transport_is_bbb() case. > > Exactly. > > Alan Stern -barry -- 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
