[PATCH v2] iio: trigger: Don't use RT priority

[PATCH v2] iio: trigger: Don't use RT priority

[Christian Eggers]

Triggers may raise transactions on slow busses like I2C.  Using the
original RT priority of a threaded IRQ may prevent other important IRQ
handlers from being run.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Cc: stable@vger.kernel.org
---
In my particular case (on a RT kernel), the RT priority of the sysfstrig
threaded IRQ handler caused (temporarily) raising the prio of a user
space process which was holding the I2C bus mutex.

Due to a bug in the i2c-imx driver, this process spent 500 ms in a busy-wait
loop and prevented all threaded IRQ handlers from being run during this
time.

v2:
- Use sched_set_normal() instead of sched_setscheduler_nocheck()

 drivers/iio/industrialio-trigger.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 6f16357fd732..7ed00ad695c7 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -9,7 +9,10 @@
 #include <linux/err.h>
 #include <linux/device.h>
 #include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdesc.h>
 #include <linux/list.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 
 #include <linux/iio/iio.h>
@@ -245,6 +248,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	int ret = 0;
 	bool notinuse
 		= bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
+	struct irq_desc *irq_desc;
 
 	/* Prevent the module from being removed whilst attached to a trigger */
 	__module_get(pf->indio_dev->driver_module);
@@ -264,6 +268,12 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	if (ret < 0)
 		goto out_put_irq;
 
+	/* Triggers may raise transactions on slow busses like I2C.  Using the original RT priority
+	 * of a threaded IRQ may prevent other threaded IRQ handlers from being run.
+	 */
+	irq_desc = irq_to_desc(pf->irq);
+	sched_set_normal(irq_desc->action->thread, 0);
+
 	/* Enable trigger in driver */
 	if (trig->ops && trig->ops->set_trigger_state && notinuse) {
 		ret = trig->ops->set_trigger_state(trig, true);
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

Re: [PATCH v2] iio: trigger: Don't use RT priority

[Jonathan Cameron]

On Thu, 17 Sep 2020 14:03:33 +0200
Christian Eggers <ceggers@arri.de> wrote:

> Triggers may raise transactions on slow busses like I2C.  Using the
> original RT priority of a threaded IRQ may prevent other important IRQ
> handlers from being run.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Cc: stable@vger.kernel.org
> ---
> In my particular case (on a RT kernel), the RT priority of the sysfstrig
> threaded IRQ handler caused (temporarily) raising the prio of a user
> space process which was holding the I2C bus mutex.
> 
> Due to a bug in the i2c-imx driver, this process spent 500 ms in a busy-wait
> loop and prevented all threaded IRQ handlers from being run during this
> time.
I'm not sure I fully understand the impacts of this yet.

What is the impact on cases where we don't have any nasty side affects
due to users of the trigger?

I presume reducing the priority will cause some reduction in
performance?  If so is there any chance that would count as a regression?

Jonathan

> 
> v2:
> - Use sched_set_normal() instead of sched_setscheduler_nocheck()
> 
>  drivers/iio/industrialio-trigger.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
> index 6f16357fd732..7ed00ad695c7 100644
> --- a/drivers/iio/industrialio-trigger.c
> +++ b/drivers/iio/industrialio-trigger.c
> @@ -9,7 +9,10 @@
>  #include <linux/err.h>
>  #include <linux/device.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdesc.h>
>  #include <linux/list.h>
> +#include <linux/sched.h>
>  #include <linux/slab.h>
>  
>  #include <linux/iio/iio.h>
> @@ -245,6 +248,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>  	int ret = 0;
>  	bool notinuse
>  		= bitmap_empty(trig->pool, CONFIG_IIO_CONSUMERS_PER_TRIGGER);
> +	struct irq_desc *irq_desc;
>  
>  	/* Prevent the module from being removed whilst attached to a trigger */
>  	__module_get(pf->indio_dev->driver_module);
> @@ -264,6 +268,12 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
>  	if (ret < 0)
>  		goto out_put_irq;
>  
> +	/* Triggers may raise transactions on slow busses like I2C.  Using the original RT priority
> +	 * of a threaded IRQ may prevent other threaded IRQ handlers from being run.
> +	 */
> +	irq_desc = irq_to_desc(pf->irq);
> +	sched_set_normal(irq_desc->action->thread, 0);
> +
>  	/* Enable trigger in driver */
>  	if (trig->ops && trig->ops->set_trigger_state && notinuse) {
>  		ret = trig->ops->set_trigger_state(trig, true);

Re: [PATCH v2] iio: trigger: Don't use RT priority

[Christian Eggers]

Hi Jonathan,

On Thursday, 17 September 2020, 19:19:42 CEST, Jonathan Cameron wrote:
> On Thu, 17 Sep 2020 14:03:33 +0200 Christian Eggers <ceggers@arri.de> wrote:
> > Triggers may raise transactions on slow busses like I2C.  Using the
> > original RT priority of a threaded IRQ may prevent other important IRQ
> > handlers from being run.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > Cc: stable@vger.kernel.org
> > ---
> > In my particular case (on a RT kernel), the RT priority of the sysfstrig
> > threaded IRQ handler caused (temporarily) raising the prio of a user
> > space process which was holding the I2C bus mutex.
> > 
> > Due to a bug in the i2c-imx driver, this process spent 500 ms in a
> > busy-wait loop and prevented all threaded IRQ handlers from being run
> > during this time.
> 
> I'm not sure I fully understand the impacts of this yet.
> 
> What is the impact on cases where we don't have any nasty side affects
> due to users of the trigger?
The problem was not the user of the trigger. The problem was the (shared)
resource (I2C bus) which the triggered iio driver uses. I would say
that the i2c-imx driver is not "RT safe" [1]. This means that the driver performs
busy-waiting, which is less a problem for normal priorities than for RT. If the
busy-wait loop is run with RT prio, it will block everything else, even
(threaded) interrupt handlers.

> I presume reducing the priority will cause some reduction in
> performance?  If so is there any chance that would count as a regression?
I expect that other user will complain if we do this, yes. But I would like to
open the discussion, which priority is the "correct" one, or how this could be
set up from user space. According to [2], there is not much value choosing the
priority inside the kernel. Simply changing the priority of the trigger task
using "chrt" seems difficult, as this can (currently) not be done of using
libiio.

> 
> Jonathan
> 
> > v2:
> > - Use sched_set_normal() instead of sched_setscheduler_nocheck()
> > 
> >  drivers/iio/industrialio-trigger.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 

[1] https://lore.kernel.org/patchwork/cover/1307330/
[2] https://lwn.net/Articles/818388/