[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 05:48:33 UTC 2017
Hello all,
Am 19.04.2017 um 14:07 schrieb Lukasz Majewski:
> Dear All,
>
>> On 04/19/2017 12:39 PM, Heiko Schocher wrote:
>>> Hello Marek,
>>>
>>> Am 19.04.2017 um 11:51 schrieb Marek Vasut:
>>>> On 04/19/2017 11:46 AM, Heiko Schocher wrote:
>>>>> Hello Marek,
>>>>>
>>>>> Am 19.04.2017 um 10:43 schrieb Marek Vasut:
>>>>>> On 04/19/2017 07:29 AM, Heiko Schocher wrote:
>>>>>>> Hello Tom,
>>>>>>>
>>>>>>> added Lukasz, Marek and Felipe,
>>>>>>>
>>>>>>> Am 18.04.2017 um 00:22 schrieb Tom Rini:
>>>>>>>> Hey all,
>>>>>>>>
>>>>>>>> It's release day and v2017.05-rc2 is out. I think my patchwork
>>>>>>>> queue is
>>>>>>>> looking good currently. I have some outstanding removal
>>>>>>>> patches to take
>>>>>>>> from Masahiro related to architectures that I removed as
>>>>>>>> promised. The
>>>>>>>> release is bigger than I really wanted, but since I was on
>>>>>>>> vacation for
>>>>>>>> most of the normal -rc1 window, stuff came in that would have
>>>>>>>> come in then now, instead. Things are on track for -rc3 on
>>>>>>>> the 1st.
>>>>>>>
>>>>>>> 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?
>
>
>
> [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 I think, currently we
have no way to set the serial number field, or?
bye,
Heiko
--
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