2022-08-03 06:17:19

by Christophe JAILLET

Subject: [PATCH] ARM: pxa: Fix a memory leak in pxa310_otg_exit()

When otg_ulpi_create() is called, 2 kzalloc() are performed.
Only one is freed in pxa310_otg_exit().

The second kzalloc() has been introduced in the commit in the Fixes: tag.

Add the missing kfree().

Fixes: 298b083cf9dd ("usb: otg: ulpi: Start using struct usb_otg")
Signed-off-by: Christophe JAILLET <[email protected]>
---
This patch is NOT compile tested (I never cross compile even if I've been
tolled many times that it was easy...)

Another solution is to use devm_otg_ulpi_create() and further simplify
arch/arm/mach-pxa/pxa3xx-ulpi.c.

This would remove the only caller of otg_ulpi_create() and could also
simplify drivers/usb/phy/phy-ulpi.[ch]
---
arch/arm/mach-pxa/pxa3xx-ulpi.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
index c29a7f0fa1b0..82b4e2706f86 100644
--- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
+++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
@@ -229,6 +229,7 @@ static int pxa310_otg_init(struct pxa3xx_u2d_platform_data *pdata)

static void pxa310_otg_exit(void)
{
+ kfree(u2d->otg->otg);
kfree(u2d->otg);
}
#else
--
2.34.1

2022-08-03 13:42:55

by Dan Carpenter

Subject: Re: [PATCH] ARM: pxa: Fix a memory leak in pxa310_otg_exit()

On Wed, Aug 03, 2022 at 08:00:44AM +0200, Christophe JAILLET wrote:
> arch/arm/mach-pxa/pxa3xx-ulpi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> index c29a7f0fa1b0..82b4e2706f86 100644
> --- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
> +++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
> @@ -229,6 +229,7 @@ static int pxa310_otg_init(struct pxa3xx_u2d_platform_data *pdata)
>
> static void pxa310_otg_exit(void)
> {
> + kfree(u2d->otg->otg);
> kfree(u2d->otg);

It's unfortunate that we kept the "otg" name when changed u2d->otg to
a phy. A lot of the stuff was renamed... I don't have a cross compile
system set up either so I can't patch it but looks reasonably simple to
rename the remaining instance to phy?

diff --git a/arch/arm/mach-pxa/pxa3xx-ulpi.c b/arch/arm/mach-pxa/pxa3xx-ulpi.c
index c29a7f0fa1b0..a002fe1e96a2 100644
--- a/arch/arm/mach-pxa/pxa3xx-ulpi.c
+++ b/arch/arm/mach-pxa/pxa3xx-ulpi.c
@@ -30,7 +30,7 @@ struct pxa3xx_u2d_ulpi {
struct clk *clk;
void __iomem *mmio_base;

- struct usb_phy *otg;
+ struct usb_phy *phy;
unsigned int ulpi_mode;
};

@@ -76,7 +76,7 @@ static int pxa310_ulpi_poll(void)
return -ETIMEDOUT;
}

-static int pxa310_ulpi_read(struct usb_phy *otg, u32 reg)
+static int pxa310_ulpi_read(struct usb_phy *phy, u32 reg)
{
int err;

@@ -95,7 +95,7 @@ static int pxa310_ulpi_read(struct usb_phy *otg, u32 reg)
return u2d_readl(U2DOTGUCR) & U2DOTGUCR_RDATA;
}

-static int pxa310_ulpi_write(struct usb_phy *otg, u32 val, u32 reg)
+static int pxa310_ulpi_write(struct usb_phy *phy, u32 val, u32 reg)
{
if (pxa310_ulpi_get_phymode() != SYNCH) {
pr_warn("%s: PHY is not in SYNCH mode!\n", __func__);
@@ -136,19 +136,19 @@ static int pxa310_start_otg_host_transcvr(struct usb_bus *host)

pxa310_otg_transceiver_rtsm();

- err = usb_phy_init(u2d->otg);
+ err = usb_phy_init(u2d->phy);
if (err) {
pr_err("OTG transceiver init failed");
return err;
}

- err = otg_set_vbus(u2d->otg->otg, 1);
+ err = otg_set_vbus(u2d->phy->otg, 1);
if (err) {
pr_err("OTG transceiver VBUS set failed");
return err;
}

- err = otg_set_host(u2d->otg->otg, host);
+ err = otg_set_host(u2d->phy->otg, host);
if (err)
pr_err("OTG transceiver Host mode set failed");

@@ -186,9 +186,9 @@ static void pxa310_stop_otg_hc(void)
{
pxa310_otg_transceiver_rtsm();

- otg_set_host(u2d->otg->otg, NULL);
- otg_set_vbus(u2d->otg->otg, 0);
- usb_phy_shutdown(u2d->otg);
+ otg_set_host(u2d->phy->otg, NULL);
+ otg_set_vbus(u2d->phy->otg, 0);
+ usb_phy_shutdown(u2d->phy);
}

static void pxa310_u2d_setup_otg_hc(void)
@@ -218,18 +218,18 @@ static int pxa310_otg_init(struct pxa3xx_u2d_platform_data *pdata)

u2d->ulpi_mode = ulpi_mode;

- u2d->otg = otg_ulpi_create(&pxa310_ulpi_access_ops, ulpi_mode);
- if (!u2d->otg)
+ u2d->phy = otg_ulpi_create(&pxa310_ulpi_access_ops, ulpi_mode);
+ if (!u2d->phy)
return -ENOMEM;

- u2d->otg->io_priv = u2d->mmio_base;
+ u2d->phy->io_priv = u2d->mmio_base;

return 0;
}

static void pxa310_otg_exit(void)
{
- kfree(u2d->otg);
+ kfree(u2d->phy);
}
#else
static inline void pxa310_u2d_setup_otg_hc(void) {}