[PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

If the WM831x pen down and data IRQs run in parallel it is possible for the
data and pen down IRQs to deadlock themselves as one is part way through
disabling its operation while the other is part way through enabling. Fix
this by always disabling the pen down interrupt while data is active and
vice versa.  When a changeover is required we disable the IRQ that is to
be stopped then schedule work that will enable the new IRQ.

We need to handle the data flow in the data IRQ as the readback from the
device needs to be ordered correctly with the IRQ for robust operation.

This also fixes an issue when using the built in IRQs due to enable_irq()
not being valid from interrupt context on an interrupt controller with bus
operations like the built in IRQ controller - this issue may also have
affected other interrupt controllers.  We can't rely on having the data
and pen down IRQs available via GPIOs on the CPU on every system.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

Main change since last time is that the two struct works have been
merged and we now use disable_irq_nosync() consistently in both IRQ
handlers.

 drivers/input/touchscreen/wm831x-ts.c |   54 +++++++++++++++++++++++++++++---
 1 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 6ae054f..b937301 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -68,8 +68,23 @@ struct wm831x_ts {
 	unsigned int pd_irq;
 	bool pressure;
 	bool pen_down;
+	struct work_struct pd_data_work;
 };
 
+static void wm831x_pd_data_work(struct work_struct *work)
+{
+	struct wm831x_ts *wm831x_ts =
+		container_of(work, struct wm831x_ts, pd_data_work);
+
+	if (wm831x_ts->pen_down) {
+		enable_irq(wm831x_ts->data_irq);
+		dev_dbg(wm831x_ts->wm831x->dev, "IRQ PD->DATA done\n");
+	} else {
+		enable_irq(wm831x_ts->pd_irq);
+		dev_dbg(wm831x_ts->wm831x->dev, "IRQ DATA->PD done\n");
+	}
+}
+
 static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data)
 {
 	struct wm831x_ts *wm831x_ts = irq_data;
@@ -110,6 +125,9 @@ static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data)
 	}
 
 	if (!wm831x_ts->pen_down) {
+		/* Switch from data to pen down */
+		dev_dbg(wm831x->dev, "IRQ DATA->PD\n");
+
 		disable_irq_nosync(wm831x_ts->data_irq);
 
 		/* Don't need data any more */
@@ -128,6 +146,8 @@ static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data)
 					 ABS_PRESSURE, 0);
 
 		input_report_key(wm831x_ts->input_dev, BTN_TOUCH, 0);
+
+		schedule_work(&wm831x_ts->pd_data_work);
 	}
 
 	input_sync(wm831x_ts->input_dev);
@@ -141,6 +161,11 @@ static irqreturn_t wm831x_ts_pen_down_irq(int irq, void *irq_data)
 	struct wm831x *wm831x = wm831x_ts->wm831x;
 	int ena = 0;
 
+	if (wm831x_ts->pen_down)
+		return IRQ_HANDLED;
+
+	disable_irq_nosync(wm831x_ts->pd_irq);
+
 	/* Start collecting data */
 	if (wm831x_ts->pressure)
 		ena |= WM831X_TCH_Z_ENA;
@@ -156,7 +181,10 @@ static irqreturn_t wm831x_ts_pen_down_irq(int irq, void *irq_data)
 			WM831X_TCHPD_EINT, WM831X_TCHPD_EINT);
 
 	wm831x_ts->pen_down = true;
-	enable_irq(wm831x_ts->data_irq);
+
+	/* Switch from pen down to data */
+	dev_dbg(wm831x->dev, "IRQ PD->DATA\n");
+	schedule_work(&wm831x_ts->pd_data_work);
 
 	return IRQ_HANDLED;
 }
@@ -182,13 +210,28 @@ static void wm831x_ts_input_close(struct input_dev *idev)
 	struct wm831x_ts *wm831x_ts = input_get_drvdata(idev);
 	struct wm831x *wm831x = wm831x_ts->wm831x;
 
+	/* Shut the controller down, disabling all other functionality too */
 	wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1,
-			WM831X_TCH_ENA | WM831X_TCH_CVT_ENA |
-			WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA |
-			WM831X_TCH_Z_ENA, 0);
+			WM831X_TCH_ENA | WM831X_TCH_X_ENA |
+			WM831X_TCH_Y_ENA | WM831X_TCH_Z_ENA, 0);
 
