Hi Alan, 2011/10/13 Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>: > On Tue, 11 Oct 2011, Barry Song wrote: > >> From: YuPing Luo <yuping.luo@xxxxxxx> >> >> With Peiyu's patch "gadget: mass_storage: adapt logic block size to bound block >> devices" (http://www.spinics.net/lists/linux-usb/msg50791.html), now mass storage >> can adjust logic block size dynamically based on real devices. >> Then there is one issue caused by it, if two luns have different logic block size, >> mass storage can't work. >> Let's check the current software flow: >> 1. get_next_command(): call received_cbw(); >> 2. received_cbw(): update common->lun = cbw->Lun, but common->curlen is not updated; >> 3. do_scsi_command(): in READ_X and WRITE_X commands, common->data_size_from_cmnd is >> updated by common->curlun->blkbits; >> 4. check_command(): update common->curlun according to common->lun >> As you can see, the step 3 uses wrong common->curlun, then wrong common->curlun->blkbits. >> If the two luns have same blkbits, there isn't issue. Otherwise, both will fail. >> This patch moves the common->curlun update to step 2(received_cbw()), then make sure >> step 3 gets right blkbits and right data_size_from_cmnd. > > Yes, this is a very real problem -- good work finding it! However your > solution could be improved. Concentrating just on file_storage.c: > >> @@ -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. and another issue is we have to add codes to check this flag in check_command(). it doesn't seem to save many codes. > >> @@ -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. for file_storage, update curlen in received_cbw() will miss !transport_is_bbb() case. > > 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
