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

From: Greg KH
Date: Fri Jul 09 2010 - 15:05:20 EST

Next message: Pavel Machek: "Re: high power consumption in recent kernels"
Previous message: Fernando Lopez-Lezcano: "Re: 2.6.33.5 rt23: machine lockup (nfs/autofs related?)"
In reply to: Michal Nazarewicz: "[PATCHv3 3/3] USB: gadget: g_multi: code clean up and refactoring"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> Reported-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@xxxxxxxxxx>
> Tested-by: Dries Van Puymbroeck <Dries.VanPuymbroeck@xxxxxxxxxx>
> Cc: stable <stable@xxxxxxxxxx>
> ---
> > 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@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Pavel Machek: "Re: high power consumption in recent kernels"
Previous message: Fernando Lopez-Lezcano: "Re: 2.6.33.5 rt23: machine lockup (nfs/autofs related?)"
In reply to: Michal Nazarewicz: "[PATCHv3 3/3] USB: gadget: g_multi: code clean up and refactoring"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]