Bottom half trigger function never called when using the sysfs trigger

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.

Quoted 7 lines · 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.

Quoted 6 lines

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.

Quoted 14 lines · Jonathan Cameron

Quoted 7 lines · 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.

Quoted 6 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

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.

Quoted 7 lines

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).

drivers/staging/iio/trigger/Kconfig +1 -0

index b8abf54..d44d3ad 100644
@@ -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").

drivers/staging/iio/trigger/iio-trig-sysfs.c +13 -3

index 552763b..fde93a8 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html