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

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

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.

Although this process did nothing more than blocking on i2c-dev ioctl(),
no other threaded IRQ handlers (like DMA) were switched in during this
time.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Cc: stable@vger.kernel.org
---
 drivers/iio/industrialio-trigger.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iio/industrialio-trigger.c b/drivers/iio/industrialio-trigger.c
index 6f16357fd732..b74180293da2 100644
--- a/drivers/iio/industrialio-trigger.c
+++ b/drivers/iio/industrialio-trigger.c
@@ -9,8 +9,12 @@
 #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 <uapi/linux/sched/types.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger.h>
@@ -245,6 +249,8 @@ 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 sched_param sched_param = { .sched_priority = 0 };
+	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 +270,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_setscheduler_nocheck(irq_desc->action->thread, SCHED_NORMAL, &sched_param);
+
 	/* 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] iio: trigger: Don't use RT priority

[Jonathan Cameron]

On Thu, 10 Sep 2020 09:54:06 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Christian,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on iio/togreg]
> [also build test ERROR on v5.9-rc4 next-20200909]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Christian-Eggers/iio-trigger-Don-t-use-RT-priority/20200910-002619
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> config: i386-randconfig-r026-20200909 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> reproduce (this is a W=1 build):
>         # save the attached .config to linux build tree
>         make W=1 ARCH=i386 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
> >> ERROR: modpost: "sched_setscheduler_nocheck" [drivers/iio/industrialio.ko] undefined!
 
Looks like we can't do this unless we have a precusor patch to export that
function for module use.

Jonathan

> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[Christian Eggers]

Hi Jonathan,

On Sunday, 13 September 2020, 11:43:05 CEST, Jonathan Cameron wrote:
> Looks like we can't do this unless we have a precusor patch to export that
> function for module use.

This patch compiles fine on my development kernel (5.4-rt). I guess that it 
doesn't link on latest due to https://lwn.net/Articles/818388/

This raises the important question, which priority to use for the iio trigger. 
According to the above link, this should better be chosen by user space than 
by the kernel. Some systems may require a medium/high priority thread for data 
acquisition, on other systems (like mine) this exposed a number of bugs in the 
i2c bus driver which blocked the whole system.

Use space can change the priority of the iio trigger thread using the chrt 
command. The question is, which priority should be used by default (RT / non 
RT). If desired, I'll port the patch to latest.

What happened on my system (only if interested):
A normal priority user space process was using the i2c bus. Due to noise on 
the bus and several bugs in the (noise related) error handling in i2c-imx, the 
driver code was caught in a busy wait loop with a 500 ms timeout. This was bad 
enough but after the iio trigger irq thread with a RT priority equal to 
interrupt threads wanted to access the same i2c bus, the prio of the user 
space process was boosted and busy waiting was done with IRQ priority. For the 
rest of the 500 ms timeout, no further IRQ threads were serviced.

Best regards
Christian