On Thu, Oct 20, 2011 at 4:29 PM, Yuping Luo <lypingsh@xxxxxxxxx> wrote: > 2011/10/18 Michal Nazarewicz <mina86@xxxxxxxxxx>: >> On Mon, 17 Oct 2011 19:56:20 -0700, Yuping Luo <lypingsh@xxxxxxxxx> wrote: >> >>> On Tue, Oct 18, 2011 at 4:30 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> >>> wrote: >>>> >>>> On Mon, 17 Oct 2011, Michal Nazarewicz wrote: >>>> >>>>> On Mon, 17 Oct 2011 02:01:48 -0700, Yuping Luo <lypingsh@xxxxxxxxx> >>>>> wrote: >>>>> > here you are , :) >>>>> > >>>>> > diff --git a/drivers/usb/gadget/f_mass_storage.c >>>>> > b/drivers/usb/gadget/f_mass_storage.c >>>>> > index 524381a..6a5c42d 100644 >>>>> > --- a/drivers/usb/gadget/f_mass_storage.c >>>>> > +++ b/drivers/usb/gadget/f_mass_storage.c >>>>> > @@ -379,6 +379,7 @@ struct fsg_common { >>>>> > enum data_direction data_dir; >>>>> > u32 data_size; >>>>> > u32 data_size_from_cmnd; >>>>> > + u32 data_size_is_in_blocks; >>>>> >>>>> Personally, I'd make it an argument to check_command() or create a >>>>> simple >>>>> wrapper for check_command() that calculates the size. >>>> >>>> I considered making this a new argument to check_command(). In the end >>>> I decided against it, because check_command() already has a lot of >>>> arguments, and also it would be necessary to add the new argument to >>>> every call (of which there are quite a few). >>>> >>>> Using a wrapper routine is a good idea -- it didn't occur to me before. >>>> >>>> Alan Stern >>>> >>> the codes only occur one time, which makes the wrapper unnecessary. >> >> In do_scsi_command() you add a bunch of ->data_size_is_in_blocks = 1 >> statements. Instead of doing that, you might create a >> check_command_size_in_blocks() function like so: >> >> static int check_command_size_in_blocks(...) >> { >> if (*->curlun) >> *->->data_size_from_cmnd <<= *->curlun->blkbits; >> return check_command(...); >> } >> >> and call it instead. This would make data_size_is_in_blocks unnecessary >> which IMO is a good thing since it is a hidden state. >> > got it , thanks. >> -- >> Best regards, _ _ >> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o >> ..o | Computer Science, Michał “mina86” Nazarewicz (o o) >> ooo +----<email/xmpp: mpn@xxxxxxxxxx>--------------ooO--(_)--Ooo-- >> I send the whole fixed patch for your review, if desirable , Barry will send one formal patch for push diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index a354648..6ecaef6 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -1889,17 +1890,14 @@ static int check_command(struct fsg_common *common, int cmnd_size, common->lun, lun); /* Check the LUN */ - if (common->lun < common->nluns) { - curlun = &common->luns[common->lun]; - common->curlun = curlun; + curlun = common->curlun; + if (curlun) { if (common->cmnd[0] != REQUEST_SENSE) { curlun->sense_data = SS_NO_SENSE; curlun->sense_data_info = 0; curlun->info_valid = 0; } } else { - common->curlun = NULL; - curlun = NULL; common->bad_lun_okay = 0; /* @@ -1945,6 +1943,18 @@ static int check_command(struct fsg_common *common, int cmnd_size, return 0; } +/* wrapper of check_command for data size in blocks handling */ +static int check_command_size_in_blocks(struct fsg_common *common, + int cmnd_size, enum data_direction data_dir, + unsigned int mask, int needs_medium, const char *name) +{ + if (common->curlun) + common->data_size_from_cmnd <<= common->curlun->blkbits; + return check_command(common, cmnd_size, data_dir, + mask, needs_medium, name); +} + + static int do_scsi_command(struct fsg_common *common) { struct fsg_buffhd *bh; @@ -2027,9 +2037,9 @@ static int do_scsi_command(struct fsg_common *common) case READ_6: i = common->cmnd[4]; - common->data_size_from_cmnd = (i == 0 ? 256 : i) << - common->curlun->blkbits; - reply = check_command(common, 6, DATA_DIR_TO_HOST, + common->data_size_from_cmnd = (i == 0 ? 256 : i); + reply = check_command_size_in_blocks(common, 6, + DATA_DIR_TO_HOST, (7<<1) | (1<<4), 1, "READ(6)"); if (reply == 0) @@ -2038,9 +2048,9 @@ static int do_scsi_command(struct fsg_common *common) case READ_10: common->data_size_from_cmnd = - get_unaligned_be16(&common->cmnd[7]) << - common->curlun->blkbits; - reply = check_command(common, 10, DATA_DIR_TO_HOST, + get_unaligned_be16(&common->cmnd[7]); + reply = check_command_size_in_blocks(common, 10, + DATA_DIR_TO_HOST, (1<<1) | (0xf<<2) | (3<<7), 1, "READ(10)"); if (reply == 0) @@ -2049,9 +2059,9 @@ static int do_scsi_command(struct fsg_common *common) case READ_12: common->data_size_from_cmnd = - get_unaligned_be32(&common->cmnd[6]) << - common->curlun->blkbits; - reply = check_command(common, 12, DATA_DIR_TO_HOST, + get_unaligned_be32(&common->cmnd[6]); + reply = check_command_size_in_blocks(common, 12, + DATA_DIR_TO_HOST, (1<<1) | (0xf<<2) | (0xf<<6), 1, "READ(12)"); if (reply == 0) @@ -2150,9 +2160,9 @@ static int do_scsi_command(struct fsg_common *common) case WRITE_6: i = common->cmnd[4]; - common->data_size_from_cmnd = (i == 0 ? 256 : i) << - common->curlun->blkbits; - reply = check_command(common, 6, DATA_DIR_FROM_HOST, + common->data_size_from_cmnd = (i == 0 ? 256 : i); + reply = check_command_size_in_blocks(common, 6, + DATA_DIR_FROM_HOST, (7<<1) | (1<<4), 1, "WRITE(6)"); if (reply == 0) @@ -2161,9 +2171,9 @@ static int do_scsi_command(struct fsg_common *common) case WRITE_10: common->data_size_from_cmnd = - get_unaligned_be16(&common->cmnd[7]) << - common->curlun->blkbits; - reply = check_command(common, 10, DATA_DIR_FROM_HOST, + get_unaligned_be16(&common->cmnd[7]); + reply = check_command_size_in_blocks(common, 10, + DATA_DIR_FROM_HOST, (1<<1) | (0xf<<2) | (3<<7), 1, "WRITE(10)"); if (reply == 0) @@ -2172,9 +2182,9 @@ static int do_scsi_command(struct fsg_common *common) case WRITE_12: common->data_size_from_cmnd = - get_unaligned_be32(&common->cmnd[6]) << - common->curlun->blkbits; - reply = check_command(common, 12, DATA_DIR_FROM_HOST, + get_unaligned_be32(&common->cmnd[6]); + reply = check_command_size_in_blocks(common, 12, + DATA_DIR_FROM_HOST, (1<<1) | (0xf<<2) | (0xf<<6), 1, "WRITE(12)"); if (reply == 0) @@ -2289,6 +2299,10 @@ static int received_cbw(struct fsg_dev *fsg, struct fsg_buffhd *bh) if (common->data_size == 0) common->data_dir = DATA_DIR_NONE; common->lun = cbw->Lun; + if (common->lun >= 0 && common->lun < common->nluns) + common->curlun = &common->luns[common->lun]; + else + common->curlun = NULL; common->tag = cbw->Tag; return 0; } diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c index e8600f2..06490ca 100644 --- a/drivers/usb/gadget/file_storage.c +++ b/drivers/usb/gadget/file_storage.c @@ -2289,15 +2295,14 @@ static int check_command(struct fsg_dev *fsg, int cmnd_size, fsg->lun = lun; // Use LUN from the command /* Check the LUN */ - if (fsg->lun < fsg->nluns) { - fsg->curlun = curlun = &fsg->luns[fsg->lun]; + curlun = fsg->curlun; + if (curlun) { if (fsg->cmnd[0] != REQUEST_SENSE) { curlun->sense_data = SS_NO_SENSE; curlun->sense_data_info = 0; curlun->info_valid = 0; } } else { - fsg->curlun = curlun = NULL; fsg->bad_lun_okay = 0; /* INQUIRY and REQUEST SENSE commands are explicitly allowed @@ -2339,6 +2344,16 @@ static int check_command(struct fsg_dev *fsg, int cmnd_size, return 0; } +/* wrapper of check_command for data size in blocks handling */ +static int check_command_size_in_blocks(struct fsg_dev *fsg, int cmnd_size, + enum data_direction data_dir, unsigned int mask, + int needs_medium, const char *name) +{ + if (fsg->curlun) + fsg->data_size_from_cmnd <<= fsg->curlun->blkbits; + return check_command(fsg, cmnd_size, data_dir, + mask, needs_medium, name); +} static int do_scsi_command(struct fsg_dev *fsg) { @@ -2413,8 +2428,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case READ_6: i = fsg->cmnd[4]; - fsg->data_size_from_cmnd = (i == 0 ? 256 : i) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 6, DATA_DIR_TO_HOST, + fsg->data_size_from_cmnd = (i == 0 ? 256 : i); + if ((reply = check_command_size_in_blocks(fsg, 6, + DATA_DIR_TO_HOST, (7<<1) | (1<<4), 1, "READ(6)")) == 0) reply = do_read(fsg); @@ -2422,8 +2438,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case READ_10: fsg->data_size_from_cmnd = - get_unaligned_be16(&fsg->cmnd[7]) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 10, DATA_DIR_TO_HOST, + get_unaligned_be16(&fsg->cmnd[7]); + if ((reply = check_command_size_in_blocks(fsg, 10, + DATA_DIR_TO_HOST, (1<<1) | (0xf<<2) | (3<<7), 1, "READ(10)")) == 0) reply = do_read(fsg); @@ -2431,8 +2448,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case READ_12: fsg->data_size_from_cmnd = - get_unaligned_be32(&fsg->cmnd[6]) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 12, DATA_DIR_TO_HOST, + get_unaligned_be32(&fsg->cmnd[6]); + if ((reply = check_command_size_in_blocks(fsg, 12, + DATA_DIR_TO_HOST, (1<<1) | (0xf<<2) | (0xf<<6), 1, "READ(12)")) == 0) reply = do_read(fsg); @@ -2517,8 +2535,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case WRITE_6: i = fsg->cmnd[4]; - fsg->data_size_from_cmnd = (i == 0 ? 256 : i) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 6, DATA_DIR_FROM_HOST, + fsg->data_size_from_cmnd = (i == 0 ? 256 : i); + if ((reply = check_command_size_in_blocks(fsg, 6, + DATA_DIR_FROM_HOST, (7<<1) | (1<<4), 1, "WRITE(6)")) == 0) reply = do_write(fsg); @@ -2526,8 +2545,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case WRITE_10: fsg->data_size_from_cmnd = - get_unaligned_be16(&fsg->cmnd[7]) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 10, DATA_DIR_FROM_HOST, + get_unaligned_be16(&fsg->cmnd[7]); + if ((reply = check_command_size_in_blocks(fsg, 10, + DATA_DIR_FROM_HOST, (1<<1) | (0xf<<2) | (3<<7), 1, "WRITE(10)")) == 0) reply = do_write(fsg); @@ -2535,8 +2555,9 @@ static int do_scsi_command(struct fsg_dev *fsg) case WRITE_12: fsg->data_size_from_cmnd = - get_unaligned_be32(&fsg->cmnd[6]) << fsg->curlun->blkbits; - if ((reply = check_command(fsg, 12, DATA_DIR_FROM_HOST, + get_unaligned_be32(&fsg->cmnd[6]); + if ((reply = check_command_size_in_blocks(fsg, 12, + DATA_DIR_FROM_HOST, (1<<1) | (0xf<<2) | (0xf<<6), 1, "WRITE(12)")) == 0) reply = do_write(fsg); @@ -2683,6 +2704,14 @@ static int get_next_command(struct fsg_dev *fsg) bh->state = BUF_STATE_EMPTY; } else { // USB_PR_CB or USB_PR_CBI + struct usb_request *req; + struct fsg_bulk_cb_wrap *cbw; + + /* update lun */ + bh = fsg->next_buffhd_to_fill; + req = bh->outreq; + cbw = req->buf; + fsg->lun = cbw->Lun; /* Wait for the next command to arrive */ while (fsg->cbbuf_cmnd_size == 0) { @@ -2705,6 +2734,13 @@ static int get_next_command(struct fsg_dev *fsg) fsg->cbbuf_cmnd_size = 0; spin_unlock_irq(&fsg->lock); } + + /* Update current lun */ + if (fsg->lun >= 0 && fsg->lun < fsg->nluns) + fsg->curlun = &fsg->luns[fsg->lun]; + else + fsg->curlun = NULL; + return rc; } thanks Yuping Luo -- 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