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. > @@ -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. 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
