| Message ID | 1494470582-23661-1-git-send-email-peng.fan@nxp.com |
|---|---|
| State | Superseded |
| Headers | show |
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!
> -----Original Message----- > From: Jagan Teki [mailto:jagannadh.teki@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@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.
> -----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); > > 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
> -----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); > > > > 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
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; }
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(+)