| Message ID | 20170626121706.611f417a@jawa |
|---|---|
| State | Accepted |
| Delegated to: | Lukasz Majewski |
| Headers | show |
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@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@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@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@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@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. > --- > 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 >
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@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@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@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@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@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?) 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 >> >
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@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@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@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@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@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@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> --- 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