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

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

[Yuho Choi]

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.

Taking the device off the two lists only needs two list_del() calls, so
do it regardless of DM_DEVICE_REMOVE. 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>
---
 drivers/core/device.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 6024534ff93..ea7beeb662d 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -206,10 +206,13 @@ fail_bind:
 			dm_warn("Failed to unbind dev '%s' on error path\n",
 				dev->name);
 		}
+	} else {
+		/* dev is freed below, so it must not stay in the uclass */
+		list_del(&dev->uclass_node);
 	}
 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);
-- 
2.43.0

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

[Simon Glass]

Hi Yuho,

On 2026-10-26T03:09:22, Yuho Choi <oss.patchbox@gmail.com> wrote:
> dm: core: Unlink the device when bind fails without DM_DEVICE_REMOVE
>
> 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.
>
> Taking the device off the two lists only needs two list_del() calls, so
> [...]
>
>  drivers/core/device.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

> diff --git a/drivers/core/device.c b/drivers/core/device.c
> @@ -206,10 +206,13 @@ fail_bind:
>  			dm_warn("Failed to unbind dev '%s' on error path\n",
>  				dev->name);
>  		}
> +	} else {
> +		/* dev is freed below, so it must not stay in the uclass */
> +		list_del(&dev->uclass_node);
>  	}

uclass_unbind_device() only does list_del(&dev->uclass_node), so both
branches now do the same thing. Please can you move
uclass_unbind_device() out of the #if in uclass.c (and drop the inline
stub in uclass-internal.h which returns 0 without doing anything),
then call it here unconditionally? That avoids the else and keeps the
uclass list handling in uclass.c. What do you think?

Regards,
Simon

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

[Yuho Choi]

On Tue, 27 Oct 2026 at 09:28, Simon Glass <sjg@chromium.org> wrote:
>
> uclass_unbind_device() only does list_del(&dev->uclass_node), so both
> branches now do the same thing. Please can you move
> uclass_unbind_device() out of the #if in uclass.c (and drop the inline
> stub in uclass-internal.h which returns 0 without doing anything),
> then call it here unconditionally? That avoids the else and keeps the
> uclass list handling in uclass.c. What do you think?

Hi Simon,

Thanks for the suggestion. That makes sense. I'll move
uclass_unbind_device() out of the conditional compilation and call it
unconditionally, while keeping the sibling_node cleanup as is.

I'll send a v2 with these changes.

Best regards,
Yuho

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

[Yuho Choi]

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

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2929 bytes --]

On Tue, Nov 03, 2026 at 02:36:06PM -0500, Yuho Choi wrote:

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

We're sensitive in SPL to size growth and it's ~500 boards that have
SPL_DM=y SPL_DM_DEVICE_REMOVE=n. There's also the expectation that in
SPL things may not always be as robust, since it's going for just enough
to get U-Boot itself running. So what was the real case that v1 didn't
solve again exactly? Or was it just a code-cleanup that Simon was asking
for? Thanks.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Yuho Choi]

On Tue, 3 Nov 2026 at 15:22, Tom Rini <trini@konsulko.com> wrote:
>
> We're sensitive in SPL to size growth and it's ~500 boards that have
> SPL_DM=y SPL_DM_DEVICE_REMOVE=n. There's also the expectation that in
> SPL things may not always be as robust, since it's going for just enough
> to get U-Boot itself running. So what was the real case that v1 didn't
> solve again exactly? Or was it just a code-cleanup that Simon was asking
> for? Thanks.

Hi Tom,

I appreciate your review.

Yes, v2 is just the clean-up Simon asked for. v1 and v2 behave the
same: both take the device off the uclass and sibling lists
unconditionally. v2 only calls uclass_unbind_device() instead of
open-coding the list_del().

I also don't have a real SPL case. I found it during ongoing research,
through static analysis and code inspection, not from a failure on a
board. The only reproduction I have is sandbox U-Boot proper built
with DM_DEVICE_REMOVE disabled, with the multimatch-test node
in test.dts.

I didn't weigh the size cost against the number of SPL boards it
affects. Rebuilding xilinx_zynq_virt, SPL grows by 58 bytes with v1
and by 64 bytes of text with v2, so the 70 in my note was off.

If that is not worth it for SPL, I'm fine with dropping the patch.

Thanks,
Yuho

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1530 bytes --]

On Tue, Nov 03, 2026 at 05:44:09PM -0500, Yuho Choi wrote:
> On Tue, 3 Nov 2026 at 15:22, Tom Rini <trini@konsulko.com> wrote:
> >
> > We're sensitive in SPL to size growth and it's ~500 boards that have
> > SPL_DM=y SPL_DM_DEVICE_REMOVE=n. There's also the expectation that in
> > SPL things may not always be as robust, since it's going for just enough
> > to get U-Boot itself running. So what was the real case that v1 didn't
> > solve again exactly? Or was it just a code-cleanup that Simon was asking
> > for? Thanks.
> 
> Hi Tom,
> 
> I appreciate your review.
> 
> Yes, v2 is just the clean-up Simon asked for. v1 and v2 behave the
> same: both take the device off the uclass and sibling lists
> unconditionally. v2 only calls uclass_unbind_device() instead of
> open-coding the list_del().
> 
> I also don't have a real SPL case. I found it during ongoing research,
> through static analysis and code inspection, not from a failure on a
> board. The only reproduction I have is sandbox U-Boot proper built
> with DM_DEVICE_REMOVE disabled, with the multimatch-test node
> in test.dts.
> 
> I didn't weigh the size cost against the number of SPL boards it
> affects. Rebuilding xilinx_zynq_virt, SPL grows by 58 bytes with v1
> and by 64 bytes of text with v2, so the 70 in my note was off.
> 
> If that is not worth it for SPL, I'm fine with dropping the patch.

Thanks, I think we'll just defer this then until / unless someone runs
in to this as a problem on real hardware.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]