Skip to content

Commit

Permalink
usb: gadget: storage: make FSG_NUM_BUFFERS variable size
Browse files Browse the repository at this point in the history 
FSG_NUM_BUFFERS is set to 2 as default.
Usually 2 buffers are enough to establish a good buffering pipeline.
The number may be increased in order to compensate a for bursty VFS
behaviour.

Here follows a description of system that may require more than
2 buffers.
 * CPU ondemand governor active
 * latency cost for wake up and/or frequency change
 * DMA for IO

Use case description.
 * Data transfer from MMC via VFS to USB.
 * DMA shuffles data from MMC and to USB.
 * The CPU wakes up every now and then to pass data in and out from VFS,
   which cause the bursty VFS behaviour.

Test set up
 * Running dd on the host reading from the mass storage device
 * cmdline: dd if=/dev/sdb of=/dev/null bs=4k count=$((256*100))
 * Caches are dropped on the host and on the device before each run

Measurements on a Snowball board with ondemand_governor active.

FSG_NUM_BUFFERS 2
104857600 bytes (105 MB) copied, 5.62173 s, 18.7 MB/s
104857600 bytes (105 MB) copied, 5.61811 s, 18.7 MB/s
104857600 bytes (105 MB) copied, 5.57817 s, 18.8 MB/s

FSG_NUM_BUFFERS 4
104857600 bytes (105 MB) copied, 5.26839 s, 19.9 MB/s
104857600 bytes (105 MB) copied, 5.2691 s, 19.9 MB/s
104857600 bytes (105 MB) copied, 5.2711 s, 19.9 MB/s

There may not be one optimal number for all boards. This is why
the number is added to Kconfig. If selecting USB_GADGET_DEBUG_FILES
this value may be set by a module parameter as well.

Signed-off-by: Per Forlin <per.forlin@linaro.org>
Acked-by: Michal Nazarewicz <mina86@mina86.com>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Felipe Balbi <balbi@ti.com>
  • Loading branch information
Per Forlin authored and Felipe Balbi committed Sep 9, 2011
1 parent 04eee25 commit 6532c7f
Show file tree
Hide file tree
Showing 4 changed files with 85 additions and 20 deletions.
16 changes: 16 additions & 0 deletions drivers/usb/gadget/Kconfig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,22 @@ config USB_GADGET_VBUS_DRAW
This value will be used except for system-specific gadget
drivers that have more specific information.

config USB_GADGET_STORAGE_NUM_BUFFERS
int "Number of storage pipeline buffers"
range 2 4
default 2
help
Usually 2 buffers are enough to establish a good buffering
pipeline. The number may be increased in order to compensate
for a bursty VFS behaviour. For instance there may be CPU wake up
latencies that makes the VFS to appear bursty in a system with
an CPU on-demand governor. Especially if DMA is doing IO to
offload the CPU. In this case the CPU will go into power
save often and spin up occasionally to move data within VFS.
If selecting USB_GADGET_DEBUG_FILES this value may be set by
a module parameter as well.
If unsure, say 2.

