On Tue, Oct 18, 2011 at 12:20 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, 17 Oct 2011, Yuping Luo wrote:
>
>> here you are , :)
>
> This is the right idea, but a few things need to be improved. I'll
> comment on just the changes to file_storage.c.
>
>> diff --git a/drivers/usb/gadget/file_storage.c
>> b/drivers/usb/gadget/file_storage.c
>> index e081e28..2775012 100644
>> --- a/drivers/usb/gadget/file_storage.c
>> +++ b/drivers/usb/gadget/file_storage.c
>> @@ -472,6 +472,7 @@ struct fsg_dev {
>> enum data_direction data_dir;
>> u32 data_size;
>> u32 data_size_from_cmnd;
>> + u32 data_size_is_in_blocks;
>
> This should not be a u32. It should be a single-bit flag, like
> bad_lun_okay higher up.
ok, will change.
>> u32 tag;
>> unsigned int lun;
>> u32 residue;
>> @@ -2249,6 +2250,18 @@ static int check_command(struct fsg_dev *fsg,
>> int cmnd_size,
>> } else
>> cmnd_size = 12;
>>
>> + curlun = fsg->curlun;
>> + /* Convert the data size's unit from CDB */
>> + if (fsg->data_size_is_in_blocks == 1) {
>
> You don't need to check for == 1. Implicitly comparing to 0 is enough,
> and it also is better English grammar:
>
> if (fsg->data_size_is_in_blocks) {
ok, will change.
>> + fsg->data_size_is_in_blocks = 0;
>> + if (!curlun) {
>> + DBG(fsg, "curlun NULL\n");
>> + return -EINVAL;
>> + }
>
> Neither the debug message nor the return is needed. Just reverse the
> sense of the test, and ...
>
if enter !curlun case, something is wrong in the incoming packet, the
data_size_from_cmnd is uncertain. correct ?
>> + fsg->data_size_from_cmnd =
>> + fsg->data_size_from_cmnd << curlun->blkbits;
>
> ... use a compound assignment operator:
>
ok, will change.
> if (curlun)
> fsg->data_size_from_cmnd <<= curlun->blkbits;
>
>> @@ -2443,8 +2454,8 @@ 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;
>> + fsg->data_size_from_cmnd = (i == 0 ? 256 : i);
>> + fsg->data_size_is_in_blocks = 1;
>> if ((reply = check_command(fsg, 6, DATA_DIR_TO_HOST,
>> (7<<1) | (1<<4), 1,
>> "READ(6)")) == 0)
>
> That's good. I suspect the code size will end up smaller than in your
> original patch -- it looks larger only because of the rather long field
> name.
>
>> @@ -2688,6 +2699,12 @@ static int get_next_command(struct fsg_dev *fsg)
>> struct fsg_buffhd *bh;
>> int rc = 0;
>>
>> + /* Update current lun */
>> + if (fsg->lun >= 0 && fsg->lun < fsg->nluns)
>> + fsg->curlun = &fsg->luns[fsg->lun];
>> + else
>> + fsg->curlun = NULL;
>> +
>> if (transport_is_bbb()) {
>
> Dear me, no! The new code has to come _after_ the big "if" statement,
> not _before_, because fsg->lun gets set in received_cbw().
>
ok, my mistake.
> Also, you forgot to add a line setting fsg->lun in the !bbb case.
>
ok, will add.
> Alan Stern
>
>
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
