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@xxxxxxx> 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@xxxxxxx> > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > 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/
