Hi,
On Mon, Apr 4, 2011 at 5:49 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> Felipe:
>
> Here is a patch that does what $SUBJECT says, together with removing
> the unwanted usages of the short_not_ok flag. It also removes some
> questionable optimizations that involved aligning VFS I/O requests with
> the page size. It does nothing to address the issue of bulk-OUT
> requests not being an even multiple of the maxpacket size.
>
> Like yours, this patch is completely untested.
>
> Alan Stern
>
>
>
> drivers/usb/gadget/file_storage.c | 71 +++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 46 deletions(-)
>
> Index: usb-2.6/drivers/usb/gadget/file_storage.c
> ===================================================================
> --- usb-2.6.orig/drivers/usb/gadget/file_storage.c
> +++ usb-2.6/drivers/usb/gadget/file_storage.c
> @@ -436,7 +436,6 @@ struct fsg_dev {
> int intreq_busy;
> struct fsg_buffhd *intr_buffhd;
>
> - unsigned int bulk_out_maxpacket;
> enum fsg_state state; // For exception handling
> unsigned int exception_req_tag;
>
> @@ -1116,7 +1115,6 @@ static int do_read(struct fsg_dev *fsg)
> u32 amount_left;
> loff_t file_offset, file_offset_tmp;
> unsigned int amount;
> - unsigned int partial_page;
> ssize_t nread;
>
> /* Get the starting Logical Block Address and check that it's
> @@ -1151,17 +1149,10 @@ static int do_read(struct fsg_dev *fsg)
> * Try to read the remaining amount.
> * But don't read more than the buffer size.
> * And don't try to read past the end of the file.
> - * Finally, if we're not at a page boundary, don't read past
> - * the next page.
> - * If this means reading 0 then we were asked to read past
> - * the end of file. */
> + */
> amount = min((unsigned int) amount_left, mod_data.buflen);
> amount = min((loff_t) amount,
> curlun->file_length - file_offset);
> - partial_page = file_offset & (PAGE_CACHE_SIZE - 1);
> - if (partial_page > 0)
> - amount = min(amount, (unsigned int) PAGE_CACHE_SIZE -
> - partial_page);
>
> /* Wait for the next buffer to become available */
> bh = fsg->next_buffhd_to_fill;
> @@ -1206,6 +1197,11 @@ static int do_read(struct fsg_dev *fsg)
> file_offset += nread;
> amount_left -= nread;
> fsg->residue -= nread;
> +
> + /* Except at the end of the transfer, nread will be
> + * equal to the buffer size, which is divisible by the
> + * page size and hence by the bulk-in maxpacket size.
> + */
> bh->inreq->length = nread;
> bh->state = BUF_STATE_FULL;
>
> @@ -1242,7 +1238,6 @@ static int do_write(struct fsg_dev *fsg)
> u32 amount_left_to_req, amount_left_to_write;
> loff_t usb_offset, file_offset, file_offset_tmp;
> unsigned int amount;
> - unsigned int partial_page;
> ssize_t nwritten;
> int rc;
>
> @@ -1293,38 +1288,21 @@ static int do_write(struct fsg_dev *fsg)
> if (bh->state == BUF_STATE_EMPTY && get_some_more) {
>
> /* Figure out how much we want to get:
> - * Try to get the remaining amount.
> - * But don't get more than the buffer size.
> - * And don't try to go past the end of the file.
> - * If we're not at a page boundary,
> - * don't go past the next page.
> - * If this means getting 0, then we were asked
> - * to write past the end of file.
> - * Finally, round down to a block boundary. */
> + * Try to get the remaining amount,
> + * but not more than the buffer size.
> + */
> amount = min(amount_left_to_req, mod_data.buflen);
> - amount = min((loff_t) amount, curlun->file_length -
> - usb_offset);
> - partial_page = usb_offset & (PAGE_CACHE_SIZE - 1);
> - if (partial_page > 0)
> - amount = min(amount,
> - (unsigned int) PAGE_CACHE_SIZE - partial_page);
>
> - if (amount == 0) {
> + /* Beyond the end of the backing file? */
> + if (usb_offset >= curlun->file_length) {
> get_some_more = 0;
> curlun->sense_data =
> SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE;
> - curlun->sense_data_info = usb_offset >> 9;
> + curlun->sense_data_info =
> + curlun->file_length >> 9;
> curlun->info_valid = 1;
> continue;
> }
> - amount -= (amount & 511);
> - if (amount == 0) {
> -
> - /* Why were we were asked to transfer a
> - * partial block? */
> - get_some_more = 0;
> - continue;
> - }
>
> /* Get the next buffer */
> usb_offset += amount;
> @@ -1333,10 +1311,12 @@ static int do_write(struct fsg_dev *fsg)
> if (amount_left_to_req == 0)
> get_some_more = 0;
>
> - /* amount is always divisible by 512, hence by
> - * the bulk-out maxpacket size */
> + /* Except at the end of the transfer, amount will be
> + * equal to the buffer size, which is divisible by
> + * the page size and hence by the bulk-out maxpacket
> + * size.
> + */
> bh->outreq->length = amount;
> - bh->outreq->short_not_ok = 1;
Instead of removing short_not_ok can we use something like following?
bh->outreq->short_not_ok = (amount % maxp) ? 0 : 1;
short_not_ok is useful for example when it is not possible to do DMA
tranfers from usb controller's buffer to the memory for unaligned
lengths.
controller driver can decide to use dma or pio mode based on if short
packets are expected or not.
> start_transfer(fsg, fsg->bulk_out, bh->outreq,
> &bh->outreq_busy, &bh->state);
> fsg->next_buffhd_to_fill = bh->next;
> @@ -1369,6 +1349,11 @@ static int do_write(struct fsg_dev *fsg)
> amount = curlun->file_length - file_offset;
> }
>
> + /* Don't write a partial block */
> + amount -= (amount & 511);
> + if (amount == 0)
> + goto empty_write;
> +
> /* Perform the write */
> file_offset_tmp = file_offset;
> nwritten = vfs_write(curlun->filp,
> @@ -1402,6 +1387,7 @@ static int do_write(struct fsg_dev *fsg)
> break;
> }
>
> + empty_write:
> /* Did the host decide to stop early? */
> if (bh->outreq->actual != bh->outreq->length) {
> fsg->short_packet_received = 1;
> @@ -1498,8 +1484,7 @@ static int do_verify(struct fsg_dev *fsg
> * Try to read the remaining amount, but not more than
> * the buffer size.
> * And don't try to read past the end of the file.
> - * If this means reading 0 then we were asked to read
> - * past the end of file. */
> + */
> amount = min((unsigned int) amount_left, mod_data.buflen);
> amount = min((loff_t) amount,
> curlun->file_length - file_offset);
> @@ -1961,11 +1946,7 @@ static int throw_away_data(struct fsg_de
> if (bh->state == BUF_STATE_EMPTY && fsg->usb_amount_left > 0) {
> amount = min(fsg->usb_amount_left,
> (u32) mod_data.buflen);
> -
> - /* amount is always divisible by 512, hence by
> - * the bulk-out maxpacket size */
> bh->outreq->length = amount;
> - bh->outreq->short_not_ok = 1;
Same as above.
> start_transfer(fsg, fsg->bulk_out, bh->outreq,
> &bh->outreq_busy, &bh->state);
> fsg->next_buffhd_to_fill = bh->next;
> @@ -2644,7 +2625,6 @@ static int get_next_command(struct fsg_d
>
> /* Queue a request to read a Bulk-only CBW */
> bh->outreq->length = USB_BULK_CB_WRAP_LEN;
> - bh->outreq->short_not_ok = 0;
> start_transfer(fsg, fsg->bulk_out, bh->outreq,
> &bh->outreq_busy, &bh->state);
>
> @@ -2778,7 +2758,6 @@ reset:
> if ((rc = enable_endpoint(fsg, fsg->bulk_out, d)) != 0)
> goto reset;
> fsg->bulk_out_enabled = 1;
> - fsg->bulk_out_maxpacket = le16_to_cpu(d->wMaxPacketSize);
> clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);
>
> if (transport_is_cbi()) {
>
> --
> 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
>
Thanks,
--
Mian Yousaf Kaukab
--
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
