From: Kevin Hilman <khilman@deeprootsystems.com> To: Mike Frysinger <vapier.adi@gmail.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>, linux-input@vger.kernel.org, linux-omap@vger.kernel.org, Michael Roth <mroth@nessie.de>, Pavel Machek <pavel@ucw.cz>, Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org Subject: Re: [PATCH] touchscreen: ads7846: please don't touch free'd memory Date: Tue, 25 May 2010 15:18:13 -0700 [thread overview] Message-ID: <87r5kzk4m2.fsf@deeprootsystems.com> (raw) In-Reply-To: <AANLkTik72Q50eHX5cLW4uZ13BiMVCKQ0_bKdrCGZxpeE@mail.gmail.com> (Mike Frysinger's message of "Tue\, 25 May 2010 17\:56\:27 -0400") 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> --- 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
WARNING: multiple messages have this Message-ID (diff)
From: Kevin Hilman <khilman@deeprootsystems.com> To: Mike Frysinger <vapier.adi@gmail.com> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>, linux-input@vger.kernel.org, linux-omap@vger.kernel.org, Michael Roth <mroth@nessie.de>, Pavel Machek <pavel@ucw.cz>, Andrew Morton <akpm@linux-foundation.org>, linux-kernel@vger.kernel.org Subject: Re: [PATCH] touchscreen: ads7846: please don't touch free'd memory Date: Tue, 25 May 2010 15:18:13 -0700 [thread overview] Message-ID: <87r5kzk4m2.fsf@deeprootsystems.com> (raw) In-Reply-To: <AANLkTik72Q50eHX5cLW4uZ13BiMVCKQ0_bKdrCGZxpeE@mail.gmail.com> (Mike Frysinger's message of "Tue\, 25 May 2010 17\:56\:27 -0400") 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> --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2010-05-25 22:18 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-05-18 23:46 [PATCH] touchscreen: ads7846: please don't touch free'd memory Kevin Hilman 2010-05-19 0:00 ` Dmitry Torokhov 2010-05-25 19:52 ` Kevin Hilman 2010-05-25 20:21 ` Dmitry Torokhov 2010-05-25 21:46 ` Kevin Hilman 2010-05-25 21:56 ` Mike Frysinger 2010-05-25 21:56 ` Mike Frysinger 2010-05-25 22:18 ` Kevin Hilman [this message] 2010-05-25 22:18 ` Kevin Hilman 2010-05-25 22:25 ` Dmitry Torokhov 2010-05-25 22:25 ` Dmitry Torokhov 2010-05-26 0:09 ` David Brownell
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87r5kzk4m2.fsf@deeprootsystems.com \ --to=khilman@deeprootsystems.com \ --cc=akpm@linux-foundation.org \ --cc=dmitry.torokhov@gmail.com \ --cc=linux-input@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=mroth@nessie.de \ --cc=pavel@ucw.cz \ --cc=vapier.adi@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.