[U-Boot] drivers: usb: dfu: set serial number from board code was: [ANN] U-Boot v2017.05-rc2 released
Heiko Schocher
hs at denx.de
Mon Jun 26 10:50:34 UTC 2017
Hello Felipe, Lukasz,
Am 26.06.2017 um 12:22 schrieb Felipe Balbi:
>
> Hi,
>
> Lukasz Majewski <lukma at 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 at 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 at 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 at 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 at denx.de>
>
> looks good to me.
Acked-by: Heiko Schocher <hs at denx.de>
@Lukasz: Do you send this as a patch? (Or did I missed it?)
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
>>
>
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
More information about the U-Boot
mailing list