[PATCH] pcf50633: use a dedicated workqueue for irq processing

[PATCH] pcf50633: use a dedicated workqueue for irq processing

[Paul Fertser]

Using the default kernel "events" workqueue causes problems with
synchronous adc readings if initiated from some task on the same
workqueue.

I had a deadlock trying to use pcf50633_adc_sync_read from a
power_supply class driver because the reading was initiated from the
workqueue and it waited for the irq processing to complete (to get the
result) and that was put on the same workqueue.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c       |    5 ++++-
 include/linux/mfd/pcf50633/core.h |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 8d3c38b..d26d774 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -444,7 +444,7 @@ static irqreturn_t pcf50633_irq(int irq, void *data)
 
 	get_device(pcf->dev);
 	disable_irq_nosync(pcf->irq);
-	schedule_work(&pcf->irq_work);
+	queue_work(pcf->work_queue, &pcf->irq_work);
 
 	return IRQ_HANDLED;
 }
@@ -575,6 +575,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	pcf->dev = &client->dev;
 	pcf->i2c_client = client;
 	pcf->irq = client->irq;
+	pcf->work_queue = create_singlethread_workqueue("pcf50633");
 
 	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
 
@@ -651,6 +652,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	return 0;
 
 err:
+	destroy_workqueue(pcf->work_queue);
 	kfree(pcf);
 	return ret;
 }
@@ -661,6 +663,7 @@ static int __devexit pcf50633_remove(struct i2c_client *client)
 	int i;
 
 	free_irq(pcf->irq, pcf);
+	destroy_workqueue(pcf->work_queue);
 
 	platform_device_unregister(pcf->input_pdev);
 	platform_device_unregister(pcf->rtc_pdev);
diff --git a/include/linux/mfd/pcf50633/core.h b/include/linux/mfd/pcf50633/core.h
index c8f51c3..9aba7b7 100644
--- a/include/linux/mfd/pcf50633/core.h
+++ b/include/linux/mfd/pcf50633/core.h
@@ -136,6 +136,7 @@ struct pcf50633 {
 	int irq;
 	struct pcf50633_irq irq_handler[PCF50633_NUM_IRQ];
 	struct work_struct irq_work;
+	struct workqueue_struct *work_queue;
 	struct mutex lock;
 
 	u8 mask_regs[5];
-- 
1.6.0.6

Re: [PATCH] pcf50633: use a dedicated workqueue for irq processing

[Andrew Morton]

On Tue, 28 Jul 2009 00:41:15 +0400
Paul Fertser <fercerpav@gmail.com> wrote:

> Using the default kernel "events" workqueue causes problems with
> synchronous adc readings if initiated from some task on the same
> workqueue.
> 
> I had a deadlock trying to use pcf50633_adc_sync_read from a
> power_supply class driver because the reading was initiated from the
> workqueue and it waited for the irq processing to complete (to get the
> result) and that was put on the same workqueue.

I don't get it.

Do you meant that pcf50633_adc_sync_read() was called via a
schedule_work() handler?  If so, wasn't that a bug?

The patch looks reasonable, but creating yet another kernel thread is a
bit sad.  And I suspect that other MFD drivers are doing the same thing
as pcf50633-core.c, in which case we should change those as well?

So..  please provide a more complete description of the problem which
is being solved so that we can explore alternative fixes.

Re: [PATCH] pcf50633: use a dedicated workqueue for irq processing

[Paul Fertser]

On Thu, Jul 30, 2009 at 03:34:25PM -0700, Andrew Morton wrote:
> On Tue, 28 Jul 2009 00:41:15 +0400
> Paul Fertser <fercerpav@gmail.com> wrote:
> 
> > Using the default kernel "events" workqueue causes problems with
> > synchronous adc readings if initiated from some task on the same
> > workqueue.
> > 
> > I had a deadlock trying to use pcf50633_adc_sync_read from a
> > power_supply class driver because the reading was initiated from the
> > workqueue and it waited for the irq processing to complete (to get the
> > result) and that was put on the same workqueue.
> 
> I don't get it.
> 
> Do you meant that pcf50633_adc_sync_read() was called via a
> schedule_work() handler?  If so, wasn't that a bug?

Andrew, i think it is called this way:

power_supply_changed -> schedule_work(&psy->changed_work) ->
power_supply_changed_work -> kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE)
-> power_supply_uevent -> power_supply_show_property -> get_property ->
bat->pdata->get_voltage -> pcf50633_adc_sync_read

I assumed that since power_supply class is well-established and widely used
that's an intended behaviour. CCing relevant maintainers to get their
opinions.

A bit of background: we sometimes want to use dumb batteries with our
openmoko devices (for gta01 aka Neo1973 there's no alternative even), so
power_class driver needs to be able to obtain battery voltage measured by
PMU.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Re: [PATCH] pcf50633: use a dedicated workqueue for irq processing

[Mark Brown]

On Thu, Jul 30, 2009 at 03:34:25PM -0700, Andrew Morton wrote:

> The patch looks reasonable, but creating yet another kernel thread is a
> bit sad.  And I suspect that other MFD drivers are doing the same thing
> as pcf50633-core.c, in which case we should change those as well?

It's just pcf50633 that's immediately affected from the looks of it.
What's going wrong for it is that it's using interrupts from the device
to determine when an ADC conversion is complete and since it's an I2C
device it's doing interrupt handling in the standard work queue.  Other
MFDs that might be affected appear to be already using a separate work
queue or thread for their interrupts which would allow them to push any
work that needs to wait for interrupts from the device into the standard
workqueue.

The issue shouldn't need so much handling by drivers when these I2C/SPI
driven devices can use genirq (which looks likely soon although it's not
possible currently) since then the interrupt handling will occur within
IRQ threads.  Devices that can interact with the device from interrupt
context aren't affected since they don't need to defer interrupt
handling to a work queue.

Re: [PATCH] pcf50633: use a dedicated workqueue for irq processing

[Anton Vorontsov]

On Fri, Jul 31, 2009 at 01:23:13PM +0400, Paul Fertser wrote:
> On Thu, Jul 30, 2009 at 03:34:25PM -0700, Andrew Morton wrote:
> > On Tue, 28 Jul 2009 00:41:15 +0400
> > Paul Fertser <fercerpav@gmail.com> wrote:
> > 
> > > Using the default kernel "events" workqueue causes problems with
> > > synchronous adc readings if initiated from some task on the same
> > > workqueue.
> > > 
> > > I had a deadlock trying to use pcf50633_adc_sync_read from a
> > > power_supply class driver because the reading was initiated from the
> > > workqueue and it waited for the irq processing to complete (to get the
> > > result) and that was put on the same workqueue.
> > 
> > I don't get it.
> > 
> > Do you meant that pcf50633_adc_sync_read() was called via a
> > schedule_work() handler?  If so, wasn't that a bug?
> 
> Andrew, i think it is called this way:
> 
> power_supply_changed -> schedule_work(&psy->changed_work) ->
> power_supply_changed_work -> kobject_uevent(&psy->dev->kobj, KOBJ_CHANGE)
> -> power_supply_uevent -> power_supply_show_property -> get_property ->
> bat->pdata->get_voltage -> pcf50633_adc_sync_read
> 
> I assumed that since power_supply class is well-established and widely used
> that's an intended behaviour. CCing relevant maintainers to get their
> opinions.

The thing is that power_supply_changed() is supposed to be callable
from any context (including hard IRQs), so we use schedule_work().

I see the deadlock problem, and it could be fixed by a dedicated
workqueue either in the pcf50633 driver, or power supply class.
I don't mind either way.

Another option is to implement power_supply_changed_can_sleep(),
which won't use schedule_work(), and so we won't need another
thread.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2