[PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Mark Brown]

Saves a little code and eliminates the possibility of introducing some
leaks.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/input/touchscreen/wm831x-ts.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index d81482e..e34c10a 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -245,7 +245,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 	if (core_pdata)
 		pdata = core_pdata->touch;
 
-	wm831x_ts = kzalloc(sizeof(struct wm831x_ts), GFP_KERNEL);
+	wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts),
+				 GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!wm831x_ts || !input_dev) {
 		error = -ENOMEM;
@@ -375,7 +376,6 @@ err_data_irq:
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 err_alloc:
 	input_free_device(input_dev);
-	kfree(wm831x_ts);
 
 	return error;
 }
@@ -387,7 +387,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev)
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 	input_unregister_device(wm831x_ts->input_dev);
-	kfree(wm831x_ts);
 
 	return 0;
 }
-- 
1.7.10

[PATCH 1/3] Input: wm831x-ts - Remove unneeded clearing of driver data

[Mark Brown]

This is unneeded, only a bound driver can use driver data and a driver
relying on the state prior to probe() is buggy.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/input/touchscreen/wm831x-ts.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 52abb98..bf0a4a6 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -390,7 +390,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev)
 	input_unregister_device(wm831x_ts->input_dev);
 	kfree(wm831x_ts);
 
-	platform_set_drvdata(pdev, NULL);
 	return 0;
 }
 
-- 
1.7.10.4

[PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Mark Brown]

Saves a little code and eliminates the possibility of introducing some
leaks.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/input/touchscreen/wm831x-ts.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index bf0a4a6..c7eee56 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -245,7 +245,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 	if (core_pdata)
 		pdata = core_pdata->touch;
 
-	wm831x_ts = kzalloc(sizeof(struct wm831x_ts), GFP_KERNEL);
+	wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts),
+				 GFP_KERNEL);
 	input_dev = input_allocate_device();
 	if (!wm831x_ts || !input_dev) {
 		error = -ENOMEM;
@@ -376,7 +377,6 @@ err_data_irq:
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 err_alloc:
 	input_free_device(input_dev);
-	kfree(wm831x_ts);
 
 	return error;
 }
@@ -388,7 +388,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev)
 	free_irq(wm831x_ts->pd_irq, wm831x_ts);
 	free_irq(wm831x_ts->data_irq, wm831x_ts);
 	input_unregister_device(wm831x_ts->input_dev);
-	kfree(wm831x_ts);
 
 	return 0;
 }
-- 
1.7.10.4

[PATCH 3/3] Input: wm831x-on - Convert to devm_kzalloc()

[Mark Brown]

Saves a small amount of code and reduces the potential for leaks.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/input/misc/wm831x-on.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/wm831x-on.c b/drivers/input/misc/wm831x-on.c
index 6790a81..fa8b390 100644
--- a/drivers/input/misc/wm831x-on.c
+++ b/drivers/input/misc/wm831x-on.c
@@ -76,7 +76,8 @@ static int __devinit wm831x_on_probe(struct platform_device *pdev)
 	int irq = wm831x_irq(wm831x, platform_get_irq(pdev, 0));
 	int ret;
 
-	wm831x_on = kzalloc(sizeof(struct wm831x_on), GFP_KERNEL);
+	wm831x_on = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_on),
+				 GFP_KERNEL);
 	if (!wm831x_on) {
 		dev_err(&pdev->dev, "Can't allocate data\n");
 		return -ENOMEM;
@@ -120,7 +121,6 @@ err_irq:
 err_input_dev:
 	input_free_device(wm831x_on->dev);
 err:
-	kfree(wm831x_on);
 	return ret;
 }
 
@@ -132,7 +132,6 @@ static int __devexit wm831x_on_remove(struct platform_device *pdev)
 	free_irq(irq, wm831x_on);
 	cancel_delayed_work_sync(&wm831x_on->work);
 	input_unregister_device(wm831x_on->dev);
-	kfree(wm831x_on);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Dmitry Torokhov]

On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote:
> Saves a little code and eliminates the possibility of introducing some
> leaks.
> 

*sigh* OK, I guess devm_* is here to stay and I have to get on with the
program. I am still unhappy that half of the patches converting/using
devm_* APIs are wrong (not these ones), but I will apply these 3.

Thanks.

> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/input/touchscreen/wm831x-ts.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
> index bf0a4a6..c7eee56 100644
> --- a/drivers/input/touchscreen/wm831x-ts.c
> +++ b/drivers/input/touchscreen/wm831x-ts.c
> @@ -245,7 +245,8 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
>  	if (core_pdata)
>  		pdata = core_pdata->touch;
>  
> -	wm831x_ts = kzalloc(sizeof(struct wm831x_ts), GFP_KERNEL);
> +	wm831x_ts = devm_kzalloc(&pdev->dev, sizeof(struct wm831x_ts),
> +				 GFP_KERNEL);
>  	input_dev = input_allocate_device();
>  	if (!wm831x_ts || !input_dev) {
>  		error = -ENOMEM;
> @@ -376,7 +377,6 @@ err_data_irq:
>  	free_irq(wm831x_ts->data_irq, wm831x_ts);
>  err_alloc:
>  	input_free_device(input_dev);
> -	kfree(wm831x_ts);
>  
>  	return error;
>  }
> @@ -388,7 +388,6 @@ static __devexit int wm831x_ts_remove(struct platform_device *pdev)
>  	free_irq(wm831x_ts->pd_irq, wm831x_ts);
>  	free_irq(wm831x_ts->data_irq, wm831x_ts);
>  	input_unregister_device(wm831x_ts->input_dev);
> -	kfree(wm831x_ts);
>  
>  	return 0;
>  }
> -- 
> 1.7.10.4
> 

-- 
Dmitry

Re: [PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Mark Brown]

On Thu, Oct 11, 2012 at 12:39:55AM -0700, Dmitry Torokhov wrote:
> On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote:
> > Saves a little code and eliminates the possibility of introducing some
> > leaks.

> *sigh* OK, I guess devm_* is here to stay and I have to get on with the
> program. I am still unhappy that half of the patches converting/using
> devm_* APIs are wrong (not these ones), but I will apply these 3.

What's the error pattern you're seeing?  I've not noticed much of an
issue here, but if there is one perhaps we can do something to make the
error more obvious or harder to introduce.

Re: [PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Dmitry Torokhov]

On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote:
> On Thu, Oct 11, 2012 at 12:39:55AM -0700, Dmitry Torokhov wrote:
> > On Wed, Oct 10, 2012 at 10:20:35PM +0900, Mark Brown wrote:
> > > Saves a little code and eliminates the possibility of introducing some
> > > leaks.
> 
> > *sigh* OK, I guess devm_* is here to stay and I have to get on with the
> > program. I am still unhappy that half of the patches converting/using
> > devm_* APIs are wrong (not these ones), but I will apply these 3.
> 
> What's the error pattern you're seeing?  I've not noticed much of an
> issue here, but if there is one perhaps we can do something to make the
> error more obvious or harder to introduce.

int driver_probe()
{
	devm_kzalloc();
	input_allocate_device();
	...
	devm_request_irq();
	...
	input_register_device();
....
}

void driver_remove()
{
	input_unregister_device();
	/* rely on deves for cleanup */
}

The problem is that input device is freed but interrupts are still fully
functional.

-- 
Dmitry

Re: [PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Mark Brown]

On Thu, Oct 11, 2012 at 01:27:49AM -0700, Dmitry Torokhov wrote:
> On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote:

> > 
> > What's the error pattern you're seeing?  I've not noticed much of an
> > issue here, but if there is one perhaps we can do something to make the
> > error more obvious or harder to introduce.

> 	devm_request_irq();

> The problem is that input device is freed but interrupts are still fully
> functional.

Ah, yes - that one I do spot all the time.  I agree that devm_request_irq()
is a menace, that error is far too easy to introduce and it always seems
more work to work out if it's safe than the benefit in the cases where
it can be used.

The other devm APIs are less problematic, though.

Re: [PATCH 2/3] Input: wm831x-ts - Convert to devm_kzalloc()

[Dmitry Torokhov]

On Thu, Oct 11, 2012 at 05:33:24PM +0900, Mark Brown wrote:
> On Thu, Oct 11, 2012 at 01:27:49AM -0700, Dmitry Torokhov wrote:
> > On Thu, Oct 11, 2012 at 05:07:24PM +0900, Mark Brown wrote:
> 
> > > 
> > > What's the error pattern you're seeing?  I've not noticed much of an
> > > issue here, but if there is one perhaps we can do something to make the
> > > error more obvious or harder to introduce.
> 
> > 	devm_request_irq();
> 
> > The problem is that input device is freed but interrupts are still fully
> > functional.
> 
> Ah, yes - that one I do spot all the time.  I agree that devm_request_irq()
> is a menace, that error is far too easy to introduce and it always seems
> more work to work out if it's safe than the benefit in the cases where
> it can be used.

Right. Another one (the IRQ again): have IRQ schedule [delayed] work and
then use cancel_delayed_work() in ->remove() but rely on devres to free
IRQ which is the wrong order.

> 
> The other devm APIs are less problematic, though.

I agree, they are indeed safer. I guess if I add devm_input* the some of
the devm_request_*irq() will be safe as well, but we are not there yet.

-- 
Dmitry