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;... >> >> >> >> 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