* [U-Boot] [PATCH] pinctrl: imx: fix memory leak
@ 2017-05-11 2:43 Peng Fan
2017-05-11 9:17 ` Jagan Teki
[not found] ` <20170511111320.2316b6f3@ipc1.ka-ro>
0 siblings, 2 replies; 5+ messages in thread
From: Peng Fan @ 2017-05-11 2:43 UTC (permalink / raw)
To: u-boot
Each time set_state is called, a new piece memory will
be allocated for pin_data, but not freed, this will
incur memory leak.
When error, the devm API could not free memory automatically.
So need call devm_kfree when error.
Issue reported by Coverity
Signed-off-by: Peng Fan <peng.fan@nxp.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Stefan Agner <stefan.agner@toradex.com>
Cc: Stefano Babic <sbabic@denx.de>
---
drivers/pinctrl/nxp/pinctrl-imx.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/pinctrl/nxp/pinctrl-imx.c b/drivers/pinctrl/nxp/pinctrl-imx.c
index f0321c4..31a28e1 100644
--- a/drivers/pinctrl/nxp/pinctrl-imx.c
+++ b/drivers/pinctrl/nxp/pinctrl-imx.c
@@ -53,6 +53,7 @@ static int imx_pinctrl_set_state(struct udevice *dev, struct udevice *config)
if (fdtdec_get_int_array(gd->fdt_blob, node, "fsl,pins",
pin_data, size >> 2)) {
dev_err(dev, "Error reading pin data.\n");
+ devm_kfree(pin_data);
return -EINVAL;
}
@@ -78,6 +79,7 @@ static int imx_pinctrl_set_state(struct udevice *dev, struct udevice *config)
if ((mux_reg == -1) || (conf_reg == -1)) {
dev_err(dev, "Error mux_reg or conf_reg\n");
+ devm_kfree(pin_data);
return -EINVAL;
}
@@ -166,6 +168,8 @@ static int imx_pinctrl_set_state(struct udevice *dev, struct udevice *config)
}
}
+ devm_kfree(pin_data);
+
return 0;
}
--
2.6.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pinctrl: imx: fix memory leak
2017-05-11 2:43 [U-Boot] [PATCH] pinctrl: imx: fix memory leak Peng Fan
@ 2017-05-11 9:17 ` Jagan Teki
2017-05-11 9:19 ` Peng Fan
[not found] ` <20170511111320.2316b6f3@ipc1.ka-ro>
1 sibling, 1 reply; 5+ messages in thread
From: Jagan Teki @ 2017-05-11 9:17 UTC (permalink / raw)
To: u-boot
On Thu, May 11, 2017 at 8:13 AM, Peng Fan <peng.fan@nxp.com> wrote:
> Each time set_state is called, a new piece memory will
> be allocated for pin_data, but not freed, this will
> incur memory leak.
>
> When error, the devm API could not free memory automatically.
> So need call devm_kfree when error.
>
> Issue reported by Coverity
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Stefan Agner <stefan.agner@toradex.com>
> Cc: Stefano Babic <sbabic@denx.de>
> ---
> drivers/pinctrl/nxp/pinctrl-imx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/pinctrl/nxp/pinctrl-imx.c b/drivers/pinctrl/nxp/pinctrl-imx.c
> index f0321c4..31a28e1 100644
> --- a/drivers/pinctrl/nxp/pinctrl-imx.c
> +++ b/drivers/pinctrl/nxp/pinctrl-imx.c
> @@ -53,6 +53,7 @@ static int imx_pinctrl_set_state(struct udevice *dev, struct udevice *config)
> if (fdtdec_get_int_array(gd->fdt_blob, node, "fsl,pins",
> pin_data, size >> 2)) {
> dev_err(dev, "Error reading pin data.\n");
> + devm_kfree(pin_data);
we need to specify the pointer from where to free?
thanks!
--
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pinctrl: imx: fix memory leak
2017-05-11 9:17 ` Jagan Teki
@ 2017-05-11 9:19 ` Peng Fan
0 siblings, 0 replies; 5+ messages in thread
From: Peng Fan @ 2017-05-11 9:19 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Jagan Teki [mailto:jagannadh.teki at gmail.com]
> Sent: Thursday, May 11, 2017 5:18 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: Simon Glass <sjg@chromium.org>; u-boot at lists.denx.de; Stefan Agner
> <stefan.agner@toradex.com>
> Subject: Re: [U-Boot] [PATCH] pinctrl: imx: fix memory leak
>
> On Thu, May 11, 2017 at 8:13 AM, Peng Fan <peng.fan@nxp.com> wrote:
> > Each time set_state is called, a new piece memory will be allocated
> > for pin_data, but not freed, this will incur memory leak.
> >
> > When error, the devm API could not free memory automatically.
> > So need call devm_kfree when error.
> >
> > Issue reported by Coverity
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Stefan Agner <stefan.agner@toradex.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > drivers/pinctrl/nxp/pinctrl-imx.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/pinctrl/nxp/pinctrl-imx.c
> > b/drivers/pinctrl/nxp/pinctrl-imx.c
> > index f0321c4..31a28e1 100644
> > --- a/drivers/pinctrl/nxp/pinctrl-imx.c
> > +++ b/drivers/pinctrl/nxp/pinctrl-imx.c
> > @@ -53,6 +53,7 @@ static int imx_pinctrl_set_state(struct udevice *dev,
> struct udevice *config)
> > if (fdtdec_get_int_array(gd->fdt_blob, node, "fsl,pins",
> > pin_data, size >> 2)) {
> > dev_err(dev, "Error reading pin data.\n");
> > + devm_kfree(pin_data);
>
> we need to specify the pointer from where to free?
Oh. Missed to add dev pointer here.
Thanks,
Peng.
>
> thanks!
> --
> Jagan Teki
> Free Software Engineer | www.openedev.com U-Boot, Linux | Upstream
> Maintainer Hyderabad, India.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pinctrl: imx: fix memory leak
[not found] ` <20170511111320.2316b6f3@ipc1.ka-ro>
@ 2017-05-11 9:22 ` Peng Fan
[not found] ` <20170511120958.535e2e33@ipc1.ka-ro>
0 siblings, 1 reply; 5+ messages in thread
From: Peng Fan @ 2017-05-11 9:22 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Lothar Waßmann [mailto:LW at KARO-electronics.de]
> Sent: Thursday, May 11, 2017 5:13 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: sjg at chromium.org; u-boot at lists.denx.de; Stefan Agner
> <stefan.agner@toradex.com>
> Subject: Re: [U-Boot] [PATCH] pinctrl: imx: fix memory leak
>
> Hi,
>
> On Thu, 11 May 2017 10:43:02 +0800 Peng Fan wrote:
> > Each time set_state is called, a new piece memory will be allocated
> > for pin_data, but not freed, this will incur memory leak.
> >
> > When error, the devm API could not free memory automatically.
> > So need call devm_kfree when error.
> >
> > Issue reported by Coverity
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Stefan Agner <stefan.agner@toradex.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> > drivers/pinctrl/nxp/pinctrl-imx.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/pinctrl/nxp/pinctrl-imx.c
> > b/drivers/pinctrl/nxp/pinctrl-imx.c
> > index f0321c4..31a28e1 100644
> > --- a/drivers/pinctrl/nxp/pinctrl-imx.c
> > +++ b/drivers/pinctrl/nxp/pinctrl-imx.c
> > @@ -53,6 +53,7 @@ static int imx_pinctrl_set_state(struct udevice *dev,
> struct udevice *config)
> > if (fdtdec_get_int_array(gd->fdt_blob, node, "fsl,pins",
> > pin_data, size >> 2)) {
> > dev_err(dev, "Error reading pin data.\n");
> > + devm_kfree(pin_data);
> > return -EINVAL;
> > }
> >
> > @@ -78,6 +79,7 @@ static int imx_pinctrl_set_state(struct udevice
> > *dev, struct udevice *config)
> >
> > if ((mux_reg == -1) || (conf_reg == -1)) {
> > dev_err(dev, "Error mux_reg or conf_reg\n");
> > + devm_kfree(pin_data);
> > return -EINVAL;
> > }
> >
> > @@ -166,6 +168,8 @@ static int imx_pinctrl_set_state(struct udevice *dev,
> struct udevice *config)
> > }
> > }
> >
> > + devm_kfree(pin_data);
> > +
> > return 0;
> > }
> >
> IMO 'devm'_kzalloc() doesn't make any sense here.
> It would make sense for allocations that have the same lifetime as the 'struct
> udevice', but not for a temporary buffer that is used within on function.
Size is different for different fsl,pins group. So need to use devm_kzalloc.
Thanks,
Peng.
>
>
> Lothar Waßmann
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] pinctrl: imx: fix memory leak
[not found] ` <20170511120958.535e2e33@ipc1.ka-ro>
@ 2017-05-11 13:18 ` Peng Fan
0 siblings, 0 replies; 5+ messages in thread
From: Peng Fan @ 2017-05-11 13:18 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Lothar Waßmann [mailto:LW at KARO-electronics.de]
> Sent: Thursday, May 11, 2017 6:10 PM
> To: Peng Fan <peng.fan@nxp.com>
> Cc: sjg at chromium.org; u-boot at lists.denx.de; Stefan Agner
> <stefan.agner@toradex.com>
> Subject: Re: [U-Boot] [PATCH] pinctrl: imx: fix memory leak
>
> Hi,
>
> On Thu, 11 May 2017 09:22:22 +0000 Peng Fan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Lothar Waßmann [mailto:LW at KARO-electronics.de]
> > > Sent: Thursday, May 11, 2017 5:13 PM
> > > To: Peng Fan <peng.fan@nxp.com>
> > > Cc: sjg at chromium.org; u-boot at lists.denx.de; Stefan Agner
> > > <stefan.agner@toradex.com>
> > > Subject: Re: [U-Boot] [PATCH] pinctrl: imx: fix memory leak
> > >
> > > Hi,
> > >
> > > On Thu, 11 May 2017 10:43:02 +0800 Peng Fan wrote:
> > > > Each time set_state is called, a new piece memory will be
> > > > allocated for pin_data, but not freed, this will incur memory leak.
> > > >
> > > > When error, the devm API could not free memory automatically.
> > > > So need call devm_kfree when error.
> > > >
> > > > Issue reported by Coverity
> > > >
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Stefan Agner <stefan.agner@toradex.com>
> > > > Cc: Stefano Babic <sbabic@denx.de>
> > > > ---
> > > > drivers/pinctrl/nxp/pinctrl-imx.c | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/pinctrl/nxp/pinctrl-imx.c
> > > > b/drivers/pinctrl/nxp/pinctrl-imx.c
> > > > index f0321c4..31a28e1 100644
> > > > --- a/drivers/pinctrl/nxp/pinctrl-imx.c
> > > > +++ b/drivers/pinctrl/nxp/pinctrl-imx.c
> > > > @@ -53,6 +53,7 @@ static int imx_pinctrl_set_state(struct udevice
> > > > *dev,
> > > struct udevice *config)
> > > > if (fdtdec_get_int_array(gd->fdt_blob, node, "fsl,pins",
> > > > pin_data, size >> 2)) {
> > > > dev_err(dev, "Error reading pin data.\n");
> > > > + devm_kfree(pin_data);
> > > > return -EINVAL;
> > > > }
> > > >
> > > > @@ -78,6 +79,7 @@ static int imx_pinctrl_set_state(struct udevice
> > > > *dev, struct udevice *config)
> > > >
> > > > if ((mux_reg == -1) || (conf_reg == -1)) {
> > > > dev_err(dev, "Error mux_reg or conf_reg\n");
> > > > + devm_kfree(pin_data);
> > > > return -EINVAL;
> > > > }
> > > >
> > > > @@ -166,6 +168,8 @@ static int imx_pinctrl_set_state(struct
> > > > udevice *dev,
> > > struct udevice *config)
> > > > }
> > > > }
> > > >
> > > > + devm_kfree(pin_data);
> > > > +
> > > > return 0;
> > > > }
> > > >
> > > IMO 'devm'_kzalloc() doesn't make any sense here.
> > > It would make sense for allocations that have the same lifetime as
> > > the 'struct udevice', but not for a temporary buffer that is used within on
> function.
> >
> > Size is different for different fsl,pins group. So need to use devm_kzalloc.
> >
> You obviously did not understand what I meant!
> Sure you need dynamic allocation, but it's complete nonsense to use the dev_-
> variant here, since the memory is only used for this one function call and
> deallocated immediately after use.
>
> There is no need to have it automatically released when the underlying udevice
> is destroyed.
Yeah. I thought when error, we could let devm_ to free the memory. But missed the point
that each time set_state, need to free the memory.
Regards,
Peng.
>
>
> Lothar Waßmann
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-11 13:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-11 2:43 [U-Boot] [PATCH] pinctrl: imx: fix memory leak Peng Fan
2017-05-11 9:17 ` Jagan Teki
2017-05-11 9:19 ` Peng Fan
[not found] ` <20170511111320.2316b6f3@ipc1.ka-ro>
2017-05-11 9:22 ` Peng Fan
[not found] ` <20170511120958.535e2e33@ipc1.ka-ro>
2017-05-11 13:18 ` Peng Fan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.