[PATCH] Input: wm831x-ts - Fix races with IRQ management

[PATCH] 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.

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.

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

diff --git a/drivers/input/touchscreen/wm831x-ts.c b/drivers/input/touchscreen/wm831x-ts.c
index 1022f71..3c397c8 100644
--- a/drivers/input/touchscreen/wm831x-ts.c
+++ b/drivers/input/touchscreen/wm831x-ts.c
@@ -68,8 +68,29 @@ struct wm831x_ts {
 	unsigned int pd_irq;
 	bool pressure;
 	bool pen_down;
+	struct work_struct pd_data_work;
+	struct work_struct data_pd_work;
 };
 
+static void wm831x_pd_data(struct work_struct *work)
+{
+	struct wm831x_ts *wm831x_ts =
+		container_of(work, struct wm831x_ts, pd_data_work);
+
+	disable_irq(wm831x_ts->pd_irq);
+	enable_irq(wm831x_ts->data_irq);
+	dev_dbg(wm831x_ts->wm831x->dev, "IRQ PD->DATA done\n");
+}
+
+static void wm831x_data_pd(struct work_struct *work)
+{
+	struct wm831x_ts *wm831x_ts =
+		container_of(work, struct wm831x_ts, data_pd_work);
+
+	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,7 +131,10 @@ 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);
+		schedule_work(&wm831x_ts->data_pd_work);
 
 		/* Don't need data any more */
 		wm831x_set_bits(wm831x, WM831X_TOUCH_CONTROL_1,
@@ -156,7 +180,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;
 }
@@ -185,8 +212,13 @@ static void wm831x_ts_input_close(struct input_dev *idev)
 			WM831X_TCH_X_ENA | WM831X_TCH_Y_ENA |
 			WM831X_TCH_Z_ENA, 0);
 
-	if (wm831x_ts->pen_down)
+	if (wm831x_ts->pen_down) {
+		flush_work_sync(&wm831x_ts->data_pd_work);
 		disable_irq(wm831x_ts->data_irq);
+		enable_irq(wm831x_ts->pd_irq);
+	} else {
+		flush_work_sync(&wm831x_ts->pd_data_work);
+	}
 }
 
 static __devinit int wm831x_ts_probe(struct platform_device *pdev)
@@ -210,6 +242,8 @@ 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);
+	INIT_WORK(&wm831x_ts->data_pd_work, wm831x_data_pd);
 
 	/*
 	 * If we have a direct IRQ use it, otherwise use the interrupt
-- 
1.7.2.3

Re: [PATCH] Input: wm831x-ts - Fix races with IRQ management

[Dmitry Torokhov]

On Tue, Mar 01, 2011 at 04:54:19PM +0000, 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.
> 
> 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.
> 

Hmm, this is quite ugly. The race that you mentioned should be dealt
with by rearranging the order in which you disable IRQ and enable the
conversions. I.e. in wm831x_ts_pen_down_irq() you should do:

	disable_irq_nosync(wm831x_ts->pd_irq);
	wm831x_ts->pendown = true;

	...
	enable_irq(wm831x_ts->data_irq);

The issue is that wm831x_ts_input_close() is racy; it simply writes into
WM831X_TOUCH_CONTROL_1 and hopes that it will stick...

What is needed I guess is wm831x_ts->open flag that
wm831x_ts_input_open() would set and wm831x_ts_input_close() would reset
and the interrupt handlers would check before touching the registers
or enabling their counterparts. wm831x_ts_input_close() would reset the
flag first and then disable first pd_irq and then data_irq, before
writing to the control register. The only issue is to keep track of irq
disble counters.

The issue with enable_irq() not working in the irq handler thread should
be dealt at the controller level (i.e. fix it so that it works).

Thanks.

-- 
Dmitry

Re: [PATCH] Input: wm831x-ts - Fix races with IRQ management

[Mark Brown]

On Sat, Mar 12, 2011 at 10:14:10PM -0800, Dmitry Torokhov wrote:

> Hmm, this is quite ugly. The race that you mentioned should be dealt
> with by rearranging the order in which you disable IRQ and enable the
> conversions. I.e. in wm831x_ts_pen_down_irq() you should do:

> 	disable_irq_nosync(wm831x_ts->pd_irq);
> 	wm831x_ts->pendown = true;
> 
> 	...
> 	enable_irq(wm831x_ts->data_irq);

Sadly there's also some sensitivities in the hardware with regard to the
interaction between the interrupt controller and the touchscreen
controller which complicate things somewhat.

> What is needed I guess is wm831x_ts->open flag that
> wm831x_ts_input_open() would set and wm831x_ts_input_close() would reset
> and the interrupt handlers would check before touching the registers
> or enabling their counterparts. wm831x_ts_input_close() would reset the
> flag first and then disable first pd_irq and then data_irq, before
> writing to the control register. The only issue is to keep track of irq
> disble counters.

IIRC I did implement a version of this approach but it causes problems
as if the interrupts fire you really do need to handle them to have
things restart correctly when you reenable.  Possibly we can do
something that fiddles with the workqueue scheduling only.  Also...

> The issue is that wm831x_ts_input_close() is racy; it simply writes into
> WM831X_TOUCH_CONTROL_1 and hopes that it will stick...    

The register I/O operations are locked so they can't race with each
other and the only bit that's really important here is TCH_ENA which is
only updated by open and close - it will stop any of the other bits
taking effect later on.  We ought to explicitly disable the X, Y and Z
conversions in open and have an explicit synchronise_irq() after turning
off the controller, though.  We can also just flush the workqueues
unconditionally and clean up the interrupts once we've done that.

> The issue with enable_irq() not working in the irq handler thread should
> be dealt at the controller level (i.e. fix it so that it works).

This is a genirq restriction which doesn't seem entirely unreasonable to
me, the enable operation needs to take the bus lock which can't be done
within IRQ context as the whole point is to deal with things that can't
run there.  You'd have to check that you were running in the context of
an operation which held the bus lock and then skip the bus locking in
that case only which doesn't seem tasteful and won't work at all if
we're using two different locking IRQ controllers for the two interrupts
(which is entirely possible here).  Scheduling out of IRQ context to
avoid these issues seems much more tasteful.