On Tue, 5 Apr 2011, Felipe Balbi wrote:
> > Of course, that's not long enough. With SuperSpeed, the buffer size
> > should be increased to at least 64 KB for optimal throughput.
>
> I think 64kb is still too small, we will get Interrupt on every 164 uS.
> I think it's better to either start support sglists and allocate more
> space, or have many different 16kb buffers and set no_interrupt
> whenever necessary
Either one is doable. But again, I have to wonder if it is worth the
effort.
> > Is it really worthwhile to worry about this? With USB-3 you probably
> > want to run a UAS gadget instead of g_file_storage anyway.
>
> I can't be sure what customers will do :-) Of course I can always
> suggest to use UAS as that's definitely better.
If they want to use g_file_storage in a SuperSpeed gadget, it will
still work. The throughput will just be lower than it could be with
UAS.
> cool, thanks for the fruitful discussion Alan. I'll wait until you send
> your patch and will rebase mine on top of that :-) Thanks
And here it is. This patch is _not_ based on Mian's. It also includes
a separate patch that has been submitted to Greg but not yet merged;
you should be able to apply it with little trouble.
It bumps bulk-OUT transfers up to the next maxpacket boundary, and it
removes the assumption that the maxpacket size divides the block size.
These are more-or-less independent changes, so it will have to be
broken up into several patches for submission. But you can test it
now.
Alan Stern
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
@@ -1130,7 +1130,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
@@ -1165,17 +1164,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;
@@ -1220,6 +1212,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;
@@ -1256,7 +1253,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;
@@ -1307,38 +1303,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;
@@ -1347,11 +1326,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 */
- bh->outreq->length = bh->bulk_out_intended_length =
- amount;
- bh->outreq->short_not_ok = 1;
+ /* 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.
+ */
+ set_bulk_out_req_length(fsg, bh, amount);
start_transfer(fsg, fsg->bulk_out, bh->outreq,
&bh->outreq_busy, &bh->state);
fsg->next_buffhd_to_fill = bh->next;
@@ -1384,6 +1364,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,
@@ -1417,8 +1402,9 @@ 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) {
+ if (bh->outreq->actual < bh->bulk_out_intended_length) {
fsg->short_packet_received = 1;
break;
}
@@ -1513,8 +1499,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);
@@ -1947,37 +1932,6 @@ static int wedge_bulk_in_endpoint(struct
return rc;
}
-static int pad_with_zeros(struct fsg_dev *fsg)
-{
- struct fsg_buffhd *bh = fsg->next_buffhd_to_fill;
- u32 nkeep = bh->inreq->length;
- u32 nsend;
- int rc;
-
- bh->state = BUF_STATE_EMPTY; // For the first iteration
- fsg->usb_amount_left = nkeep + fsg->residue;
- while (fsg->usb_amount_left > 0) {
-
- /* Wait for the next buffer to be free */
- while (bh->state != BUF_STATE_EMPTY) {
- rc = sleep_thread(fsg);
- if (rc)
- return rc;
- }
-
- nsend = min(fsg->usb_amount_left, (u32) mod_data.buflen);
- memset(bh->buf + nkeep, 0, nsend - nkeep);
- bh->inreq->length = nsend;
- bh->inreq->zero = 0;
- start_transfer(fsg, fsg->bulk_in, bh->inreq,
- &bh->inreq_busy, &bh->state);
- bh = fsg->next_buffhd_to_fill = bh->next;
- fsg->usb_amount_left -= nsend;
- nkeep = 0;
- }
- return 0;
-}
-
static int throw_away_data(struct fsg_dev *fsg)
{
struct fsg_buffhd *bh;
@@ -1994,7 +1948,7 @@ static int throw_away_data(struct fsg_de
fsg->next_buffhd_to_drain = bh->next;
/* A short packet or an error ends everything */
- if (bh->outreq->actual != bh->outreq->length ||
+ if (bh->outreq->actual < bh->bulk_out_intended_length ||
bh->outreq->status != 0) {
raise_exception(fsg, FSG_STATE_ABORT_BULK_OUT);
return -EINTR;
@@ -2007,12 +1961,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 = bh->bulk_out_intended_length =
- amount;
- bh->outreq->short_not_ok = 1;
+ set_bulk_out_req_length(fsg, bh, amount);
start_transfer(fsg, fsg->bulk_out, bh->outreq,
&bh->outreq_busy, &bh->state);
fsg->next_buffhd_to_fill = bh->next;
@@ -2082,18 +2031,20 @@ static int finish_reply(struct fsg_dev *
}
}
- /* For Bulk-only, if we're allowed to stall then send the
- * short packet and halt the bulk-in endpoint. If we can't
- * stall, pad out the remaining data with 0's. */
+ /*
+ * For Bulk-only, mark the end of the data with a short
+ * packet. If we are allowed to stall, halt the bulk-in
+ * endpoint. (Note: This violates the Bulk-Only Transport
+ * specification, which requires us to pad the data if we
+ * don't halt the endpoint. Presumably nobody will mind.)
+ */
else {
- if (mod_data.can_stall) {
- bh->inreq->zero = 1;
- start_transfer(fsg, fsg->bulk_in, bh->inreq,
- &bh->inreq_busy, &bh->state);
- fsg->next_buffhd_to_fill = bh->next;
+ bh->inreq->zero = 1;
+ start_transfer(fsg, fsg->bulk_in, bh->inreq,
+ &bh->inreq_busy, &bh->state);
+ fsg->next_buffhd_to_fill = bh->next;
+ if (mod_data.can_stall)
rc = halt_bulk_in_endpoint(fsg);
- } else
- rc = pad_with_zeros(fsg);
}
break;
@@ -2689,7 +2640,6 @@ static int get_next_command(struct fsg_d
/* Queue a request to read a Bulk-only CBW */
set_bulk_out_req_length(fsg, bh, USB_BULK_CB_WRAP_LEN);
- bh->outreq->short_not_ok = 1;
start_transfer(fsg, fsg->bulk_out, bh->outreq,
&bh->outreq_busy, &bh->state);
--
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
