Re: [PATCH] touchscreen: ads7846: please don't touch free'd memory

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Tue, May 25, 2010 at 03:18:13PM -0700, Kevin Hilman wrote:
> Mike Frysinger <vapier.adi@gmail.com> writes:
> 
> > On Tue, May 25, 2010 at 17:46, Kevin Hilman wrote:
> >> After digging into the driver core and realizing that it seemed to
> >> have sane error handling itself, I took a closer look at
> >> ads7846_probe() and discovered it doesn't actually return an error
> >> code for certain failure cases!  That was the root cause.
> >
> > that is crappy
> >
> >> Subject: [PATCH] input: touchscreen: ads7846: return error on probe failure
> >
> > i'd refer to the specific probe issue rather than just "probe".  maybe:
> > input: touchscreen: ads7846: return error on regulator_get() failure
> 
> Thanks for the review, here's one with updated subject and ack added.
> 
> Kevin
> 
> From 8ce49a91341d8713f870d2a931969f227a82b8ad Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@deeprootsystems.com>
> Date: Tue, 25 May 2010 14:38:04 -0700
> Subject: [PATCH] input: touchscreen: ads7846: return error on regulator_get() failure
> 
> In probe(), if regulator_get() failed, an error code was not being
> returned causing the driver to be successfully bound, even though
> probe failed.  This in turn caused the suspend, resume and remove
> methods to be registered and accessed via the SPI core.  Since these
> functions all access private driver data using pointers that had been
> freed during the failed probe, this would lead to unpredictable
> behavior.
> 
> This patch ensures that probe() returns an error code in this failure
> case so the driver is not bound.
> 
> Found using lockdep and noticing the lock used in the suspend/resum
> path pointed to a bogus lock due to the freed memory.
> 
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> Acked-by: Mike Frysinger <vapier@gentoo.org>

OK, this makes much better sense. Will aplly, thank you Kevin.

> ---
>  drivers/input/touchscreen/ads7846.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index 532279c..634f6f6 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -1163,8 +1163,8 @@ static int __devinit ads7846_probe(struct spi_device *spi)
>  
>  	ts->reg = regulator_get(&spi->dev, "vcc");
>  	if (IS_ERR(ts->reg)) {
> -		dev_err(&spi->dev, "unable to get regulator: %ld\n",
> -			PTR_ERR(ts->reg));
> +		err = PTR_ERR(ts->reg);
> +		dev_err(&spi->dev, "unable to get regulator: %ld\n", err);
>  		goto err_free_gpio;
>  	}
>  
> -- 
> 1.7.0.2
> 

-- 
Dmitry