[U-Boot] [PATCH 0/5] dm: core: Fix up test failures
While implementing the USB tests some strange failures crept in. I was not able to debug them at the time and they were seemingly independent of the USB tests. I then forgot about it.
I have now tracked these failures down. This series corrects them so that the driver model tests run from start to finish.
The main problems are:
- recursive calls to uclass device_unbind() are not handled from within uclass_destroy() - dm_test_uclass_before_ready() can stop sandbox from working correctly for tests that run after it
This series fixes these as well as a minor LCD console problem with sandbox. It also updates the driver model README for recently added tests.
Simon Glass (5): dm: core: Handle recursive unbinding of uclass devices dm: usb: Add a terminator to the string destructor list lcd: Call lcd_sync() after completing the scroll dm: Update the README to reflect the current test output dm: test: Don't clear global_data in dm_test_uclass_before_ready()
common/lcd_console.c | 2 +- doc/driver-model/README.txt | 58 ++++++++++++++++++++++++++++++++++++------ drivers/core/uclass.c | 12 +++++++-- drivers/usb/emul/sandbox_hub.c | 1 + test/dm/core.c | 6 +++-- 5 files changed, 66 insertions(+), 13 deletions(-)
Since a device can have children in the same uclass as itself, we need to handle unbinding carefully: we must allow that unbinding a device in a uclass may cause another device in the same uclass to be unbound.
Adjust the code to cope.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/core/uclass.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c index 98c15e5..45fcd08 100644 --- a/drivers/core/uclass.c +++ b/drivers/core/uclass.c @@ -99,10 +99,18 @@ fail_mem: int uclass_destroy(struct uclass *uc) { struct uclass_driver *uc_drv; - struct udevice *dev, *tmp; + struct udevice *dev; int ret;
- list_for_each_entry_safe(dev, tmp, &uc->dev_head, uclass_node) { + /* + * We cannot use list_for_each_entry_safe() here. If a device in this + * uclass has a child device also in this uclass, it will be also be + * unbound (by the recursion in the call to device_unbind() below). + * We can loop until the list is empty. + */ + while (!list_empty(&uc->dev_head)) { + dev = list_first_entry(&uc->dev_head, struct udevice, + uclass_node); ret = device_remove(dev); if (ret) return ret;
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
Since a device can have children in the same uclass as itself, we need to handle unbinding carefully: we must allow that unbinding a device in a uclass may cause another device in the same uclass to be unbound.
Adjust the code to cope.
Signed-off-by: Simon Glass sjg@chromium.org
This fixed the seg fault I was seeing. Thanks!
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
On 21 April 2015 at 09:30, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
Since a device can have children in the same uclass as itself, we need to handle unbinding carefully: we must allow that unbinding a device in a uclass may cause another device in the same uclass to be unbound.
Adjust the code to cope.
Signed-off-by: Simon Glass sjg@chromium.org
This fixed the seg fault I was seeing. Thanks!
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
Applied to u-boot-dm.
The terminator is missing. Add it for completeness.
Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/usb/emul/sandbox_hub.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/usb/emul/sandbox_hub.c b/drivers/usb/emul/sandbox_hub.c index 280c708..baf8bdc 100644 --- a/drivers/usb/emul/sandbox_hub.c +++ b/drivers/usb/emul/sandbox_hub.c @@ -32,6 +32,7 @@ static struct usb_string hub_strings[] = { {STRING_MANUFACTURER, "sandbox"}, {STRING_PRODUCT, "hub"}, {STRING_SERIAL, "2345"}, + {}, };
static struct usb_device_descriptor hub_device_desc = {
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
The terminator is missing. Add it for completeness.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
On Tuesday, April 21, 2015 at 05:40:00 PM, Joe Hershberger wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
The terminator is missing. Add it for completeness.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
Hi Simon,
will you pick these please ?
Best regards, Marek Vasut
Hi Marek,
Yes, will do. I need to review the pmic stuff too.
Regards, Simon
On 21 April 2015 at 11:08, Marek Vasut marex@denx.de wrote:
On Tuesday, April 21, 2015 at 05:40:00 PM, Joe Hershberger wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
The terminator is missing. Add it for completeness.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
Hi Simon,
will you pick these please ?
Best regards, Marek Vasut
On 21 April 2015 at 11:11, Simon Glass sjg@chromium.org wrote:
Hi Marek,
Yes, will do. I need to review the pmic stuff too.
Regards, Simon
On 21 April 2015 at 11:08, Marek Vasut marex@denx.de wrote:
On Tuesday, April 21, 2015 at 05:40:00 PM, Joe Hershberger wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:20 AM, Simon Glass sjg@chromium.org wrote:
The terminator is missing. Add it for completeness.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
Hi Simon,
will you pick these please ?
Best regards, Marek Vasut
Applied to u-boot-dm.
On sandbox, if you add a printf() to malloc() for debugging, the output will eventually cause the screen to scroll. Since lcd_sync() calls SDL functions which allocate memory, and this happens before we have updated console_curr_row, U-Boot gets locked in an infinite loop.
Flip the order of the two statements to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org ---
common/lcd_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/lcd_console.c b/common/lcd_console.c index 8bf83b9..ab48fd6 100644 --- a/common/lcd_console.c +++ b/common/lcd_console.c @@ -120,8 +120,8 @@ static void console_scrollup(void) *ppix++ = bg_color; } #endif - lcd_sync(); console_curr_row -= rows; + lcd_sync(); }
static inline void console_back(void)
Hi,
On 19 April 2015 at 07:21, Simon Glass sjg@chromium.org wrote:
On sandbox, if you add a printf() to malloc() for debugging, the output will eventually cause the screen to scroll. Since lcd_sync() calls SDL functions which allocate memory, and this happens before we have updated console_curr_row, U-Boot gets locked in an infinite loop.
Flip the order of the two statements to avoid this.
Signed-off-by: Simon Glass sjg@chromium.org
common/lcd_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/lcd_console.c b/common/lcd_console.c index 8bf83b9..ab48fd6 100644 --- a/common/lcd_console.c +++ b/common/lcd_console.c @@ -120,8 +120,8 @@ static void console_scrollup(void) *ppix++ = bg_color; } #endif
lcd_sync(); console_curr_row -= rows;
lcd_sync();}
static inline void console_back(void)
2.2.0.rc0.207.ga3a616c
This patch is obsolete now since this problem is fixed by an earlier patch, now applied.
I am dropping it.
Regards, Simon
There are a lot more tests now. To avoid confusion add the updated test output to the driver model README.
Signed-off-by: Simon Glass sjg@chromium.org ---
doc/driver-model/README.txt | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 8 deletions(-)
diff --git a/doc/driver-model/README.txt b/doc/driver-model/README.txt index f83264d..f0276b1 100644 --- a/doc/driver-model/README.txt +++ b/doc/driver-model/README.txt @@ -95,43 +95,82 @@ are provided in test/dm. To run them, try: You should see something like this:
<...U-Boot banner...> - Running 29 driver model tests + Running 53 driver model tests Test: dm_test_autobind Test: dm_test_autoprobe + Test: dm_test_bus_child_post_bind + Test: dm_test_bus_child_post_bind_uclass + Test: dm_test_bus_child_pre_probe_uclass Test: dm_test_bus_children - Device 'd-test': seq 3 is in use by 'b-test' - Device 'c-test@0': seq 0 is in use by 'a-test' - Device 'c-test@1': seq 1 is in use by 'd-test' + Device 'c-test@0': seq 0 is in use by 'd-test' + Device 'c-test@1': seq 1 is in use by 'f-test' Test: dm_test_bus_children_funcs Test: dm_test_bus_children_iterators Test: dm_test_bus_parent_data + Test: dm_test_bus_parent_data_uclass Test: dm_test_bus_parent_ops + Test: dm_test_bus_parent_platdata + Test: dm_test_bus_parent_platdata_uclass Test: dm_test_children + Test: dm_test_device_get_uclass_id + Test: dm_test_eth + Using eth@10002000 device + Using eth@10003000 device + Using eth@10004000 device + Test: dm_test_eth_alias + Using eth@10002000 device + Using eth@10004000 device + Using eth@10002000 device + Using eth@10003000 device + Test: dm_test_eth_prime + Using eth@10003000 device + Using eth@10002000 device + Test: dm_test_eth_rotate + + Error: eth@10004000 address not set. + + Error: eth@10004000 address not set. + Using eth@10002000 device + + Error: eth@10004000 address not set. + + Error: eth@10004000 address not set. + Using eth@10004000 device Test: dm_test_fdt - Device 'd-test': seq 3 is in use by 'b-test' Test: dm_test_fdt_offset Test: dm_test_fdt_pre_reloc Test: dm_test_fdt_uclass_seq - Device 'd-test': seq 3 is in use by 'b-test' - Device 'a-test': seq 0 is in use by 'd-test' Test: dm_test_gpio extra-gpios: get_value: error: gpio b5 not reserved Test: dm_test_gpio_anon Test: dm_test_gpio_copy Test: dm_test_gpio_leak extra-gpios: get_value: error: gpio b5 not reserved + Test: dm_test_gpio_phandles Test: dm_test_gpio_requestf + Test: dm_test_i2c_bytewise + Test: dm_test_i2c_find + Test: dm_test_i2c_offset + Test: dm_test_i2c_offset_len + Test: dm_test_i2c_probe_empty + Test: dm_test_i2c_read_write + Test: dm_test_i2c_speed Test: dm_test_leak Test: dm_test_lifecycle + Test: dm_test_net_retry + Using eth@10004000 device + Using eth@10002000 device + Using eth@10004000 device Test: dm_test_operations Test: dm_test_ordering + Test: dm_test_pci_base + Test: dm_test_pci_swapcase Test: dm_test_platdata Test: dm_test_pre_reloc Test: dm_test_remove Test: dm_test_spi_find Invalid chip select 0:0 (err=-19) SF: Failed to get idcodes - Device 'name-emul': seq 0 is in use by 'name-emul' SF: Detected M25P16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB Test: dm_test_spi_flash 2097152 bytes written in 0 ms @@ -150,6 +189,9 @@ You should see something like this: SF: Detected M25P16 with page size 256 Bytes, erase size 64 KiB, total 2 MiB Test: dm_test_uclass Test: dm_test_uclass_before_ready + Test: dm_test_usb_base + Test: dm_test_usb_flash + USB-1: scanning bus 1 for devices... 2 USB Device(s) found Failures: 0
Hi Simon,
On Sun, Apr 19, 2015 at 8:21 AM, Simon Glass sjg@chromium.org wrote:
There are a lot more tests now. To avoid confusion add the updated test output to the driver model README.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
On 21 April 2015 at 09:40, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:21 AM, Simon Glass sjg@chromium.org wrote:
There are a lot more tests now. To avoid confusion add the updated test output to the driver model README.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Applied to u-boot-dm.
We must not clear global_data even in tests, since the ram_buffer (which is used by malloc()) will also be lost, and subsequent tests will fail.
Zero only the global_data fields that are required for the test to function.
Signed-off-by: Simon Glass sjg@chromium.org ---
test/dm/core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/test/dm/core.c b/test/dm/core.c index 990d390..4b9c987 100644 --- a/test/dm/core.c +++ b/test/dm/core.c @@ -596,12 +596,14 @@ static int dm_test_uclass_before_ready(struct dm_test_state *dms)
ut_assertok(uclass_get(UCLASS_TEST, &uc));
- memset(gd, '\0', sizeof(*gd)); + gd->dm_root = NULL; + gd->dm_root_f = NULL; + memset(&gd->uclass_root, '\0', sizeof(gd->uclass_root)); + ut_asserteq_ptr(NULL, uclass_find(UCLASS_TEST));
return 0; } - DM_TEST(dm_test_uclass_before_ready, 0);
static int dm_test_device_get_uclass_id(struct dm_test_state *dms)
Hi Simon,
On Sun, Apr 19, 2015 at 8:21 AM, Simon Glass sjg@chromium.org wrote:
We must not clear global_data even in tests, since the ram_buffer (which is used by malloc()) will also be lost, and subsequent tests will fail.
Zero only the global_data fields that are required for the test to function.
Signed-off-by: Simon Glass sjg@chromium.org
For me, this fixed:
""" Test: dm_test_usb_base /home/joe/u-boot/test/dm/test-main.c:27, dm_test_init(): 0 == dm_init(): Expected 0, got -12 /home/joe/u-boot/test/dm/test-main.c:93, dm_test_main(): 0 == dm_test_init(dms): Expected 0, got -1 """
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
On 21 April 2015 at 10:00, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Apr 19, 2015 at 8:21 AM, Simon Glass sjg@chromium.org wrote:
We must not clear global_data even in tests, since the ram_buffer (which is used by malloc()) will also be lost, and subsequent tests will fail.
Zero only the global_data fields that are required for the test to function.
Signed-off-by: Simon Glass sjg@chromium.org
For me, this fixed:
""" Test: dm_test_usb_base /home/joe/u-boot/test/dm/test-main.c:27, dm_test_init(): 0 == dm_init(): Expected 0, got -12 /home/joe/u-boot/test/dm/test-main.c:93, dm_test_main(): 0 == dm_test_init(dms): Expected 0, got -1 """
Reviewed-by: Joe Hershberger joe.hershberger@ni.com Tested-by: Joe Hershberger joe.hershberger@ni.com
Applied to u-boot-dm.
participants (3)
-
Joe Hershberger -
Marek Vasut -
Simon Glass