[U-Boot] drivers: usb: dfu: set serial number from board code was: [ANN] U-Boot v2017.05-rc2 released

[Felipe Balbi <felipe.balbi@linux.intel.com>]

Hi,

Lukasz Majewski <lukma@denx.de> writes:
>> >>>>>>> My weekly dfu test on the siemens smartweb board failed with
>> >>>>>>> current HEAD.
>> >>>>>>>
>> >>>>>>> I started an automated git bisect with tbot, and found:
>> >>>>>>>
>> >>>>>>> 2017-04-19 07:24:30,717:CON    :tbotlib   # tb_ctrl: git
>> >>>>>>> bisect visualize
>> >>>>>>> 2017-04-19 07:24:30,783:CON    :tbotlib   # tb_ctrl: commit
>> >>>>>>> 842778a091047b0c868efa12229633959f711152
>> >>>>>>> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
>> >>>>>>> Date:   Wed Feb 22 17:12:40 2017 +0200
>> >>>>>>>        usb: gadget: g_dnl: only set iSerialNumber if we have a
>> >>>>>>> serial#
>> >>>>>>>
>> >>>>>>>        We don't want to claim that we support a serial number
>> >>>>>>> string and
>> >>>>>>>        later return nothing. Because of that, if g_dnl_serial
>> >>>>>>> is an empty
>> >>>>>>>        string, let's skip setting iSerialNumber to a valid
>> >>>>>>> number.
>> >>>>>>>
>> >>>>>>>        Signed-off-by: Felipe Balbi
>> >>>>>>> <felipe.balbi@linux.intel.com> hs at pollux [ 7:24:30] ttbott>
>> >>>>>>> 2017-04-19 07:24:31,769:CON    :tbotlib   # tb_ctrl: git
>> >>>>>>> bisect log 2017-04-19 07:24:31,836:CON    :tbotlib   #
>> >>>>>>> tb_ctrl: git bisect start
>> >>>>> [...]
>> >>>>>>>
>> >>>>>>> Any ideas?
>> >>>>>>
>> >>>>>> Is your board setting up the serial number with
>> >>>>>> g_dnl_set_serialnumber()
>> >>>>>> correctly ?
>> >>>>>
>> >>>>> Hmm.. good question ... its done here:
>> >>>>>
>> >>>>> http://git.denx.de/?p=u-boot.git;a=blob;f=board/siemens/common/factoryset.c;h=6c869ed2b035a0e9f840e1f6f960fe0e6ac824e5;hb=f6c1df44b815a08585e7fd3805a1db51a5955d09#l313
>> >>>>>
>> >>>>>
>> >>>>>
>> >>>>> but may this does not work correct and now pops up.
>> >>>>>
>> >>>>> I try to find out more, thanks for the hint!
>> >>>>
>> >>>> Just check if you're not passing in NULL or empty string, that
>> >>>> might be it. Otherwise the USB code is botched.
>> >>>
>> >>> Hmm... OK, on the smartweb board there is no factory set, so never
>> >>> calling g_dnl_set_serialnumber()
>> >>>
>> >>> :-(
>> >>>
>> >>> why did this worked before commit 842778a091?
>> >>>
>> >>> So, I added for a fast dirty test:
>> >>>
>> >>> diff --git a/board/siemens/smartweb/smartweb.c
>> >>> b/board/siemens/smartweb/smartweb.c
>> >>> index 78a7946..01a3dd2 100644
>> >>> --- a/board/siemens/smartweb/smartweb.c
>> >>> +++ b/board/siemens/smartweb/smartweb.c
>> >>> @@ -34,6 +34,7 @@
>> >>>   #ifndef CONFIG_DM_ETH
>> >>>   # include <netdev.h>
>> >>>   #endif
>> >>> +#include <g_dnl.h>
>> >>>
>> >>>   DECLARE_GLOBAL_DATA_PTR;
>> >>>
>> >>> @@ -265,3 +266,17 @@ U_BOOT_DEVICE(at91sam9260_serial) = {
>> >>>          .name   = "serial_atmel",
>> >>>          .platdata = &at91sam9260_serial_plat,
>> >>>   };
>> >>> +
>> >>> +int g_dnl_bind_fixup(struct usb_device_descriptor *dev, const
>> >>> char *name) +{
>> >>> +       printf("%s: *********\n", __func__);
>> >>> +       g_dnl_set_serialnumber("0123456789");
>> >>> +
>> >>> +       return 0;
>> >>> +}
>> >>> +
>> >>> +int g_dnl_get_board_bcd_device_number(int gcnum)
>> >>> +{
>> >>> +       return 0;
>> >>> +}
>> >>>
>> >>> Now I see this printf:
>> >>> (also enabled debug in ./drivers/usb/gadget/g_dnl.c)
>> >>>
>> >>> dfu 0 nand 0
>> >>> using id 'nand0,4'
>> >>> g_dnl_register: g_dnl_driver.name = usb_dnl_dfu
>> >>> g_dnl_bind: gadget: 0x23adf6c0 cdev: 0x23b262d0
>> >>> g_dnl_bind_fixup: *********
>> >>> g_dnl_do_config: configuration: 0x23b263c0 composite dev:
>> >>> 0x23b262d0 g_dnl_bind: calling usb_gadget_connect for controller
>> >>> 'at91_udc'
>> >>>
>> >>> but result is the same:
>> >>> # ./src/dfu-util -l
>> >>> dfu-util 0.7
>> >>>
>> >>> Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
>> >>> Copyright 2010-2012 Tormod Volden and Stefan Schmidt
>> >>> This program is Free Software and has ABSOLUTELY NO WARRANTY
>> >>> Please report bugs to dfu-util at lists.gnumonks.org
>> >>> tb_ctrl: Found DFU: [0908:02d2] ver=0000, devnum=0, cfg=1, intf=0,
>> >>> alt=0, name="Linux", serial="UNDEFINED"
>> >>>
>> >>> reverting commit 842778a091 and it works as before ... console
>> >>> output for this case:
>> >>>
>> >>> ./src/dfu-util -l
>> >>> dfu-util 0.7
>> >>>
>> >>> Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
>> >>> Copyright 2010-2012 Tormod Volden and Stefan Schmidt
>> >>> This program is Free Software and has ABSOLUTELY NO WARRANTY
>> >>> Please report bugs to dfu-util at lists.gnumonks.org
>> >>> tb_ctrl: Found DFU: [0908:02d2] ver=0000, devnum=0, cfg=1, intf=0,
>> >>> alt=0, name="Linux", serial="0123456789"
>> >>>
>> >>> Ok, before commit 842778a091 is in mainline I had the follwoing
>> >>> output:
>> >>>
>> >>> # tb_ctrl: ./src/dfu-util -l
>> >>> # tb_ctrl: dfu-util 0.7
>> >>>
>> >>> Copyright 2005-2008 Weston Schmidt, Harald Welte and OpenMoko Inc.
>> >>> Copyright 2010-2012 Tormod Volden and Stefan Schmidt
>> >>> This program is Free Software and has ABSOLUTELY NO WARRANTY
>> >>> Please report bugs to dfu-util at lists.gnumonks.org
>> >>>
>> >>> Found DFU: [0908:02d2] ver=0212, devnum=0, cfg=1, intf=0, alt=0,
>> >>>      name="Linux", serial=""
>> >>>
>> >>> serial is an empty string ... It seems to me, that commit
>> >>> 842778a091 broke here something fundamental ...
>> >>>
>> >>> Hmm ... looking into drivers/usb/gadget/g_dnl.c g_dnl_bind()
>> >>>
>> >>> if (strlen(g_dnl_serial)) {
>> >>>
>> >>> is *before* g_dnl_bind_fixup() is called ... ?
>> >>>
>> >>> Yup, with patch:
>> >>>
>> >>> diff --git a/drivers/usb/gadget/g_dnl.c
>> >>> b/drivers/usb/gadget/g_dnl.c index d4bee9b..813d4b8 100644
>> >>> --- a/drivers/usb/gadget/g_dnl.c
>> >>> +++ b/drivers/usb/gadget/g_dnl.c
>> >>> @@ -224,6 +224,7 @@ static int g_dnl_bind(struct usb_composite_dev
>> >>> *cdev) g_dnl_string_defs[1].id = id;
>> >>>          device_desc.iProduct = id;
>> >>>
>> >>> +       g_dnl_bind_fixup(&device_desc, cdev->driver->name);
>> >>>          if (strlen(g_dnl_serial)) {
>> >>>                  id = usb_string_id(cdev);
>> >>>                  if (id < 0)
>> >>> @@ -233,7 +234,6 @@ static int g_dnl_bind(struct usb_composite_dev
>> >>> *cdev) device_desc.iSerialNumber = id;
>> >>>          }
>> >>>
>> >>> -       g_dnl_bind_fixup(&device_desc, cdev->driver->name);
>> >>>          ret = g_dnl_config_register(cdev);
>> >>>          if (ret)
>> >>>                  goto error;
>> >>>
>> >>> and adding g_dnl_bind_fixup() in board/siemens/smartweb/smartweb.c
>> >>> dfu work again for the smartweb board ... is this an valid fix?
>> >>>
>> >>> BTW: is an empty serial string not allowed?
>> >>
>> >> This is Lukasz's domain of expertise, so I'll pull out of this
>> >> discussion and wait for a PR from him.
>> >>
>> >
>> > The problem is with providing "iSerialNumber" visible (at USB
>> > descriptor) only when it is defined.
>> >
>> > Before the Felipe commit (SHA1: 842778a091)  we had exposed it
>> > unconditionally - even when we had a zeroed char table (which was
>> > the "" string)
>> >
>> > Now we do not have the iStringNumber field defined in descriptor
>> > when the "serial" string size is zero.
>> >
>> > I'm wondering which behavior is correct - i.e. comply with the USB
>> > standard.
>> >
>> > Felipe - have you tried to run the USB compliance tests [1] (Windows
>> > one) after applying this patch?
>
> I was waiting for Felipe answer.....

sorry, I completely missed this.

if iSerialNumber is set to a non-zero value, then the actual string
should exist. if the string is empty, then iSerialNumber should be
cleared to zero in the device descriptor.

>> > [1] http://www.usb.org/developers/tools/usb20_tools/
>> >
>> > Best regards,
>> >
>> > Lukasz Majewski
>> 
>> How to proceed here? If the current behaviour of U-Boot is correct,
>> then I simple adapt my tbot testcase ... 
>
> ... but none was given.
>
> However, IMHO it is better to not expose the string when it is empty.

right

>> but I think, currently we
>> have no way to set the serial number field, or?
>
> :-( Yes, this commit has introduced regression (the g_dnl_serial is
> always NULL there because we setup the serial number in a latter
> function:
>
> g_dnl_bind_fixup @ for example board/siemens/common/factoryreset.c
>
> which calls: g_dnl_set_serialnumber() and only there the g_dnl_serial
> is set.
>
> Please find attached patch (if it fixes siemens boards).

> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> From 5af562a7b43184ea5ab5bb5a18ff3b14f48b2475 Mon Sep 17 00:00:00 2001
> From: Lukasz Majewski <lukma@denx.de>
> Date: Mon, 26 Jun 2017 12:15:09 +0200
> Subject: [PATCH] usb: gadget: Call g_dnl_bind_fixup() before testing
>  g_dnl_serial length
>
> After the commit SHA1: 842778a091 - the serial number descriptor is only
> visible when we have non zero length of g_dnl_serial.
>
> However, on some platforms (e.g. Siemens) the serial number is set at
> g_dnl_bind_fixup(), so with the current code we will always omit the
> serial (since it is not set).
>
> This commit moves the g_dnl_bind_fixup() call before the g_dnl_serial
> length test.
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>

looks good to me.

> ---
>  drivers/usb/gadget/g_dnl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/g_dnl.c b/drivers/usb/gadget/g_dnl.c
> index d4bee9b..0491a0e 100644
> --- a/drivers/usb/gadget/g_dnl.c
> +++ b/drivers/usb/gadget/g_dnl.c
> @@ -224,6 +224,8 @@ static int g_dnl_bind(struct usb_composite_dev *cdev)
>  	g_dnl_string_defs[1].id = id;
>  	device_desc.iProduct = id;
>  
> +	g_dnl_bind_fixup(&device_desc, cdev->driver->name);
> +
>  	if (strlen(g_dnl_serial)) {
>  		id = usb_string_id(cdev);
>  		if (id < 0)
> @@ -233,7 +235,6 @@ static int g_dnl_bind(struct usb_composite_dev *cdev)
>  		device_desc.iSerialNumber = id;
>  	}
>  
> -	g_dnl_bind_fixup(&device_desc, cdev->driver->name);
>  	ret = g_dnl_config_register(cdev);
>  	if (ret)
>  		goto error;
> -- 
> 2.1.4
>

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170626/47ce3c3d/attachment.sig>