On Thu, Oct 20, 2011 at 10:09 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 20 Oct 2011, Yuping Luo wrote:
>
>> I send the whole fixed patch for your review, if desirable , Barry
>> will send one formal patch for push
>
>> --- a/drivers/usb/gadget/file_storage.c
>> +++ b/drivers/usb/gadget/file_storage.c
>
>> @@ -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) {
>
> This part is wrong. The new code has to be added at the end of the
> block, not the beginning -- at the beginning the new command may not
> even have been received yet. Also, the CB and CBI transports don't use
> CBWs.
>
> What you have to add is (after the spin_unlock_irq):
>
> fsg->lun = fsg->cmnd[1] >> 5;
>
> Just like the declaration in check_command(). In fact, with this
> addition you can remove the line in check_command() that says:
>
> fsg->lun = lun; // Use LUN from the command
>
Thanks. mainly focused on the controller code, still need more time to
under these gadgets.
> You really should test new code before submitting patches. Proper
> testing would have revealed these errors.
>
> Alan Stern
>
>
After testing of the two gadgets, like the basic copying, in-house
mass storage tool and ATTO tool under windowsXP/Ubuntu 11.10,
everything is ok, except fsg_lun_fsync_sub() under do_prevent_allow()
take too much time under some case, seeming it's has nothing to to
with this patch, I will open another thread to give more details, so
the updated patch:
diff --git a/drivers/usb/gadget/f_mass_storage.c
b/drivers/usb/gadget/f_mass_storage.c
index f45124b..b02ff32 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -1916,17 +1916,14 @@ static int check_command(struct fsg_common
*common, int cmnd_size,
common->lun, lun);
/* Check the LUN */
- if (common->lun >= 0 && 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;
/*
@@ -1972,6 +1969,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;
@@ -2054,9 +2063,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)
@@ -2065,9 +2074,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)
@@ -2076,9 +2085,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)
@@ -2177,9 +2186,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)
@@ -2188,9 +2197,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)
@@ -2199,9 +2208,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)
@@ -2316,6 +2325,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 e081e28..0333cc5 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -2315,19 +2315,17 @@ static int check_command(struct fsg_dev *fsg,
int cmnd_size,
DBG(fsg, "using LUN %d from CBW, "
"not LUN %d from CDB\n",
fsg->lun, lun);
- } else
- fsg->lun = lun; // Use LUN from the command
+ }
/* Check the LUN */
- if (fsg->lun >= 0 && 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
@@ -2369,6 +2367,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)
{
@@ -2443,9 +2451,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);
@@ -2453,9 +2461,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);
@@ -2463,9 +2471,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);
@@ -2550,9 +2558,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);
@@ -2560,9 +2568,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);
@@ -2570,9 +2578,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);
@@ -2740,7 +2748,17 @@ static int get_next_command(struct fsg_dev *fsg)
memcpy(fsg->cmnd, fsg->cbbuf_cmnd, fsg->cmnd_size);
fsg->cbbuf_cmnd_size = 0;
spin_unlock_irq(&fsg->lock);
+
+ /* Use LUN from the command */
+ fsg->lun = fsg->cmnd[1] >> 5;
}
+
+ /* Update current lun */
+ if (fsg->lun >= 0 && fsg->lun < fsg->nluns)
+ fsg->curlun = &fsg->luns[fsg->lun];
+ else
+ fsg->curlun = NULL;
+
return rc;
}
best regards,
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
