[PATCH 1/2] USB: gadget: mass/file storage: set serial number

[PATCH 1/2] USB: gadget: mass/file storage: set serial number

[Michal Nazarewicz]

This commit adds iSerialNumber to all gadgets that use the Mass
Storage Function.  It appears that Windows has problems when
it is not set.

Not to copy the same code all over again, a fsg_string_serial_fill()
macro has been added to the storage_common.c file which is now also
used in the File Storage Gadget.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Cc: stable <stable@kernel.org>
---
Dries Van Puymbroeck reported problems with Windows when serial was
not set.  This patch fikes this.  As a bug fix, it should get into
the stable.

This particular patch does not apply to the .33.5 nor .34 kernels so
I've also prepered another patch for those versions.

 drivers/usb/gadget/file_storage.c   |   10 +------
 drivers/usb/gadget/mass_storage.c   |   47 +++++++++++++++-------------------
 drivers/usb/gadget/multi.c          |    9 ++++++
 drivers/usb/gadget/storage_common.c |   12 +++++++++
 4 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 7993267..16decb8 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3447,15 +3447,7 @@ static int __ref fsg_bind(struct usb_gadget *gadget)
 			init_utsname()->sysname, init_utsname()->release,
 			gadget->name);
 
-	/* On a real device, serial[] would be loaded from permanent
-	 * storage.  We just encode it from the driver version string. */
-	for (i = 0; i < sizeof fsg_string_serial - 2; i += 2) {
-		unsigned char		c = DRIVER_VERSION[i / 2];
-
-		if (!c)
-			break;
-		sprintf(&fsg_string_serial[i], "%02X", c);
-	}
+	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
 
 	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
 			"file-storage-gadget");
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 585f255..584d2ed 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -45,7 +45,7 @@
 /*-------------------------------------------------------------------------*/
 
 #define DRIVER_DESC		"Mass Storage Gadget"
-#define DRIVER_VERSION		"2009/09/11"
+#define DRIVER_VERSION		"2010/07/01"
 
 /*-------------------------------------------------------------------------*/
 
@@ -72,21 +72,16 @@ static struct usb_device_descriptor msg_device_desc = {
 	.bcdUSB =		cpu_to_le16(0x0200),
 	.bDeviceClass =		USB_CLASS_PER_INTERFACE,
 
-	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(FSG_VENDOR_ID),
 	.idProduct =		cpu_to_le16(FSG_PRODUCT_ID),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
 static struct usb_otg_descriptor otg_descriptor = {
 	.bLength =		sizeof otg_descriptor,
 	.bDescriptorType =	USB_DT_OTG,
 
-	/* REVISIT SRP-only hardware is possible, although
+	/*
+	 * REVISIT SRP-only hardware is possible, although
 	 * it would not be called "OTG" ...
 	 */
 	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
@@ -100,16 +95,21 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_CONFIGURATION_IDX	2
+enum {
+	STRING_MANUFACTURER_IDX,
+	STRING_PRODUCT_IDX,
+	STRING_CONFIGURATION_IDX,
+	STRING_SERIAL_IDX
+};
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
 	[STRING_CONFIGURATION_IDX].s = "Self Powered",
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -173,7 +173,6 @@ static struct usb_configuration msg_config_driver = {
 	.label			= "Linux File-Backed Storage",
 	.bind			= msg_do_config,
 	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
 	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
 };
 
@@ -187,25 +186,21 @@ static int __ref msg_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget *gadget = cdev->gadget;
 	int status;
 
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
+	/* Take care of strings */
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
-	status = usb_string_id(cdev);
-	if (status < 0)
-		return status;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	msg_device_desc.iManufacturer = status;
 
-	status = usb_string_id(cdev);
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
+	status = usb_string_ids_tab(cdev, strings_dev);
 	if (status < 0)
 		return status;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	msg_device_desc.iProduct = status;
+
+	msg_device_desc.iManufacturer =
+		strings_dev[STRING_MANUFACTURER_IDX].id;
+	msg_device_desc.iProduct = strings_dev[STRING_PRODUCT_IDX].id;
+	msg_device_desc.iSerialNumber = strings_dev[STRING_SERIAL_IDX].id;
 
 	status = usb_string_id(cdev);
 	if (status < 0)
@@ -213,7 +208,7 @@ static int __ref msg_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_CONFIGURATION_IDX].id = status;
 	msg_config_driver.iConfiguration = status;
 
-	/* register our second configuration */
+	/* Register our second configuration */
 	status = usb_add_config(cdev, &msg_config_driver);
 	if (status < 0)
 		return status;
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 795d762..c8e83ff 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -36,6 +36,7 @@
 
 
 #define DRIVER_DESC		"Multifunction Composite Gadget"
+#define DRIVER_VERSION		"2010/07/01"
 
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_AUTHOR("Michal Nazarewicz");
@@ -123,6 +124,7 @@ static const struct usb_descriptor_header *otg_desc[] = {
 enum {
 	MULTI_STRING_MANUFACTURER_IDX,
 	MULTI_STRING_PRODUCT_IDX,
+	MULTI_STRING_SERIAL_IDX,
 #ifdef CONFIG_USB_G_MULTI_RNDIS
 	MULTI_STRING_RNDIS_CONFIG_IDX,
 #endif
@@ -132,10 +134,13 @@ enum {
 };
 
 static char manufacturer[50];
+static char serial[13];
+
 
 static struct usb_string strings_dev[] = {
 	[MULTI_STRING_MANUFACTURER_IDX].s = manufacturer,
 	[MULTI_STRING_PRODUCT_IDX].s      = DRIVER_DESC,
+	[MULTI_STRING_SERIAL_IDX].s       = serial,
 #ifdef CONFIG_USB_G_MULTI_RNDIS
 	[MULTI_STRING_RNDIS_CONFIG_IDX].s = "Multifunction with RNDIS",
 #endif
@@ -319,6 +324,8 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
 
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
 	status = usb_string_ids_tab(cdev, strings_dev);
 	if (unlikely(status < 0))
 		goto fail2;
@@ -327,6 +334,8 @@ static int __ref multi_bind(struct usb_composite_dev *cdev)
 		strings_dev[MULTI_STRING_MANUFACTURER_IDX].id;
 	device_desc.iProduct      =
 		strings_dev[MULTI_STRING_PRODUCT_IDX].id;
+	device_desc.iSerialNumber =
+		strings_dev[MULTI_STRING_SERIAL_IDX].id;
 
 	/* register configurations */
 	status = rndis_config_register(cdev);
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 557b119..a808879 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -559,6 +559,18 @@ static struct usb_gadget_strings	fsg_stringtab = {
 };
 
 
+#define fsg_string_serial_fill_n(serial, n, version) do {	\
+		unsigned char *c = version;			\
+		unsigned i = ((n) + 1) / 2;			\
+		char *s = serial;				\
+		for (; *c && --i; s += 2, ++c)			\
+			sprintf(s, "%02X", *c);			\
+	} while (0)
+
+#define fsg_string_serial_fill(serial, version)				\
+	fsg_string_serial_fill_n(serial, ARRAY_SIZE(serial), version)
+
+
  /*-------------------------------------------------------------------------*/
 
 /*
-- 
1.7.1

[PATCH 2/2] USB: gadget: mass/file storage: set serial number

[Michal Nazarewicz]

This commit adds iSerialNumber to all gadgets that use the Mass
Storage Function.  It appears that Windows has problems when
it is not set.

Not to copy the same code all over again, a fsg_string_serial_fill()
macro has been added to the storage_common.c file which is now also
used in the File Storage Gadget.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Cc: stable <stable@kernel.org>
---
This is the version of the "set serial number" patch for older stable
kernels.

 drivers/usb/gadget/file_storage.c   |   10 +-------
 drivers/usb/gadget/mass_storage.c   |   45 +++++++++++++++-------------------
 drivers/usb/gadget/multi.c          |   12 +++++++++
 drivers/usb/gadget/storage_common.c |   12 +++++++++
 4 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 29dfb02..4366af8 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3451,15 +3451,7 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 			init_utsname()->sysname, init_utsname()->release,
 			gadget->name);
 
-	/* On a real device, serial[] would be loaded from permanent
-	 * storage.  We just encode it from the driver version string. */
-	for (i = 0; i < sizeof fsg_string_serial - 2; i += 2) {
-		unsigned char		c = DRIVER_VERSION[i / 2];
-
-		if (!c)
-			break;
-		sprintf(&fsg_string_serial[i], "%02X", c);
-	}
+	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
 
 	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
 			"file-storage-gadget");
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 19619fb..91661c0 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -72,21 +72,16 @@ static struct usb_device_descriptor msg_device_desc = {
 	.bcdUSB =		cpu_to_le16(0x0200),
 	.bDeviceClass =		USB_CLASS_PER_INTERFACE,
 
-	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(FSG_VENDOR_ID),
 	.idProduct =		cpu_to_le16(FSG_PRODUCT_ID),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
 static struct usb_otg_descriptor otg_descriptor = {
 	.bLength =		sizeof otg_descriptor,
 	.bDescriptorType =	USB_DT_OTG,
 
-	/* REVISIT SRP-only hardware is possible, although
+	/*
+	 * REVISIT SRP-only hardware is possible, although
 	 * it would not be called "OTG" ...
 	 */
 	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
@@ -100,16 +95,21 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_CONFIGURATION_IDX	2
+enum {
+	STRING_MANUFACTURER_IDX,
+	STRING_PRODUCT_IDX,
+	STRING_CONFIGURATION_IDX,
+	STRING_SERIAL_IDX
+};
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
 	[STRING_CONFIGURATION_IDX].s = "Self Powered",
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -161,7 +161,6 @@ static struct usb_configuration msg_config_driver = {
 	.label			= "Linux File-Backed Storage",
 	.bind			= msg_do_config,
 	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
 	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
 };
 
@@ -175,25 +174,21 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget *gadget = cdev->gadget;
 	int status;
 
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
+	/* Take care of strings */
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
-	status = usb_string_id(cdev);
-	if (status < 0)
-		return status;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	msg_device_desc.iManufacturer = status;
 
-	status = usb_string_id(cdev);
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
+	status = usb_string_ids_tab(cdev, strings_dev);
 	if (status < 0)
 		return status;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	msg_device_desc.iProduct = status;
+
+	msg_device_desc.iManufacturer =
+		strings_dev[STRING_MANUFACTURER_IDX].id;
+	msg_device_desc.iProduct = strings_dev[STRING_PRODUCT_IDX].id;
+	msg_device_desc.iSerialNumber = strings_dev[STRING_SERIAL_IDX].id;
 
 	status = usb_string_id(cdev);
 	if (status < 0)
@@ -201,7 +196,7 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_CONFIGURATION_IDX].id = status;
 	msg_config_driver.iConfiguration = status;
 
-	/* register our second configuration */
+	/* Register our second configuration */
 	status = usb_add_config(cdev, &msg_config_driver);
 	if (status < 0)
 		return status;
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 76496f5..4b47f19 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -120,12 +120,15 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 #define STRING_MANUFACTURER_IDX		0
 #define STRING_PRODUCT_IDX		1
+#define STRING_PRODUCT_IDX		2
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -283,6 +286,9 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
+
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
 	status = usb_string_id(cdev);
 	if (status < 0)
 		goto fail2;
@@ -295,6 +301,12 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_PRODUCT_IDX].id = status;
 	device_desc.iProduct = status;
 
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail2;
+	strings_dev[STRING_SERIAL_IDX].id = status;
+	device_desc.iSerialNumber = status;
+
 #ifdef USB_ETH_RNDIS
 	/* register our first configuration */
 	status = usb_add_config(cdev, &rndis_config_driver);
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 868d8ee..1ba1004 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -545,6 +545,18 @@ static struct usb_gadget_strings	fsg_stringtab = {
 };
 
 
+#define fsg_string_serial_fill_n(serial, n, version) do {	\
+		unsigned char *c = version;			\
+		unsigned i = ((n) + 1) / 2;			\
+		char *s = serial;				\
+		for (; *c && --i; s += 2, ++c)			\
+			sprintf(s, "%02X", *c);			\
+	} while (0)
+
+#define fsg_string_serial_fill(serial, version)				\
+	fsg_string_serial_fill_n(serial, ARRAY_SIZE(serial), version)
+
+
  /*-------------------------------------------------------------------------*/
 
 /* If the next two routines are called while the gadget is registered,
-- 
1.7.1

Re: [PATCH 1/2] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Thu, 01 Jul 2010 11:17:46 +0200, Michal Nazarewicz <m.nazarewicz@samsung.com> wrote:
> This commit adds iSerialNumber to all gadgets that use the Mass
> Storage Function.  It appears that Windows has problems when
> it is not set.
>
> Not to copy the same code all over again, a fsg_string_serial_fill()
> macro has been added to the storage_common.c file which is now also
> used in the File Storage Gadget.

This patch applies to the -next and will work on it but neither of the
patches will work on .35-rc3 or older.

I've been testing everything on -next forgetting that it is a bit
ahead of the .35-rc3 and that there is no usb_string_ids_tab() in

Therefore, as far as stable kernels are concerned, disregard those two
patches.  I'll send an updated patch for stable in a minute.

Sorry about the confusion.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

[PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Michal Nazarewicz]

This commit adds iSerialNumber to all gadgets that use the Mass
Storage Function.  It appears that Windows has problems when
it is not set.

Not to copy the same code all over again, a fsg_string_serial_fill()
macro has been added to the storage_common.c file which is now also
used in the File Storage Gadget.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Cc: stable <stable@kernel.org>
---
This time a code that works with .35-rc3.

Because I feel this should get into .35 and this patch breaks the
"usb-gadget-g_multi-code-clean-up-and-refactoring.patch" patch updated
g_multi clean up patch follows this one.

This is patch fixes an issue with Windows not recognising the mass
storage properly so it's not a stability or security fix.  As such, it
is non-critical to include it in stable kernels.  Nonetheless, it does
fix some bug.

 drivers/usb/gadget/file_storage.c   |   10 +---------
 drivers/usb/gadget/mass_storage.c   |   31 +++++++++++++++++++------------
 drivers/usb/gadget/multi.c          |   12 ++++++++++++
 drivers/usb/gadget/storage_common.c |   12 ++++++++++++
 4 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index 7993267..16decb8 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3447,15 +3447,7 @@ static int __ref fsg_bind(struct usb_gadget *gadget)
 			init_utsname()->sysname, init_utsname()->release,
 			gadget->name);
 
-	/* On a real device, serial[] would be loaded from permanent
-	 * storage.  We just encode it from the driver version string. */
-	for (i = 0; i < sizeof fsg_string_serial - 2; i += 2) {
-		unsigned char		c = DRIVER_VERSION[i / 2];
-
-		if (!c)
-			break;
-		sprintf(&fsg_string_serial[i], "%02X", c);
-	}
+	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
 
 	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
 			"file-storage-gadget");
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 904d14b..c3690b8 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -72,21 +72,16 @@ static struct usb_device_descriptor msg_device_desc = {
 	.bcdUSB =		cpu_to_le16(0x0200),
 	.bDeviceClass =		USB_CLASS_PER_INTERFACE,
 
-	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(FSG_VENDOR_ID),
 	.idProduct =		cpu_to_le16(FSG_PRODUCT_ID),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
 static struct usb_otg_descriptor otg_descriptor = {
 	.bLength =		sizeof otg_descriptor,
 	.bDescriptorType =	USB_DT_OTG,
 
-	/* REVISIT SRP-only hardware is possible, although
+	/*
+	 * REVISIT SRP-only hardware is possible, although
 	 * it would not be called "OTG" ...
 	 */
 	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
@@ -100,16 +95,21 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_CONFIGURATION_IDX	2
+enum {
+	STRING_MANUFACTURER_IDX,
+	STRING_PRODUCT_IDX,
+	STRING_CONFIGURATION_IDX,
+	STRING_SERIAL_IDX
+};
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
 	[STRING_CONFIGURATION_IDX].s = "Self Powered",
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -167,7 +167,6 @@ static struct usb_configuration msg_config_driver = {
 	.label			= "Linux File-Backed Storage",
 	.bind			= msg_do_config,
 	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
 	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
 };
 
@@ -181,7 +180,8 @@ static int __ref msg_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget *gadget = cdev->gadget;
 	int status;
 
-	/* Allocate string descriptor numbers ... note that string
+	/*
+	 * Allocate string descriptor numbers ... note that string
 	 * contents can be overridden by the composite_dev glue.
 	 */
 
@@ -201,6 +201,13 @@ static int __ref msg_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_PRODUCT_IDX].id = status;
 	msg_device_desc.iProduct = status;
 
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		return status;
+	strings_dev[STRING_SERIAL_IDX].id = status;
+	msg_device_desc.iSerialNumber = status;
+
 	status = usb_string_id(cdev);
 	if (status < 0)
 		return status;
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index a930d7f..6f7447b 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -120,12 +120,15 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 #define STRING_MANUFACTURER_IDX		0
 #define STRING_PRODUCT_IDX		1
+#define STRING_SERIAL_IDX		2
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -281,6 +284,9 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
+
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
 	status = usb_string_id(cdev);
 	if (status < 0)
 		goto fail2;
@@ -293,6 +299,12 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_PRODUCT_IDX].id = status;
 	device_desc.iProduct = status;
 
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail2;
+	strings_dev[STRING_SERIAL_IDX].id = status;
+	device_desc.iSerialNumber = status;
+
 #ifdef USB_ETH_RNDIS
 	/* register our first configuration */
 	status = usb_add_config(cdev, &rndis_config_driver);
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 04c462f..00acae1 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -545,6 +545,18 @@ static struct usb_gadget_strings	fsg_stringtab = {
 };
 
 
