-----Original Message----- From: Lothar Waßmann [mailto:LW@KARO-electronics.de] Sent: Thursday, May 11, 2017 6:10 PM To: Peng Fan peng.fan@nxp.com Cc: sjg@chromium.org; u-boot@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@KARO-electronics.de] Sent: Thursday, May 11, 2017 5:13 PM To: Peng Fan peng.fan@nxp.com Cc: sjg@chromium.org; u-boot@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);@@ -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