[U-Boot] drivers: usb: dfu: set serial number from board code was: [ANN] U-Boot v2017.05-rc2 released
Lukasz Majewski
lukma at denx.de
Mon Jun 26 10:17:06 UTC 2017
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 <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.....
> >
> >
> >
> > [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: <http://lists.denx.de/pipermail/u-boot/attachments/20170626/fb2a3862/attachment.bin>
More information about the U-Boot
mailing list