+#define fsg_string_serial_fill_n(serial, n, version) do {	\
+		unsigned char *c = version;			\
+		unsigned i = ((n) + 1) / 2;			\
+		char *s = serial;				\
+		for (; *c && --i; s += 2, ++c)			\
+			sprintf(s, "%02X", *c);			\
+	} while (0)
+
+#define fsg_string_serial_fill(serial, version)				\
+	fsg_string_serial_fill_n(serial, ARRAY_SIZE(serial), version)
+
+
  /*-------------------------------------------------------------------------*/
 
 /* If the next two routines are called while the gadget is registered,
-- 
1.7.1

[PATCHv2 2/2] USB: gadget: g_multi: code clean up and refactoring

[Michal Nazarewicz]

The Multifunction Composite Gadget have been cleaned up
and refactored so hopefully it looks prettier and works
at least as good as before changes.

A Kconfig has also been fixed to make it impossible to build
FunctionFS gadget with no configurations.  With this patch, if
RNDIS is not chosen by the user CDC is force-selected.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/gadget/Kconfig |    1 +
 drivers/usb/gadget/multi.c |  274 ++++++++++++++++++++++++--------------------
 2 files changed, 152 insertions(+), 123 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 375279c..41a8a3e 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -899,6 +899,7 @@ config USB_G_NOKIA
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
+	select USB_G_MULTI_CDC if !USB_G_MULTI_RNDIS
 	help
 	  The Multifunction Composite Gadget provides Ethernet (RNDIS
 	  and/or CDC Ethernet), mass storage and ACM serial link
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 777fdbf..eef7336 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2008 David Brownell
  * Copyright (C) 2008 Nokia Corporation
- * Copyright (C) 2009 Samsung Electronics
+ * Copyright (C) 2009-2010 Samsung Electronics
  * Author: Michal Nazarewicz (m.nazarewicz@samsung.com)
  *
  * This program is free software; you can redistribute it and/or modify
@@ -24,6 +24,7 @@
 
 #include <linux/kernel.h>
 #include <linux/utsname.h>
+#include <linux/module.h>
 
 
 #if defined USB_ETH_RNDIS
@@ -35,14 +36,14 @@
 
 
 #define DRIVER_DESC		"Multifunction Composite Gadget"
-#define DRIVER_VERSION		"2009/07/21"
+#define DRIVER_VERSION		"2010/07/01"
 
-/*-------------------------------------------------------------------------*/
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Michal Nazarewicz");
+MODULE_LICENSE("GPL");
 
-#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
-#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
 
-/*-------------------------------------------------------------------------*/
+/***************************** All the files... *****************************/
 
 /*
  * kbuild is not very cooperative with respect to linking separately
@@ -57,6 +58,8 @@
 #include "config.c"
 #include "epautoconf.c"
 
+#include "f_mass_storage.c"
+
 #include "u_serial.c"
 #include "f_acm.c"
 
@@ -68,13 +71,24 @@
 #endif
 #include "u_ether.c"
 
-#undef DBG     /* u_ether.c has broken idea about macros */
-#undef VDBG    /* so clean up after it */
-#undef ERROR
-#undef INFO
-#include "f_mass_storage.c"
 
-/*-------------------------------------------------------------------------*/
+
+/***************************** Device Descriptor ****************************/
+
+#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
+#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
+
+
+enum {
+	__MULTI_NO_CONFIG,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_RNDIS_CONFIG_NUM,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_CDC_CONFIG_NUM,
+#endif
+};
+
 
 static struct usb_device_descriptor device_desc = {
 	.bLength =		sizeof device_desc,
@@ -82,83 +96,87 @@ static struct usb_device_descriptor device_desc = {
 
 	.bcdUSB =		cpu_to_le16(0x0200),
 
-	/* .bDeviceClass =		USB_CLASS_COMM, */
-	/* .bDeviceSubClass =	0, */
-	/* .bDeviceProtocol =	0, */
-	.bDeviceClass =		0xEF,
+	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
 	.bDeviceSubClass =	2,
 	.bDeviceProtocol =	1,
-	/* .bMaxPacketSize0 = f(hardware) */
 
 	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(MULTI_VENDOR_NUM),
 	.idProduct =		cpu_to_le16(MULTI_PRODUCT_NUM),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
-static struct usb_otg_descriptor otg_descriptor = {
-	.bLength =		sizeof otg_descriptor,
-	.bDescriptorType =	USB_DT_OTG,
-
-	/* REVISIT SRP-only hardware is possible, although
-	 * it would not be called "OTG" ...
-	 */
-	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
-};
 
 static const struct usb_descriptor_header *otg_desc[] = {
-	(struct usb_descriptor_header *) &otg_descriptor,
+	(struct usb_descriptor_header *) &(struct usb_otg_descriptor){
+		.bLength =		sizeof(struct usb_otg_descriptor),
+		.bDescriptorType =	USB_DT_OTG,
+
+		/*
+		 * REVISIT SRP-only hardware is possible, although
+		 * it would not be called "OTG" ...
+		 */
+		.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+	},
 	NULL,
 };
 
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_SERIAL_IDX		2
+enum {
+	MULTI_STRING_MANUFACTURER_IDX,
+	MULTI_STRING_PRODUCT_IDX,
+	MULTI_STRING_SERIAL_IDX,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_STRING_RNDIS_CONFIG_IDX,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_STRING_CDC_CONFIG_IDX,
+#endif
+};
 
 static char manufacturer[50];
 static char serial[13];
 
 static struct usb_string strings_dev[] = {
-	[STRING_MANUFACTURER_IDX].s = manufacturer,
-	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
-	[STRING_SERIAL_IDX].s = serial,
+	[MULTI_STRING_MANUFACTURER_IDX].s = manufacturer,
+	[MULTI_STRING_PRODUCT_IDX].s      = DRIVER_DESC,
+	[MULTI_STRING_SERIAL_IDX].s       = serial,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	[MULTI_STRING_RNDIS_CONFIG_IDX].s = "Multifunction with RNDIS",
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	[MULTI_STRING_CDC_CONFIG_IDX].s   = "Multifunction with CDC ECM",
+#endif
 	{  } /* end of list */
 };
 
-static struct usb_gadget_strings stringtab_dev = {
-	.language	= 0x0409,	/* en-us */
-	.strings	= strings_dev,
-};
-
 static struct usb_gadget_strings *dev_strings[] = {
-	&stringtab_dev,
+	&(struct usb_gadget_strings){
+		.language	= 0x0409,	/* en-us */
+		.strings	= strings_dev,
+	},
 	NULL,
 };
 
-static u8 hostaddr[ETH_ALEN];
 
 
 
 /****************************** Configurations ******************************/
 
-static struct fsg_module_parameters mod_data = {
-	.stall = 1
-};
-FSG_MODULE_PARAMETERS(/* no prefix */, mod_data);
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
 
-static struct fsg_common *fsg_common;
+static struct fsg_common fsg_common;
 
+static u8 hostaddr[ETH_ALEN];
+
+
+/********** RNDIS **********/
 
 #ifdef USB_ETH_RNDIS
 
-static int __init rndis_do_config(struct usb_configuration *c)
+static __ref int rndis_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -175,26 +193,42 @@ static int __init rndis_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration rndis_config_driver = {
-	.label			= "Multifunction Composite (RNDIS + MS + ACM)",
-	.bind			= rndis_do_config,
-	.bConfigurationValue	= 2,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= rndis_do_config,
+		.bConfigurationValue	= MULTI_RNDIS_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
+
+/********** CDC ECM **********/
+
 #ifdef CONFIG_USB_G_MULTI_CDC
 
-static int __init cdc_do_config(struct usb_configuration *c)
+static __ref int cdc_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -211,20 +245,33 @@ static int __init cdc_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration cdc_config_driver = {
-	.label			= "Multifunction Composite (CDC + MS + ACM)",
-	.bind			= cdc_do_config,
-	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= cdc_do_config,
+		.bConfigurationValue	= MULTI_CDC_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
@@ -233,7 +280,7 @@ static struct usb_configuration cdc_config_driver = {
 /****************************** Gadget Bind ******************************/
 
 
-static int __init multi_bind(struct usb_composite_dev *cdev)
+static int __ref multi_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 	int status, gcnum;
@@ -255,76 +302,60 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 		goto fail0;
 
 	/* set up mass storage function */
-	fsg_common = fsg_common_from_params(0, cdev, &mod_data);
-	if (IS_ERR(fsg_common)) {
-		status = PTR_ERR(fsg_common);
-		goto fail1;
+	{
+		void *retp;
+		retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+		if (IS_ERR(retp)) {
+			status = PTR_ERR(retp);
+			goto fail1;
+		}
 	}
 
-
+	/* set bcdDevice */
 	gcnum = usb_gadget_controller_number(gadget);
-	if (gcnum >= 0)
+	if (gcnum >= 0) {
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
-	else {
-		/* We assume that can_support_ecm() tells the truth;
-		 * but if the controller isn't recognized at all then
-		 * that assumption is a bit more likely to be wrong.
-		 */
-		WARNING(cdev, "controller '%s' not recognized\n",
-		        gadget->name);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized\n", gadget->name);
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | 0x0099);
 	}
 
-
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
+	/* allocate string descriptor numbers */
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
 
 	fsg_string_serial_fill(serial, DRIVER_VERSION);
 
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	device_desc.iManufacturer = status;
-
-	status = usb_string_id(cdev);
-	if (status < 0)
+	status = usb_string_ids_tab(cdev, strings_dev);
+	if (unlikely(status < 0))
 		goto fail2;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	device_desc.iProduct = status;
 
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_SERIAL_IDX].id = status;
-	device_desc.iSerialNumber = status;
+	device_desc.iManufacturer =
+		strings_dev[MULTI_STRING_MANUFACTURER_IDX].id;
+	device_desc.iProduct      =
+		strings_dev[MULTI_STRING_PRODUCT_IDX].id;
+	device_desc.iSerialNumber =
+		strings_dev[MULTI_STRING_SERIAL_IDX].id;
 
-#ifdef USB_ETH_RNDIS
-	/* register our first configuration */
-	status = usb_add_config(cdev, &rndis_config_driver);
-	if (status < 0)
+	/* register configurations */
+	status = rndis_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-#ifdef CONFIG_USB_G_MULTI_CDC
-	/* register our second configuration */
-	status = usb_add_config(cdev, &cdc_config_driver);
-	if (status < 0)
+	status = cdc_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-	dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
-	fsg_common_put(fsg_common);
+	/* we're done */
+	dev_info(&gadget->dev, DRIVER_DESC "\n");
+	fsg_common_put(&fsg_common);
 	return 0;
 
+
+	/* error recovery */
 fail2:
-	fsg_common_put(fsg_common);
+	fsg_common_put(&fsg_common);
 fail1:
 	gserial_cleanup();
 fail0:
@@ -351,18 +382,15 @@ static struct usb_composite_driver multi_driver = {
 	.unbind		= __exit_p(multi_unbind),
 };
 
-MODULE_DESCRIPTION(DRIVER_DESC);
-MODULE_AUTHOR("Michal Nazarewicz");
-MODULE_LICENSE("GPL");
 
-static int __init g_multi_init(void)
+static int __init multi_init(void)
 {
 	return usb_composite_register(&multi_driver);
 }
-module_init(g_multi_init);
+module_init(multi_init);
 
-static void __exit g_multi_cleanup(void)
+static void __exit multi_exit(void)
 {
 	usb_composite_unregister(&multi_driver);
 }
-module_exit(g_multi_cleanup);
+module_exit(multi_exit);
-- 
1.7.1

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Greg KH]

On Thu, Jul 01, 2010 at 03:42:19PM +0200, Michal Nazarewicz wrote:
> This commit adds iSerialNumber to all gadgets that use the Mass
> Storage Function.  It appears that Windows has problems when
> it is not set.
> 
> Not to copy the same code all over again, a fsg_string_serial_fill()
> macro has been added to the storage_common.c file which is now also
> used in the File Storage Gadget.

This patch conflicts with the patch in my tree:
	Subject: USB: Add a serial number parameter to g_file_storage module

So, could you fix the above patch up to play nice with your change so
that I can accept it?

thanks,

greg k-h

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

> On Thu, Jul 01, 2010 at 03:42:19PM +0200, Michal Nazarewicz wrote:
>> This commit adds iSerialNumber to all gadgets that use the Mass
>> Storage Function.  It appears that Windows has problems when
>> it is not set.
>>
>> Not to copy the same code all over again, a fsg_string_serial_fill()
>> macro has been added to the storage_common.c file which is now also
>> used in the File Storage Gadget.

On Thu, 08 Jul 2010 20:34:25 +0200, Greg KH <greg@kroah.com> wrote:
> This patch conflicts with the patch in my tree:
> 	Subject: USB: Add a serial number parameter to g_file_storage module

Ah, there it is...  I remembered this patch but couldn't find it so assumed
that it didn't make it to your tree after all.  But there it is...

> So, could you fix the above patch up to play nice with your change so
> that I can accept it?

Sure thing.  If I won't post it within 2h I'll do that after the weekend
(read Monday).

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Greg KH]

On Thu, Jul 08, 2010 at 08:58:11PM +0200, Michał Nazarewicz wrote:
> >On Thu, Jul 01, 2010 at 03:42:19PM +0200, Michal Nazarewicz wrote:
> >>This commit adds iSerialNumber to all gadgets that use the Mass
> >>Storage Function.  It appears that Windows has problems when
> >>it is not set.
> >>
> >>Not to copy the same code all over again, a fsg_string_serial_fill()
> >>macro has been added to the storage_common.c file which is now also
> >>used in the File Storage Gadget.
> 
> On Thu, 08 Jul 2010 20:34:25 +0200, Greg KH <greg@kroah.com> wrote:
> >This patch conflicts with the patch in my tree:
> >	Subject: USB: Add a serial number parameter to g_file_storage module
> 
> Ah, there it is...  I remembered this patch but couldn't find it so assumed
> that it didn't make it to your tree after all.  But there it is...
> 
> >So, could you fix the above patch up to play nice with your change so
> >that I can accept it?
> 
> Sure thing.  If I won't post it within 2h I'll do that after the weekend
> (read Monday).

Sounds good.

Note, Monday I'll be somewhere in Europe so my email access might be a
bit limited for that week.

thanks,

greg k-h

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[David Brownell]

> >> This commit adds iSerialNumber to all gadgets that
> use the Mass Storage Function.

Can it be made to do so only when the gadget
hasn't been provided with one already?  Serial
number module options are standard parts of the
composite gadget framework...

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

>> >> This commit adds iSerialNumber to all gadgets that
>> use the Mass Storage Function.

On Thu, 08 Jul 2010 21:31:23 +0200, David Brownell <david-b@pacbell.net> wrote:
> Can it be made to do so only when the gadget
> hasn't been provided with one already?  Serial
> number module options are standard parts of the
> composite gadget framework...

I don't see a way for the gadget to know whether user gave the iSerialNumber
parameter (other then reading the iSerialNumber variable but I consider that
ugly).

Plus, in any event, gadget has to reserve a string ID for serial number
anyway.  Otherwise composite won't override the string.


PS. I've just spotted a bug in Yann's patch (string is not filled if
CONFIG_USB_FILE_STORAGE_TEST is undefined).  It's a trivial issue so
I'll fix it with updated version.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[David Brownell]

> > Can it be made to do so only when the gadget
> > hasn't been provided with one already?  Serial
> > number module options are standard parts of the
> > composite gadget framework...
> 
> I don't see a way for the gadget to know whether user gave
> the iSerialNumber
> parameter (other then reading the iSerialNumber variable
> but I consider that  ugly).

Uglier still is the current patch overriding
that explicitly-given parameter.

NAK until this issue is resolved.

Re: [PATCHv2 1/2] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Thu, 08 Jul 2010 22:20:11 +0200, David Brownell <david-b@pacbell.net> wrote:

>> > Can it be made to do so only when the gadget
>> > hasn't been provided with one already?  Serial
>> > number module options are standard parts of the
>> > composite gadget framework...
>>
>> I don't see a way for the gadget to know whether user gave
>> the iSerialNumber
>> parameter (other then reading the iSerialNumber variable
>> but I consider that  ugly).
>
> Uglier still is the current patch overriding
> that explicitly-given parameter.

How does it override explicitly-given parameter?  I don't follow...

All it does is add a iSerialNumber.  Without this patch the
explicitly-given parameter won't even work:

	if (cdev->desc.iSerialNumber && iSerialNumber)
		string_override(composite->strings,
			cdev->desc.iSerialNumber, iSerialNumber);

Composite checks if iSerialNumber is allocated and overrides the
string only if it is.  Without my patch, the string ID is never
allocated.

Also, the overriding is performed *after* composite device's bind
callback is called so there is no way for the bind callback to
override explicitly-given parameter.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

[PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michal Nazarewicz]

This commit adds iSerialNumber to all gadgets that use the Mass
Storage Function.  It appears that Windows has problems when
it is not set.

Not to copy the same code all over again, a fsg_string_serial_fill()
macro has been added to the storage_common.c file which is now also
used in the File Storage Gadget.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Cc: stable <stable@kernel.org>
---
> On Thu, Jul 01, 2010 at 03:42:19PM +0200, Michal Nazarewicz wrote:
> This patch conflicts with the patch in my tree:
> 	Subject: USB: Add a serial number parameter to g_file_storage module
>
> So, could you fix the above patch up to play nice with your change so
> that I can accept it?

Sending all 3 patches.  The first and last are identical to v2 (only
updated offsets and some such).

The second has been modified as it stopped applying after applying the
first plus there was a bug: the fsg_string_serial was never filled if
CONFIG_USB_FILE_STORAGE_TEST was not defined.


Please also note that David has some concerns about this patch (if I
understood everything correctly).  However, I wasn't sure what was the
issue here...


The delta for the second patch is:

> --- drivers/usb/gadget/file_storage.c	2010-07-08 22:19:21.428413976 +0200
> +++ /home/mina86/file_storage.c	2010-07-08 22:17:23.792913751 +0200
> @@ -3315,20 +3315,13 @@
>  		fsg_strings[FSG_STRING_SERIAL - 1].s = mod_data.serial_parm;
>  	} else {
>  fill_serial:
> -		/* Serial number not specified or invalid, make our own.
> -		 * We just encode it from the driver version string,
> -		 * 12 characters to comply with both CB[I] and BBB spec.
> -		 * Warning : Two devices running the same kernel will have
> -		 * the same fallback serial number. */
> -		for (i = 0; i < 12; i += 2) {
> -			unsigned char	c = DRIVER_VERSION[i / 2];
> -
> -			if (!c)
> -				break;
> -			sprintf(&fsg_string_serial[i], "%02X", c);
> -		}
> +		fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
>  	}
>  
> +#else /* !CONFIG_USB_FILE_STORAGE_TEST */
> +
> +	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
> +
>  #endif /* CONFIG_USB_FILE_STORAGE_TEST */
>  
>  	return 0;


On Thu, 08 Jul 2010 21:03:28 +0200, Greg KH <greg@kroah.com> wrote:
> Note, Monday I'll be somewhere in Europe so my email access might be a
> bit limited for that week.

We do have pretty good Internet here you know... ;)  Any way, come visit
Warsaw then! :P

At any rate, I think there is no rush, no rush at all.

 drivers/usb/gadget/file_storage.c   |   10 +---------
 drivers/usb/gadget/mass_storage.c   |   31 +++++++++++++++++++------------
 drivers/usb/gadget/multi.c          |   12 ++++++++++++
 drivers/usb/gadget/storage_common.c |   12 ++++++++++++
 4 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index b49d86e..f1e6956 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -3447,15 +3447,7 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 			init_utsname()->sysname, init_utsname()->release,
 			gadget->name);
 
