| Message ID | 20200909162216.13765-1-ceggers@arri.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: trigger: Don't use RT priority | expand |
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
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
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);
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(+)