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

[Lukasz Majewski <lukma@denx.de>]

On Mon, 26 Jun 2017 12:50:34 +0200
Heiko Schocher <hs@denx.de> wrote:

> Hello Felipe, Lukasz,
> 
> Am 26.06.2017 um 12:22 schrieb Felipe Balbi:
> >
> > 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.
> 
> Acked-by: Heiko Schocher <hs@denx.de>
> 
> @Lukasz: Do you send this as a patch? (Or did I missed it?)

I will send it immediately to ML :-)

> 
> Thanks!
> 
> bye,
> Heiko
> >
> >> ---
> >>   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
> >>
> >
> 




Best regards,

Lukasz Majewski

--

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