-	if (wm831x_ts->pen_down)
+	/* Make sure any pending IRQs are done, the above will prevent
+	 * new ones firing.
+	 */
+	synchronize_irq(wm831x_ts->data_irq);
+	synchronize_irq(wm831x_ts->pd_irq);
+
+	/* Make sure the IRQ completion work is quiesced */
+	flush_work_sync(&wm831x_ts->pd_data_work);
+
+	/* If we ended up with the pen down then make sure we revert back
+	 * to pen detection state for the next time we start up.
+	 */
+	if (wm831x_ts->pen_down) {
 		disable_irq(wm831x_ts->data_irq);
+		enable_irq(wm831x_ts->pd_irq);
+		wm831x_ts->pen_down = false;
+	}
 }
 
 static __devinit int wm831x_ts_probe(struct platform_device *pdev)
@@ -212,6 +255,7 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 
 	wm831x_ts->wm831x = wm831x;
 	wm831x_ts->input_dev = input_dev;
+	INIT_WORK(&wm831x_ts->pd_data_work, wm831x_pd_data_work);
 
 	/*
 	 * If we have a direct IRQ use it, otherwise use the interrupt
-- 
1.7.4.1

[PATCH 2/3] Input: wm831x-ts - Allow IRQ flags to be specified

[Mark Brown]

This allows maximum flexibility for configuring the direct GPIO based
interrupts.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/input/touchscreen/wm831x-ts.c |   16 +++++++++++++---
 include/linux/mfd/wm831x/pdata.h      |    2 ++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index b937301..78e8705 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -241,7 +241,7 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 	struct wm831x_pdata *core_pdata = dev_get_platdata(pdev->dev.parent);
 	struct wm831x_touch_pdata *pdata = NULL;
 	struct input_dev *input_dev;
-	int error;
+	int error, irqf;
 
 	if (core_pdata)
 		pdata = core_pdata->touch;
@@ -314,9 +314,14 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 	wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1,
 			WM831X_TCH_RATE_MASK, 6);
 
+	if (pdata && pdata->data_irqf)
+		irqf = pdata->data_irqf;
+	else
+		irqf = IRQF_TRIGGER_HIGH;
+
 	error = request_threaded_irq(wm831x_ts->data_irq,
 				     NULL, wm831x_ts_data_irq,
-				     IRQF_ONESHOT,
+				     irqf | IRQF_ONESHOT,
 				     "Touchscreen data", wm831x_ts);
 	if (error) {
 		dev_err(&pdev->dev, "Failed to request data IRQ %d: %d\n",
@@ -325,9 +330,14 @@ static __devinit int wm831x_ts_probe(struct platform_device *pdev)
 	}
 	disable_irq(wm831x_ts->data_irq);
 
+	if (pdata && pdata->pd_irqf)
+		irqf = pdata->pd_irqf;
+	else
+		irqf = IRQF_TRIGGER_HIGH;
+
 	error = request_threaded_irq(wm831x_ts->pd_irq,
 				     NULL, wm831x_ts_pen_down_irq,
-				     IRQF_ONESHOT,
+				     irqf | IRQF_ONESHOT,
 				     "Touchscreen pen down", wm831x_ts);
 	if (error) {
 		dev_err(&pdev->dev, "Failed to request pen down IRQ %d: %d\n",
diff --git a/include/linux/mfd/wm831x/pdata.h b/include/linux/mfd/wm831x/pdata.h
index cef19f0..ff42d70 100644
--- a/include/linux/mfd/wm831x/pdata.h
+++ b/include/linux/mfd/wm831x/pdata.h
@@ -81,7 +81,9 @@ struct wm831x_touch_pdata {
 	int rpu;               /** Pen down sensitivity resistor divider */
 	int pressure;          /** Report pressure (boolean) */
 	unsigned int data_irq; /** Touch data ready IRQ */
+	int data_irqf;         /** IRQ flags for data ready IRQ */
 	unsigned int pd_irq;   /** Touch pendown detect IRQ */
+	int pd_irqf;           /** IRQ flags for pen down IRQ */
 };
 
 enum wm831x_watchdog_action {
-- 
1.7.4.1

[PATCH 3/3] Input: wm831x-ts - Move BTN_TOUCH reporting to data transfer

[Mark Brown]

Don't report BTN_TOUCH until we've got data as some less robust applications
can be confused by getting a touch event by itself and it doesn't seem
unreasonable for them to expect coordinates along with a touch.

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

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 78e8705..9175d49 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -148,6 +148,8 @@ static irqreturn_t wm831x_ts_data_irq(int irq, void *irq_data)
 		input_report_key(wm831x_ts->input_dev, BTN_TOUCH, 0);
 
 		schedule_work(&wm831x_ts->pd_data_work);
+	} else {
+		input_report_key(wm831x_ts->input_dev, BTN_TOUCH, 1);
 	}
 
 	input_sync(wm831x_ts->input_dev);
@@ -174,9 +176,6 @@ static irqreturn_t wm831x_ts_pen_down_irq(int irq, void *irq_data)
 			WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA | WM831X_TCH_Z_ENA,
 			WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA | ena);
 
-	input_report_key(wm831x_ts->input_dev, BTN_TOUCH, 1);
-	input_sync(wm831x_ts->input_dev);
-
 	wm831x_set_bits(wm831x, WM831X_INTERRUPT_STATUS_1,
 			WM831X_TCHPD_EINT, WM831X_TCHPD_EINT);
 
-- 
1.7.4.1

Re: [PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

On Tue, Apr 19, 2011 at 07:17:52PM +0100, Mark Brown wrote:
> If the WM831x pen down and data IRQs run in parallel it is possible for the
> data and pen down IRQs to deadlock themselves as one is part way through
> disabling its operation while the other is part way through enabling. Fix
> this by always disabling the pen down interrupt while data is active and
> vice versa.  When a changeover is required we disable the IRQ that is to
> be stopped then schedule work that will enable the new IRQ.

Any updates on this?

Re: [PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[Dmitry Torokhov]

On Tue, May 03, 2011 at 03:46:18PM +0100, Mark Brown wrote:
> On Tue, Apr 19, 2011 at 07:17:52PM +0100, Mark Brown wrote:
> > If the WM831x pen down and data IRQs run in parallel it is possible for the
> > data and pen down IRQs to deadlock themselves as one is part way through
> > disabling its operation while the other is part way through enabling. Fix
> > this by always disabling the pen down interrupt while data is active and
> > vice versa.  When a changeover is required we disable the IRQ that is to
> > be stopped then schedule work that will enable the new IRQ.
> 
> Any updates on this?

I sent a pull request to Linus last night, hopefully he'll pull.

Thanks.

-- 
Dmitry

Re: [PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

On Tue, May 03, 2011 at 08:43:03AM -0700, Dmitry Torokhov wrote:
> On Tue, May 03, 2011 at 03:46:18PM +0100, Mark Brown wrote:

> > Any updates on this?

> I sent a pull request to Linus last night, hopefully he'll pull.

Oh, OK - I hadn't noticed that you'd applied the patches at all (they
didn't vanish in my last -next rebase on Thursday and you didn't reply
to my mail) so I thought they'd slipped by when you were reviewing.

Re: [PATCH 1/3 2.6.39] Input: wm831x-ts - Fix races with IRQ management

[Dmitry Torokhov]

On Tue, May 03, 2011 at 04:44:14PM +0100, Mark Brown wrote:
> On Tue, May 03, 2011 at 08:43:03AM -0700, Dmitry Torokhov wrote:
> > On Tue, May 03, 2011 at 03:46:18PM +0100, Mark Brown wrote:
> 
> > > Any updates on this?
> 
> > I sent a pull request to Linus last night, hopefully he'll pull.
> 
> Oh, OK - I hadn't noticed that you'd applied the patches at all (they
> didn't vanish in my last -next rebase on Thursday

I think I applied them on Thursday night, rigth after -next was
released.

> and you didn't reply to my mail) 

Yeah, sorry about this, a few things are going on so I am slow with my
mail.

> so I thought they'd slipped by when you were reviewing.

-- 
Dmitry