[PATCH v2] dm: core: Unlink the device when bind fails without DM_DEVICE_REMOVE

[Yuho Choi <oss.patchbox@gmail.com>]

device_bind_common() adds the new device to its parent's list of
children and to its uclass before it calls the bind methods. If one of
them fails, the error path takes the device off both lists again, but
only when DM_DEVICE_REMOVE is enabled. The free(dev) at the end of the
error path is unconditional, so without DM_DEVICE_REMOVE the freed
device stays on both lists.

DM_DEVICE_REMOVE is disabled by default in SPL, and a failing bind is
not unusual there: lists_bind_fdt() takes -ENODEV from a bind method as
the driver refusing the node and goes on with the next driver. For
example cadence_ttc_bind() refuses TTC nodes that have a #pwm-cells
property so that the PWM driver binds them instead. Any later walk of
the parent's children or of the uclass then finds a device that was
freed. With the simple malloc() the memory is not reused, but the
refused device can still be found and probed through its uclass; with
a full malloc() the memory is handed out again. Sandbox built without
DM_DEVICE_REMOVE shows the latter with test.dts: the first driver for
the multimatch-test node refuses it, the device for the next driver
gets the same memory, and the boot hangs in uclass_find_next_free_seq().

Build uclass_unbind_device() without DM_DEVICE_REMOVE as well, call it
unconditionally on the error path, and take the device off its parent's
list unconditionally too. The remaining clean-up, which only frees
memory, stays conditional as before.

Fixes: 5a87c4174d18 ("dm: core: Drop device removal error path when not supported")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Changes in v2:
- Build uclass_unbind_device() without DM_DEVICE_REMOVE, drop its stub
  and call it unconditionally, instead of an else branch with list_del()
  (Simon)
- Mention the sandbox hang in the commit message

Tested on sandbox_defconfig with DM_DEVICE_REMOVE disabled and LTO
disabled: without this patch, booting with test.dtb hangs in
uclass_find_next_free_seq() while binding multimatch-test (gdb shows
the device of test_multimatch_second at the address just freed for
test_multimatch_first); with it, sandbox boots and multimatch-test is
bound to test_multimatch_second. 'ut dm' on sandbox_defconfig gives the
same results with and without this patch. On xilinx_zynq_virt, which
has SPL_DM_DEVICE_REMOVE disabled, SPL grows by 70 bytes of text.

 drivers/core/device.c        | 10 ++++------
 drivers/core/uclass.c        |  2 +-
 include/dm/uclass-internal.h |  8 +++-----
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 6024534ff93..08edba62d96 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -201,15 +201,13 @@ fail_child_post_bind:
 	}
 
 fail_bind:
-	if (CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)) {
-		if (uclass_unbind_device(dev)) {
-			dm_warn("Failed to unbind dev '%s' on error path\n",
-				dev->name);
-		}
+	if (uclass_unbind_device(dev)) {
+		dm_warn("Failed to unbind dev '%s' on error path\n",
+			dev->name);
 	}
 fail_uclass_bind:
+	list_del(&dev->sibling_node);
 	if (CONFIG_IS_ENABLED(DM_DEVICE_REMOVE)) {
-		list_del(&dev->sibling_node);
 		if (dev_get_flags(dev) & DM_FLAG_ALLOC_PARENT_PDATA) {
 			free(dev_get_parent_plat(dev));
 			dev_set_parent_plat(dev, NULL);
diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
index 5365ac68f9e..867db8bc20d 100644
--- a/drivers/core/uclass.c
+++ b/drivers/core/uclass.c
@@ -751,6 +751,7 @@ int uclass_pre_unbind_device(struct udevice *dev)
 
 	return 0;
 }
+#endif
 
 int uclass_unbind_device(struct udevice *dev)
 {
@@ -758,7 +759,6 @@ int uclass_unbind_device(struct udevice *dev)
 
 	return 0;
 }
-#endif
 
 int uclass_pre_probe_device(struct udevice *dev)
 {
diff --git a/include/dm/uclass-internal.h b/include/dm/uclass-internal.h
index 9cb3f090271..e004e911a02 100644
--- a/include/dm/uclass-internal.h
+++ b/include/dm/uclass-internal.h
@@ -267,6 +267,9 @@ int uclass_bind_device(struct udevice *dev);
  * Return: 0 on success, -ve on error
  */
 int uclass_pre_unbind_device(struct udevice *dev);
+#else
+static inline int uclass_pre_unbind_device(struct udevice *dev) { return 0; }
+#endif
 
 /**
  * uclass_unbind_device() - Deassociate device with a uclass
@@ -278,11 +281,6 @@ int uclass_pre_unbind_device(struct udevice *dev);
  */
 int uclass_unbind_device(struct udevice *dev);
 
-#else
-static inline int uclass_pre_unbind_device(struct udevice *dev) { return 0; }
-static inline int uclass_unbind_device(struct udevice *dev) { return 0; }
-#endif
-
 /**
  * uclass_pre_probe_device() - Deal with a device that is about to be probed
  *
-- 
2.43.0