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.
> 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) {
> + 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 ...
> + fsg->data_size_from_cmnd =
> + fsg->data_size_from_cmnd << curlun->blkbits;
... use a compound assignment operator:
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().
Also, you forgot to add a line setting fsg->lun in the !bbb case.
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