#
# USB Peripheral Controller Support
#
Expand Down
29 changes: 21 additions & 8 deletions drivers/usb/gadget/f_mass_storage.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ struct fsg_common {

struct fsg_buffhd *next_buffhd_to_fill;
struct fsg_buffhd *next_buffhd_to_drain;
struct fsg_buffhd buffhds[FSG_NUM_BUFFERS];
struct fsg_buffhd *buffhds;

int cmnd_size;
u8 cmnd[MAX_COMMAND_SIZE];
Expand Down Expand Up @@ -2340,7 +2340,7 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg)
if (common->fsg) {
fsg = common->fsg;

for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
struct fsg_buffhd *bh = &common->buffhds[i];

if (bh->inreq) {
Expand Down Expand Up @@ -2397,7 +2397,7 @@ static int do_set_interface(struct fsg_common *common, struct fsg_dev *new_fsg)
clear_bit(IGNORE_BULK_OUT, &fsg->atomic_bitflags);

/* Allocate the requests */
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
struct fsg_buffhd *bh = &common->buffhds[i];

rc = alloc_request(common, fsg->bulk_in, &bh->inreq);
Expand Down Expand Up @@ -2466,7 +2466,7 @@ static void handle_exception(struct fsg_common *common)

/* Cancel all the pending transfers */
if (likely(common->fsg)) {
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &common->buffhds[i];
if (bh->inreq_busy)
usb_ep_dequeue(common->fsg->bulk_in, bh->inreq);
Expand All @@ -2478,7 +2478,7 @@ static void handle_exception(struct fsg_common *common)
/* Wait until everything is idle */
for (;;) {
int num_active = 0;
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &common->buffhds[i];
num_active += bh->inreq_busy + bh->outreq_busy;
}
Expand All @@ -2501,7 +2501,7 @@ static void handle_exception(struct fsg_common *common)
*/
spin_lock_irq(&common->lock);

for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &common->buffhds[i];
bh->state = BUF_STATE_EMPTY;
}
Expand Down Expand Up @@ -2710,6 +2710,10 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
int nluns, i, rc;
char *pathbuf;

rc = fsg_num_buffers_validate();
if (rc != 0)
return ERR_PTR(rc);

/* Find out how many LUNs there should be */
nluns = cfg->nluns;
if (nluns < 1 || nluns > FSG_MAX_LUNS) {
Expand All @@ -2728,6 +2732,14 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,
common->free_storage_on_release = 0;
}

common->buffhds = kcalloc(fsg_num_buffers,
sizeof *(common->buffhds), GFP_KERNEL);
if (!common->buffhds) {
if (common->free_storage_on_release)
kfree(common);
return ERR_PTR(-ENOMEM);
}

common->ops = cfg->ops;
common->private_data = cfg->private_data;

Expand Down Expand Up @@ -2805,7 +2817,7 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common,

/* Data buffers cyclic list */
bh = common->buffhds;
i = FSG_NUM_BUFFERS;
i = fsg_num_buffers;
goto buffhds_first_it;
do {
bh->next = bh + 1;
Expand Down Expand Up @@ -2931,12 +2943,13 @@ static void fsg_common_release(struct kref *ref)

{
struct fsg_buffhd *bh = common->buffhds;
unsigned i = FSG_NUM_BUFFERS;
unsigned i = fsg_num_buffers;
do {
kfree(bh->buf);
} while (++bh, --i);
}

kfree(common->buffhds);
if (common->free_storage_on_release)
kfree(common);
}
Expand Down
27 changes: 17 additions & 10 deletions drivers/usb/gadget/file_storage.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,6 @@ struct fsg_dev {

struct fsg_buffhd *next_buffhd_to_fill;
struct fsg_buffhd *next_buffhd_to_drain;
struct fsg_buffhd buffhds[FSG_NUM_BUFFERS];

int thread_wakeup_needed;
struct completion thread_notifier;
Expand All @@ -487,6 +486,8 @@ struct fsg_dev {
unsigned int nluns;
struct fsg_lun *luns;
struct fsg_lun *curlun;
/* Must be the last entry */
struct fsg_buffhd buffhds[];
};

typedef void (*fsg_routine_t)(struct fsg_dev *);
Expand Down Expand Up @@ -2737,7 +2738,7 @@ static int do_set_interface(struct fsg_dev *fsg, int altsetting)

reset:
/* Deallocate the requests */
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
struct fsg_buffhd *bh = &fsg->buffhds[i];

if (bh->inreq) {
Expand Down Expand Up @@ -2798,7 +2799,7 @@ static int do_set_interface(struct fsg_dev *fsg, int altsetting)
}

/* Allocate the requests */
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
struct fsg_buffhd *bh = &fsg->buffhds[i];

if ((rc = alloc_request(fsg, fsg->bulk_in, &bh->inreq)) != 0)
Expand Down Expand Up @@ -2894,7 +2895,7 @@ static void handle_exception(struct fsg_dev *fsg)
/* Cancel all the pending transfers */
if (fsg->intreq_busy)
usb_ep_dequeue(fsg->intr_in, fsg->intreq);
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &fsg->buffhds[i];
if (bh->inreq_busy)
usb_ep_dequeue(fsg->bulk_in, bh->inreq);
Expand All @@ -2905,7 +2906,7 @@ static void handle_exception(struct fsg_dev *fsg)
/* Wait until everything is idle */
for (;;) {
num_active = fsg->intreq_busy;
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &fsg->buffhds[i];
num_active += bh->inreq_busy + bh->outreq_busy;
}
Expand All @@ -2927,7 +2928,7 @@ static void handle_exception(struct fsg_dev *fsg)
* state, and the exception. Then invoke the handler. */
spin_lock_irq(&fsg->lock);

for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
bh = &fsg->buffhds[i];
bh->state = BUF_STATE_EMPTY;
}
Expand Down Expand Up @@ -3157,7 +3158,7 @@ static void /* __init_or_exit */ fsg_unbind(struct usb_gadget *gadget)
}

/* Free the data buffers */
for (i = 0; i < FSG_NUM_BUFFERS; ++i)
for (i = 0; i < fsg_num_buffers; ++i)
kfree(fsg->buffhds[i].buf);

/* Free the request and buffer for endpoint 0 */
Expand Down Expand Up @@ -3445,7 +3446,7 @@ static int __init fsg_bind(struct usb_gadget *gadget)
req->complete = ep0_complete;

/* Allocate the data buffers */
for (i = 0; i < FSG_NUM_BUFFERS; ++i) {
for (i = 0; i < fsg_num_buffers; ++i) {
struct fsg_buffhd *bh = &fsg->buffhds[i];

/* Allocate for the bulk-in endpoint. We assume that
Expand All @@ -3456,7 +3457,7 @@ static int __init fsg_bind(struct usb_gadget *gadget)
goto out;
bh->next = bh + 1;
}
fsg->buffhds[FSG_NUM_BUFFERS - 1].next = &fsg->buffhds[0];
fsg->buffhds[fsg_num_buffers - 1].next = &fsg->buffhds[0];

/* This should reflect the actual gadget power source */
usb_gadget_set_selfpowered(gadget);
Expand Down Expand Up @@ -3572,7 +3573,9 @@ static int __init fsg_alloc(void)
{
struct fsg_dev *fsg;

fsg = kzalloc(sizeof *fsg, GFP_KERNEL);
fsg = kzalloc(sizeof *fsg +
fsg_num_buffers * sizeof *(fsg->buffhds), GFP_KERNEL);

if (!fsg)
return -ENOMEM;
spin_lock_init(&fsg->lock);
Expand All @@ -3590,6 +3593,10 @@ static int __init fsg_init(void)
int rc;
struct fsg_dev *fsg;

rc = fsg_num_buffers_validate();
if (rc != 0)
return rc;

if ((rc = fsg_alloc()) != 0)
return rc;
fsg = the_fsg;
Expand Down
33 changes: 31 additions & 2 deletions drivers/usb/gadget/storage_common.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,12 @@
* characters rather then a pointer to void.
*/

/*
* When USB_GADGET_DEBUG_FILES is defined the module param num_buffers
* sets the number of pipeline buffers (length of the fsg_buffhd array).
* The valid range of num_buffers is: num >= 2 && num <= 4.
*/


#include <linux/usb/storage.h>
#include <scsi/scsi.h>
Expand Down Expand Up @@ -264,8 +270,31 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev)
#define EP0_BUFSIZE 256
#define DELAYED_STATUS (EP0_BUFSIZE + 999) /* An impossibly large value */

/* Number of buffers we will use. 2 is enough for double-buffering */
#define FSG_NUM_BUFFERS 2
#ifdef CONFIG_USB_GADGET_DEBUG_FILES

static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS;
module_param_named(num_buffers, fsg_num_buffers, uint, S_IRUGO);
MODULE_PARM_DESC(num_buffers, "Number of pipeline buffers");

#else

/*
* Number of buffers we will use.
* 2 is usually enough for good buffering pipeline
*/
#define fsg_num_buffers CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS

#endif /* CONFIG_USB_DEBUG */

/* check if fsg_num_buffers is within a valid range */
static inline int fsg_num_buffers_validate(void)
{
if (fsg_num_buffers >= 2 && fsg_num_buffers <= 4)
return 0;
pr_err("fsg_num_buffers %u is out of range (%d to %d)\n",
fsg_num_buffers, 2 ,4);
return -EINVAL;
}

/* Default size of buffer length. */
#define FSG_BUFLEN ((u32)16384)
Expand Down

0 comments on commit 6532c7f

Please sign in to comment.