| Message ID | 1406537216-25936-1-git-send-email-pramod.gurav@smartplayin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pramod, On Mon, Jul 28, 2014 at 02:16:55PM +0530, pramod.gurav.etc@gmail.com wrote: > From: Pramod Gurav <pramod.gurav@smartplayin.com> > > This switches memory allocations from kzalloc to devm_kzalloc. > This also changes the way return checks were done on failure cases > of three allocations. The checks were clubbed together and hence > must be done seperately to avoid calling kfree on unallocated memory. > > Moreover in case of input_allocate_device failure, we were calling > input_free_device which is not needed. But not hurting either - like kfree() and many other "cleanup" functions in kernel they are happily accept NULL input (so that cleanup code is less branchy). > > input device must be released(input_free_device) when ads7846_probe_dt > fails hence adds a fix there as well. That is the real bug, could you send me a patch that fixes just that (by jumping to err_free_mem)? The rest I'd like to leave as is unless you can convert _all_ resources to be managed ones: mixing up 2 styles (manual and automatic release) is bound to have issues (like we had with ads7846_probe_dt which assumed that since it was releasing its resources automatically the rest would be released automatically as well. Thanks.
Thanks Dmitry for reviewing. On Mon, Jul 28, 2014 at 10:19 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi Pramod, > > On Mon, Jul 28, 2014 at 02:16:55PM +0530, pramod.gurav.etc@gmail.com wrote: >> From: Pramod Gurav <pramod.gurav@smartplayin.com> >> >> This switches memory allocations from kzalloc to devm_kzalloc. >> This also changes the way return checks were done on failure cases >> of three allocations. The checks were clubbed together and hence >> must be done seperately to avoid calling kfree on unallocated memory. >> >> Moreover in case of input_allocate_device failure, we were calling >> input_free_device which is not needed. > > But not hurting either - like kfree() and many other "cleanup" functions > in kernel they are happily accept NULL input (so that cleanup code is > less branchy). >> Great. Good to learn. >> input device must be released(input_free_device) when ads7846_probe_dt >> fails hence adds a fix there as well. > > That is the real bug, could you send me a patch that fixes just that (by > jumping to err_free_mem)? Sure I will. > > The rest I'd like to leave as is unless you can convert _all_ resources > to be managed ones: mixing up 2 styles (manual and automatic release) is > bound to have issues (like we had with ads7846_probe_dt which assumed > that since it was releasing its resources automatically the rest would > be released automatically as well. I thought I would use a managed input_device allocation in a separate patch. I will convert all allocations to managed then. This driver also registers a irq. You suggest to convert it to managed as well? I was planning to do that? > > Thanks. > > -- > Dmitry
On Mon, Jul 28, 2014 at 10:37:18PM +0530, Pramod Gurav wrote: > Thanks Dmitry for reviewing. > > On Mon, Jul 28, 2014 at 10:19 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > Hi Pramod, > > > > On Mon, Jul 28, 2014 at 02:16:55PM +0530, pramod.gurav.etc@gmail.com wrote: > >> From: Pramod Gurav <pramod.gurav@smartplayin.com> > >> > >> This switches memory allocations from kzalloc to devm_kzalloc. > >> This also changes the way return checks were done on failure cases > >> of three allocations. The checks were clubbed together and hence > >> must be done seperately to avoid calling kfree on unallocated memory. > >> > >> Moreover in case of input_allocate_device failure, we were calling > >> input_free_device which is not needed. > > > > But not hurting either - like kfree() and many other "cleanup" functions > > in kernel they are happily accept NULL input (so that cleanup code is > > less branchy). > >> > Great. Good to learn. > >> input device must be released(input_free_device) when ads7846_probe_dt > >> fails hence adds a fix there as well. > > > > That is the real bug, could you send me a patch that fixes just that (by > > jumping to err_free_mem)? > Sure I will. > > > > > The rest I'd like to leave as is unless you can convert _all_ resources > > to be managed ones: mixing up 2 styles (manual and automatic release) is > > bound to have issues (like we had with ads7846_probe_dt which assumed > > that since it was releasing its resources automatically the rest would > > be released automatically as well. > > I thought I would use a managed input_device allocation in a separate patch. > I will convert all allocations to managed then. > > This driver also registers a irq. You suggest to convert it to managed as well? > I was planning to do that? Yes. And regulators, gpios, sysfs (you might need to have custom action for that) and chip cleanup. OTOH I do not see much value in these conversions unless they make the code much more clean. Thanks,
diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c index da201b8..d229b8d 100644 --- a/drivers/input/touchscreen/ads7846.c +++ b/drivers/input/touchscreen/ads7846.c @@ -1282,13 +1282,17 @@ static int ads7846_probe(struct spi_device *spi) if (err < 0) return err; - ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL); - packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL); + ts = devm_kzalloc(&spi->dev, sizeof(*ts), GFP_KERNEL); + if (!ts) + return -ENOMEM; + + packet = devm_kzalloc(&spi->dev, sizeof(*packet), GFP_KERNEL); + if (!packet) + return -ENOMEM; + input_dev = input_allocate_device(); - if (!ts || !packet || !input_dev) { - err = -ENOMEM; - goto err_free_mem; - } + if (!input_dev) + return -ENOMEM; spi_set_drvdata(spi, ts); @@ -1302,8 +1306,10 @@ static int ads7846_probe(struct spi_device *spi) pdata = dev_get_platdata(&spi->dev); if (!pdata) { pdata = ads7846_probe_dt(&spi->dev); - if (IS_ERR(pdata)) - return PTR_ERR(pdata); + if (IS_ERR(pdata)) { + err = PTR_ERR(pdata); + goto err_free_mem; + } } ts->model = pdata->model ? : 7846; @@ -1450,8 +1456,6 @@ static int ads7846_probe(struct spi_device *spi) ts->filter_cleanup(ts->filter_data); err_free_mem: input_free_device(input_dev); - kfree(packet); - kfree(ts); return err; } @@ -1484,9 +1488,6 @@ static int ads7846_remove(struct spi_device *spi) if (ts->filter_cleanup) ts->filter_cleanup(ts->filter_data); - kfree(ts->packet); - kfree(ts); - dev_dbg(&spi->dev, "unregistered touchscreen\n"); return 0;