| Message ID | 1406288074-28725-1-git-send-email-pramod.gurav@smartplayin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pramod, On Fri, Jul 25, 2014 at 05:04:34PM +0530, pramod.gurav.etc@gmail.com wrote: > From: Pramod Gurav <pramod.gurav@smartplayin.com> > > This patch does below: > - Removes kfree done on data allocated with devm_zalloc in probe > path of the driver. > - Adds a check on return value from devm_kzalloc which was missing > > CC: Dmitry Torokhov <dmitry.torokhov@gmail.com> > CC: Lejun Zhu <lejun.zhu@linux.intel.com> > CC: Sachin Kamat <sachin.kamat@linaro.org> > > Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> > --- > drivers/input/misc/soc_button_array.c | 17 +++++++---------- > 1 file changed, 7 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c > index 5a6334b..fc64ec6 100644 > --- a/drivers/input/misc/soc_button_array.c > +++ b/drivers/input/misc/soc_button_array.c > @@ -83,6 +83,9 @@ soc_button_device_create(struct pnp_dev *pdev, > sizeof(*gpio_keys_pdata) + > sizeof(*gpio_keys) * MAX_NBUTTONS, > GFP_KERNEL); > + if (!gpio_keys_pdata) > + return ERR_PTR(-ENOMEM); OK, that makes sense. > + > gpio_keys = (void *)(gpio_keys_pdata + 1); > > for (info = button_info; info->name; info++) { > @@ -102,20 +105,16 @@ soc_button_device_create(struct pnp_dev *pdev, > n_buttons++; > } > > - if (n_buttons == 0) { > - error = -ENODEV; > - goto err_free_mem; > - } > + if (n_buttons == 0) > + return ERR_PTR(-ENODEV); But that one and the rest don't, because failure in soc_button_device_create() does not necessarily mean that binding for the whole device will fail. In this case we do not want unused memory hang around. Thanks.
Hi Dmitry, Thanks for the review. On Fri, Jul 25, 2014 at 9:52 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Pramod, > > On Fri, Jul 25, 2014 at 05:04:34PM +0530, pramod.gurav.etc@gmail.com wrote: >> From: Pramod Gurav <pramod.gurav@smartplayin.com> >> >> This patch does below: >> - Removes kfree done on data allocated with devm_zalloc in probe >> path of the driver. >> - Adds a check on return value from devm_kzalloc which was missing >> >> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> CC: Lejun Zhu <lejun.zhu@linux.intel.com> >> CC: Sachin Kamat <sachin.kamat@linaro.org> >> >> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> >> --- >> drivers/input/misc/soc_button_array.c | 17 +++++++---------- >> 1 file changed, 7 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c >> index 5a6334b..fc64ec6 100644 >> --- a/drivers/input/misc/soc_button_array.c >> +++ b/drivers/input/misc/soc_button_array.c >> @@ -83,6 +83,9 @@ soc_button_device_create(struct pnp_dev *pdev, >> sizeof(*gpio_keys_pdata) + >> sizeof(*gpio_keys) * MAX_NBUTTONS, >> GFP_KERNEL); >> + if (!gpio_keys_pdata) >> + return ERR_PTR(-ENOMEM); > > OK, that makes sense. > >> + >> gpio_keys = (void *)(gpio_keys_pdata + 1); >> >> for (info = button_info; info->name; info++) { >> @@ -102,20 +105,16 @@ soc_button_device_create(struct pnp_dev *pdev, >> n_buttons++; >> } >> >> - if (n_buttons == 0) { >> - error = -ENODEV; >> - goto err_free_mem; >> - } >> + if (n_buttons == 0) >> + return ERR_PTR(-ENODEV); > > But that one and the rest don't, because failure in > soc_button_device_create() does not necessarily mean that binding for > the whole device will fail. In this case we do not want unused memory > hang around. Agree. Should resend the patch with only the error check after mem allocation and will be little more careful while sending any such change. :) > > Thanks. > > -- > Dmitry
On July 27, 2014 11:50:41 PM PDT, pramod gurav <pramod.gurav.etc@gmail.com> wrote: >Hi Dmitry, > >Thanks for the review. > >On Fri, Jul 25, 2014 at 9:52 PM, Dmitry Torokhov ><dmitry.torokhov@gmail.com> wrote: >> Hi Pramod, >> >> On Fri, Jul 25, 2014 at 05:04:34PM +0530, pramod.gurav.etc@gmail.com >wrote: >>> From: Pramod Gurav <pramod.gurav@smartplayin.com> >>> >>> This patch does below: >>> - Removes kfree done on data allocated with devm_zalloc in probe >>> path of the driver. >>> - Adds a check on return value from devm_kzalloc which was missing >>> >>> CC: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>> CC: Lejun Zhu <lejun.zhu@linux.intel.com> >>> CC: Sachin Kamat <sachin.kamat@linaro.org> >>> >>> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> >>> --- >>> drivers/input/misc/soc_button_array.c | 17 +++++++---------- >>> 1 file changed, 7 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/input/misc/soc_button_array.c >b/drivers/input/misc/soc_button_array.c >>> index 5a6334b..fc64ec6 100644 >>> --- a/drivers/input/misc/soc_button_array.c >>> +++ b/drivers/input/misc/soc_button_array.c >>> @@ -83,6 +83,9 @@ soc_button_device_create(struct pnp_dev *pdev, >>> sizeof(*gpio_keys_pdata) + >>> sizeof(*gpio_keys) * >MAX_NBUTTONS, >>> GFP_KERNEL); >>> + if (!gpio_keys_pdata) >>> + return ERR_PTR(-ENOMEM); >> >> OK, that makes sense. >> >>> + >>> gpio_keys = (void *)(gpio_keys_pdata + 1); >>> >>> for (info = button_info; info->name; info++) { >>> @@ -102,20 +105,16 @@ soc_button_device_create(struct pnp_dev *pdev, >>> n_buttons++; >>> } >>> >>> - if (n_buttons == 0) { >>> - error = -ENODEV; >>> - goto err_free_mem; >>> - } >>> + if (n_buttons == 0) >>> + return ERR_PTR(-ENODEV); >> >> But that one and the rest don't, because failure in >> soc_button_device_create() does not necessarily mean that binding for >> the whole device will fail. In this case we do not want unused memory >> hang around. >Agree. Should resend the patch with only the error check after mem >allocation and will be little more careful while sending any such >change. :) No need to resend, I picked out the good bits and applied. Thanks.
On Mon, Jul 28, 2014 at 12:40 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On July 27, 2014 11:50:41 PM PDT, pramod gurav <pramod.gurav.etc@gmail.com> wrote: >>Hi Dmitry, >> > > No need to resend, I picked out the good bits and applied. Thanks. :)
diff --git a/drivers/input/misc/soc_button_array.c b/drivers/input/misc/soc_button_array.c index 5a6334b..fc64ec6 100644 --- a/drivers/input/misc/soc_button_array.c +++ b/drivers/input/misc/soc_button_array.c @@ -83,6 +83,9 @@ soc_button_device_create(struct pnp_dev *pdev, sizeof(*gpio_keys_pdata) + sizeof(*gpio_keys) * MAX_NBUTTONS, GFP_KERNEL); + if (!gpio_keys_pdata) + return ERR_PTR(-ENOMEM); + gpio_keys = (void *)(gpio_keys_pdata + 1); for (info = button_info; info->name; info++) { @@ -102,20 +105,16 @@ soc_button_device_create(struct pnp_dev *pdev, n_buttons++; } - if (n_buttons == 0) { - error = -ENODEV; - goto err_free_mem; - } + if (n_buttons == 0) + return ERR_PTR(-ENODEV); gpio_keys_pdata->buttons = gpio_keys; gpio_keys_pdata->nbuttons = n_buttons; gpio_keys_pdata->rep = autorepeat; pd = platform_device_alloc("gpio-keys", PLATFORM_DEVID_AUTO); - if (!pd) { - error = -ENOMEM; - goto err_free_mem; - } + if (!pd) + return ERR_PTR(-ENOMEM); error = platform_device_add_data(pd, gpio_keys_pdata, sizeof(*gpio_keys_pdata)); @@ -130,8 +129,6 @@ soc_button_device_create(struct pnp_dev *pdev, err_free_pdev: platform_device_put(pd); -err_free_mem: - devm_kfree(&pdev->dev, gpio_keys_pdata); return ERR_PTR(error); }