[PATCH] input: ads7846: Switch to managed version of kzalloc and cleanups

[PATCH] input: ads7846: Switch to managed version of kzalloc and cleanups

[pramod.gurav.etc]

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.

input device must be released(input_free_device) when ads7846_probe_dt
fails hence adds a fix there as well.

CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Guenter Roeck <linux@roeck-us.net>
CC: Paul Gortmaker <paul.gortmaker@windriver.com>
CC: Mark Brown <broonie@linaro.org>
CC: Fabio Estevam <fabio.estevam@freescale.com>

Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com>
---
Earlier when kzalloc was used to allocate, the error check for all three
resources (ts, packet, input_dev) was done together. This I felt might
cause a problem in case any of these allocations fails and we may end up
calling kfree on unallocated memory. Moving to managed resource solves this
anyway as we remove references of kfree in probe fail path.



Moving to managed resources solves this anyway.
 drivers/input/touchscreen/ads7846.c |   27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

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;
-- 
1.7.9.5

Re: [PATCH] input: ads7846: Switch to managed version of kzalloc and cleanups

[Dmitry Torokhov]

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.

-- 
Dmitry

Re: [PATCH] input: ads7846: Switch to managed version of kzalloc and cleanups

[Pramod Gurav]

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



-- 
Thanks and Regards
Pramod

Re: [PATCH] input: ads7846: Switch to managed version of kzalloc and cleanups

[Dmitry Torokhov]

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,

-- 
Dmitry