[LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[Manish Narani]

This patch adds support to configure bulk maxburst through
module parameter. This parameter can be used to modify bulk
maxburst in case if one wants to measure peak Bulk/Isoc-IN/OUT
performance.

Signed-off-by: Manish Narani <mnarani@xilinx.com>
---
 drivers/usb/gadget/function/f_sourcesink.c |   14 ++++++++++++++
 drivers/usb/gadget/function/g_zero.h       |    2 ++
 drivers/usb/gadget/legacy/zero.c           |    5 +++++
 3 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
index df0189d..81274ba 100644
--- a/drivers/usb/gadget/function/f_sourcesink.c
+++ b/drivers/usb/gadget/function/f_sourcesink.c
@@ -49,6 +49,7 @@ struct f_sourcesink {
 	unsigned isoc_maxpacket;
 	unsigned isoc_mult;
 	unsigned isoc_maxburst;
+	unsigned bulk_maxburst;
 	unsigned buflen;
 	unsigned bulk_qlen;
 	unsigned iso_qlen;
@@ -334,6 +335,10 @@ sourcesink_bind(struct usb_configuration *c, struct usb_function *f)
 	source_sink_intf_alt0.bInterfaceNumber = id;
 	source_sink_intf_alt1.bInterfaceNumber = id;
 
+	/* sanity check the bulk module parameters */
+	if (ss->bulk_maxburst > 15)
+		ss->bulk_maxburst = 15;
+
 	/* allocate bulk endpoints */
 	ss->in_ep = usb_ep_autoconfig(cdev->gadget, &fs_source_desc);
 	if (!ss->in_ep) {
@@ -347,6 +352,14 @@ autoconf_fail:
 	if (!ss->out_ep)
 		goto autoconf_fail;
 
+	/*
+	 * Fill in the SS bulk descriptors from the module parameters.
+	 * We assume that the user knows what they are doing and won't
+	 * give parameters that their UDC doesn't support.
+	 */
+	ss_source_comp_desc.bMaxBurst = ss->bulk_maxburst;
+	ss_sink_comp_desc.bMaxBurst = ss->bulk_maxburst;
+
 	/* sanity check the isoc module parameters */
 	if (ss->isoc_interval < 1)
 		ss->isoc_interval = 1;
@@ -857,6 +870,7 @@ static struct usb_function *source_sink_alloc_func(
 	ss->isoc_maxpacket = ss_opts->isoc_maxpacket;
 	ss->isoc_mult = ss_opts->isoc_mult;
 	ss->isoc_maxburst = ss_opts->isoc_maxburst;
+	ss->bulk_maxburst = ss_opts->bulk_maxburst;
 	ss->buflen = ss_opts->bulk_buflen;
 	ss->bulk_qlen = ss_opts->bulk_qlen;
 	ss->iso_qlen = ss_opts->iso_qlen;
diff --git a/drivers/usb/gadget/function/g_zero.h b/drivers/usb/gadget/function/g_zero.h
index 492924d..b3234e7 100644
--- a/drivers/usb/gadget/function/g_zero.h
+++ b/drivers/usb/gadget/function/g_zero.h
@@ -19,6 +19,7 @@ struct usb_zero_options {
 	unsigned isoc_maxpacket;
 	unsigned isoc_mult;
 	unsigned isoc_maxburst;
+	unsigned bulk_maxburst;
 	unsigned bulk_buflen;
 	unsigned qlen;
 	unsigned ss_bulk_qlen;
@@ -32,6 +33,7 @@ struct f_ss_opts {
 	unsigned isoc_maxpacket;
 	unsigned isoc_mult;
 	unsigned isoc_maxburst;
+	unsigned bulk_maxburst;
 	unsigned bulk_buflen;
 	unsigned bulk_qlen;
 	unsigned iso_qlen;
diff --git a/drivers/usb/gadget/legacy/zero.c b/drivers/usb/gadget/legacy/zero.c
index d02e2ce..c88f5e0 100644
--- a/drivers/usb/gadget/legacy/zero.c
+++ b/drivers/usb/gadget/legacy/zero.c
@@ -247,6 +247,10 @@ MODULE_PARM_DESC(isoc_maxpacket, "0 - 1023 (fs), 0 - 1024 (hs/ss)");
 module_param_named(isoc_mult, gzero_options.isoc_mult, uint, S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(isoc_mult, "0 - 2 (hs/ss only)");
 
+module_param_named(bulk_maxburst, gzero_options.bulk_maxburst, uint,
+		S_IRUGO|S_IWUSR);
+MODULE_PARM_DESC(bulk_maxburst, "0 - 15 (ss only)");
+
 module_param_named(isoc_maxburst, gzero_options.isoc_maxburst, uint,
 		S_IRUGO|S_IWUSR);
 MODULE_PARM_DESC(isoc_maxburst, "0 - 15 (ss only)");
@@ -294,6 +298,7 @@ static int zero_bind(struct usb_composite_dev *cdev)
 	ss_opts->isoc_maxpacket = gzero_options.isoc_maxpacket;
 	ss_opts->isoc_mult = gzero_options.isoc_mult;
 	ss_opts->isoc_maxburst = gzero_options.isoc_maxburst;
+	ss_opts->bulk_maxburst = gzero_options.bulk_maxburst;
 	ss_opts->bulk_buflen = gzero_options.bulk_buflen;
 	ss_opts->bulk_qlen = gzero_options.ss_bulk_qlen;
 	ss_opts->iso_qlen = gzero_options.ss_iso_qlen;
-- 
1.7.1

Re: [LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[Greg KH]

On Thu, Sep 29, 2016 at 02:16:44PM +0530, Manish Narani wrote:
> This patch adds support to configure bulk maxburst through
> module parameter. This parameter can be used to modify bulk
> maxburst in case if one wants to measure peak Bulk/Isoc-IN/OUT
> performance.

Eeek, this isn't the 1990's, please don't add new module parameters :)

We have much better ways of handling configuration options for a device,
why not use them (configfs, sysfs files, etc.)?

Ugh, it looks like iso_maxburst is handled this way as well.

Felipe, why is this?

thanks,

greg k-h

Re: [LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]


Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Thu, Sep 29, 2016 at 02:16:44PM +0530, Manish Narani wrote:
>> This patch adds support to configure bulk maxburst through
>> module parameter. This parameter can be used to modify bulk
>> maxburst in case if one wants to measure peak Bulk/Isoc-IN/OUT
>> performance.
>
> Eeek, this isn't the 1990's, please don't add new module parameters :)
>
> We have much better ways of handling configuration options for a device,
> why not use them (configfs, sysfs files, etc.)?
>
> Ugh, it looks like iso_maxburst is handled this way as well.
>
> Felipe, why is this?

Yeah, g_zero has a few module parameters. In fact many of the gadget
drivers have them. This is all legacy stuff, pre-configfs. We can't
simply remove the legacy gadget drivers and tell everybody to start
using configfs ;-) That's what we want, no doubt.

That being said, we're not taking any new module parameters to any of
the gadget drivers OR UDC drivers. Anything new is configfs-only. Legacy
gadgets only get bug fixes.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

RE: [LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[Manish Narani]

Hi Greg,

Thanks for the suggestion. I will fix it and send you in next version. :)

Regards,
Manish

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org]
Sent: Thursday, September 29, 2016 2:24 PM
To: Manish Narani <MNARANI@xilinx.com>
Cc: balbi@kernel.org; k.opasiak@samsung.com; r.baldyga@samsung.com; peter.chen@freescale.com; Manish Narani <MNARANI@xilinx.com>; John.Youn@synopsys.com; eu@felipetonello.com; i.kotrasinsk@samsung.com; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; Anurag Kumar Vulisha <anuragku@xilinx.com>; Punnaiah Choudary Kalluri <punnaia@xilinx.com>
Subject: Re: [LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

On Thu, Sep 29, 2016 at 02:16:44PM +0530, Manish Narani wrote:
> This patch adds support to configure bulk maxburst through module
> parameter. This parameter can be used to modify bulk maxburst in case
> if one wants to measure peak Bulk/Isoc-IN/OUT performance.

Eeek, this isn't the 1990's, please don't add new module parameters :)

We have much better ways of handling configuration options for a device, why not use them (configfs, sysfs files, etc.)?

Ugh, it looks like iso_maxburst is handled this way as well.

Felipe, why is this?

thanks,

greg k-h


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [LINUX PATCH] usb: gadget: Configure bulk maxburst through module parameter in gadget zero.

[Krzysztof Opasiak]

On 09/29/2016 10:46 AM, Manish Narani wrote:
> This patch adds support to configure bulk maxburst through
> module parameter. This parameter can be used to modify bulk
> maxburst in case if one wants to measure peak Bulk/Isoc-IN/OUT
> performance.
> 

If you would like such option to f_sourcesink then you should probably
do this as configfs attribute not only g_zero module param.

Best regards,
-- 
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics