[PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[Yuho Choi]

The MUSB glue drivers below allocate the struct musb with
musb_init_controller() in their host probe and return the error when
musb_lowlevel_init() fails, for example when no device is connected.
None of them frees the struct musb on that path, and the driver core
then frees the private data that holds the only pointer to it, so each
'usb reset', or 'usb start' after 'usb stop', leaks another one. This
series frees it in ux500, sc5xx, pic32, omap2430, ti-musb and da8xx.

The same fix for sunxi and mt85xx was sent as separate patches:
"usb: musb-new: sunxi: Free the controller if the host probe fails"
"usb: musb-new: mt85xx: Free the controller if the host probe fails"

The patches are independent of each other. They are build-tested only,
as noted in each patch, and pic32 is not built at all. None was tested
on hardware.

Yuho Choi (6):
  usb: musb-new: ux500: Free the controller if the host probe fails
  usb: musb-new: sc5xx: Free the controller if the host probe fails
  usb: musb-new: pic32: Free the controller if the host probe fails
  usb: musb-new: omap2430: Free the controller if the host probe fails
  usb: musb-new: ti-musb: Free the controller if the host probe fails
  usb: musb-new: da8xx: Free the controller if the host probe fails

 drivers/usb/musb-new/da8xx.c    | 2 ++
 drivers/usb/musb-new/omap2430.c | 6 +++++-
 drivers/usb/musb-new/pic32.c    | 2 ++
 drivers/usb/musb-new/sc5xx.c    | 2 ++
 drivers/usb/musb-new/ti-musb.c  | 2 ++
 drivers/usb/musb-new/ux500.c    | 6 +++++-
 6 files changed, 18 insertions(+), 2 deletions(-)


base-commit: feb8913982e4462c01fd914271f92254a25dc101
-- 
2.43.0

[PATCH 1/6] usb: musb-new: ux500: Free the controller if the host probe fails

[Yuho Choi]

In host mode, ux500_musb_probe() allocates the struct musb with
musb_init_controller() and returns the error when musb_lowlevel_init()
fails, for example when no device is connected. The struct musb is not
freed on this path, and the driver core then frees the private data that
holds the only pointer to it. Each 'usb reset', or 'usb start' after
'usb stop', probes the controller again and leaks another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: 845d9cf61c3a ("usb: musb-new: Add glue driver for ST-Ericsson Ux500")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, with stemmy_defconfig and CONFIG_USB_MUSB_HOST=y.

When musb_start() fails, as it does when ux500_musb_enable() cannot
power on the PHY, musb_lowlevel_init() returns without calling
musb_stop(), so the platform init done by musb_init_controller() is not
undone either. That is not changed here.

 drivers/usb/musb-new/ux500.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb-new/ux500.c b/drivers/usb/musb-new/ux500.c
index be0085f403d..c38faa24582 100644
--- a/drivers/usb/musb-new/ux500.c
+++ b/drivers/usb/musb-new/ux500.c
@@ -123,7 +123,11 @@ static int ux500_musb_probe(struct udevice *dev)
 	if (!host->host)
 		return -EIO;
 
-	return musb_lowlevel_init(host);
+	ret = musb_lowlevel_init(host);
+	if (ret)
+		free(host->host);
+
+	return ret;
 #else
 	pdata.mode = MUSB_PERIPHERAL;
 	host->host = musb_init_controller(&pdata, &glue->dev, base);
-- 
2.43.0

Re: [PATCH 1/6] usb: musb-new: ux500: Free the controller if the host probe fails

[Linus Walleij]

On Fri, Nov 6, 2026 at 8:46 PM Yuho Choi <oss.patchbox@gmail.com> wrote:

> In host mode, ux500_musb_probe() allocates the struct musb with
> musb_init_controller() and returns the error when musb_lowlevel_init()
> fails, for example when no device is connected. The struct musb is not
> freed on this path, and the driver core then frees the private data that
> holds the only pointer to it. Each 'usb reset', or 'usb start' after
> 'usb stop', probes the controller again and leaks another one.
>
> Free the struct musb when musb_lowlevel_init() fails.
>
> Fixes: 845d9cf61c3a ("usb: musb-new: Add glue driver for ST-Ericsson Ux500")
> Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

[PATCH 2/6] usb: musb-new: sc5xx: Free the controller if the host probe fails

[Yuho Choi]

In host mode, musb_usb_probe() allocates the struct musb with
musb_init_controller() and returns the error when musb_lowlevel_init()
fails, for example when no device is connected. The struct musb is not
freed on this path, and the driver core then frees the private data that
holds the only pointer to it. Each 'usb reset', or 'usb start' after
'usb stop', probes the controller again and leaks another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: 2f6a86a61256 ("usb: musb-new: Add support for Analog Devices SC5xx SoCs")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, with sc584-ezkit_defconfig.

musb_usb_remove() does not free the struct musb either; that is not
changed here.

 drivers/usb/musb-new/sc5xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb-new/sc5xx.c b/drivers/usb/musb-new/sc5xx.c
index b1765abe827..c14ba821a2e 100644
--- a/drivers/usb/musb-new/sc5xx.c
+++ b/drivers/usb/musb-new/sc5xx.c
@@ -165,6 +165,8 @@ static int musb_usb_probe(struct udevice *dev)
 			return -EIO;
 
 		ret = musb_lowlevel_init(mdata);
+		if (ret)
+			free(mdata->host);
 	} else {
 		sc5xx_musb_plat.mode = MUSB_PERIPHERAL;
 		mdata->host = musb_register(&sc5xx_musb_plat, &pdata->dev, mregs);
-- 
2.43.0

Re: [PATCH 2/6] usb: musb-new: sc5xx: Free the controller if the host probe fails

[Greg Malysa]

Hi Yuho,

On Fri, Nov 6, 2026 at 2:46 PM Yuho Choi <oss.patchbox@gmail.com> wrote:
>
> In host mode, musb_usb_probe() allocates the struct musb with
> musb_init_controller() and returns the error when musb_lowlevel_init()
> fails, for example when no device is connected. The struct musb is not
> freed on this path, and the driver core then frees the private data that
> holds the only pointer to it. Each 'usb reset', or 'usb start' after
> 'usb stop', probes the controller again and leaks another one.
>
> Free the struct musb when musb_lowlevel_init() fails.
>
> Fixes: 2f6a86a61256 ("usb: musb-new: Add support for Analog Devices SC5xx SoCs")
> Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
> ---
> Build-tested only, with sc584-ezkit_defconfig.
>
> musb_usb_remove() does not free the struct musb either; that is not
> changed here.
>
>  drivers/usb/musb-new/sc5xx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/musb-new/sc5xx.c b/drivers/usb/musb-new/sc5xx.c
> index b1765abe827..c14ba821a2e 100644
> --- a/drivers/usb/musb-new/sc5xx.c
> +++ b/drivers/usb/musb-new/sc5xx.c
> @@ -165,6 +165,8 @@ static int musb_usb_probe(struct udevice *dev)
>                         return -EIO;
>
>                 ret = musb_lowlevel_init(mdata);
> +               if (ret)
> +                       free(mdata->host);
>         } else {
>                 sc5xx_musb_plat.mode = MUSB_PERIPHERAL;
>                 mdata->host = musb_register(&sc5xx_musb_plat, &pdata->dev, mregs);
> --
> 2.43.0
>

Is freeing mdata->host the appropriate cleanup action?
musb_init_controller registers the pointed-to struct with the pm
subsystem, potentially allocates a dma controller to store in ->c, and
does many other things I haven't traced through yet. I would need to
review in more detail but it seems a more appropriate action would be
to use musb_free() which is currently a static function inside
musb_core.c.

Taking an even further step back, musb_lowlevel_init fails under two
circumstances: host->host is not set (impossible due to the earlier
check) or musb_start fails which only happens when the
musb_usb_ops->enable() function is present and fails (via
musb_platform_enable) which is not part of the default musb_ops that
we are using. It is therefore impossible for musb_lowlevel_init to set
ret to a nonzero value in a way that leaks resources on this platform.
I have not checked the other drivers referenced in this series, but I
honestly assume we all copied the same driver and behave in basically
the same way, including default musb_ops.

I don't think we should make this change for now.

Thanks,
Greg

Re: [PATCH 2/6] usb: musb-new: sc5xx: Free the controller if the host probe fails

[Greg Malysa]

On Sat, Nov 7, 2026 at 6:18 PM Greg Malysa <malysagreg@gmail.com> wrote:
>
> Taking an even further step back, musb_lowlevel_init fails under two
> circumstances: host->host is not set (impossible due to the earlier
> check) or musb_start fails which only happens when the
> musb_usb_ops->enable() function is present and fails (via
> musb_platform_enable) which is not part of the default musb_ops that
> we are using. It is therefore impossible for musb_lowlevel_init to set
> ret to a nonzero value in a way that leaks resources on this platform.
> I have not checked the other drivers referenced in this series, but I
> honestly assume we all copied the same driver and behave in basically
> the same way, including default musb_ops.

I think I was off here (sorry I didn't write this driver initially and
musb is a bit of a labyrinth), so it probably actually calls
sc5xx_musb_enable through the sc5xx_musb_ops struct, but that function
also cannot fail and always returns zero, so musb_start() will not
fail either way.

Re: [PATCH 2/6] usb: musb-new: sc5xx: Free the controller if the host probe fails

[Yuho Choi]

On Sat, Nov 7, 2026 at 6:18 PM Greg Malysa <malysagreg@gmail.com> wrote:
>
> Is freeing mdata->host the appropriate cleanup action?
> musb_init_controller registers the pointed-to struct with the pm
> subsystem, potentially allocates a dma controller to store in ->c, and
> does many other things I haven't traced through yet. I would need to
> review in more detail but it seems a more appropriate action would be
> to use musb_free() which is currently a static function inside
> musb_core.c.

Thanks for the review. I agree that musb_free() is the better
choice.

On Sat, Nov 7, 2026 at 6:46 PM Greg Malysa <malysagreg@gmail.com> wrote:
>
> I think I was off here (sorry I didn't write this driver initially and
> musb is a bit of a labyrinth), so it probably actually calls
> sc5xx_musb_enable through the sc5xx_musb_ops struct, but that function
> also cannot fail and always returns zero, so musb_start() will not
> fail either way.

Agreed, musb_start() cannot fail on sc5xx. However,
musb_lowlevel_init() has a third failure path. After musb_start()
succeeds, it waits up to one second for MUSB_DEVCTL_HM to be set.
If the bit is not set by then, it calls musb_stop() and returns
-ENODEV.

The leak is small so I am fine with dropping this patch if this is not
needed.

Thanks,
Yuho

Re: [PATCH 2/6] usb: musb-new: sc5xx: Free the controller if the host probe fails

[Greg Malysa]

Hi Yuho,

On Sat, Nov 7, 2026 at 10:21 PM Yuho Choi <oss.patchbox@gmail.com> wrote:
>
> On Sat, Nov 7, 2026 at 6:18 PM Greg Malysa <malysagreg@gmail.com> wrote:
> >
> > Is freeing mdata->host the appropriate cleanup action?
> > musb_init_controller registers the pointed-to struct with the pm
> > subsystem, potentially allocates a dma controller to store in ->c, and
> > does many other things I haven't traced through yet. I would need to
> > review in more detail but it seems a more appropriate action would be
> > to use musb_free() which is currently a static function inside
> > musb_core.c.
>
> Thanks for the review. I agree that musb_free() is the better
> choice.
>
> On Sat, Nov 7, 2026 at 6:46 PM Greg Malysa <malysagreg@gmail.com> wrote:
> >
> > I think I was off here (sorry I didn't write this driver initially and
> > musb is a bit of a labyrinth), so it probably actually calls
> > sc5xx_musb_enable through the sc5xx_musb_ops struct, but that function
> > also cannot fail and always returns zero, so musb_start() will not
> > fail either way.
>
> Agreed, musb_start() cannot fail on sc5xx. However,
> musb_lowlevel_init() has a third failure path. After musb_start()
> succeeds, it waits up to one second for MUSB_DEVCTL_HM to be set.
> If the bit is not set by then, it calls musb_stop() and returns
> -ENODEV.

Sorry I missed that third one when looking at it earlier. I agree we
should get a fix in place to avoid leaking when this happens. Let's
see what Marek says about the best way to deallocate the resources
from outside of musb_core. It looks like there is a lot of Linux code
that is removed by #ifndef __UBOOT__ blocks so I want to hear from
someone more familiar about whether it is reasonable to just expose
musb_free() and call it in this case, or if we need to migrate more of
the logic from the Linux code for musb_remove() such as
musb_shutdown(), which is also not included in the uboot build for
now.

Also it seems the pm_runtime_* functions I was worried about earlier
expand to empty macros per include/linux/compat.h so there may be less
to do than I had thought.

>
> The leak is small so I am fine with dropping this patch if this is not
> needed.

No, the third condition you identified is worth handling and fixing.

>
> Thanks,
> Yuho

Thanks,
Greg

[PATCH 3/6] usb: musb-new: pic32: Free the controller if the host probe fails

[Yuho Choi]

In host mode, musb_usb_probe() allocates the struct musb with
musb_init_controller() and returns the error when musb_lowlevel_init()
fails, for example when no device is connected. The struct musb is not
freed on this path, and the driver core then frees the private data that
holds the only pointer to it. Each 'usb reset', or 'usb start' after
'usb stop', probes the controller again and leaks another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: 03b8e04632ca ("drivers: musb-new: Add USB DRC driver for Microchip PIC32 OTG controller.")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Not build-tested: I have no MIPS toolchain.

musb_usb_remove() does not free the struct musb either; that is not
changed here.

 drivers/usb/musb-new/pic32.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb-new/pic32.c b/drivers/usb/musb-new/pic32.c
index b878e979d93..2623e6e526a 100644
--- a/drivers/usb/musb-new/pic32.c
+++ b/drivers/usb/musb-new/pic32.c
@@ -253,6 +253,8 @@ static int musb_usb_probe(struct udevice *dev)
 		return -EIO;
 
 	ret = musb_lowlevel_init(mdata);
+	if (ret)
+		free(mdata->host);
 #else
 	pic32_musb_plat.mode = MUSB_PERIPHERAL;
 	mdata->host = musb_register(&pic32_musb_plat, &pdata->dev, mregs);
-- 
2.43.0

[PATCH 4/6] usb: musb-new: omap2430: Free the controller if the host probe fails

[Yuho Choi]

In host mode, omap2430_musb_probe() allocates the struct musb with
musb_init_controller() and returns the error when musb_lowlevel_init()
fails, for example when no device is connected. The struct musb is not
freed on this path, and the driver core then frees the private data that
holds the only pointer to it. Each 'usb reset', or 'usb start' after
'usb stop', probes the controller again and leaks another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: 1a35526e1d54 ("usb: musb-new: omap2430: Enable DM_USB and OF support")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, with omap3_evm_defconfig and CONFIG_USB_MUSB_HOST=y.

omap2430_musb_remove() does not free the struct musb either; that is
not changed here.

 drivers/usb/musb-new/omap2430.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb-new/omap2430.c b/drivers/usb/musb-new/omap2430.c
index c8c6bf0c84f..c5607fd388e 100644
--- a/drivers/usb/musb-new/omap2430.c
+++ b/drivers/usb/musb-new/omap2430.c
@@ -229,7 +229,11 @@ static int omap2430_musb_probe(struct udevice *dev)
 		if (!host->host)
 			return -EIO;
 
-		return musb_lowlevel_init(host);
+		ret = musb_lowlevel_init(host);
+		if (ret)
+			free(host->host);
+
+		return ret;
 	} else if (CONFIG_IS_ENABLED(DM_USB_GADGET)) {
 		struct musb_host_data *host = dev_get_priv(dev);
 
-- 
2.43.0

[PATCH 5/6] usb: musb-new: ti-musb: Free the controller if the host probe fails

[Yuho Choi]

ti_musb_host_probe() allocates the struct musb with
musb_init_controller() and returns the error when musb_lowlevel_init()
fails, for example when no device is connected. The struct musb is not
freed on this path, and the driver core then frees the private data that
holds the only pointer to it. Each 'usb reset', or 'usb start' after
'usb stop', probes the controller again and leaks another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: ae6acf9fe2c8 ("drivers: usb: musb: add ti musb host driver with driver model support")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, with pcm051_rev3_defconfig.

ti_musb_host_remove() does not free the struct musb either, and the
PHY stays powered on after the failure; neither is changed here.

 drivers/usb/musb-new/ti-musb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb-new/ti-musb.c b/drivers/usb/musb-new/ti-musb.c
index cc6c3b94a65..5a93708bc75 100644
--- a/drivers/usb/musb-new/ti-musb.c
+++ b/drivers/usb/musb-new/ti-musb.c
@@ -168,6 +168,8 @@ static int ti_musb_host_probe(struct udevice *dev)
 
 	ti_musb_set_phy_power(dev, 1);
 	ret = musb_lowlevel_init(host);
+	if (ret)
+		free(host->host);
 
 	return ret;
 }
-- 
2.43.0

[PATCH 6/6] usb: musb-new: da8xx: Free the controller if the host probe fails

[Yuho Choi]

da8xx_musb_probe() allocates the struct musb with musb_init_controller()
and returns the error when musb_lowlevel_init() fails, for example when
no device is connected. The error path turns off the PHY and the PSC but
does not free the struct musb, and the driver core then frees the
private data that holds the only pointer to it. Each 'usb reset', or
'usb start' after 'usb stop', probes the controller again and leaks
another one.

Free the struct musb when musb_lowlevel_init() fails.

Fixes: 5608331d8136 ("usb: musb-new: Add support for da8xx-musb")
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
Build-tested only, with omapl138_lcdk_defconfig.

da8xx_musb_remove() does not free the struct musb either; that is not
changed here.

 drivers/usb/musb-new/da8xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/musb-new/da8xx.c b/drivers/usb/musb-new/da8xx.c
index 7caf03cc2e1..17a953c9744 100644
--- a/drivers/usb/musb-new/da8xx.c
+++ b/drivers/usb/musb-new/da8xx.c
@@ -319,6 +319,8 @@ static int da8xx_musb_probe(struct udevice *dev)
 
 	if (ret == 0)
 		return 0;
+
+	free(host->host);
 shutdown:
 	/* Turn off the phy if we fail */
 	generic_phy_power_off(&plat->phy);
-- 
2.43.0

Re: [PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[Tom Rini]

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

On Fri, Nov 06, 2026 at 02:45:52PM -0500, Yuho Choi wrote:

> The MUSB glue drivers below allocate the struct musb with
> musb_init_controller() in their host probe and return the error when
> musb_lowlevel_init() fails, for example when no device is connected.
> None of them frees the struct musb on that path, and the driver core
> then frees the private data that holds the only pointer to it, so each
> 'usb reset', or 'usb start' after 'usb stop', leaks another one. This
> series frees it in ux500, sc5xx, pic32, omap2430, ti-musb and da8xx.
> 
> The same fix for sunxi and mt85xx was sent as separate patches:
> "usb: musb-new: sunxi: Free the controller if the host probe fails"
> "usb: musb-new: mt85xx: Free the controller if the host probe fails"
> 
> The patches are independent of each other. They are build-tested only,
> as noted in each patch, and pic32 is not built at all. None was tested
> on hardware.

How are you finding and investigating these issues, exactly? To cut to
the chase, this series (and the other two patches) sure seem like what
happens when someone sends an LLM to look for resource leaks. Which are
not great. But the bigger concern is if we handle the error case
correctly. Thanks.

-- 
Tom

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

Re: [PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[Yuho Choi]

On Fri, 6 Nov 2026 at 15:56, Tom Rini <trini@konsulko.com> wrote:
>
> How are you finding and investigating these issues, exactly? To cut to
> the chase, this series (and the other two patches) sure seem like what
> happens when someone sends an LLM to look for resource leaks. Which are
> not great. But the bigger concern is if we handle the error case
> correctly. Thanks.

Thanks for the review.
The bugs were found as part of ongoing static analysis research on
configuration-dependent resource management bug patterns.
I used an LLM to help verify the bugs.

The patches only fix the memory leak. They do not restore the
hardware state on the error path. Would you want patches that
restore all of the hardware state on these error paths instead?

Thanks.

Re: [PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[Tom Rini]

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

On Fri, Nov 06, 2026 at 06:19:43PM -0500, Yuho Choi wrote:
> On Fri, 6 Nov 2026 at 15:56, Tom Rini <trini@konsulko.com> wrote:
> >
> > How are you finding and investigating these issues, exactly? To cut to
> > the chase, this series (and the other two patches) sure seem like what
> > happens when someone sends an LLM to look for resource leaks. Which are
> > not great. But the bigger concern is if we handle the error case
> > correctly. Thanks.
> 
> Thanks for the review.
> The bugs were found as part of ongoing static analysis research on
> configuration-dependent resource management bug patterns.
> I used an LLM to help verify the bugs.
> 
> The patches only fix the memory leak. They do not restore the
> hardware state on the error path. Would you want patches that
> restore all of the hardware state on these error paths instead?

So, the general project policy for LLMs is please don't. Using this as
an example, it's less interesting that we might have a small leak (we
aren't a persistent OS), but as the part of this thread with Greg and
the sc5xx driver show, there are potentially interesting problems in the
failure case (and we don't need to restore the HW state, just ensure our
internal state is consistent). Digging past the initial leak to fully
understand the error path and that on the software side we handle errors
gracefully would be an interesting task.

-- 
Tom

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

Re: [PATCH 0/6] usb: musb-new: Free the controller if the host probe fails

[Yuho Choi]

On Mon, 9 Nov 2026 at 12:29, Tom Rini <trini@konsulko.com> wrote:
> So, the general project policy for LLMs is please don't. Using this as
> an example, it's less interesting that we might have a small leak (we
> aren't a persistent OS), but as the part of this thread with Greg and
> the sc5xx driver show, there are potentially interesting problems in the
> failure case (and we don't need to restore the HW state, just ensure our
> internal state is consistent). Digging past the initial leak to fully
> understand the error path and that on the software side we handle errors
> gracefully would be an interesting task.

Understood, thank you for explaining. I will not send a v2 of this
series or of the separate sunxi and mt85xx patches, and will leave
it to Marek whether any of them are worth taking as they are.

Thanks,
Yuho