From patchwork Thu Aug 18 09:28:46 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Per Forlin X-Patchwork-Id: 3503 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 6E3A323F41 for ; Thu, 18 Aug 2011 09:30:15 +0000 (UTC) Received: from mail-ew0-f52.google.com (mail-ew0-f52.google.com [209.85.215.52]) by fiordland.canonical.com (Postfix) with ESMTP id 5BC7CA1806D for ; Thu, 18 Aug 2011 09:30:15 +0000 (UTC) Received: by ewy28 with SMTP id 28so958244ewy.11 for ; Thu, 18 Aug 2011 02:30:15 -0700 (PDT) Received: by 10.213.3.155 with SMTP id 27mr1625713ebn.18.1313659815057; Thu, 18 Aug 2011 02:30:15 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.213.102.5 with SMTP id e5cs63148ebo; Thu, 18 Aug 2011 02:30:14 -0700 (PDT) Received: by 10.213.111.4 with SMTP id q4mr114920ebp.113.1313659814527; Thu, 18 Aug 2011 02:30:14 -0700 (PDT) Received: from eu1sys200aog104.obsmtp.com (eu1sys200aog104.obsmtp.com [207.126.144.117]) by mx.google.com with SMTP id s5si738344eef.27.2011.08.18.02.29.34 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 18 Aug 2011 02:30:14 -0700 (PDT) Received-SPF: neutral (google.com: 207.126.144.117 is neither permitted nor denied by best guess record for domain of per.forlin@stericsson.com) client-ip=207.126.144.117; Authentication-Results: mx.google.com; spf=neutral (google.com: 207.126.144.117 is neither permitted nor denied by best guess record for domain of per.forlin@stericsson.com) smtp.mail=per.forlin@stericsson.com Received: from beta.dmz-us.st.com ([167.4.1.35]) (using TLSv1) by eu1sys200aob104.postini.com ([207.126.147.11]) with SMTP ID DSNKTkzbfqnOgyvXQlpu9UNBVmmJ37J3SX7O@postini.com; Thu, 18 Aug 2011 09:30:14 UTC Received: from zeta.dmz-us.st.com (ns4.st.com [167.4.16.71]) by beta.dmz-us.st.com (STMicroelectronics) with ESMTP id B58E94E; Thu, 18 Aug 2011 09:29:21 +0000 (GMT) Received: from relay2.stm.gmessaging.net (unknown [10.230.100.18]) by zeta.dmz-us.st.com (STMicroelectronics) with ESMTP id D0A444A; Thu, 18 Aug 2011 09:29:20 +0000 (GMT) Received: from exdcvycastm003.EQ1STM.local (alteon-source-exch [10.230.100.61]) (using TLSv1 with cipher RC4-MD5 (128/128 bits)) (Client CN "exdcvycastm003", Issuer "exdcvycastm003" (not verified)) by relay2.stm.gmessaging.net (Postfix) with ESMTPS id 680D5A807E; Thu, 18 Aug 2011 11:29:16 +0200 (CEST) Received: from localhost.localdomain (10.230.100.153) by smtp.stericsson.com (10.230.100.1) with Microsoft SMTP Server (TLS) id 8.3.83.0; Thu, 18 Aug 2011 11:29:20 +0200 From: Per Forlin To: Felipe Balbi , Greg Kroah-Hartman , Michal Nazarewicz Cc: , , , Per Forlin Subject: [PATCH v4] usb: gadget: storage_common: make FSG_NUM_BUFFERS variable size Date: Thu, 18 Aug 2011 11:28:46 +0200 Message-ID: <1313659726-9406-1-git-send-email-per.forlin@stericsson.com> X-Mailer: git-send-email 1.6.3.3 MIME-Version: 1.0 From: Per Forlin 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_govenor 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_DEBUG this value may be set by a module parameter as well. Signed-off-by: Per Forlin --- Change log. v2: Update after proofreading comments from Michal Nazarewicz v3: Clarify the description of this patch based on input from Alan Stern v4: - Introduce a module_param to set number of pipeline buffers if USB_DEBUG is set. In order to add this support fsg_common is allocated at runtime. The fsg_buffhd list size is appended to fsg_dev and fsg_common at runtime allocation. - The previous acks from Michal and Alan on v3 are not applicable for this version since it's a new implementation. drivers/usb/gadget/Kconfig | 16 ++++++++++++++++ drivers/usb/gadget/f_mass_storage.c | 10 +++++++--- drivers/usb/gadget/file_storage.c | 10 ++++++++-- drivers/usb/gadget/mass_storage.c | 16 +++++++++------- drivers/usb/gadget/storage_common.c | 20 ++++++++++++++++++-- 5 files changed, 58 insertions(+), 14 deletions(-) diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 44b6b40..37ec2ce 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -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_DEBUG this value may be set by a module + parameter as well. + If unsure, say 2. + # # USB Peripheral Controller Support # diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 5b93395..3e546d9 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -363,7 +363,6 @@ struct fsg_common { struct fsg_buffhd *next_buffhd_to_fill; struct fsg_buffhd *next_buffhd_to_drain; - struct fsg_buffhd buffhds[FSG_NUM_BUFFERS]; int cmnd_size; u8 cmnd[MAX_COMMAND_SIZE]; @@ -407,6 +406,8 @@ struct fsg_common { char inquiry_string[8 + 16 + 4 + 1]; struct kref ref; + /* Must be the last entry */ + struct fsg_buffhd buffhds[0]; }; struct fsg_config { @@ -2728,12 +2729,15 @@ static struct fsg_common *fsg_common_init(struct fsg_common *common, /* Allocate? */ if (!common) { - common = kzalloc(sizeof *common, GFP_KERNEL); + common = kzalloc(sizeof(struct fsg_common) + + sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS, + GFP_KERNEL); if (!common) return ERR_PTR(-ENOMEM); common->free_storage_on_release = 1; } else { - memset(common, 0, sizeof *common); + memset(common, 0, sizeof(struct fsg_common) + + sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS); common->free_storage_on_release = 0; } diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c index 639e14a..21d366d 100644 --- a/drivers/usb/gadget/file_storage.c +++ b/drivers/usb/gadget/file_storage.c @@ -461,7 +461,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; @@ -488,6 +487,8 @@ struct fsg_dev { unsigned int nluns; struct fsg_lun *luns; struct fsg_lun *curlun; + /* Must be the last entry */ + struct fsg_buffhd buffhds[0]; }; typedef void (*fsg_routine_t)(struct fsg_dev *); @@ -3587,7 +3588,9 @@ static int __init fsg_alloc(void) { struct fsg_dev *fsg; - fsg = kzalloc(sizeof *fsg, GFP_KERNEL); + fsg = kzalloc(sizeof(struct fsg_dev) + + sizeof(struct fsg_buffhd) * FSG_NUM_BUFFERS, GFP_KERNEL); + if (!fsg) return -ENOMEM; spin_lock_init(&fsg->lock); @@ -3605,6 +3608,9 @@ static int __init fsg_init(void) int rc; struct fsg_dev *fsg; + if (!FSG_NUM_BUFFERS_IS_VALID(FSG_NUM_BUFFERS)) + return -EINVAL; + if ((rc = fsg_alloc()) != 0) return rc; fsg = the_fsg; diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c index d3eb274..67c58d0 100644 --- a/drivers/usb/gadget/mass_storage.c +++ b/drivers/usb/gadget/mass_storage.c @@ -116,9 +116,8 @@ static int __init msg_do_config(struct usb_configuration *c) static const struct fsg_operations ops = { .thread_exits = msg_thread_exits, }; - static struct fsg_common common; + struct fsg_common *common = NULL; - struct fsg_common *retp; struct fsg_config config; int ret; @@ -130,12 +129,12 @@ static int __init msg_do_config(struct usb_configuration *c) fsg_config_from_params(&config, &mod_data); config.ops = &ops; - retp = fsg_common_init(&common, c->cdev, &config); - if (IS_ERR(retp)) - return PTR_ERR(retp); + common = fsg_common_init(common, c->cdev, &config); + if (IS_ERR(common)) + return PTR_ERR(common); - ret = fsg_bind_config(c->cdev, c, &common); - fsg_common_put(&common); + ret = fsg_bind_config(c->cdev, c, common); + fsg_common_put(common); return ret; } @@ -179,6 +178,9 @@ MODULE_LICENSE("GPL"); static int __init msg_init(void) { + if (!FSG_NUM_BUFFERS_IS_VALID(FSG_NUM_BUFFERS)) + return -EINVAL; + return usb_composite_probe(&msg_driver, msg_bind); } module_init(msg_init); diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c index d3dd227..197eace 100644 --- a/drivers/usb/gadget/storage_common.c +++ b/drivers/usb/gadget/storage_common.c @@ -262,8 +262,24 @@ 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_DEBUG + +static unsigned int fsg_num_buffers = CONFIG_USB_GADGET_STORAGE_NUM_BUFFERS; +module_param(fsg_num_buffers, uint, S_IRUGO); +MODULE_PARM_DESC(fsg_num_buffers, "Number of pipeline buffers"); +#define FSG_NUM_BUFFERS fsg_num_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 */ + +#define FSG_NUM_BUFFERS_IS_VALID(num) (num >= 2 && num <= 4 ? true : false) /* Default size of buffer length. */ #define FSG_BUFLEN ((u32)16384)