-	/* On a real device, serial[] would be loaded from permanent
-	 * storage.  We just encode it from the driver version string. */
-	for (i = 0; i < sizeof fsg_string_serial - 2; i += 2) {
-		unsigned char		c = DRIVER_VERSION[i / 2];
-
-		if (!c)
-			break;
-		sprintf(&fsg_string_serial[i], "%02X", c);
-	}
+	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
 
 	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
 			"file-storage-gadget");
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 705cc1f..16e0d4b 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -72,21 +72,16 @@ static struct usb_device_descriptor msg_device_desc = {
 	.bcdUSB =		cpu_to_le16(0x0200),
 	.bDeviceClass =		USB_CLASS_PER_INTERFACE,
 
-	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(FSG_VENDOR_ID),
 	.idProduct =		cpu_to_le16(FSG_PRODUCT_ID),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
 static struct usb_otg_descriptor otg_descriptor = {
 	.bLength =		sizeof otg_descriptor,
 	.bDescriptorType =	USB_DT_OTG,
 
-	/* REVISIT SRP-only hardware is possible, although
+	/*
+	 * REVISIT SRP-only hardware is possible, although
 	 * it would not be called "OTG" ...
 	 */
 	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
@@ -100,16 +95,21 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_CONFIGURATION_IDX	2
+enum {
+	STRING_MANUFACTURER_IDX,
+	STRING_PRODUCT_IDX,
+	STRING_CONFIGURATION_IDX,
+	STRING_SERIAL_IDX
+};
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
 	[STRING_CONFIGURATION_IDX].s = "Self Powered",
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -167,7 +167,6 @@ static struct usb_configuration msg_config_driver = {
 	.label			= "Linux File-Backed Storage",
 	.bind			= msg_do_config,
 	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
 	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
 };
 
@@ -181,7 +180,8 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget *gadget = cdev->gadget;
 	int status;
 
-	/* Allocate string descriptor numbers ... note that string
+	/*
+	 * Allocate string descriptor numbers ... note that string
 	 * contents can be overridden by the composite_dev glue.
 	 */
 
@@ -201,6 +201,13 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_PRODUCT_IDX].id = status;
 	msg_device_desc.iProduct = status;
 
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+	status = usb_string_id(cdev);
+	if (status < 0)
+		return status;
+	strings_dev[STRING_SERIAL_IDX].id = status;
+	msg_device_desc.iSerialNumber = status;
+
 	status = usb_string_id(cdev);
 	if (status < 0)
 		return status;
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index a930d7f..6f7447b 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -120,12 +120,15 @@ static const struct usb_descriptor_header *otg_desc[] = {
 
 #define STRING_MANUFACTURER_IDX		0
 #define STRING_PRODUCT_IDX		1
+#define STRING_SERIAL_IDX		2
 
 static char manufacturer[50];
+static char serial[13];
 
 static struct usb_string strings_dev[] = {
 	[STRING_MANUFACTURER_IDX].s = manufacturer,
 	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
+	[STRING_SERIAL_IDX].s = serial,
 	{  } /* end of list */
 };
 
@@ -281,6 +284,9 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
+
+	fsg_string_serial_fill(serial, DRIVER_VERSION);
+
 	status = usb_string_id(cdev);
 	if (status < 0)
 		goto fail2;
@@ -293,6 +299,12 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 	strings_dev[STRING_PRODUCT_IDX].id = status;
 	device_desc.iProduct = status;
 
+	status = usb_string_id(cdev);
+	if (status < 0)
+		goto fail2;
+	strings_dev[STRING_SERIAL_IDX].id = status;
+	device_desc.iSerialNumber = status;
+
 #ifdef USB_ETH_RNDIS
 	/* register our first configuration */
 	status = usb_add_config(cdev, &rndis_config_driver);
diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
index 04c462f..00acae1 100644
--- a/drivers/usb/gadget/storage_common.c
+++ b/drivers/usb/gadget/storage_common.c
@@ -545,6 +545,18 @@ static struct usb_gadget_strings	fsg_stringtab = {
 };
 
 
+#define fsg_string_serial_fill_n(serial, n, version) do {	\
+		unsigned char *c = version;			\
+		unsigned i = ((n) + 1) / 2;			\
+		char *s = serial;				\
+		for (; *c && --i; s += 2, ++c)			\
+			sprintf(s, "%02X", *c);			\
+	} while (0)
+
+#define fsg_string_serial_fill(serial, version)				\
+	fsg_string_serial_fill_n(serial, ARRAY_SIZE(serial), version)
+
+
  /*-------------------------------------------------------------------------*/
 
 /* If the next two routines are called while the gadget is registered,
-- 
1.7.1

[PATCHv3 2/3] USB: Add a serial number parameter to g_file_storage module

[Michal Nazarewicz]

From: Yann Cantin <yann.cantin@laposte.net>

This patch add a serial number parameter to the g_file_storage
module. There's validity checks against the string passed to comply
with the specs.

Signed-off-by: Yann Cantin <yann.cantin@laposte.net>
[m.nazarewicz@samsung.com: modified to use fsg_string_serial_fill]
Cc: Michał Nazarewicz <m.nazarewicz@samsung.com>
Cc: David Brownell <dbrownell@users.sourceforge.net>
---
 drivers/usb/gadget/file_storage.c |   54 ++++++++++++++++++++++++++++++++++--
 1 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index f1e6956..9663176 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -56,7 +56,7 @@
  * following protocols: RBC (0x01), ATAPI or SFF-8020i (0x02), QIC-157 (0c03),
  * UFI (0x04), SFF-8070i (0x05), and transparent SCSI (0x06), selected by
  * the optional "protocol" module parameter.  In addition, the default
- * Vendor ID, Product ID, and release number can be overridden.
+ * Vendor ID, Product ID, release number and serial number can be overridden.
  *
  * There is support for multiple logical units (LUNs), each of which has
  * its own backing file.  The number of LUNs can be set using the optional
@@ -106,6 +106,7 @@
  *	vendor=0xVVVV		Default 0x0525 (NetChip), USB Vendor ID
  *	product=0xPPPP		Default 0xa4a5 (FSG), USB Product ID
  *	release=0xRRRR		Override the USB release number (bcdDevice)
+ *	serial=HHHH...		Override serial number (string of hex chars)
  *	buflen=N		Default N=16384, buffer size used (will be
  *					rounded down to a multiple of
  *					PAGE_CACHE_SIZE)
@@ -270,6 +271,8 @@
 
 #define DRIVER_DESC		"File-backed Storage Gadget"
 #define DRIVER_NAME		"g_file_storage"
+/* DRIVER_VERSION must be at least 6 characters long, as it is used
+ * to generate a fallback serial number. */
 #define DRIVER_VERSION		"20 November 2008"
 
 static       char fsg_string_manufacturer[64];
@@ -314,6 +317,7 @@ static struct {
 	unsigned short	vendor;
 	unsigned short	product;
 	unsigned short	release;
+	char		*serial_parm;
 	unsigned int	buflen;
 
 	int		transport_type;
@@ -374,6 +378,9 @@ MODULE_PARM_DESC(product, "USB Product ID");
 module_param_named(release, mod_data.release, ushort, S_IRUGO);
 MODULE_PARM_DESC(release, "USB release number");
 
+module_param_named(serial, mod_data.serial_parm, charp, S_IRUGO);
+MODULE_PARM_DESC(serial, "USB serial number");
+
 module_param_named(buflen, mod_data.buflen, uint, S_IRUGO);
 MODULE_PARM_DESC(buflen, "I/O buffer size");
 
@@ -3197,6 +3204,7 @@ static int __init check_parameters(struct fsg_dev *fsg)
 {
 	int	prot;
 	int	gcnum;
+	int	i;
 
 	/* Store the default values */
 	mod_data.transport_type = USB_PR_BULK;
@@ -3272,6 +3280,48 @@ static int __init check_parameters(struct fsg_dev *fsg)
 		ERROR(fsg, "invalid buflen\n");
 		return -ETOOSMALL;
 	}
+
+	/* Serial string handling.
+	 * On a real device, the serial string would be loaded
+	 * from permanent storage. */
+	if (mod_data.serial_parm) {
+		const char *ch;
+		unsigned len = 0;
+
+		/* Sanity check :
+		 * The CB[I] specification limits the serial string to
+		 * 12 uppercase hexadecimal characters.
+		 * BBB need at least 12 uppercase hexadecimal characters,
+		 * with a maximum of 126. */
+		for (ch = mod_data.serial_parm; *ch; ++ch) {
+			++len;
+			if ((*ch < '0' || *ch > '9') &&
+			    (*ch < 'A' || *ch > 'F')) { /* not uppercase hex */
+				WARNING(fsg,
+					"Invalid serial string character: %c; "
+					"Failing back to default\n",
+					*ch);
+				goto fill_serial;
+			}
+		}
+		if (len > 126 ||
+		    (mod_data.transport_type == USB_PR_BULK && len < 12) ||
+		    (mod_data.transport_type != USB_PR_BULK && len > 12)) {
+			WARNING(fsg,
+				"Invalid serial string length; "
+				"Failing back to default\n");
+			goto fill_serial;
+		}
+		fsg_strings[FSG_STRING_SERIAL - 1].s = mod_data.serial_parm;
+	} else {
+fill_serial:
+		fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
+	}
+
+#else /* !CONFIG_USB_FILE_STORAGE_TEST */
+
+	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
+
 #endif /* CONFIG_USB_FILE_STORAGE_TEST */
 
 	return 0;
@@ -3447,8 +3497,6 @@ static int __init fsg_bind(struct usb_gadget *gadget)
 			init_utsname()->sysname, init_utsname()->release,
 			gadget->name);
 
-	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
-
 	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
 			"file-storage-gadget");
 	if (IS_ERR(fsg->thread_task)) {
-- 
1.7.1

[PATCHv3 3/3] USB: gadget: g_multi: code clean up and refactoring

[Michal Nazarewicz]

The Multifunction Composite Gadget have been cleaned up
and refactored so hopefully it looks prettier and works
at least as good as before changes.

A Kconfig has also been fixed to make it impossible to build
FunctionFS gadget with no configurations.  With this patch, if
RNDIS is not chosen by the user CDC is force-selected.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/usb/gadget/Kconfig |    1 +
 drivers/usb/gadget/multi.c |  274 ++++++++++++++++++++++++--------------------
 2 files changed, 152 insertions(+), 123 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 591ae9f..027f61b 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -863,6 +863,7 @@ config USB_G_NOKIA
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
+	select USB_G_MULTI_CDC if !USB_G_MULTI_RNDIS
 	help
 	  The Multifunction Composite Gadget provides Ethernet (RNDIS
 	  and/or CDC Ethernet), mass storage and ACM serial link
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index 777fdbf..eef7336 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -3,7 +3,7 @@
  *
  * Copyright (C) 2008 David Brownell
  * Copyright (C) 2008 Nokia Corporation
- * Copyright (C) 2009 Samsung Electronics
+ * Copyright (C) 2009-2010 Samsung Electronics
  * Author: Michal Nazarewicz (m.nazarewicz@samsung.com)
  *
  * This program is free software; you can redistribute it and/or modify
@@ -24,6 +24,7 @@
 
 #include <linux/kernel.h>
 #include <linux/utsname.h>
+#include <linux/module.h>
 
 
 #if defined USB_ETH_RNDIS
@@ -35,14 +36,14 @@
 
 
 #define DRIVER_DESC		"Multifunction Composite Gadget"
-#define DRIVER_VERSION		"2009/07/21"
+#define DRIVER_VERSION		"2010/07/01"
 
-/*-------------------------------------------------------------------------*/
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Michal Nazarewicz");
+MODULE_LICENSE("GPL");
 
-#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
-#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
 
-/*-------------------------------------------------------------------------*/
+/***************************** All the files... *****************************/
 
 /*
  * kbuild is not very cooperative with respect to linking separately
@@ -57,6 +58,8 @@
 #include "config.c"
 #include "epautoconf.c"
 
+#include "f_mass_storage.c"
+
 #include "u_serial.c"
 #include "f_acm.c"
 
@@ -68,13 +71,24 @@
 #endif
 #include "u_ether.c"
 
-#undef DBG     /* u_ether.c has broken idea about macros */
-#undef VDBG    /* so clean up after it */
-#undef ERROR
-#undef INFO
-#include "f_mass_storage.c"
 
-/*-------------------------------------------------------------------------*/
+
+/***************************** Device Descriptor ****************************/
+
+#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
+#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
+
+
+enum {
+	__MULTI_NO_CONFIG,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_RNDIS_CONFIG_NUM,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_CDC_CONFIG_NUM,
+#endif
+};
+
 
 static struct usb_device_descriptor device_desc = {
 	.bLength =		sizeof device_desc,
@@ -82,83 +96,87 @@ static struct usb_device_descriptor device_desc = {
 
 	.bcdUSB =		cpu_to_le16(0x0200),
 
-	/* .bDeviceClass =		USB_CLASS_COMM, */
-	/* .bDeviceSubClass =	0, */
-	/* .bDeviceProtocol =	0, */
-	.bDeviceClass =		0xEF,
+	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
 	.bDeviceSubClass =	2,
 	.bDeviceProtocol =	1,
-	/* .bMaxPacketSize0 = f(hardware) */
 
 	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(MULTI_VENDOR_NUM),
 	.idProduct =		cpu_to_le16(MULTI_PRODUCT_NUM),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
 };
 
-static struct usb_otg_descriptor otg_descriptor = {
-	.bLength =		sizeof otg_descriptor,
-	.bDescriptorType =	USB_DT_OTG,
-
-	/* REVISIT SRP-only hardware is possible, although
-	 * it would not be called "OTG" ...
-	 */
-	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
-};
 
 static const struct usb_descriptor_header *otg_desc[] = {
-	(struct usb_descriptor_header *) &otg_descriptor,
+	(struct usb_descriptor_header *) &(struct usb_otg_descriptor){
+		.bLength =		sizeof(struct usb_otg_descriptor),
+		.bDescriptorType =	USB_DT_OTG,
+
+		/*
+		 * REVISIT SRP-only hardware is possible, although
+		 * it would not be called "OTG" ...
+		 */
+		.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+	},
 	NULL,
 };
 
 
 /* string IDs are assigned dynamically */
 
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_SERIAL_IDX		2
+enum {
+	MULTI_STRING_MANUFACTURER_IDX,
+	MULTI_STRING_PRODUCT_IDX,
+	MULTI_STRING_SERIAL_IDX,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_STRING_RNDIS_CONFIG_IDX,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_STRING_CDC_CONFIG_IDX,
+#endif
+};
 
 static char manufacturer[50];
 static char serial[13];
 
 static struct usb_string strings_dev[] = {
-	[STRING_MANUFACTURER_IDX].s = manufacturer,
-	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
-	[STRING_SERIAL_IDX].s = serial,
+	[MULTI_STRING_MANUFACTURER_IDX].s = manufacturer,
+	[MULTI_STRING_PRODUCT_IDX].s      = DRIVER_DESC,
+	[MULTI_STRING_SERIAL_IDX].s       = serial,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	[MULTI_STRING_RNDIS_CONFIG_IDX].s = "Multifunction with RNDIS",
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	[MULTI_STRING_CDC_CONFIG_IDX].s   = "Multifunction with CDC ECM",
+#endif
 	{  } /* end of list */
 };
 
-static struct usb_gadget_strings stringtab_dev = {
-	.language	= 0x0409,	/* en-us */
-	.strings	= strings_dev,
-};
-
 static struct usb_gadget_strings *dev_strings[] = {
-	&stringtab_dev,
+	&(struct usb_gadget_strings){
+		.language	= 0x0409,	/* en-us */
+		.strings	= strings_dev,
+	},
 	NULL,
 };
 
-static u8 hostaddr[ETH_ALEN];
 
 
 
 /****************************** Configurations ******************************/
 
-static struct fsg_module_parameters mod_data = {
-	.stall = 1
-};
-FSG_MODULE_PARAMETERS(/* no prefix */, mod_data);
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
 
-static struct fsg_common *fsg_common;
+static struct fsg_common fsg_common;
 
+static u8 hostaddr[ETH_ALEN];
+
+
+/********** RNDIS **********/
 
 #ifdef USB_ETH_RNDIS
 
-static int __init rndis_do_config(struct usb_configuration *c)
+static __ref int rndis_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -175,26 +193,42 @@ static int __init rndis_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration rndis_config_driver = {
-	.label			= "Multifunction Composite (RNDIS + MS + ACM)",
-	.bind			= rndis_do_config,
-	.bConfigurationValue	= 2,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= rndis_do_config,
+		.bConfigurationValue	= MULTI_RNDIS_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
+
+/********** CDC ECM **********/
+
 #ifdef CONFIG_USB_G_MULTI_CDC
 
-static int __init cdc_do_config(struct usb_configuration *c)
+static __ref int cdc_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -211,20 +245,33 @@ static int __init cdc_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration cdc_config_driver = {
-	.label			= "Multifunction Composite (CDC + MS + ACM)",
-	.bind			= cdc_do_config,
-	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= cdc_do_config,
+		.bConfigurationValue	= MULTI_CDC_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
@@ -233,7 +280,7 @@ static struct usb_configuration cdc_config_driver = {
 /****************************** Gadget Bind ******************************/
 
 
-static int __init multi_bind(struct usb_composite_dev *cdev)
+static int __ref multi_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 	int status, gcnum;
@@ -255,76 +302,60 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 		goto fail0;
 
 	/* set up mass storage function */
-	fsg_common = fsg_common_from_params(0, cdev, &mod_data);
-	if (IS_ERR(fsg_common)) {
-		status = PTR_ERR(fsg_common);
-		goto fail1;
+	{
+		void *retp;
+		retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+		if (IS_ERR(retp)) {
+			status = PTR_ERR(retp);
+			goto fail1;
+		}
 	}
 
-
+	/* set bcdDevice */
 	gcnum = usb_gadget_controller_number(gadget);
-	if (gcnum >= 0)
+	if (gcnum >= 0) {
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
-	else {
-		/* We assume that can_support_ecm() tells the truth;
-		 * but if the controller isn't recognized at all then
-		 * that assumption is a bit more likely to be wrong.
-		 */
-		WARNING(cdev, "controller '%s' not recognized\n",
-		        gadget->name);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized\n", gadget->name);
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | 0x0099);
 	}
 
-
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
+	/* allocate string descriptor numbers */
 	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
 	         init_utsname()->sysname, init_utsname()->release,
 	         gadget->name);
 
 	fsg_string_serial_fill(serial, DRIVER_VERSION);
 
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	device_desc.iManufacturer = status;
-
-	status = usb_string_id(cdev);
-	if (status < 0)
+	status = usb_string_ids_tab(cdev, strings_dev);
+	if (unlikely(status < 0))
 		goto fail2;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	device_desc.iProduct = status;
 
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_SERIAL_IDX].id = status;
-	device_desc.iSerialNumber = status;
+	device_desc.iManufacturer =
+		strings_dev[MULTI_STRING_MANUFACTURER_IDX].id;
+	device_desc.iProduct      =
+		strings_dev[MULTI_STRING_PRODUCT_IDX].id;
+	device_desc.iSerialNumber =
+		strings_dev[MULTI_STRING_SERIAL_IDX].id;
 
-#ifdef USB_ETH_RNDIS
-	/* register our first configuration */
-	status = usb_add_config(cdev, &rndis_config_driver);
-	if (status < 0)
+	/* register configurations */
+	status = rndis_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-#ifdef CONFIG_USB_G_MULTI_CDC
-	/* register our second configuration */
-	status = usb_add_config(cdev, &cdc_config_driver);
-	if (status < 0)
+	status = cdc_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-	dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
-	fsg_common_put(fsg_common);
+	/* we're done */
+	dev_info(&gadget->dev, DRIVER_DESC "\n");
+	fsg_common_put(&fsg_common);
 	return 0;
 
+
+	/* error recovery */
 fail2:
-	fsg_common_put(fsg_common);
+	fsg_common_put(&fsg_common);
 fail1:
 	gserial_cleanup();
 fail0:
@@ -351,18 +382,15 @@ static struct usb_composite_driver multi_driver = {
 	.unbind		= __exit_p(multi_unbind),
 };
 
-MODULE_DESCRIPTION(DRIVER_DESC);
-MODULE_AUTHOR("Michal Nazarewicz");
-MODULE_LICENSE("GPL");
 
-static int __init g_multi_init(void)
+static int __init multi_init(void)
 {
 	return usb_composite_register(&multi_driver);
 }
-module_init(g_multi_init);
+module_init(multi_init);
 
-static void __exit g_multi_cleanup(void)
+static void __exit multi_exit(void)
 {
 	usb_composite_unregister(&multi_driver);
 }
-module_exit(g_multi_cleanup);
+module_exit(multi_exit);
-- 
1.7.1

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Greg KH]

David, any thoughts on these versions of the patch?

thanks,

greg k-h

On Thu, Jul 08, 2010 at 10:52:26PM +0200, Michal Nazarewicz wrote:
> This commit adds iSerialNumber to all gadgets that use the Mass
> Storage Function.  It appears that Windows has problems when
> it is not set.
> 
> Not to copy the same code all over again, a fsg_string_serial_fill()
> macro has been added to the storage_common.c file which is now also
> used in the File Storage Gadget.
> 
> Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
> Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
> Cc: stable <stable@kernel.org>
> ---
> > On Thu, Jul 01, 2010 at 03:42:19PM +0200, Michal Nazarewicz wrote:
> > This patch conflicts with the patch in my tree:
> > 	Subject: USB: Add a serial number parameter to g_file_storage module
> >
> > So, could you fix the above patch up to play nice with your change so
> > that I can accept it?
> 
> Sending all 3 patches.  The first and last are identical to v2 (only
> updated offsets and some such).
> 
> The second has been modified as it stopped applying after applying the
> first plus there was a bug: the fsg_string_serial was never filled if
> CONFIG_USB_FILE_STORAGE_TEST was not defined.
> 
> 
> Please also note that David has some concerns about this patch (if I
> understood everything correctly).  However, I wasn't sure what was the
> issue here...
> 
> 
> The delta for the second patch is:
> 
> > --- drivers/usb/gadget/file_storage.c	2010-07-08 22:19:21.428413976 +0200
> > +++ /home/mina86/file_storage.c	2010-07-08 22:17:23.792913751 +0200
> > @@ -3315,20 +3315,13 @@
> >  		fsg_strings[FSG_STRING_SERIAL - 1].s = mod_data.serial_parm;
> >  	} else {
> >  fill_serial:
> > -		/* Serial number not specified or invalid, make our own.
> > -		 * We just encode it from the driver version string,
> > -		 * 12 characters to comply with both CB[I] and BBB spec.
> > -		 * Warning : Two devices running the same kernel will have
> > -		 * the same fallback serial number. */
> > -		for (i = 0; i < 12; i += 2) {
> > -			unsigned char	c = DRIVER_VERSION[i / 2];
> > -
> > -			if (!c)
> > -				break;
> > -			sprintf(&fsg_string_serial[i], "%02X", c);
> > -		}
> > +		fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
> >  	}
> >  
> > +#else /* !CONFIG_USB_FILE_STORAGE_TEST */
> > +
> > +	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
> > +
> >  #endif /* CONFIG_USB_FILE_STORAGE_TEST */
> >  
> >  	return 0;
> 
> 
> On Thu, 08 Jul 2010 21:03:28 +0200, Greg KH <greg@kroah.com> wrote:
> > Note, Monday I'll be somewhere in Europe so my email access might be a
> > bit limited for that week.
> 
> We do have pretty good Internet here you know... ;)  Any way, come visit
> Warsaw then! :P
> 
> At any rate, I think there is no rush, no rush at all.
> 
>  drivers/usb/gadget/file_storage.c   |   10 +---------
>  drivers/usb/gadget/mass_storage.c   |   31 +++++++++++++++++++------------
>  drivers/usb/gadget/multi.c          |   12 ++++++++++++
>  drivers/usb/gadget/storage_common.c |   12 ++++++++++++
>  4 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> index b49d86e..f1e6956 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -3447,15 +3447,7 @@ static int __init fsg_bind(struct usb_gadget *gadget)
>  			init_utsname()->sysname, init_utsname()->release,
>  			gadget->name);
>  
> -	/* On a real device, serial[] would be loaded from permanent
> -	 * storage.  We just encode it from the driver version string. */
> -	for (i = 0; i < sizeof fsg_string_serial - 2; i += 2) {
> -		unsigned char		c = DRIVER_VERSION[i / 2];
> -
> -		if (!c)
> -			break;
> -		sprintf(&fsg_string_serial[i], "%02X", c);
> -	}
> +	fsg_string_serial_fill(fsg_string_serial, DRIVER_VERSION);
>  
>  	fsg->thread_task = kthread_create(fsg_main_thread, fsg,
>  			"file-storage-gadget");
> diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
> index 705cc1f..16e0d4b 100644
> --- a/drivers/usb/gadget/mass_storage.c
> +++ b/drivers/usb/gadget/mass_storage.c
> @@ -72,21 +72,16 @@ static struct usb_device_descriptor msg_device_desc = {
>  	.bcdUSB =		cpu_to_le16(0x0200),
>  	.bDeviceClass =		USB_CLASS_PER_INTERFACE,
>  
> -	/* Vendor and product id can be overridden by module parameters.  */
>  	.idVendor =		cpu_to_le16(FSG_VENDOR_ID),
>  	.idProduct =		cpu_to_le16(FSG_PRODUCT_ID),
> -	/* .bcdDevice = f(hardware) */
> -	/* .iManufacturer = DYNAMIC */
> -	/* .iProduct = DYNAMIC */
> -	/* NO SERIAL NUMBER */
> -	.bNumConfigurations =	1,
>  };
>  
>  static struct usb_otg_descriptor otg_descriptor = {
>  	.bLength =		sizeof otg_descriptor,
>  	.bDescriptorType =	USB_DT_OTG,
>  
> -	/* REVISIT SRP-only hardware is possible, although
> +	/*
> +	 * REVISIT SRP-only hardware is possible, although
>  	 * it would not be called "OTG" ...
>  	 */
>  	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
> @@ -100,16 +95,21 @@ static const struct usb_descriptor_header *otg_desc[] = {
>  
>  /* string IDs are assigned dynamically */
>  
> -#define STRING_MANUFACTURER_IDX		0
> -#define STRING_PRODUCT_IDX		1
> -#define STRING_CONFIGURATION_IDX	2
> +enum {
> +	STRING_MANUFACTURER_IDX,
> +	STRING_PRODUCT_IDX,
> +	STRING_CONFIGURATION_IDX,
> +	STRING_SERIAL_IDX
> +};
>  
>  static char manufacturer[50];
> +static char serial[13];
>  
>  static struct usb_string strings_dev[] = {
>  	[STRING_MANUFACTURER_IDX].s = manufacturer,
>  	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
>  	[STRING_CONFIGURATION_IDX].s = "Self Powered",
> +	[STRING_SERIAL_IDX].s = serial,
>  	{  } /* end of list */
>  };
>  
> @@ -167,7 +167,6 @@ static struct usb_configuration msg_config_driver = {
>  	.label			= "Linux File-Backed Storage",
>  	.bind			= msg_do_config,
>  	.bConfigurationValue	= 1,
> -	/* .iConfiguration = DYNAMIC */
>  	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
>  };
>  
> @@ -181,7 +180,8 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
>  	struct usb_gadget *gadget = cdev->gadget;
>  	int status;
>  
> -	/* Allocate string descriptor numbers ... note that string
> +	/*
> +	 * Allocate string descriptor numbers ... note that string
>  	 * contents can be overridden by the composite_dev glue.
>  	 */
>  
> @@ -201,6 +201,13 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
>  	strings_dev[STRING_PRODUCT_IDX].id = status;
>  	msg_device_desc.iProduct = status;
>  
> +	fsg_string_serial_fill(serial, DRIVER_VERSION);
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		return status;
> +	strings_dev[STRING_SERIAL_IDX].id = status;
> +	msg_device_desc.iSerialNumber = status;
> +
>  	status = usb_string_id(cdev);
>  	if (status < 0)
>  		return status;
> diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
> index a930d7f..6f7447b 100644
> --- a/drivers/usb/gadget/multi.c
> +++ b/drivers/usb/gadget/multi.c
> @@ -120,12 +120,15 @@ static const struct usb_descriptor_header *otg_desc[] = {
>  
>  #define STRING_MANUFACTURER_IDX		0
>  #define STRING_PRODUCT_IDX		1
> +#define STRING_SERIAL_IDX		2
>  
>  static char manufacturer[50];
> +static char serial[13];
>  
>  static struct usb_string strings_dev[] = {
>  	[STRING_MANUFACTURER_IDX].s = manufacturer,
>  	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
> +	[STRING_SERIAL_IDX].s = serial,
>  	{  } /* end of list */
>  };
>  
> @@ -281,6 +284,9 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
>  	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
>  	         init_utsname()->sysname, init_utsname()->release,
>  	         gadget->name);
> +
> +	fsg_string_serial_fill(serial, DRIVER_VERSION);
> +
>  	status = usb_string_id(cdev);
>  	if (status < 0)
>  		goto fail2;
> @@ -293,6 +299,12 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
>  	strings_dev[STRING_PRODUCT_IDX].id = status;
>  	device_desc.iProduct = status;
>  
> +	status = usb_string_id(cdev);
> +	if (status < 0)
> +		goto fail2;
> +	strings_dev[STRING_SERIAL_IDX].id = status;
> +	device_desc.iSerialNumber = status;
> +
>  #ifdef USB_ETH_RNDIS
>  	/* register our first configuration */
>  	status = usb_add_config(cdev, &rndis_config_driver);
> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c
> index 04c462f..00acae1 100644
> --- a/drivers/usb/gadget/storage_common.c
> +++ b/drivers/usb/gadget/storage_common.c
> @@ -545,6 +545,18 @@ static struct usb_gadget_strings	fsg_stringtab = {
>  };
>  
>  
> +#define fsg_string_serial_fill_n(serial, n, version) do {	\
> +		unsigned char *c = version;			\
> +		unsigned i = ((n) + 1) / 2;			\
> +		char *s = serial;				\
> +		for (; *c && --i; s += 2, ++c)			\
> +			sprintf(s, "%02X", *c);			\
> +	} while (0)
> +
> +#define fsg_string_serial_fill(serial, version)				\
> +	fsg_string_serial_fill_n(serial, ARRAY_SIZE(serial), version)
> +
> +
>   /*-------------------------------------------------------------------------*/
>  
>  /* If the next two routines are called while the gadget is registered,
> -- 
> 1.7.1

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

2010/7/9 Greg KH <greg@kroah.com>:
> David, any thoughts on these versions of the patch?

So how's the status on this one? I did say that there is
no rush but hate to have it hanging over me. ;)

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

> > David, any thoughts on these versions

Still no fan, since it just duplicates
existing functionality from the composite
framework.  Using  the existing mechanism
seems preferable to me:  you want a serial
number? provide one using the existing scheme
and just make sure it follows the rules.

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Sun, 18 Jul 2010 01:57:30 +0200, David Brownell <david-b@pacbell.net> wrote:
> Still no fan, since it just duplicates
> existing functionality from the composite
> framework.  Using  the existing mechanism
> seems preferable to me:  you want a serial
> number? provide one using the existing scheme
> and just make sure it follows the rules.

Hello David,

I'm still not sure what you mean.

As a matter of fact I think you might be confusing Mass Storage Gadget
with File Storage Gadget.

The first patch in the series merely adds the initialisation for the
iSerialNumber field of the descriptor of Mass Storage Gadget (which
is a composite gadget).  It in no way duplicates functionality of
the composite layer.  As a matter of fact, without this patch, the
iSerialNumber module parameter won't work (not tested, just looked
at the code).

The second patch (by Yann Cantin) adds a serial module parameter to
the File Storage Gadget.  FSG is not a composite gadget so the patch
does not duplicate composite functionality (well it does but it's
irrelevant since FSG cannot use composite anyway).

The third patch is irrelevant in this discussion I believe.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

>   It in no way duplicates functionality of
> the composite layer.

Beyond the obvious "Set serial number" ...


> As a matter of fact, without
> this patch, the
> iSerialNumber module parameter won't work

So submit a bugfix for that alone, making it
work everywhere... 

> (not tested, just looked at the code).

Do you know when it broke?  That code has been
tested in the past, and observed to work.  So if
it's not working now, someone broke it and that
should just be fixed.



> The second patch (by Yann Cantin) adds a serial module
> parameter to
> the File Storage Gadget.  FSG is not a composite
> gadget 

OK, so it should add a module param with the same
name as used elsewhere ... makes sense to me.  If
Alan Acks that patch, go for it.

- Dave

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Mon, 19 Jul 2010 12:08:36 +0200, David Brownell <david-b@pacbell.net> wrote:
>> It in no way duplicates functionality of the composite layer.

> Beyond the obvious "Set serial number" ...

In that case, should all the composite gadgets stop setting
the iManufacturer, iProduct and iSerialNumber?  My understanding
is that the composite module parameters are only meant as a way
to override what the module uses as default.  Using your
rationale, modules should not only stop setting those fields but
also the idVendor and idProduct.

>> As a matter of fact, without this patch, the
>> iSerialNumber module parameter won't work

> So submit a bugfix for that alone, making it
> work everywhere...

iSerialNumber not working is not the main issue.  The main issue
is that the serial number is not set by default which makes
Windows sad (Windows uses serial number to distinguish different
devices with the same vendor and product ID).  The serial number
has to be set by default without the need for user to give a
module parameter.

So even if composite.c were to be modified the code in mass
storage gadget would have to stay anyway.

> Do you know when it broke?  That code has been
> tested in the past, and observed to work.  So if
> it's not working now, someone broke it and that
> should just be fixed.

It never broke.  It was "broken" from the beginning.  Here's part of
composite.c that handles the iSerialNumber:

	if (cdev->desc.iSerialNumber && iSerialNumber)
		string_override(composite->strings,
			cdev->desc.iSerialNumber, iSerialNumber);

As you see, the iSerialNumber module parameter is ignored unless the
gadget driver sets the iSerialNumber field of the device descriptor.
This patch makes mass storage gadget and g_multi set it.  As said
above, this is orthogonal to changing the behaviour of the
iSerialNumber module parameter.


>> The second patch (by Yann Cantin) adds a serial module
>> parameter to
>> the File Storage Gadget.  FSG is not a composite
>> gadget

> OK, so it should add a module param with the same
> name as used elsewhere ...

I was going to propose that but file storage gadget uses names
different from composite anyway (ie. vendor and product instead of
idVendor and idProduct) so I dunno if it's worth it.  As a matter
of fact, "serial" seems to match the other names better.

> makes sense to me.  I Alan Acks that patch, go for it.

I believe Alan already agreed on this patch.  I'm merely updating
it to use the changes introduced by my patch to mass storage and
g_multi.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> >> The second patch (by Yann Cantin) adds a serial module
> >> parameter to
> >> the File Storage Gadget.  FSG is not a composite
> >> gadget
> 
> > OK, so it should add a module param with the same
> > name as used elsewhere ...
> 
> I was going to propose that but file storage gadget uses names
> different from composite anyway (ie. vendor and product instead of
> idVendor and idProduct) so I dunno if it's worth it.  As a matter
> of fact, "serial" seems to match the other names better.
> 
> > makes sense to me.  I Alan Acks that patch, go for it.
> 
> I believe Alan already agreed on this patch.  I'm merely updating
> it to use the changes introduced by my patch to mass storage and
> g_multi.

I have lost track of the current state of that patch.  When you finish
updating it, please post it for my review.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Mon, 19 Jul 2010 16:19:06 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:

>>> OK, so it should add a module param with the same
>>> name as used elsewhere ...

> On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:
>> I was going to propose that but file storage gadget uses names
>> different from composite anyway (ie. vendor and product instead of
>> idVendor and idProduct) so I dunno if it's worth it.  As a matter
>> of fact, "serial" seems to match the other names better.
>>
>> > makes sense to me.  I Alan Acks that patch, go for it.

>> I believe Alan already agreed on this patch.  I'm merely updating
>> it to use the changes introduced by my patch to mass storage and
>> g_multi.

> I have lost track of the current state of that patch.  When you finish
> updating it, please post it for my review.

This is still the v3 patchset (only the first two should be of
interest to you), still want me to resend?

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> > I have lost track of the current state of that patch.  When you finish
> > updating it, please post it for my review.
> 
> This is still the v3 patchset (only the first two should be of
> interest to you), still want me to resend?

Or provide a URL for them in the mailing list archives.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

>>> I have lost track of the current state of that patch.  When you finish
>>> updating it, please post it for my review.

> On Mon, 19 Jul 2010, Michal‚ Nazarewicz wrote:
>> This is still the v3 patchset (only the first two should be of
>> interest to you), still want me to resend?

On Mon, 19 Jul 2010 18:14:01 +0200, Alan Stern wrote:
> Or provide a URL for them in the mailing list archives.

1/3: http://lkml.org/lkml/2010/7/8/317
	Adds serial to mass storage gadget and g_multi introducing
	fsg_string_serial_fill() macro used by abovementioned
	gadgets and file storage gadget.

2/3: http://lkml.org/lkml/2010/7/8/318
	Modification of Yann Cantin's patch, delta in comment for 1/3.

3/3: http://lkml.org/lkml/2010/7/8/316
	Modification of patch for earlier patch for g_multi.  Dunno if
	you want to look at it.  Providing URL for completeness.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> >>> I have lost track of the current state of that patch.  When you finish
> >>> updating it, please post it for my review.
> 
> > On Mon, 19 Jul 2010, Michal‚ Nazarewicz wrote:
> >> This is still the v3 patchset (only the first two should be of
> >> interest to you), still want me to resend?
> 
> On Mon, 19 Jul 2010 18:14:01 +0200, Alan Stern wrote:
> > Or provide a URL for them in the mailing list archives.
> 
> 1/3: http://lkml.org/lkml/2010/7/8/317
> 	Adds serial to mass storage gadget and g_multi introducing
> 	fsg_string_serial_fill() macro used by abovementioned
> 	gadgets and file storage gadget.

Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n 
as an inline routine instead of as a macro, and not try to make it 
separate from fsg_string_serial_fill.

> 2/3: http://lkml.org/lkml/2010/7/8/318
> 	Modification of Yann Cantin's patch, delta in comment for 1/3.

I have only one objection to this patch: The new parameter's name 
should be "serial", not "serial_parm".  (Compare it to all the other 
parameter names.)

> 3/3: http://lkml.org/lkml/2010/7/8/316
> 	Modification of patch for earlier patch for g_multi.  Dunno if
> 	you want to look at it.  Providing URL for completeness.

No comment on that one.

As for the discussion between you and David...  I haven't tried to 
follow the details very carefully.  I get the impression that the two 
of you are talking past each other instead of coming to grips with a 
genuine issue.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Mon, 19 Jul 2010 19:06:32 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
>> 1/3: http://lkml.org/lkml/2010/7/8/317

> Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n
> as an inline routine instead of as a macro, and not try to make it
> separate from fsg_string_serial_fill.

>> 2/3: http://lkml.org/lkml/2010/7/8/318

> I have only one objection to this patch: The new parameter's name
> should be "serial", not "serial_parm".  (Compare it to all the other
> parameter names.)

Will change the two tomorrow.

> As for the discussion between you and David...  I haven't tried to
> follow the details very carefully.  I get the impression that the two
> of you are talking past each other instead of coming to grips with a
> genuine issue.

I'm not entirely sure of what the issue with the patches is really.
It merely adds a serial number to the gadgets using MSF and that's all.

In any event, I am sure we will come to an agreement at one point. :)

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

--- On Mon, 7/19/10, Michał Nazarewicz <m.nazarewicz@samsung.com> wrote:

> I'm not entirely sure of what the issue with
> the patches is really. It merely adds a serial
> number to the gadgets using MSF and that's all.

Go back and read what I wrote then.  The issue is
that THERE ALREADY IS SUCH A MECHANISM.  We neither
need or want another way to do it.  The answer is to
use the existing mechanism correctly.

Plus, you seem to be overlooking the basic need
(for userspace) to manage these IDs so they're
properly unique.  Two gadgets should never end up
using the same serial number.

I tire of repeating these basic points...

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Mon, 19 Jul 2010 19:41:10 +0200, David Brownell <david-b@pacbell.net> wrote:
>> I'm not entirely sure of what the issue with
>> the patches is really. It merely adds a serial
>> number to the gadgets using MSF and that's all.
>
> Go back and read what I wrote then.  The issue is
> that THERE ALREADY IS SUCH A MECHANISM.  We neither
> need or want another way to do it.  The answer is to
> use the existing mechanism correctly.

There is no existing mechanism.  If the module does not set the
iSerialNumber field the iSerialNumber module parameter won't work
and I don't see any other way to set the string.  If there is one,
please show it to me.

> Plus, you seem to be overlooking the basic need
> (for userspace) to manage these IDs so they're
> properly unique.  Two gadgets should never end up
> using the same serial number.

I'm not overlooking that.  I simply consider that a separate issue.
Driver should provide some kind of default (just like File Storage
Gadget) and the fact that user space should override it is another
matter in my opinion.

This is especially true, since the iSerialNumber module parameter
won't work without iSerialNumber set (which I pointed several
times).

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Tue, 20 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> On Mon, 19 Jul 2010 19:41:10 +0200, David Brownell <david-b@pacbell.net> wrote:
> >> I'm not entirely sure of what the issue with
> >> the patches is really. It merely adds a serial
> >> number to the gadgets using MSF and that's all.
> >
> > Go back and read what I wrote then.  The issue is
> > that THERE ALREADY IS SUCH A MECHANISM.  We neither
> > need or want another way to do it.  The answer is to
> > use the existing mechanism correctly.
> 
> There is no existing mechanism.  If the module does not set the
> iSerialNumber field the iSerialNumber module parameter won't work
> and I don't see any other way to set the string.  If there is one,
> please show it to me.

That's clearly a bug.  Change the code so that a module parameter will
always work, even if a function driver doesn't specify a serial number
string.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

> On Tue, 20 Jul 2010, [utf-8] Michał Nazarewicz wrote:
>> There is no existing mechanism.  If the module does not set the
>> iSerialNumber field the iSerialNumber module parameter won't work
>> and I don't see any other way to set the string.  If there is one,
>> please show it to me.

On Tue, 20 Jul 2010 16:07:35 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> That's clearly a bug.  Change the code so that a module parameter will
> always work, even if a function driver doesn't specify a serial number
> string.

Ah!  So that's what I wasn't getting!  I'll get to it then.  Will
send an updated v4 later this week (maybe today).

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

--- On Tue, 7/20/10, Michał Nazarewicz <m.nazarewicz@samsung.com> wrote:

> > Plus, you seem to be overlooking the basic need
> > (for userspace) to manage these IDs so they're
> > properly unique.  Two gadgets should never end
> up
> > using the same serial number.
> 
> I'm not overlooking that.  I simply consider that a
> separate issue.
> Driver should provide some kind of default

It can't do that and guarantee uniqueness.
(Unless you consult the random number system,
which may not be available that early, and is
in any case not appropriate for this task.

Your concept is (still/again) broken.

 (just like File
> Storage
> Gadget) and the fact that user space should
> override it is another
> matter in my opinion.

The value must come from userspace in the first
place, else it can't be correctly managed!!

> 

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

>>> Plus, you seem to be overlooking the basic need
>>> (for userspace) to manage these IDs so they're
>>> properly unique.  Two gadgets should never end
>>> up using the same serial number

> --- On Tue, 7/20/10, Michał Nazarewicz <m.nazarewicz@samsung.com> wrote:
>> I'm not overlooking that.  I simply consider that a separate
>> issue. Driver should provide some kind of default.

On Tue, 20 Jul 2010 17:01:37 +0200, David Brownell <david-b@pacbell.net> wrote:
> It can't do that and guarantee uniqueness.

I never claimed otherwise.

> Your concept is (still/again) broken.

I don't buy that.  The driver works better with a non-unique
serial number then without any.  As such, a default serial
number should, in my opinion, be provided.

>> (just like File Storage Gadget) and the fact that user space
>> should override it is another matter in my opinion.

> The value must come from userspace in the first
> place, else it can't be correctly managed!!

I would say it *should* come from userspace but if userspace fails to
provide one *something* is better then *nothing*.  I see how gadgets
should print a warning if serial is not provided from user space but
other then that I still think gadget should provide some value.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Mon, 19 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> On Mon, 19 Jul 2010 19:06:32 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> 1/3: http://lkml.org/lkml/2010/7/8/317
> 
> > Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n
> > as an inline routine instead of as a macro, and not try to make it
> > separate from fsg_string_serial_fill.
> 
> >> 2/3: http://lkml.org/lkml/2010/7/8/318
> 
> > I have only one objection to this patch: The new parameter's name
> > should be "serial", not "serial_parm".  (Compare it to all the other
> > parameter names.)
> 
> Will change the two tomorrow.

Come to think of it, there was one other issue.  The serial number
parameter is important enough that it should be available even on
builds without CONFIG_USB_FILE_STORAGE_TEST (because for general use,
we must make it possible to set the serial number to a unique value for
every instance of the gadget).  Moving the code around should be fairly
easy.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

> On Mon, 19 Jul 2010, [utf-8] Michal‚ Nazarewicz wrote:
>> 1/3: http://lkml.org/lkml/2010/7/8/317
>> 	Adds serial to mass storage gadget and g_multi introducing
>> 	fsg_string_serial_fill() macro used by abovementioned
>> 	gadgets and file storage gadget.

On Mon, 19 Jul 2010 19:06:32 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n
> as an inline routine instead of as a macro, and not try to make it
> separate from fsg_string_serial_fill.

Not sure what you meant by "make it separate from fsg_string_serial_fill".

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Tue, 20 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> > On Mon, 19 Jul 2010, [utf-8] Michal‚ Nazarewicz wrote:
> >> 1/3: http://lkml.org/lkml/2010/7/8/317
> >> 	Adds serial to mass storage gadget and g_multi introducing
> >> 	fsg_string_serial_fill() macro used by abovementioned
> >> 	gadgets and file storage gadget.
> 
> On Mon, 19 Jul 2010 19:06:32 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
> > Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n
> > as an inline routine instead of as a macro, and not try to make it
> > separate from fsg_string_serial_fill.
> 
> Not sure what you meant by "make it separate from fsg_string_serial_fill".

I mean have a single function, called "fsg_string_serial_fill", instead 
of two separate macros called "fsg_string_serial_fill" and 
"fsg_string_serial_fill_n".

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

On Tue, 20 Jul 2010 16:08:53 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 20 Jul 2010, [utf-8] Michał Nazarewicz wrote:
>
>> > On Mon, 19 Jul 2010, [utf-8] Michal‚ Nazarewicz wrote:
>> >> 1/3: http://lkml.org/lkml/2010/7/8/317
>> >> 	Adds serial to mass storage gadget and g_multi introducing
>> >> 	fsg_string_serial_fill() macro used by abovementioned
>> >> 	gadgets and file storage gadget.
>>
>> On Mon, 19 Jul 2010 19:06:32 +0200, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > Ah, yes.  My personal taste would be to write fsg_string_serial_fill_n
>> > as an inline routine instead of as a macro, and not try to make it
>> > separate from fsg_string_serial_fill.
>>
>> Not sure what you meant by "make it separate from fsg_string_serial_fill".
>
> I mean have a single function, called "fsg_string_serial_fill", instead
> of two separate macros called "fsg_string_serial_fill" and
> "fsg_string_serial_fill_n".

I wanted to keep fsg_string_serial_fill() as a macro so that it can
use ARRAY_SIZE() on the first argument to check the size.  If there
was a single function it would have to explicitly take the length of
the destination array as an argument -- that's what the *_n() function
is for.

The rationale is that not having to use ARRAY_SIZE() is, well,
simpler. ;)

Basically, what you are proposing is to remove the
fsg_string_serial_fill() macro and leave only the *_n() changed to
an inline function and force all callers use sizeof/ARRAY_SIZE().

Am I getting that right?  Personally, I'd leave things like they are
changing the *_n() to a function.  What do you think?

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

--- On Tue, 7/20/10, Michał Nazarewicz <m.nazarewicz@samsung.com> wrote:

> The rationale is that not having to use ARRAY_SIZE() is,
> well, simpler. ;)

And more foolish.  Do work at compile
time, not run time, when it's that easy.

- Dave

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Alan Stern]

On Tue, 20 Jul 2010, [utf-8] Michał Nazarewicz wrote:

> I wanted to keep fsg_string_serial_fill() as a macro so that it can
> use ARRAY_SIZE() on the first argument to check the size.  If there
> was a single function it would have to explicitly take the length of
> the destination array as an argument -- that's what the *_n() function
> is for.
> 
> The rationale is that not having to use ARRAY_SIZE() is, well,
> simpler. ;)

My advice is don't bother.  Let callers give explicitly the size of 
their buffer.  How many other routines in the kernel do an implicit 
ARRAY_SIZE on behalf of their callers?  What if the buffer is passed as 
a pointer instead of as an array?

> Basically, what you are proposing is to remove the
> fsg_string_serial_fill() macro and leave only the *_n() changed to
> an inline function and force all callers use sizeof/ARRAY_SIZE().

Or determine the size in some other way.  Yes.  (And then remove the 
"_n" from the name since it will be unnecessary.)

> Am I getting that right?  Personally, I'd leave things like they are
> changing the *_n() to a function.  What do you think?

See above.

Alan Stern

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[David Brownell]

> In that case, should all the composite gadgets stop
> setting
> the iManufacturer, iProduct 

Certainly not.  Those are interface constants
that don't relate to device instances.  Every
instance of those gadgets should appear the same
(except for instance-specific state like serial#).


and iSerialNumber?

... that's an instance-specific thing which should
be managed properly.  The kernel can't do that, but
userspace can (ensuring the USB host always sees
unique serial IDs for devices).


>  My understanding
> is that the composite module parameters are only meant as a way
> to override what the module uses as default. 

They're meant as a way to specify values that may
not otherwise have been specified, or which need
to be overridden.  Standard module param behavior.


> 
> >> As a matter of fact, without this patch, the
> >> iSerialNumber module parameter won't work
> 
> > So submit a bugfix for that alone, making it
> > work everywhere...
> 
> iSerialNumber not working is not the main issue.  The
> main issue
> is that the serial number is not set by default
> which makes Windows sad (Windows uses serial
> number to distinguish different
> devices with the same vendor and product ID).  The
> serial number
> has to be set by default without the need for user to give

Bullcrap.

Do you understand the basic concept of "managed
identifiers"??  Like serial numbers. They need
 to be explicitly managed to be unique.

So for example two Linux-USB peripherals must never
re-use the same ID.  The way to do that is just not
commit the user error of forgetting to assign one
(or assigning a duplicate).

>            
> As you see, the iSerialNumber module parameter
> is ignored unless the
> gadget driver sets the iSerialNumber field of
> the device descriptor.
> 
So make that gadget code do that ... as I said,
fix such bugs, don't add more breakage.

Re: [PATCHv3 1/3] USB: gadget: mass/file storage: set serial number

[Michał Nazarewicz]

>> As you see, the iSerialNumber module parameter
>> is ignored unless the
>> gadget driver sets the iSerialNumber field of
>> the device descriptor.

On Mon, 19 Jul 2010 16:44:55 +0200, David Brownell <david-b@pacbell.net> wrote:
> So make that gadget code do that ...

That's exactly what I have done.  I have added a code that sets
the iSerialNumber.

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

[PATCHv4 1/5] USB: gadget: composite: Better string override handling

[Michal Nazarewicz]

The iManufatcurer, iProduct and iSerialNumber composite module
parameters are only used when the gadget driver registers
strings for manufacturer, product and serial number.  If the
gadget never bothered to set corresponding fields in USB
device descriptors those module parameters are ignored.

This patch makes the parameters even if the strings ID have
not been assigned.  It also changes the way IDs are
overridden -- what IDs are overridden is now saved in
usb_composite_dev structure -- which makes it unnecessary to
modify the string tables the way previous code did.

What's more, the patch adds default values for manufacturer,
product and serial number:

1. If the iManufatcurer string is not registered by the gadget
   driver, composite will use a "<system> <release> with
   <gadget-name>" string as the manufacturer.

2. If the iProduct string is not registered by the gadget
   driver, composite will try to use
   a usb_composite_device::iProduct value (if gadget driver
   set it) or usb_composite_device::name.

3. If the iSerialNumber string is not registered by the gadget
   driver, and usb_composite_device::needs_serial is set,
   composite will use a fake serial and issue a warning
   informing that userspace failed to provide the
   iSerialNumber module parameter.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: stable <stable@kernel.org>
---
Hello again,

The following patchset include 5 patches.

The first one (the one you are reading ;) ) fixes composite framework
to make iManufatcurer, iProduct and iSerialNumber module parameters
even if gadget drivers did not register IDs for those strings.

It also adds code to composite which provide some defaults for those
strings.  In particular, if gadget driver sets
"usb_composite_device::needs_serial" field composite will provide some
fake serial if gadget driver did not set one and userspace failed to
provide iSerialNumber.

The second patch uses this last feature in mass storage and multi
gadgets.

The next two patches are merely updates of what Greg already have in
his tree.

The last patch adds modifications to Yann Cantin's patch requested by
Alan.


So, David, what do you think about this approach?  Before you NACK
please consider the fact that it allows to move some code from gadget
drivers to the composite framework in the end simplifying everything
overally.


Also, Greg, it's more complex then the previous version so I leave it
up to you whether it'll get into the .35.  If you decide to include it
for .36 rather then .35 consider adding the patches at the beginning
as they won't apply at the end of the queue.


 drivers/usb/gadget/composite.c |   93 ++++++++++++++++++++++++++--------------
 include/linux/usb/composite.h  |   10 ++++
 2 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 391d169..8c1d924 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -69,6 +69,8 @@ static char *iSerialNumber;
 module_param(iSerialNumber, charp, 0);
 MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
 
+static char composite_manufacturer[50];
+
 /*-------------------------------------------------------------------------*/
 
 /**
@@ -599,6 +601,7 @@ static int get_string(struct usb_composite_dev *cdev,
 	struct usb_configuration	*c;
 	struct usb_function		*f;
 	int				len;
+	const char			*str;
 
 	/* Yes, not only is USB's I18N support probably more than most
 	 * folk will ever care about ... also, it's all supported here.
@@ -638,9 +641,28 @@ static int get_string(struct usb_composite_dev *cdev,
 		return s->bLength;
 	}
 
-	/* Otherwise, look up and return a specified string.  String IDs
-	 * are device-scoped, so we look up each string table we're told
-	 * about.  These lookups are infrequent; simpler-is-better here.
+	/* Otherwise, look up and return a specified string.  First
+	 * check if the string has not been overridden.
+	 */
+	if (cdev->manufacturer_override == id)
+		str = iManufacturer ?: composite_manufacturer;
+	else if (cdev->product_override == id)
+		str = iProduct ?: composite->iProduct;
+	else if (cdev->serial_override == id)
+		str = iSerialNumber ?: "0123456789AB";
+	else
+		str = NULL;
+	if (str) {
+		struct usb_gadget_strings strings = {
+			.language = language,
+			.strings  = &(struct usb_string) { 0xff, str }
+		};
+		return usb_gadget_get_string(&strings, 0xff, buf);
+	}
+
+	/* String IDs are device-scoped, so we look up each string
+	 * table we're told about.  These lookups are infrequent;
+	 * simpler-is-better here.
 	 */
 	if (composite->strings) {
 		len = lookup_string(composite->strings, buf, language, id);
@@ -960,26 +982,17 @@ composite_unbind(struct usb_gadget *gadget)
 	composite = NULL;
 }
 
-static void
-string_override_one(struct usb_gadget_strings *tab, u8 id, const char *s)
+static u8 override_id(struct usb_composite_dev *cdev, u8 *desc)
 {
-	struct usb_string		*str = tab->strings;
-
-	for (str = tab->strings; str->s; str++) {
-		if (str->id == id) {
-			str->s = s;
-			return;
-		}
+	if (!*desc) {
+		int ret = usb_string_id(cdev);
+		if (unlikely(ret < 0))
+			WARNING(cdev, "failed to override string ID\n");
+		else
+			*desc = ret;
 	}
-}
 
-static void
-string_override(struct usb_gadget_strings **tab, u8 id, const char *s)
-{
-	while (*tab) {
-		string_override_one(*tab, id, s);
-		tab++;
-	}
+	return *desc;
 }
 
 static int composite_bind(struct usb_gadget *gadget)
@@ -1036,18 +1049,32 @@ static int composite_bind(struct usb_gadget *gadget)
 	cdev->desc = *composite->dev;
 	cdev->desc.bMaxPacketSize0 = gadget->ep0->maxpacket;
 
-	/* strings can't be assigned before bind() allocates the
-	 * releavnt identifiers
-	 */
-	if (cdev->desc.iManufacturer && iManufacturer)
-		string_override(composite->strings,
-			cdev->desc.iManufacturer, iManufacturer);
-	if (cdev->desc.iProduct && iProduct)
-		string_override(composite->strings,
-			cdev->desc.iProduct, iProduct);
-	if (cdev->desc.iSerialNumber && iSerialNumber)
-		string_override(composite->strings,
-			cdev->desc.iSerialNumber, iSerialNumber);
+	/* stirng overrides */
+	if (iManufacturer || !cdev->desc.iManufacturer) {
+		if (!cdev->desc.iManufacturer &&
+		    !*composite_manufacturer)
+			snprintf(composite_manufacturer,
+				 sizeof composite_manufacturer,
+				 "%s %s with %s",
+				 init_utsname()->sysname,
+				 init_utsname()->release,
+				 gadget->name);
+
+		cdev->manufacturer_override =
+			override_id(cdev, &cdev->desc.iManufacturer);
+	}
+
+	if (iProduct || (!cdev->desc.iProduct && composite->iProduct))
+		cdev->product_override =
+			override_id(cdev, &cdev->desc.iProduct);
+
+	if (iSerialNumber ||
+	    (composite->needs_serial && !cdev->desc.iSerialNumber)) {
+		if (!iSerialNumber)
+			WARNING(cdev, "userspace failed to provide iSerialNumber, using fake one\n");
+		cdev->serial_override =
+			override_id(cdev, &cdev->desc.iSerialNumber);
+	}
 
 	status = device_create_file(&gadget->dev, &dev_attr_suspended);
 	if (status)
@@ -1146,6 +1173,8 @@ int usb_composite_register(struct usb_composite_driver *driver)
 	if (!driver || !driver->dev || !driver->bind || composite)
 		return -EINVAL;
 
+	if (!driver->iProduct)
+		driver->iProduct = driver->name;
 	if (!driver->name)
 		driver->name = "composite";
 	composite_driver.function =  (char *) driver->name;
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 139353e..82174e5 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -237,10 +237,15 @@ int usb_add_config(struct usb_composite_dev *,
 /**
  * struct usb_composite_driver - groups configurations into a gadget
  * @name: For diagnostics, identifies the driver.
+ * @iProduct: Used as iProduct override if @dev->iProduct is not set.
+ *	If NULL value of @name is taken.
  * @dev: Template descriptor for the device, including default device
  *	identifiers.
  * @strings: tables of strings, keyed by identifiers assigned during bind()
  *	and language IDs provided in control requests
+ * @needs_serial: set to 1 if the gadget needs userspace to provide
+ * 	a serial number.  If one is not provided, warning will be printed
+ * 	and fake serial number provided.
  * @bind: (REQUIRED) Used to allocate resources that are shared across the
  *	whole device, such as string IDs, and add its configurations using
  *	@usb_add_config().  This may fail by returning a negative errno
@@ -265,8 +270,10 @@ int usb_add_config(struct usb_composite_dev *,
  */
 struct usb_composite_driver {
 	const char				*name;
+	const char				*iProduct;
 	const struct usb_device_descriptor	*dev;
 	struct usb_gadget_strings		**strings;
+	unsigned		needs_serial:1;
 
 	/* REVISIT:  bind() functions can be marked __init, which
 	 * makes trouble for section mismatch analysis.  See if
@@ -331,6 +338,9 @@ struct usb_composite_dev {
 	struct list_head		configs;
 	struct usb_composite_driver	*driver;
 	u8				next_string_id;
+	u8				manufacturer_override;
+	u8				product_override;
+	u8				serial_override;
 
 	/* the gadget driver won't enable the data pullup
 	 * while the deactivation count is nonzero.
-- 
1.7.1

[PATCHv4 2/5] USB: gadget: Use new composite features in some gadgets

[Michal Nazarewicz]

This patch FunctionFS, Mass Storage and Multifunction gadgets
use the new features of composite framework.  Because it
handles default strings there is no longer the need for the
gadgets drivers to handle many of the strings.

This also adds the "needs_serial" to Mass Storage Gadget and
Multifunction Composite Gadget which fills the iSerialNumber
as well.  This in effect, makes some hosts happier.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@dekimo.com>
Cc: stable <stable@kernel.org>
---
 drivers/usb/gadget/g_ffs.c        |   29 ++++---------------
 drivers/usb/gadget/mass_storage.c |   56 +-----------------------------------
 drivers/usb/gadget/multi.c        |   48 +------------------------------
 3 files changed, 10 insertions(+), 123 deletions(-)

diff --git a/drivers/usb/gadget/g_ffs.c b/drivers/usb/gadget/g_ffs.c
index d1af253..4ba24d1 100644
--- a/drivers/usb/gadget/g_ffs.c
+++ b/drivers/usb/gadget/g_ffs.c
@@ -105,8 +105,6 @@ static const struct usb_descriptor_header *gfs_otg_desc[] = {
 /* string IDs are assigned dynamically */
 
 enum {
-	GFS_STRING_MANUFACTURER_IDX,
-	GFS_STRING_PRODUCT_IDX,
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
 	GFS_STRING_RNDIS_CONFIG_IDX,
 #endif
@@ -118,13 +116,7 @@ enum {
 #endif
 };
 
-static       char gfs_manufacturer[50];
-static const char gfs_driver_desc[] = DRIVER_DESC;
-static const char gfs_short_name[]  = DRIVER_NAME;
-
 static struct usb_string gfs_strings[] = {
-	[GFS_STRING_MANUFACTURER_IDX].s = gfs_manufacturer,
-	[GFS_STRING_PRODUCT_IDX].s = gfs_driver_desc,
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
 	[GFS_STRING_RNDIS_CONFIG_IDX].s = "FunctionFS + RNDIS",
 #endif
@@ -201,7 +193,8 @@ static int gfs_bind(struct usb_composite_dev *cdev);
 static int gfs_unbind(struct usb_composite_dev *cdev);
 
 static struct usb_composite_driver gfs_driver = {
-	.name		= gfs_short_name,
+	.name		= DRIVER_NAME,
+	.iProduct	= DRIVER_DESC,
 	.dev		= &gfs_dev_desc,
 	.strings	= gfs_dev_strings,
 	.bind		= gfs_bind,
@@ -281,20 +274,10 @@ static int gfs_bind(struct usb_composite_dev *cdev)
 	gfs_dev_desc.idVendor  = cpu_to_le16(gfs_vendor_id);
 	gfs_dev_desc.idProduct = cpu_to_le16(gfs_product_id);
 
-	snprintf(gfs_manufacturer, sizeof gfs_manufacturer, "%s %s with %s",
-		 init_utsname()->sysname, init_utsname()->release,
-		 cdev->gadget->name);
-	ret = usb_string_id(cdev);
-	if (unlikely(ret < 0))
-		goto error;
-	gfs_strings[GFS_STRING_MANUFACTURER_IDX].id = ret;
-	gfs_dev_desc.iManufacturer = ret;
-
-	ret = usb_string_id(cdev);
-	if (unlikely(ret < 0))
-		goto error;
-	gfs_strings[GFS_STRING_PRODUCT_IDX].id = ret;
-	gfs_dev_desc.iProduct = ret;
+	/* Make sure composite glue gets fresh descriptors each time. */
+	gfs_dev_desc.iManufacturer = 0;
+	gfs_dev_desc.iProduct      = 0;
+	gfs_dev_desc.iSerialNumber = 0;
 
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
 	ret = usb_string_id(cdev);
diff --git a/drivers/usb/gadget/mass_storage.c b/drivers/usb/gadget/mass_storage.c
index 705cc1f..6a1dcd1 100644
--- a/drivers/usb/gadget/mass_storage.c
+++ b/drivers/usb/gadget/mass_storage.c
@@ -98,32 +98,6 @@ static const struct usb_descriptor_header *otg_desc[] = {
 };
 
 
-/* string IDs are assigned dynamically */
-
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-#define STRING_CONFIGURATION_IDX	2
-
-static char manufacturer[50];
-
-static struct usb_string strings_dev[] = {
-	[STRING_MANUFACTURER_IDX].s = manufacturer,
-	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
-	[STRING_CONFIGURATION_IDX].s = "Self Powered",
-	{  } /* end of list */
-};
-
-static struct usb_gadget_strings stringtab_dev = {
-	.language	= 0x0409,	/* en-us */
-	.strings	= strings_dev,
-};
-
-static struct usb_gadget_strings *dev_strings[] = {
-	&stringtab_dev,
-	NULL,
-};
-
-
 
 /****************************** Configurations ******************************/
 
@@ -181,33 +155,6 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 	struct usb_gadget *gadget = cdev->gadget;
 	int status;
 
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
-	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
-	         init_utsname()->sysname, init_utsname()->release,
-	         gadget->name);
-	status = usb_string_id(cdev);
-	if (status < 0)
-		return status;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	msg_device_desc.iManufacturer = status;
-
-	status = usb_string_id(cdev);
-	if (status < 0)
-		return status;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	msg_device_desc.iProduct = status;
-
-	status = usb_string_id(cdev);
-	if (status < 0)
-		return status;
-	strings_dev[STRING_CONFIGURATION_IDX].id = status;
-	msg_config_driver.iConfiguration = status;
-
-	/* register our second configuration */
 	status = usb_add_config(cdev, &msg_config_driver);
 	if (status < 0)
 		return status;
@@ -223,8 +170,9 @@ static int __init msg_bind(struct usb_composite_dev *cdev)
 
 static struct usb_composite_driver msg_driver = {
 	.name		= "g_mass_storage",
+	.iProduct	= DRIVER_DESC,
+	.needs_serial	= 1,
 	.dev		= &msg_device_desc,
-	.strings	= dev_strings,
 	.bind		= msg_bind,
 };
 
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index a930d7f..b9ed05b 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -116,29 +116,6 @@ static const struct usb_descriptor_header *otg_desc[] = {
 };
 
 
-/* string IDs are assigned dynamically */
-
-#define STRING_MANUFACTURER_IDX		0
-#define STRING_PRODUCT_IDX		1
-
-static char manufacturer[50];
-
-static struct usb_string strings_dev[] = {
-	[STRING_MANUFACTURER_IDX].s = manufacturer,
-	[STRING_PRODUCT_IDX].s = DRIVER_DESC,
-	{  } /* end of list */
-};
-
-static struct usb_gadget_strings stringtab_dev = {
-	.language	= 0x0409,	/* en-us */
-	.strings	= strings_dev,
-};
-
-static struct usb_gadget_strings *dev_strings[] = {
-	&stringtab_dev,
-	NULL,
-};
-
 static u8 hostaddr[ETH_ALEN];
 
 
@@ -258,7 +235,6 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 		goto fail1;
 	}
 
-
 	gcnum = usb_gadget_controller_number(gadget);
 	if (gcnum >= 0)
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
@@ -272,27 +248,6 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | 0x0099);
 	}
 
-
-	/* Allocate string descriptor numbers ... note that string
-	 * contents can be overridden by the composite_dev glue.
-	 */
-
-	/* device descriptor strings: manufacturer, product */
-	snprintf(manufacturer, sizeof manufacturer, "%s %s with %s",
-	         init_utsname()->sysname, init_utsname()->release,
-	         gadget->name);
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_MANUFACTURER_IDX].id = status;
-	device_desc.iManufacturer = status;
-
-	status = usb_string_id(cdev);
-	if (status < 0)
-		goto fail2;
-	strings_dev[STRING_PRODUCT_IDX].id = status;
-	device_desc.iProduct = status;
-
 #ifdef USB_ETH_RNDIS
 	/* register our first configuration */
 	status = usb_add_config(cdev, &rndis_config_driver);
@@ -333,8 +288,9 @@ static int __exit multi_unbind(struct usb_composite_dev *cdev)
 
 static struct usb_composite_driver multi_driver = {
 	.name		= "g_multi",
+	.iProduct	= DRIVER_DESC,
+	.needs_serial	= 1,
 	.dev		= &device_desc,
-	.strings	= dev_strings,
 	.bind		= multi_bind,
 	.unbind		= __exit_p(multi_unbind),
 };
-- 
1.7.1

[PATCHv4 3/5] USB: gadget: g_multi: code clean up and refactoring

[Michal Nazarewicz]

The Multifunction Composite Gadget have been cleaned up
and refactored so hopefully it looks prettier and works
at least as good as before changes.

A Kconfig has also been fixed to make it impossible to build
FunctionFS gadget with no configurations.  With this patch, if
RNDIS is not chosen by the user CDC is force-selected.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/gadget/Kconfig |    1 +
 drivers/usb/gadget/multi.c |  230 +++++++++++++++++++++++++++-----------------
 2 files changed, 142 insertions(+), 89 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 375279c..41a8a3e 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -899,6 +899,7 @@ config USB_G_NOKIA
 config USB_G_MULTI
 	tristate "Multifunction Composite Gadget (EXPERIMENTAL)"
 	depends on BLOCK && NET
+	select USB_G_MULTI_CDC if !USB_G_MULTI_RNDIS
 	help
 	  The Multifunction Composite Gadget provides Ethernet (RNDIS
 	  and/or CDC Ethernet), mass storage and ACM serial link
diff --git a/drivers/usb/gadget/multi.c b/drivers/usb/gadget/multi.c
index f2eda4f..8f67524 100644
--- a/drivers/usb/gadget/multi.c
+++ b/drivers/usb/gadget/multi.c
@@ -24,6 +24,7 @@
 
 #include <linux/kernel.h>
 #include <linux/utsname.h>
+#include <linux/module.h>
 
 
 #if defined USB_ETH_RNDIS
@@ -35,14 +36,13 @@
 
 
 #define DRIVER_DESC		"Multifunction Composite Gadget"
-#define DRIVER_VERSION		"2009/07/21"
 
-/*-------------------------------------------------------------------------*/
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_AUTHOR("Michal Nazarewicz");
+MODULE_LICENSE("GPL");
 
-#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
-#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
 
-/*-------------------------------------------------------------------------*/
+/***************************** All the files... *****************************/
 
 /*
  * kbuild is not very cooperative with respect to linking separately
@@ -57,6 +57,8 @@
 #include "config.c"
 #include "epautoconf.c"
 
+#include "f_mass_storage.c"
+
 #include "u_serial.c"
 #include "f_acm.c"
 
@@ -68,13 +70,24 @@
 #endif
 #include "u_ether.c"
 
-#undef DBG     /* u_ether.c has broken idea about macros */
-#undef VDBG    /* so clean up after it */
-#undef ERROR
-#undef INFO
-#include "f_mass_storage.c"
 
-/*-------------------------------------------------------------------------*/
+
+/***************************** Device Descriptor ****************************/
+
+#define MULTI_VENDOR_NUM	0x0525	/* XXX NetChip */
+#define MULTI_PRODUCT_NUM	0xa4ab	/* XXX */
+
+
+enum {
+	__MULTI_NO_CONFIG,
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_RNDIS_CONFIG_NUM,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_CDC_CONFIG_NUM,
+#endif
+};
+
 
 static struct usb_device_descriptor device_desc = {
 	.bLength =		sizeof device_desc,
@@ -82,57 +95,72 @@ static struct usb_device_descriptor device_desc = {
 
 	.bcdUSB =		cpu_to_le16(0x0200),
 
-	/* .bDeviceClass =		USB_CLASS_COMM, */
-	/* .bDeviceSubClass =	0, */
-	/* .bDeviceProtocol =	0, */
-	.bDeviceClass =		0xEF,
+	.bDeviceClass =		USB_CLASS_MISC /* 0xEF */,
 	.bDeviceSubClass =	2,
 	.bDeviceProtocol =	1,
-	/* .bMaxPacketSize0 = f(hardware) */
 
 	/* Vendor and product id can be overridden by module parameters.  */
 	.idVendor =		cpu_to_le16(MULTI_VENDOR_NUM),
 	.idProduct =		cpu_to_le16(MULTI_PRODUCT_NUM),
-	/* .bcdDevice = f(hardware) */
-	/* .iManufacturer = DYNAMIC */
-	/* .iProduct = DYNAMIC */
-	/* NO SERIAL NUMBER */
-	.bNumConfigurations =	1,
-};
-
-static struct usb_otg_descriptor otg_descriptor = {
-	.bLength =		sizeof otg_descriptor,
-	.bDescriptorType =	USB_DT_OTG,
-
-	/* REVISIT SRP-only hardware is possible, although
-	 * it would not be called "OTG" ...
-	 */
-	.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
 };
 
 static const struct usb_descriptor_header *otg_desc[] = {
-	(struct usb_descriptor_header *) &otg_descriptor,
+	(struct usb_descriptor_header *) &(struct usb_otg_descriptor){
+		.bLength =		sizeof(struct usb_otg_descriptor),
+		.bDescriptorType =	USB_DT_OTG,
+
+		/*
+		 * REVISIT SRP-only hardware is possible, although
+		 * it would not be called "OTG" ...
+		 */
+		.bmAttributes =		USB_OTG_SRP | USB_OTG_HNP,
+	},
 	NULL,
 };
 
+enum {
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	MULTI_STRING_RNDIS_CONFIG_IDX,
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	MULTI_STRING_CDC_CONFIG_IDX,
+#endif
+};
 
-static u8 hostaddr[ETH_ALEN];
+static struct usb_string strings_dev[] = {
+#ifdef CONFIG_USB_G_MULTI_RNDIS
+	[MULTI_STRING_RNDIS_CONFIG_IDX].s = "Multifunction with RNDIS",
+#endif
+#ifdef CONFIG_USB_G_MULTI_CDC
+	[MULTI_STRING_CDC_CONFIG_IDX].s   = "Multifunction with CDC ECM",
+#endif
+	{  } /* end of list */
+};
 
+static struct usb_gadget_strings *dev_strings[] = {
+	&(struct usb_gadget_strings){
+		.language	= 0x0409,	/* en-us */
+		.strings	= strings_dev,
+	},
+	NULL,
+};
 
 
 /****************************** Configurations ******************************/
 
-static struct fsg_module_parameters mod_data = {
-	.stall = 1
-};
-FSG_MODULE_PARAMETERS(/* no prefix */, mod_data);
+static struct fsg_module_parameters fsg_mod_data = { .stall = 1 };
+FSG_MODULE_PARAMETERS(/* no prefix */, fsg_mod_data);
+
+static struct fsg_common fsg_common;
 
-static struct fsg_common *fsg_common;
+static u8 hostaddr[ETH_ALEN];
 
 
+/********** RNDIS **********/
+
 #ifdef USB_ETH_RNDIS
 
-static int __init rndis_do_config(struct usb_configuration *c)
+static __ref int rndis_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -149,26 +177,42 @@ static int __init rndis_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration rndis_config_driver = {
-	.label			= "Multifunction Composite (RNDIS + MS + ACM)",
-	.bind			= rndis_do_config,
-	.bConfigurationValue	= 2,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= rndis_do_config,
+		.bConfigurationValue	= MULTI_RNDIS_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_RNDIS_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int rndis_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
+
+/********** CDC ECM **********/
+
 #ifdef CONFIG_USB_G_MULTI_CDC
 
-static int __init cdc_do_config(struct usb_configuration *c)
+static __ref int cdc_do_config(struct usb_configuration *c)
 {
 	int ret;
 
@@ -185,20 +229,33 @@ static int __init cdc_do_config(struct usb_configuration *c)
 	if (ret < 0)
 		return ret;
 
-	ret = fsg_bind_config(c->cdev, c, fsg_common);
+	ret = fsg_bind_config(c->cdev, c, &fsg_common);
 	if (ret < 0)
 		return ret;
 
 	return 0;
 }
 
-static struct usb_configuration cdc_config_driver = {
-	.label			= "Multifunction Composite (CDC + MS + ACM)",
-	.bind			= cdc_do_config,
-	.bConfigurationValue	= 1,
-	/* .iConfiguration = DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	static struct usb_configuration config = {
+		.bind			= cdc_do_config,
+		.bConfigurationValue	= MULTI_CDC_CONFIG_NUM,
+		.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
+	};
+
+	config.label          = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].s;
+	config.iConfiguration = strings_dev[MULTI_STRING_CDC_CONFIG_IDX].id;
+
+	return usb_add_config(cdev, &config);
+}
+
+#else
+
+static int cdc_config_register(struct usb_composite_dev *cdev)
+{
+	return 0;
+}
 
 #endif
 
@@ -207,7 +264,7 @@ static struct usb_configuration cdc_config_driver = {
 /****************************** Gadget Bind ******************************/
 
 
-static int __init multi_bind(struct usb_composite_dev *cdev)
+static int __ref multi_bind(struct usb_composite_dev *cdev)
 {
 	struct usb_gadget *gadget = cdev->gadget;
 	int status, gcnum;
@@ -229,45 +286,42 @@ static int __init multi_bind(struct usb_composite_dev *cdev)
 		goto fail0;
 
 	/* set up mass storage function */
-	fsg_common = fsg_common_from_params(0, cdev, &mod_data);
-	if (IS_ERR(fsg_common)) {
-		status = PTR_ERR(fsg_common);
-		goto fail1;
+	{
+		void *retp;
+		retp = fsg_common_from_params(&fsg_common, cdev, &fsg_mod_data);
+		if (IS_ERR(retp)) {
+			status = PTR_ERR(retp);
+			goto fail1;
+		}
 	}
 
+	/* set bcdDevice */
 	gcnum = usb_gadget_controller_number(gadget);
-	if (gcnum >= 0)
+	if (gcnum >= 0) {
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | gcnum);
-	else {
-		/* We assume that can_support_ecm() tells the truth;
-		 * but if the controller isn't recognized at all then
-		 * that assumption is a bit more likely to be wrong.
-		 */
-		WARNING(cdev, "controller '%s' not recognized\n",
-		        gadget->name);
+	} else {
+		WARNING(cdev, "controller '%s' not recognized\n", gadget->name);
 		device_desc.bcdDevice = cpu_to_le16(0x0300 | 0x0099);
 	}
 
-#ifdef USB_ETH_RNDIS
-	/* register our first configuration */
-	status = usb_add_config(cdev, &rndis_config_driver);
-	if (status < 0)
+	/* register configurations */
+	status = rndis_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-#ifdef CONFIG_USB_G_MULTI_CDC
-	/* register our second configuration */
-	status = usb_add_config(cdev, &cdc_config_driver);
-	if (status < 0)
+	status = cdc_config_register(cdev);
+	if (unlikely(status < 0))
 		goto fail2;
-#endif
 
-	dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
-	fsg_common_put(fsg_common);
+	/* we're done */
+	dev_info(&gadget->dev, DRIVER_DESC "\n");
+	fsg_common_put(&fsg_common);
 	return 0;
 
+
+	/* error recovery */
 fail2:
-	fsg_common_put(fsg_common);
+	fsg_common_put(&fsg_common);
 fail1:
 	gserial_cleanup();
 fail0:
@@ -289,24 +343,22 @@ static int __exit multi_unbind(struct usb_composite_dev *cdev)
 static struct usb_composite_driver multi_driver = {
 	.name		= "g_multi",
 	.iProduct	= DRIVER_DESC,
+	.strings	= dev_strings,
 	.needs_serial	= 1,
 	.dev		= &device_desc,
 	.bind		= multi_bind,
 	.unbind		= __exit_p(multi_unbind),
 };
 
-MODULE_DESCRIPTION(DRIVER_DESC);
-MODULE_AUTHOR("Michal Nazarewicz");
-MODULE_LICENSE("GPL");
 
-static int __init g_multi_init(void)
+static int __init multi_init(void)
 {
 	return usb_composite_register(&multi_driver);
 }
-module_init(g_multi_init);
+module_init(multi_init);
 
-static void __exit g_multi_cleanup(void)
+static void __exit multi_exit(void)
 {
 	usb_composite_unregister(&multi_driver);
 }
-module_exit(g_multi_cleanup);
+module_exit(multi_exit);
-- 
1.7.1

[PATCHv4 4/5] USB: gadget: g_fs: code cleanup

[Michal Nazarewicz]

This commit cleans the g_fs gadget hopefully making it more
readable.  This is achieved by usage of the usb_string_ids_tab()
function for batch string IDs registration as well as
generalising configuration so that a single routine is
used to add each configuration and bind interfaces.  As an
effect, the code is shorter and has fewer #ifdefs.

Moreover, in some circumstances previous code #defined
CONFIG_USB_FUNCTIONFS_GENERIC macro to prevent a situation
where gadget with no configurations is built.  This code removes
the #define form source code and achieves the same effect using
select in Kconfig.

This patch also changes wording and names of the Kconfig options.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/gadget/Kconfig |   23 +++---
 drivers/usb/gadget/g_ffs.c |  163 ++++++++++++--------------------------------
 2 files changed, 56 insertions(+), 130 deletions(-)

diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 41a8a3e..fc55eff 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -750,6 +750,7 @@ config USB_GADGETFS
 config USB_FUNCTIONFS
 	tristate "Function Filesystem (EXPERIMENTAL)"
 	depends on EXPERIMENTAL
+	select USB_FUNCTIONFS_GENERIC if !(USB_FUNCTIONFS_ETH || USB_FUNCTIONFS_RNDIS)
 	help
 	  The Function Filesystem (FunctioFS) lets one create USB
 	  composite functions in user space in the same way as GadgetFS
@@ -758,31 +759,31 @@ config USB_FUNCTIONFS
 	  implemented in kernel space (for instance Ethernet, serial or
 	  mass storage) and other are implemented in user space.
 
+	  If you say "y" or "m" here you will be able what kind of
+	  configurations the gadget will provide.
+
 	  Say "y" to link the driver statically, or "m" to build
 	  a dynamically linked module called "g_ffs".
 
 config USB_FUNCTIONFS_ETH
-	bool "Include CDC ECM (Ethernet) function"
+	bool "Include configuration with CDC ECM (Ethernet)"
 	depends on USB_FUNCTIONFS && NET
 	help
-	  Include an CDC ECM (Ethernet) funcion in the CDC ECM (Funcion)
-	  Filesystem.  If you also say "y" to the RNDIS query below the
-	  gadget will have two configurations.
+	  Include a configuration with CDC ECM funcion (Ethernet) and the
+	  Funcion Filesystem.
 
 config USB_FUNCTIONFS_RNDIS
-	bool "Include RNDIS (Ethernet) function"
+	bool "Include configuration with RNDIS (Ethernet)"
 	depends on USB_FUNCTIONFS && NET
 	help
-	  Include an RNDIS (Ethernet) funcion in the Funcion Filesystem.
-	  If you also say "y" to the CDC ECM query above the gadget will
-	  have two configurations.
+	  Include a configuration with RNDIS funcion (Ethernet) and the Filesystem.
 
 config USB_FUNCTIONFS_GENERIC
 	bool "Include 'pure' configuration"
-	depends on USB_FUNCTIONFS && (USB_FUNCTIONFS_ETH || USB_FUNCTIONFS_RNDIS)
+	depends on USB_FUNCTIONFS
 	help
-	  Include a configuration with FunctionFS and no Ethernet
-	  configuration.
+	  Include a configuration with the Function Filesystem alone with
+	  no Ethernet interface.
 
 config USB_FILE_STORAGE
 	tristate "File-backed Storage Gadget"
diff --git a/drivers/usb/gadget/g_ffs.c b/drivers/usb/gadget/g_ffs.c
index 503712d..a2feb7c 100644
--- a/drivers/usb/gadget/g_ffs.c
+++ b/drivers/usb/gadget/g_ffs.c
@@ -32,12 +32,13 @@
 #  include "u_ether.c"
 
 static u8 gfs_hostaddr[ETH_ALEN];
-#else
-#  if !defined CONFIG_USB_FUNCTIONFS_GENERIC
-#    define CONFIG_USB_FUNCTIONFS_GENERIC
+#  ifdef CONFIG_USB_FUNCTIONFS_ETH
+static int eth_bind_config(struct usb_configuration *c, u8 ethaddr[ETH_ALEN]);
 #  endif
+#else
 #  define gether_cleanup() do { } while (0)
 #  define gether_setup(gadget, hostaddr)   ((int)0)
+#  define gfs_hostaddr NULL
 #endif
 
 #include "f_fs.c"
@@ -104,27 +105,15 @@ static const struct usb_descriptor_header *gfs_otg_desc[] = {
 
 /* string IDs are assigned dynamically */
 
-enum {
-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-	GFS_STRING_RNDIS_CONFIG_IDX,
-#endif
-#ifdef CONFIG_USB_FUNCTIONFS_ETH
-	GFS_STRING_ECM_CONFIG_IDX,
-#endif
-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-	GFS_STRING_GENERIC_CONFIG_IDX,
-#endif
-};
-
 static struct usb_string gfs_strings[] = {
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-	[GFS_STRING_RNDIS_CONFIG_IDX].s = "FunctionFS + RNDIS",
+	{ .s = "FunctionFS + RNDIS" },
 #endif
 #ifdef CONFIG_USB_FUNCTIONFS_ETH
-	[GFS_STRING_ECM_CONFIG_IDX].s = "FunctionFS + ECM",
+	{ .s = "FunctionFS + ECM" },
 #endif
 #ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-	[GFS_STRING_GENERIC_CONFIG_IDX].s = "FunctionFS",
+	{ .s = "FunctionFS" },
 #endif
 	{  } /* end of list */
 };
@@ -138,59 +127,33 @@ static struct usb_gadget_strings *gfs_dev_strings[] = {
 };
 
 
+
+struct gfs_configuration {
+	struct usb_configuration c;
+	int (*eth)(struct usb_configuration *c, u8 *ethaddr);
+} gfs_configurations[] = {
 #ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-static int gfs_do_rndis_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_rndis_config_driver = {
-	.label			= "FunctionFS + RNDIS",
-	.bind			= gfs_do_rndis_config,
-	.bConfigurationValue	= 1,
-	/* .iConfiguration	= DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
-#  define gfs_add_rndis_config(cdev) \
-	usb_add_config(cdev, &gfs_rndis_config_driver)
-#else
-#  define gfs_add_rndis_config(cdev) 0
+	{
+		.eth		= rndis_bind_config,
+	},
 #endif
 
-
 #ifdef CONFIG_USB_FUNCTIONFS_ETH
-static int gfs_do_ecm_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_ecm_config_driver = {
-	.label			= "FunctionFS + ECM",
-	.bind			= gfs_do_ecm_config,
-	.bConfigurationValue	= 1,
-	/* .iConfiguration	= DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
-#  define gfs_add_ecm_config(cdev) \
-	usb_add_config(cdev, &gfs_ecm_config_driver)
-#else
-#  define gfs_add_ecm_config(cdev) 0
+	{
+		.eth		= eth_bind_config,
+	},
 #endif
 
-
 #ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-static int gfs_do_generic_config(struct usb_configuration *c);
-
-static struct usb_configuration gfs_generic_config_driver = {
-	.label			= "FunctionFS",
-	.bind			= gfs_do_generic_config,
-	.bConfigurationValue	= 2,
-	/* .iConfiguration	= DYNAMIC */
-	.bmAttributes		= USB_CONFIG_ATT_SELFPOWER,
-};
-#  define gfs_add_generic_config(cdev) \
-	usb_add_config(cdev, &gfs_generic_config_driver)
-#else
-#  define gfs_add_generic_config(cdev) 0
+	{
+	},
 #endif
+};
 
 
 static int gfs_bind(struct usb_composite_dev *cdev);
 static int gfs_unbind(struct usb_composite_dev *cdev);
+static int gfs_do_config(struct usb_configuration *c);
 
 static struct usb_composite_driver gfs_driver = {
 	.name		= DRIVER_NAME,
@@ -260,7 +223,7 @@ static int functionfs_check_dev_callback(const char *dev_name)
 
 static int gfs_bind(struct usb_composite_dev *cdev)
 {
-	int ret;
+	int ret, i;
 
 	ENTER();
 
@@ -279,45 +242,27 @@ static int gfs_bind(struct usb_composite_dev *cdev)
 	gfs_dev_desc.iProduct      = 0;
 	gfs_dev_desc.iSerialNumber = 0;
 
-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-	ret = usb_string_id(cdev);
-	if (unlikely(ret < 0))
-		goto error;
-	gfs_strings[GFS_STRING_RNDIS_CONFIG_IDX].id = ret;
-	gfs_rndis_config_driver.iConfiguration = ret;
-#endif
-
-#ifdef CONFIG_USB_FUNCTIONFS_ETH
-	ret = usb_string_id(cdev);
-	if (unlikely(ret < 0))
-		goto error;
-	gfs_strings[GFS_STRING_ECM_CONFIG_IDX].id = ret;
-	gfs_ecm_config_driver.iConfiguration = ret;
-#endif
-
-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-	ret = usb_string_id(cdev);
+	ret = usb_string_ids_tab(cdev, gfs_strings);
 	if (unlikely(ret < 0))
 		goto error;
-	gfs_strings[GFS_STRING_GENERIC_CONFIG_IDX].id = ret;
-	gfs_generic_config_driver.iConfiguration = ret;
-#endif
 
 	ret = functionfs_bind(gfs_ffs_data, cdev);
 	if (unlikely(ret < 0))
 		goto error;
 
-	ret = gfs_add_rndis_config(cdev);
-	if (unlikely(ret < 0))
-		goto error_unbind;
+	for (i = 0; i < ARRAY_SIZE(gfs_configurations); ++i) {
+		struct gfs_configuration *c = gfs_configurations + i;
 
-	ret = gfs_add_ecm_config(cdev);
-	if (unlikely(ret < 0))
-		goto error_unbind;
+		c->c.label			= gfs_strings[i].s;
+		c->c.iConfiguration		= gfs_strings[i].id;
+		c->c.bind			= gfs_do_config;
+		c->c.bConfigurationValue	= 1 + i;
+		c->c.bmAttributes		= USB_CONFIG_ATT_SELFPOWER;
 
-	ret = gfs_add_generic_config(cdev);
-	if (unlikely(ret < 0))
-		goto error_unbind;
+		ret = usb_add_config(cdev, &c->c);
+		if (unlikely(ret < 0))
+			goto error_unbind;
+	}
 
 	return 0;
 
@@ -351,10 +296,10 @@ static int gfs_unbind(struct usb_composite_dev *cdev)
 }
 
 
-static int __gfs_do_config(struct usb_configuration *c,
-			   int (*eth)(struct usb_configuration *c, u8 *ethaddr),
-			   u8 *ethaddr)
+static int gfs_do_config(struct usb_configuration *c)
 {
+	struct gfs_configuration *gc =
+		container_of(c, struct gfs_configuration, c);
 	int ret;
 
 	if (WARN_ON(!gfs_ffs_data))
@@ -365,8 +310,8 @@ static int __gfs_do_config(struct usb_configuration *c,
 		c->bmAttributes |= USB_CONFIG_ATT_WAKEUP;
 	}
 
-	if (eth) {
-		ret = eth(c, ethaddr);
+	if (gc->eth) {
+		ret = gc->eth(c, gfs_hostaddr);
 		if (unlikely(ret < 0))
 			return ret;
 	}
@@ -389,32 +334,12 @@ static int __gfs_do_config(struct usb_configuration *c,
 	return 0;
 }
 
-#ifdef CONFIG_USB_FUNCTIONFS_RNDIS
-static int gfs_do_rndis_config(struct usb_configuration *c)
-{
-	ENTER();
-
-	return __gfs_do_config(c, rndis_bind_config, gfs_hostaddr);
-}
-#endif
 
 #ifdef CONFIG_USB_FUNCTIONFS_ETH
-static int gfs_do_ecm_config(struct usb_configuration *c)
-{
-	ENTER();
-
-	return __gfs_do_config(c,
-			       can_support_ecm(c->cdev->gadget)
-			     ? ecm_bind_config : geth_bind_config,
-			       gfs_hostaddr);
-}
-#endif
-
-#ifdef CONFIG_USB_FUNCTIONFS_GENERIC
-static int gfs_do_generic_config(struct usb_configuration *c)
+static int eth_bind_config(struct usb_configuration *c, u8 ethaddr[ETH_ALEN])
 {
-	ENTER();
-
-	return __gfs_do_config(c, NULL, NULL);
+	return can_support_ecm(c->cdev->gadget)
+		? ecm_bind_config(c, ethaddr)
+		: geth_bind_config(c, ethaddr);
 }
 #endif
-- 
1.7.1

[PATCHv4 5/5] USB: gadget: file_storage: serial parameter even if not test mode

[Michal Nazarewicz]

Moved the serial parameter handling code out of "#ifdef
CONFIG_USB_FILE_STORAGE_TEST".

This modifies Yann Cantin's commit "USB: Add a serial number
parameter to g_file_storage" module as per Alan Stern's request.

Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Yann Cantin <yann.cantin@laposte.net>
---
Alan Stern wrote:
> I have only one objection to this [Yann Cantin's] patch: The new
> parameter's name should be "serial", not "serial_parm".

Alan Stern wrote:
> The serial number parameter is important enough that it should be
> available even on builds without CONFIG_USB_FILE_STORAGE_TEST.

 drivers/usb/gadget/file_storage.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
index d57c09f..41af34c 100644
--- a/drivers/usb/gadget/file_storage.c
+++ b/drivers/usb/gadget/file_storage.c
@@ -317,7 +317,7 @@ static struct {
 	unsigned short	vendor;
 	unsigned short	product;
 	unsigned short	release;
-	char		*serial_parm;
+	char		*serial;
 	unsigned int	buflen;
 
 	int		transport_type;
@@ -357,6 +357,8 @@ MODULE_PARM_DESC(stall, "false to prevent bulk stalls");
 module_param_named(cdrom, mod_data.cdrom, bool, S_IRUGO);
 MODULE_PARM_DESC(cdrom, "true to emulate cdrom instead of disk");
 
+module_param_named(serial, mod_data.serial, charp, S_IRUGO);
+MODULE_PARM_DESC(serial, "USB serial number");
 
 /* In the non-TEST version, only the module parameters listed above
  * are available. */
@@ -378,9 +380,6 @@ MODULE_PARM_DESC(product, "USB Product ID");
 module_param_named(release, mod_data.release, ushort, S_IRUGO);
 MODULE_PARM_DESC(release, "USB release number");
 
-module_param_named(serial, mod_data.serial_parm, charp, S_IRUGO);
-MODULE_PARM_DESC(serial, "USB serial number");
-
 module_param_named(buflen, mod_data.buflen, uint, S_IRUGO);
 MODULE_PARM_DESC(buflen, "I/O buffer size");
 
@@ -3281,10 +3280,12 @@ static int __init check_parameters(struct fsg_dev *fsg)
 		return -ETOOSMALL;
 	}
 
+#endif /* CONFIG_USB_FILE_STORAGE_TEST */
+
 	/* Serial string handling.
 	 * On a real device, the serial string would be loaded
 	 * from permanent storage. */
-	if (mod_data.serial_parm) {
+	if (mod_data.serial) {
 		const char *ch;
 		unsigned len = 0;
 
@@ -3293,7 +3294,7 @@ static int __init check_parameters(struct fsg_dev *fsg)
 		 * 12 uppercase hexadecimal characters.
 		 * BBB need at least 12 uppercase hexadecimal characters,
 		 * with a maximum of 126. */
-		for (ch = mod_data.serial_parm; *ch; ++ch) {
+		for (ch = mod_data.serial; *ch; ++ch) {
 			++len;
 			if ((*ch < '0' || *ch > '9') &&
 			    (*ch < 'A' || *ch > 'F')) { /* not uppercase hex */
@@ -3312,8 +3313,11 @@ static int __init check_parameters(struct fsg_dev *fsg)
 				"Failing back to default\n");
 			goto fill_serial;
 		}
-		fsg_strings[FSG_STRING_SERIAL - 1].s = mod_data.serial_parm;
+		fsg_strings[FSG_STRING_SERIAL - 1].s = mod_data.serial;
 	} else {
+		WARNING(fsg,
+			"Userspace failed to provide serial number; "
+			"Failing back to default\n");
 fill_serial:
 		/* Serial number not specified or invalid, make our own.
 		 * We just encode it from the driver version string,
@@ -3329,8 +3333,6 @@ fill_serial:
 		}
 	}
 
-#endif /* CONFIG_USB_FILE_STORAGE_TEST */
-
 	return 0;
 }
 
-- 
1.7.1

Re: [PATCHv4 5/5] USB: gadget: file_storage: serial parameter even if not test mode

[Alan Stern]

On Thu, 22 Jul 2010, Michal Nazarewicz wrote:

> Moved the serial parameter handling code out of "#ifdef
> CONFIG_USB_FILE_STORAGE_TEST".
> 
> This modifies Yann Cantin's commit "USB: Add a serial number
> parameter to g_file_storage" module as per Alan Stern's request.
> 
> Signed-off-by: Michal Nazarewicz <m.nazarewicz@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: Yann Cantin <yann.cantin@laposte.net>
> ---
> Alan Stern wrote:
> > I have only one objection to this [Yann Cantin's] patch: The new
> > parameter's name should be "serial", not "serial_parm".
> 
> Alan Stern wrote:
> > The serial number parameter is important enough that it should be
> > available even on builds without CONFIG_USB_FILE_STORAGE_TEST.
> 
>  drivers/usb/gadget/file_storage.c |   20 +++++++++++---------
>  1 files changed, 11 insertions(+), 9 deletions(-)

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCHv4 1/5] USB: gadget: composite: Better string override handling

[Greg KH]

On Thu, Jul 22, 2010 at 02:16:33PM +0200, Michal Nazarewicz wrote:
> The iManufatcurer, iProduct and iSerialNumber composite module
> parameters are only used when the gadget driver registers
> strings for manufacturer, product and serial number.  If the
> gadget never bothered to set corresponding fields in USB
> device descriptors those module parameters are ignored.
> 
> This patch makes the parameters even if the strings ID have
> not been assigned.  It also changes the way IDs are
> overridden -- what IDs are overridden is now saved in
> usb_composite_dev structure -- which makes it unnecessary to
> modify the string tables the way previous code did.

David, do these look better?

And they are not -stable material, no matter what, sorry, so you can
stop copying stable@kernel.org on them.

thanks,

greg k-h

Re: [PATCHv4 1/5] USB: gadget: composite: Better string override handling

[Michał Nazarewicz]

On Fri, 23 Jul 2010 01:46:30 +0200, Greg KH <greg@kroah.com> wrote:
> And they are not -stable material, no matter what, sorry, so you can
> stop copying stable@kernel.org on them.

True enough.  I probably haven't given enough thought copying the Cc.
Sorry about that.  Removing the line from the patch should solve the
issue nicely. ;)

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Michał "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

Re: [PATCHv4 1/5] USB: gadget: composite: Better string override handling

[David Brownell]

> David, do these look better?

Can I get a URL to a specific patch?  Or
a re-post

Re: [PATCHv4 1/5] USB: gadget: composite: Better string override handling

[Greg KH]

On Thu, Jul 22, 2010 at 02:16:33PM +0200, Michal Nazarewicz wrote:
> Also, Greg, it's more complex then the previous version so I leave it
> up to you whether it'll get into the .35.  If you decide to include it
> for .36 rather then .35 consider adding the patches at the beginning
> as they won't apply at the end of the queue.

I've just queued up the last one for the .36 merge window, and I already
had 2 others in my tree for .36, so that's good.

I'll wait to get David's approval for your first patch before applying
it.

thanks,

greg k-h