From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lukasz Majewski Date: Mon, 26 Jun 2017 12:17:06 +0200 Subject: [U-Boot] drivers: usb: dfu: set serial number from board code was: [ANN] U-Boot v2017.05-rc2 released In-Reply-To: <5950A031.1020100@denx.de> References: <20170417222227.GC19487@bill-the-cat> <58F6F5D4.2010006@denx.de> <5350982a-c450-9654-8e8d-d3734b01bf6f@denx.de> <58F7320E.9050706@denx.de> <58F73E66.2040600@denx.de> <56d82281-5bbf-7e1b-fc6e-bc373cf3694e@denx.de> <20170419140733.221f5010@jawa> <5950A031.1020100@denx.de> Message-ID: <20170626121706.611f417a@jawa> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Heiko, > 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 > >>>>>>> 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 > >>>>>>> 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 > >>> #endif > >>> +#include > >>> > >>> 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..... > > > > > > > > [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. > 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). > > bye, > Heiko 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-usb-gadget-Call-g_dnl_bind_fixup-before-testing-g_dn.patch Type: text/x-patch Size: 1432 bytes Desc: not available URL: