Bottom half trigger function never called when using the sysfs trigger

Bottom half trigger function never called when using the sysfs trigger

[Lars-Peter Clausen]

hi,

The sysfs trigger uses iio_trigger_poll_chained which calls
handle_nested_irq. The problem now is that for nested IRQs the primary
handler is not called. Which obviously breaks drivers which have a bottom
half trigger function.

This behaviour was introduced in commit 1f785681 ("staging:iio:trigger sysfs
userspace trigger rework."). The commit message says you are "awaiting
comments on using the nested_irq_trick", but not why it is necessary to use
nested IRQs. And honestly I don't get why it would be necessary either.
handle_nested_irq is supposed to be used with chained IRQs if we are already
running in a thread handler of parent. Neither seems to be true here.

- Lars

Re: Bottom half trigger function never called when using the sysfs trigger

[Jonathan Cameron]

On 6/15/2012 2:08 PM, Lars-Peter Clausen wrote:
> hi,
>
> The sysfs trigger uses iio_trigger_poll_chained which calls
> handle_nested_irq. The problem now is that for nested IRQs the primary
> handler is not called. Which obviously breaks drivers which have a bottom
> half trigger function.
>
> This behaviour was introduced in commit 1f785681 ("staging:iio:trigger sysfs
> userspace trigger rework."). The commit message says you are "awaiting
> comments on using the nested_irq_trick", but not why it is necessary to use
> nested IRQs. And honestly I don't get why it would be necessary either.
> handle_nested_irq is supposed to be used with chained IRQs if we are already
> running in a thread handler of parent. Neither seems to be true here.
It was a while ago so my memory is rather sketchy.
do you meant the bottom half?  Unless I'm very confused its the top half 
that doesn't
get called.. handle_nested_irq calls thread_fn which is the bottom half.

It's the only way I've come up with for cleanly running through our 
trigger distribution
given that is all irq based.  Top halves expect to be run in interrupt 
mode. I don't know
of any way to ensure this is true if one is 'creating' the interrupt 
from software.

I did wonder at the time about putting an explicit call of the interrupt 
handler in
to deal with the missing top halves. It's rather uggly though and 
suffers from things
not being run in the state they expect to be run in....

Other suggestions welcome.

Re: Bottom half trigger function never called when using the sysfs trigger

[Lars-Peter Clausen]

On 06/15/2012 03:32 PM, Jonathan Cameron wrote:
> On 6/15/2012 2:08 PM, Lars-Peter Clausen wrote:
>> hi,
>>
>> The sysfs trigger uses iio_trigger_poll_chained which calls
>> handle_nested_irq. The problem now is that for nested IRQs the primary
>> handler is not called. Which obviously breaks drivers which have a bottom
>> half trigger function.
>>
>> This behaviour was introduced in commit 1f785681 ("staging:iio:trigger sysfs
>> userspace trigger rework."). The commit message says you are "awaiting
>> comments on using the nested_irq_trick", but not why it is necessary to use
>> nested IRQs. And honestly I don't get why it would be necessary either.
>> handle_nested_irq is supposed to be used with chained IRQs if we are already
>> running in a thread handler of parent. Neither seems to be true here.
> It was a while ago so my memory is rather sketchy.
> do you meant the bottom half?  Unless I'm very confused its the top half
> that doesn't

Uhm, yes the top half.

> get called.. handle_nested_irq calls thread_fn which is the bottom half.
> 
> It's the only way I've come up with for cleanly running through our trigger
> distribution
> given that is all irq based.  Top halves expect to be run in interrupt mode.
> I don't know
> of any way to ensure this is true if one is 'creating' the interrupt from
> software.
> 
> I did wonder at the time about putting an explicit call of the interrupt
> handler in
> to deal with the missing top halves. It's rather uggly though and suffers
> from things
> not being run in the state they expect to be run in....
> 
> Other suggestions welcome.

Hm, maybe not use interrupts for the trigger events... ;)

On the other hand there is the new irq_work framework which lets you
schedule events which should be run in hardirq context. The following patch
seems to do the trick (Note the patch won't apply as my mail client will
insert line breaks where it shouldn't. I'll sent a proper patch if you agree
with the general idea).

diff --git a/drivers/staging/iio/trigger/Kconfig
b/drivers/staging/iio/trigger/Kconfig
index b8abf54..d44d3ad 100644
--- a/drivers/staging/iio/trigger/Kconfig
+++ b/drivers/staging/iio/trigger/Kconfig
@@ -21,6 +21,7 @@ config IIO_GPIO_TRIGGER
 config IIO_SYSFS_TRIGGER
 	tristate "SYSFS trigger"
 	depends on SYSFS
+	select IRQ_WORK
 	help
 	  Provides support for using SYSFS entry as IIO triggers.
 	  If unsure, say N (but it's safe to say "Y").
diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
index 552763b..fde93a8 100644
--- a/drivers/staging/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
@@ -10,12 +10,14 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/list.h>
+#include <linux/irq_work.h>

 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>

 struct iio_sysfs_trig {
 	struct iio_trigger *trig;
+	struct irq_work work;
 	int id;
 	struct list_head l;
 };
@@ -89,11 +91,17 @@ static struct device iio_sysfs_trig_dev = {
 	.release = &iio_trigger_sysfs_release,
 };

+static void iio_sysfs_trigger_work(struct irq_work *work)
+{
+	struct iio_sysfs_trig *trig = container_of(work, struct iio_sysfs_trig, work);
+	iio_trigger_poll(trig->trig, 0);
+}
+
 static ssize_t iio_sysfs_trigger_poll(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t count)
 {
-	struct iio_trigger *trig = dev_get_drvdata(dev);
-	iio_trigger_poll_chained(trig, 0);
+	struct iio_sysfs_trig *trig = dev_get_drvdata(dev);
+	irq_work_queue(&trig->work);

 	return count;
 }
@@ -148,6 +156,9 @@ static int iio_sysfs_trigger_probe(int id)
 	t->trig->dev.groups = iio_sysfs_trigger_attr_groups;
 	t->trig->ops = &iio_sysfs_trigger_ops;
 	t->trig->dev.parent = &iio_sysfs_trig_dev;
+	dev_set_drvdata(&t->trig->dev, t);
+
+	init_irq_work(&t->work, iio_sysfs_trigger_work);

 	ret = iio_trigger_register(t->trig);
 	if (ret)

Re: Bottom half trigger function never called when using the sysfs trigger

[Jonathan Cameron]

On 6/15/2012 3:57 PM, Lars-Peter Clausen wrote:
> On 06/15/2012 03:32 PM, Jonathan Cameron wrote:
>> On 6/15/2012 2:08 PM, Lars-Peter Clausen wrote:
>>> hi,
>>>
>>> The sysfs trigger uses iio_trigger_poll_chained which calls
>>> handle_nested_irq. The problem now is that for nested IRQs the primary
>>> handler is not called. Which obviously breaks drivers which have a bottom
>>> half trigger function.
>>>
>>> This behaviour was introduced in commit 1f785681 ("staging:iio:trigger sysfs
>>> userspace trigger rework."). The commit message says you are "awaiting
>>> comments on using the nested_irq_trick", but not why it is necessary to use
>>> nested IRQs. And honestly I don't get why it would be necessary either.
>>> handle_nested_irq is supposed to be used with chained IRQs if we are already
>>> running in a thread handler of parent. Neither seems to be true here.
>> It was a while ago so my memory is rather sketchy.
>> do you meant the bottom half?  Unless I'm very confused its the top half
>> that doesn't
>
> Uhm, yes the top half.
>
>> get called.. handle_nested_irq calls thread_fn which is the bottom half.
>>
>> It's the only way I've come up with for cleanly running through our trigger
>> distribution
>> given that is all irq based.  Top halves expect to be run in interrupt mode.
>> I don't know
>> of any way to ensure this is true if one is 'creating' the interrupt from
>> software.
>>
>> I did wonder at the time about putting an explicit call of the interrupt
>> handler in
>> to deal with the missing top halves. It's rather uggly though and suffers
>> from things
>> not being run in the state they expect to be run in....
>>
>> Other suggestions welcome.
>
> Hm, maybe not use interrupts for the trigger events... ;)
>
> On the other hand there is the new irq_work framework which lets you
> schedule events which should be run in hardirq context. The following patch
> seems to do the trick (Note the patch won't apply as my mail client will
> insert line breaks where it shouldn't. I'll sent a proper patch if you agree
> with the general idea).
Looks like someone else ran into the same problem and came up with
a much better solution than I did.

This certainly looks better than the current.
>
> diff --git a/drivers/staging/iio/trigger/Kconfig
> b/drivers/staging/iio/trigger/Kconfig
> index b8abf54..d44d3ad 100644
> --- a/drivers/staging/iio/trigger/Kconfig
> +++ b/drivers/staging/iio/trigger/Kconfig
> @@ -21,6 +21,7 @@ config IIO_GPIO_TRIGGER
>   config IIO_SYSFS_TRIGGER
>   	tristate "SYSFS trigger"
>   	depends on SYSFS
> +	select IRQ_WORK
>   	help
>   	  Provides support for using SYSFS entry as IIO triggers.
>   	  If unsure, say N (but it's safe to say "Y").
> diff --git a/drivers/staging/iio/trigger/iio-trig-sysfs.c
> index 552763b..fde93a8 100644
> --- a/drivers/staging/iio/trigger/iio-trig-sysfs.c
> +++ b/drivers/staging/iio/trigger/iio-trig-sysfs.c
> @@ -10,12 +10,14 @@
>   #include<linux/platform_device.h>
>   #include<linux/slab.h>
>   #include<linux/list.h>
> +#include<linux/irq_work.h>
>
>   #include<linux/iio/iio.h>
>   #include<linux/iio/trigger.h>
>
>   struct iio_sysfs_trig {
>   	struct iio_trigger *trig;
> +	struct irq_work work;
>   	int id;
>   	struct list_head l;
>   };
> @@ -89,11 +91,17 @@ static struct device iio_sysfs_trig_dev = {
>   	.release =&iio_trigger_sysfs_release,
>   };
>
> +static void iio_sysfs_trigger_work(struct irq_work *work)
> +{
> +	struct iio_sysfs_trig *trig = container_of(work, struct iio_sysfs_trig, work);
> +	iio_trigger_poll(trig->trig, 0);
> +}
> +
>   static ssize_t iio_sysfs_trigger_poll(struct device *dev,
>   		struct device_attribute *attr, const char *buf, size_t count)
>   {
> -	struct iio_trigger *trig = dev_get_drvdata(dev);
> -	iio_trigger_poll_chained(trig, 0);
> +	struct iio_sysfs_trig *trig = dev_get_drvdata(dev);
> +	irq_work_queue(&trig->work);
>
>   	return count;
>   }
> @@ -148,6 +156,9 @@ static int iio_sysfs_trigger_probe(int id)
>   	t->trig->dev.groups = iio_sysfs_trigger_attr_groups;
>   	t->trig->ops =&iio_sysfs_trigger_ops;
>   	t->trig->dev.parent =&iio_sysfs_trig_dev;
> +	dev_set_drvdata(&t->trig->dev, t);
> +
> +	init_irq_work(&t->work, iio_sysfs_trigger_work);
>
>   	ret = iio_trigger_register(t->trig);
>   	if (ret)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html