dm: core: Fix up test failures
cover 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(-)
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 1/5 dm: core: Handle recursive unbinding of uclass devices
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.
---
drivers/core/uclass.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/core/uclass.c +10 -2
@@ -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;
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 1/5 dm: core: Handle recursive unbinding of uclass devices
Hi Simon,
Quoted 7 lines · Simon Glass
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 <[email protected]>
This fixed the seg fault I was seeing. Thanks!
PATCH 1/5 dm: core: Handle recursive unbinding of uclass devices
Quoted 7 lines · Joe Hershberger
Hi Simon,
Quoted 4 lines · Simon Glass
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.
Quoted 8 lines
Quoted 3 lines
Adjust the code to cope.
Signed-off-by Simon Glass <[email protected]>This fixed the seg fault I was seeing. Thanks!
Reviewed-by Joe Hershberger <[email protected]>Tested-by Joe Hershberger <[email protected]>
Applied to u-boot-dm.
PATCH 2/5 dm: usb: Add a terminator to the string destructor list
In reply to Simon GlassThe terminator is missing. Add it for completeness.
---
drivers/usb/emul/sandbox_hub.c | 1 + 1 file changed, 1 insertion(+)
drivers/usb/emul/sandbox_hub.c +1 -0
@@ -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 = {
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 2/5 dm: usb: Add a terminator to the string destructor list
Hi Simon,
Quoted 3 lines · Simon Glass
The terminator is missing. Add it for completeness.
Signed-off-by Simon Glass <[email protected]>
PATCH 2/5 dm: usb: Add a terminator to the string destructor list
Quoted 9 lines · Joe Hershberger
Hi Simon,
Quoted 3 lines · Simon Glass
The terminator is missing. Add it for completeness.
Signed-off-by Simon Glass <[email protected]>Reviewed-by Joe Hershberger <[email protected]>Tested-by Joe Hershberger <[email protected]>
Hi Simon,
will you pick these please ?
Best regards,
Marek Vasut
PATCH 2/5 dm: usb: Add a terminator to the string destructor list
Hi Marek,
Yes, will do. I need to review the pmic stuff too.
Regards,
Simon
Quoted 17 lines · Marek Vasut
Quoted 9 lines · Joe Hershberger
Hi Simon,
Quoted 3 lines · Simon Glass
The terminator is missing. Add it for completeness.
Signed-off-by Simon Glass <[email protected]>Reviewed-by Joe Hershberger <[email protected]>Tested-by Joe Hershberger <[email protected]>Hi Simon,
will you pick these please ?
Best regards,
Marek Vasut
PATCH 2/5 dm: usb: Add a terminator to the string destructor list
Quoted 26 lines · Simon Glass
Hi Marek,
Yes, will do. I need to review the pmic stuff too.
Regards,
SimonQuoted 17 lines · Marek Vasut
Quoted 9 lines · Joe Hershberger
Hi Simon,
Quoted 3 lines · Simon Glass
The terminator is missing. Add it for completeness.
Signed-off-by Simon Glass <[email protected]>Reviewed-by Joe Hershberger <[email protected]>Tested-by Joe Hershberger <[email protected]>Hi Simon,
will you pick these please ?
Best regards,
Marek Vasut
Applied to u-boot-dm.
PATCH 3/5 lcd: Call lcd_sync() after completing the scroll
In reply to Simon GlassOn 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.
---
common/lcd_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
common/lcd_console.c +1 -1
@@ -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 )
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 3/5 lcd: Call lcd_sync() after completing the scroll
Hi,
Quoted 28 lines · Simon Glass
On sandbox, if you add a
printf()tomalloc()for debugging, the output will eventually cause the screen to scroll. Sincelcd_sync()calls SDL functions which allocate memory, and this happens before we have updatedconsole_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 <[email protected]>---
common/lcd_console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)common/lcd_console.c +1
-2@@ -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 ) --
Quoted 2 lines
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
PATCH 4/5 dm: Update the README to reflect the current test output
In reply to Simon GlassThere are a lot more tests now. To avoid confusion add the updated test output to the driver model README.
---
doc/driver-model/README.txt | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 8 deletions(-)
doc/driver-model/README.txt +50 -8
@@ -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 at 0': seq 0 is in use by 'a-test' - Device 'c-test at 1': seq 1 is in use by 'd-test' + Device 'c-test at 0': seq 0 is in use by 'd-test' + Device 'c-test at 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 at 10002000 device + Using eth at 10003000 device + Using eth at 10004000 device + Test: dm_test_eth_alias + Using eth at 10002000 device + Using eth at 10004000 device + Using eth at 10002000 device + Using eth at 10003000 device + Test: dm_test_eth_prime + Using eth at 10003000 device + Using eth at 10002000 device + Test: dm_test_eth_rotate + + Error: eth at 10004000 address not set. + + Error: eth at 10004000 address not set. + Using eth at 10002000 device + + Error: eth at 10004000 address not set. + + Error: eth at 10004000 address not set. + Using eth at 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 at 10004000 device + Using eth at 10002000 device + Using eth at 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
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 4/5 dm: Update the README to reflect the current test output
Hi Simon,
Quoted 5 lines · Simon Glass
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 <[email protected]>---
PATCH 4/5 dm: Update the README to reflect the current test output
Quoted 10 lines · Joe Hershberger
Hi Simon,
Quoted 5 lines · Simon Glass
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 <[email protected]>---
Reviewed-by Joe Hershberger <[email protected]>
Applied to u-boot-dm.
PATCH 5/5 dm: test: Don't clear global_data in dm_test_uclass_before_ready()
In reply to Simon GlassWe 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.
---
test/dm/core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
test/dm/core.c +4 -2
@@ -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)
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 5/5 dm: test: Don't clear global_data in dm_test_uclass_before_ready()
Hi Simon,
Quoted 6 lines · Simon Glass
We must not clear
global_dataeven in tests, since theram_buffer(which is used bymalloc()) will also be lost, and subsequent tests will fail.Zero only the
global_datafields that are required for the test to function.Signed-off-by Simon Glass <[email protected]>
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
"""
PATCH 5/5 dm: test: Don't clear global_data in dm_test_uclass_before_ready()
Quoted 20 lines · Joe Hershberger
Hi Simon,
Quoted 6 lines · Simon Glass
We must not clear
global_dataeven in tests, since theram_buffer(which is used bymalloc()) will also be lost, and subsequent tests will fail.Zero only the
global_datafields that are required for the test to function.Signed-off-by Simon Glass <[email protected]>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
"""
Quoted 2 lines
Reviewed-by Joe Hershberger <[email protected]>Tested-by Joe Hershberger <[email protected]>
Applied to u-boot-dm.