* [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context @ 2020-08-13 7:53 Lars-Peter Clausen 2020-08-13 7:53 ` [PATCH 2/2] iio: sysfs-trigger: Mark irq_work to expire in hardirq context Lars-Peter Clausen 2020-08-13 9:11 ` [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Sebastian Andrzej Siewior 0 siblings, 2 replies; 20+ messages in thread From: Lars-Peter Clausen @ 2020-08-13 7:53 UTC (permalink / raw) To: Jonathan Cameron Cc: Hartmut Knaack, Peter Meerwald-Stadler, Sebastian Andrzej Siewior, Christian Eggers, linux-iio, Lars-Peter Clausen On PREEMPT_RT enabled kernels unmarked hrtimers are moved into soft interrupt expiry mode by default. The IIO hrtimer-trigger needs to run in hard interrupt context since it will end up calling generic_handle_irq() which has the requirement to run in hard interrupt context. Explicitly specify that the timer needs to run in hard interrupt context by using the HRTIMER_MODE_REL_HARD flag. Fixes: f5c2f0215e36 ("hrtimer: Move unmarked hrtimers to soft interrupt expiry on RT") Reported-by: Christian Eggers <ceggers@arri.de> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/iio/trigger/iio-trig-hrtimer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iio/trigger/iio-trig-hrtimer.c b/drivers/iio/trigger/iio-trig-hrtimer.c index f59bf8d58586..b11ca915fcb2 100644 --- a/drivers/iio/trigger/iio-trig-hrtimer.c +++ b/drivers/iio/trigger/iio-trig-hrtimer.c @@ -132,7 +132,7 @@ static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name) trig_info->swt.trigger->ops = &iio_hrtimer_trigger_ops; trig_info->swt.trigger->dev.groups = iio_hrtimer_attr_groups; - hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD); trig_info->timer.function = iio_hrtimer_trig_handler; trig_info->sampling_frequency = HRTIMER_DEFAULT_SAMPLING_FREQUENCY; -- 2.20.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] iio: sysfs-trigger: Mark irq_work to expire in hardirq context 2020-08-13 7:53 [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Lars-Peter Clausen @ 2020-08-13 7:53 ` Lars-Peter Clausen 2020-08-13 9:11 ` [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Sebastian Andrzej Siewior 1 sibling, 0 replies; 20+ messages in thread From: Lars-Peter Clausen @ 2020-08-13 7:53 UTC (permalink / raw) To: Jonathan Cameron Cc: Hartmut Knaack, Peter Meerwald-Stadler, Sebastian Andrzej Siewior, Christian Eggers, linux-iio, Lars-Peter Clausen Mark the IIO sysfs-trigger irq_work with IRQ_WORK_HARD_IRQ to ensure that it is always executed in hard interrupt context, even with PREEMPT_RT=y. The IIO sysfs-trigger irq_work needs to run in hard interrupt context since it will end up calling generic_handle_irq() which has the requirement to run in hard interrupt context. Note that the IRQ_WORK_HARD_IRQ flag, while it exists, does not seem to do anything in the mainline kernel yet. It does have an effect in the RT patchset though and presumably this is sooner or later going to be added to mainline as well. Reported-by: Christian Eggers <ceggers@arri.de> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> --- drivers/iio/trigger/iio-trig-sysfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/iio/trigger/iio-trig-sysfs.c b/drivers/iio/trigger/iio-trig-sysfs.c index e09e58072872..10a3fd29362b 100644 --- a/drivers/iio/trigger/iio-trig-sysfs.c +++ b/drivers/iio/trigger/iio-trig-sysfs.c @@ -161,6 +161,7 @@ static int iio_sysfs_trigger_probe(int id) iio_trigger_set_drvdata(t->trig, t); init_irq_work(&t->work, iio_sysfs_trigger_work); + atomic_set(&t->work.flags, IRQ_WORK_HARD_IRQ); ret = iio_trigger_register(t->trig); if (ret) -- 2.20.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 7:53 [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Lars-Peter Clausen 2020-08-13 7:53 ` [PATCH 2/2] iio: sysfs-trigger: Mark irq_work to expire in hardirq context Lars-Peter Clausen @ 2020-08-13 9:11 ` Sebastian Andrzej Siewior 2020-08-13 9:46 ` Lars-Peter Clausen 1 sibling, 1 reply; 20+ messages in thread From: Sebastian Andrzej Siewior @ 2020-08-13 9:11 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio, tglx On 2020-08-13 09:53:57 [+0200], Lars-Peter Clausen wrote: > On PREEMPT_RT enabled kernels unmarked hrtimers are moved into soft > interrupt expiry mode by default. > > The IIO hrtimer-trigger needs to run in hard interrupt context since it > will end up calling generic_handle_irq() which has the requirement to run > in hard interrupt context. > > Explicitly specify that the timer needs to run in hard interrupt context by > using the HRTIMER_MODE_REL_HARD flag. No, I don't think that this is good. It basically renders threaded-irqs in context of IIO useless. This also requires that the IRQ-handler in question runs with IRQs disabled / uses raw_spinlock_t which is in not good idea either. Has this change (including the second patch in thread) been tested on RT in terms of locking and latency? > Fixes: f5c2f0215e36 ("hrtimer: Move unmarked hrtimers to soft interrupt expiry on RT") > Reported-by: Christian Eggers <ceggers@arri.de> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > --- > drivers/iio/trigger/iio-trig-hrtimer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iio/trigger/iio-trig-hrtimer.c b/drivers/iio/trigger/iio-trig-hrtimer.c > index f59bf8d58586..b11ca915fcb2 100644 > --- a/drivers/iio/trigger/iio-trig-hrtimer.c > +++ b/drivers/iio/trigger/iio-trig-hrtimer.c > @@ -132,7 +132,7 @@ static struct iio_sw_trigger *iio_trig_hrtimer_probe(const char *name) > trig_info->swt.trigger->ops = &iio_hrtimer_trigger_ops; > trig_info->swt.trigger->dev.groups = iio_hrtimer_attr_groups; > > - hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + hrtimer_init(&trig_info->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD); > trig_info->timer.function = iio_hrtimer_trig_handler; > > trig_info->sampling_frequency = HRTIMER_DEFAULT_SAMPLING_FREQUENCY; Sebastian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 9:11 ` [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Sebastian Andrzej Siewior @ 2020-08-13 9:46 ` Lars-Peter Clausen 2020-08-13 11:27 ` Sebastian Andrzej Siewior 0 siblings, 1 reply; 20+ messages in thread From: Lars-Peter Clausen @ 2020-08-13 9:46 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio, tglx On 8/13/20 11:11 AM, Sebastian Andrzej Siewior wrote: > On 2020-08-13 09:53:57 [+0200], Lars-Peter Clausen wrote: >> On PREEMPT_RT enabled kernels unmarked hrtimers are moved into soft >> interrupt expiry mode by default. >> >> The IIO hrtimer-trigger needs to run in hard interrupt context since it >> will end up calling generic_handle_irq() which has the requirement to run >> in hard interrupt context. >> >> Explicitly specify that the timer needs to run in hard interrupt context by >> using the HRTIMER_MODE_REL_HARD flag. > No, I don't think that this is good. It basically renders threaded-irqs > in context of IIO useless. This also requires that the IRQ-handler in > question runs with IRQs disabled / uses raw_spinlock_t which is in not > good idea either. It should not affect the IRQ handlers of individual drivers. The hrtimer triggers acts like an IRQ chip and will call generic_handle_irq() to multiplex the interrupt handling onto all consumers. As far as I understand it there is a requirement that generic_handle_irq() is called in hard irq context, even with PREEMT_RT=y. If you are running with forced IRQ threads the only thing that will then happen in the actual hard IRQ context is the launching of the IRQ threads. Th e IRQ handler of the device driver will run in a threaded IRQ. > > Has this change (including the second patch in thread) been tested on RT > in terms of locking and latency? It has not been tested in terms of latency. But like I said if you are running with forced IRQ threads the effect should be minimal. Without this patch there is an correctness issue when PREEMT_RT=y since generic_handle_irq() runs with interrupts on which breaks its internal assumptions. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 9:46 ` Lars-Peter Clausen @ 2020-08-13 11:27 ` Sebastian Andrzej Siewior 2020-08-13 12:19 ` Thomas Gleixner 0 siblings, 1 reply; 20+ messages in thread From: Sebastian Andrzej Siewior @ 2020-08-13 11:27 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio, tglx On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: > > It should not affect the IRQ handlers of individual drivers. The hrtimer > triggers acts like an IRQ chip and will call generic_handle_irq() to > multiplex the interrupt handling onto all consumers. As far as I understand > it there is a requirement that generic_handle_irq() is called in hard irq > context, even with PREEMT_RT=y. That is correct. > If you are running with forced IRQ threads the only thing that will then > happen in the actual hard IRQ context is the launching of the IRQ threads. > Th e IRQ handler of the device driver will run in a threaded IRQ. So if it is really just the wakeup of the IRQ-thread then it should be okay. One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This would invoke trig->ops->try_reenable callback if available. I grepped and found - bma180_trig_try_reen() It appears to perform i2c_smbus_read_byte_data() and smbus sounds sleeping. I don't know if it attempts to acquire any spinlock_t but it will be wrong on RT. - bmc150_accel_trig_try_reen() This one has mutex_lock() which is wrong even on !RT in this context (unless the previous `if' saves us). - mxc4005_trigger_try_reen() This one uses regmap_write(). regmap internally uses a lock and the config does not disable / provide a lock. This means regmap_lock_mutex() is used (or regmap_lock_spinlock() in case of bus->fast_io but I doubt it with i2c). Am I looking somehow wrong at this or did just nobody try the combination of one of the three drivers here together with the hrtimer trigger? > > > > Has this change (including the second patch in thread) been tested on RT > > in terms of locking and latency? > > It has not been tested in terms of latency. But like I said if you are > running with forced IRQ threads the effect should be minimal. > > Without this patch there is an correctness issue when PREEMT_RT=y since > generic_handle_irq() runs with interrupts on which breaks its internal > assumptions. I'm trying to understand the scope of the change. As I said above, if it is just wakeup of the thread, then it is fine. I have memory of people running iio drivers (or triggers) in hardirq-context for $reason and try avoid something like this. Sebastian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 11:27 ` Sebastian Andrzej Siewior @ 2020-08-13 12:19 ` Thomas Gleixner 2020-08-13 14:55 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Thomas Gleixner @ 2020-08-13 12:19 UTC (permalink / raw) To: Sebastian Andrzej Siewior, Lars-Peter Clausen Cc: Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: >> If you are running with forced IRQ threads the only thing that will then >> happen in the actual hard IRQ context is the launching of the IRQ threads. >> Th e IRQ handler of the device driver will run in a threaded IRQ. > > So if it is really just the wakeup of the IRQ-thread then it should be > okay. > One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This > would invoke trig->ops->try_reenable callback if available. > I grepped and found > - bma180_trig_try_reen() > It appears to perform i2c_smbus_read_byte_data() and smbus sounds > sleeping. I don't know if it attempts to acquire any spinlock_t but it > will be wrong on RT. It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt context. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 12:19 ` Thomas Gleixner @ 2020-08-13 14:55 ` Jonathan Cameron 2020-08-14 5:24 ` Lars-Peter Clausen 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2020-08-13 14:55 UTC (permalink / raw) To: Thomas Gleixner Cc: Sebastian Andrzej Siewior, Lars-Peter Clausen, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio On Thu, 13 Aug 2020 14:19:57 +0200 Thomas Gleixner <tglx@linutronix.de> wrote: > Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > > On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: > >> If you are running with forced IRQ threads the only thing that will then > >> happen in the actual hard IRQ context is the launching of the IRQ threads. > >> Th e IRQ handler of the device driver will run in a threaded IRQ. > > > > So if it is really just the wakeup of the IRQ-thread then it should be > > okay. > > One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This > > would invoke trig->ops->try_reenable callback if available. > > I grepped and found > > - bma180_trig_try_reen() > > It appears to perform i2c_smbus_read_byte_data() and smbus sounds > > sleeping. I don't know if it attempts to acquire any spinlock_t but it > > will be wrong on RT. > > It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt > context. > We would hit this (and resulting warnings) all the time if it actually happened, so my suspicion is that it doesn't. I think the path doesn't actually exist although it looks at first glance like it does. The interrupt can only be enabled if there is someone using the trigger. Thus usecount will be non zero and for at least one element trig->subirq[i].enabled == true So we will decrement trig->usecount in the call to iio_trigger_notify_done but never reach 0 thus the call to trig->ops->try_reenable never happens in the hard interrupt context. It does happen later when which ever driver we triggered finishes the threaded part of it's handler and calls iio_trigger_notify_done, but that is fine. Assuming people agree with my analysis it would be good to make it explicit that we cannot hit the problem path. Perhaps call a new iio_trigger_notify_no_needed() that simply does the decrement without test, or does it with test and spits out a warning if we hit 0. Jonathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-13 14:55 ` Jonathan Cameron @ 2020-08-14 5:24 ` Lars-Peter Clausen 2020-08-14 10:30 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Lars-Peter Clausen @ 2020-08-14 5:24 UTC (permalink / raw) To: Jonathan Cameron, Thomas Gleixner Cc: Sebastian Andrzej Siewior, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio On 8/13/20 4:55 PM, Jonathan Cameron wrote: > On Thu, 13 Aug 2020 14:19:57 +0200 > Thomas Gleixner <tglx@linutronix.de> wrote: > >> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: >>> On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: >>>> If you are running with forced IRQ threads the only thing that will then >>>> happen in the actual hard IRQ context is the launching of the IRQ threads. >>>> Th e IRQ handler of the device driver will run in a threaded IRQ. >>> So if it is really just the wakeup of the IRQ-thread then it should be >>> okay. >>> One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This >>> would invoke trig->ops->try_reenable callback if available. >>> I grepped and found >>> - bma180_trig_try_reen() >>> It appears to perform i2c_smbus_read_byte_data() and smbus sounds >>> sleeping. I don't know if it attempts to acquire any spinlock_t but it >>> will be wrong on RT. >> It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt >> context. >> > We would hit this (and resulting warnings) all the time if it actually > happened, so my suspicion is that it doesn't. > > I think the path doesn't actually exist although it looks at first glance like it does. > > The interrupt can only be enabled if there is someone using the trigger. > Thus usecount will be non zero and for at least one element > trig->subirq[i].enabled == true > > So we will decrement trig->usecount in the call to iio_trigger_notify_done > but never reach 0 thus the call to trig->ops->try_reenable never happens > in the hard interrupt context. I think there is a race condition here. If a consumer is disabled concurrently with iio_trigger_poll() there is a chance that `enabled` is false for all consumers. The odds of this happening are very low, but there is nothing that prevents it. > > It does happen later when which ever driver we triggered finishes the > threaded part of it's handler and calls iio_trigger_notify_done, but that > is fine. > > Assuming people agree with my analysis it would be good to make it explicit > that we cannot hit the problem path. > > Perhaps call a new iio_trigger_notify_no_needed() that simply does > the decrement without test, or does it with test and spits out a > warning if we hit 0. I think we need to re-think the whole try_reenable() functionality. Looking at it I think there are more issues here. For example lets say we call iio_trigger_notify_done() from the threaded handler and try_reenable() returns true. We'd now call iio_trigger_poll() from the threaded context, which is wrong. There is also the issue that iio_trigger_poll() effectively can end up calling itself recursively indefinitely via the iio_trigger_notify_done() chain, which might not be the best thing in hard IRQ context. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-14 5:24 ` Lars-Peter Clausen @ 2020-08-14 10:30 ` Jonathan Cameron 2020-09-20 18:15 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2020-08-14 10:30 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Thomas Gleixner, Sebastian Andrzej Siewior, Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio On Fri, 14 Aug 2020 07:24:52 +0200 Lars-Peter Clausen <lars@metafoo.de> wrote: > On 8/13/20 4:55 PM, Jonathan Cameron wrote: > > On Thu, 13 Aug 2020 14:19:57 +0200 > > Thomas Gleixner <tglx@linutronix.de> wrote: > > > >> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > >>> On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: > >>>> If you are running with forced IRQ threads the only thing that will then > >>>> happen in the actual hard IRQ context is the launching of the IRQ threads. > >>>> Th e IRQ handler of the device driver will run in a threaded IRQ. > >>> So if it is really just the wakeup of the IRQ-thread then it should be > >>> okay. > >>> One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This > >>> would invoke trig->ops->try_reenable callback if available. > >>> I grepped and found > >>> - bma180_trig_try_reen() > >>> It appears to perform i2c_smbus_read_byte_data() and smbus sounds > >>> sleeping. I don't know if it attempts to acquire any spinlock_t but it > >>> will be wrong on RT. > >> It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt > >> context. > >> > > We would hit this (and resulting warnings) all the time if it actually > > happened, so my suspicion is that it doesn't. > > > > I think the path doesn't actually exist although it looks at first glance like it does. > > > > The interrupt can only be enabled if there is someone using the trigger. > > Thus usecount will be non zero and for at least one element > > trig->subirq[i].enabled == true > > > > So we will decrement trig->usecount in the call to iio_trigger_notify_done > > but never reach 0 thus the call to trig->ops->try_reenable never happens > > in the hard interrupt context. > > I think there is a race condition here. If a consumer is disabled > concurrently with iio_trigger_poll() there is a chance that `enabled` is > false for all consumers. > > The odds of this happening are very low, but there is nothing that > prevents it. A consumer disable should have previously resulted in the trigger stopping (or do we have he ordering wrong there?) Might be worth checking that, but see below, as I think we can stop it having harmful effects anyway. > > > > > It does happen later when which ever driver we triggered finishes the > > threaded part of it's handler and calls iio_trigger_notify_done, but that > > is fine. > > > > Assuming people agree with my analysis it would be good to make it explicit > > that we cannot hit the problem path. > > > > Perhaps call a new iio_trigger_notify_no_needed() that simply does > > the decrement without test, or does it with test and spits out a > > warning if we hit 0. > > I think we need to re-think the whole try_reenable() functionality. > Looking at it I think there are more issues here. > > For example lets say we call iio_trigger_notify_done() from the threaded > handler and try_reenable() returns true. We'd now call > iio_trigger_poll() from the threaded context, which is wrong. The history behind that functionality was some early devices which were using level interrupts tied to edge interrupts. One of our earliest drivers had to do a dance with checking a gpio to avoid stalling. That went away a long time ago when we basically concluded that platform was too broken to support interrupts on. Probably would have helped if try_reenable returned a bool as then people wouldn't have returned error codes via it which is completely wrong as it stands (though handy given what I suggest below) From a quick look. bma180, bmc150, kxcjk1013, mxc4005m bmg180, kmx61, bmc150 all only fail if an error occurs in an bus access. That's a bug. at91 never returns anything other than 0. So I think we can probably rip the iio_trigger_poll call out entirely and replace it with an error message, or push an error message into the drivers and drop the check of the value of try_reenable. > > There is also the issue that iio_trigger_poll() effectively can end up > calling itself recursively indefinitely via the > iio_trigger_notify_done() chain, which might not be the best thing in > hard IRQ context. > :) Indeed unwise. I think the only driver that could in theory do this that we had a long time back had a counter in the try_reen to give up if it happened more than a few times. Jonathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-08-14 10:30 ` Jonathan Cameron @ 2020-09-20 18:15 ` Jonathan Cameron 2020-09-21 7:17 ` Christian Eggers 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2020-09-20 18:15 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Thomas Gleixner, Sebastian Andrzej Siewior, Hartmut Knaack, Peter Meerwald-Stadler, Christian Eggers, linux-iio On Fri, 14 Aug 2020 11:30:08 +0100 Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote: > On Fri, 14 Aug 2020 07:24:52 +0200 > Lars-Peter Clausen <lars@metafoo.de> wrote: > > > On 8/13/20 4:55 PM, Jonathan Cameron wrote: > > > On Thu, 13 Aug 2020 14:19:57 +0200 > > > Thomas Gleixner <tglx@linutronix.de> wrote: > > > > > >> Sebastian Andrzej Siewior <bigeasy@linutronix.de> writes: > > >>> On 2020-08-13 11:46:30 [+0200], Lars-Peter Clausen wrote: > > >>>> If you are running with forced IRQ threads the only thing that will then > > >>>> happen in the actual hard IRQ context is the launching of the IRQ threads. > > >>>> Th e IRQ handler of the device driver will run in a threaded IRQ. > > >>> So if it is really just the wakeup of the IRQ-thread then it should be > > >>> okay. > > >>> One thing: iio_trigger_poll() may invoke iio_trigger_notify_done(). This > > >>> would invoke trig->ops->try_reenable callback if available. > > >>> I grepped and found > > >>> - bma180_trig_try_reen() > > >>> It appears to perform i2c_smbus_read_byte_data() and smbus sounds > > >>> sleeping. I don't know if it attempts to acquire any spinlock_t but it > > >>> will be wrong on RT. > > >> It's wrong even on !RT. i2c reads cannot be invoked from hard interrupt > > >> context. > > >> > > > We would hit this (and resulting warnings) all the time if it actually > > > happened, so my suspicion is that it doesn't. > > > > > > I think the path doesn't actually exist although it looks at first glance like it does. > > > > > > The interrupt can only be enabled if there is someone using the trigger. > > > Thus usecount will be non zero and for at least one element > > > trig->subirq[i].enabled == true > > > > > > So we will decrement trig->usecount in the call to iio_trigger_notify_done > > > but never reach 0 thus the call to trig->ops->try_reenable never happens > > > in the hard interrupt context. > > > > I think there is a race condition here. If a consumer is disabled > > concurrently with iio_trigger_poll() there is a chance that `enabled` is > > false for all consumers. > > > > The odds of this happening are very low, but there is nothing that > > prevents it. > > A consumer disable should have previously resulted in the trigger stopping > (or do we have he ordering wrong there?) Might be worth checking that, but > see below, as I think we can stop it having harmful effects anyway. > > > > > > > > > It does happen later when which ever driver we triggered finishes the > > > threaded part of it's handler and calls iio_trigger_notify_done, but that > > > is fine. > > > > > > Assuming people agree with my analysis it would be good to make it explicit > > > that we cannot hit the problem path. > > > > > > Perhaps call a new iio_trigger_notify_no_needed() that simply does > > > the decrement without test, or does it with test and spits out a > > > warning if we hit 0. > > > > I think we need to re-think the whole try_reenable() functionality. > > Looking at it I think there are more issues here. > > > > For example lets say we call iio_trigger_notify_done() from the threaded > > handler and try_reenable() returns true. We'd now call > > iio_trigger_poll() from the threaded context, which is wrong. > > The history behind that functionality was some early devices which were using > level interrupts tied to edge interrupts. One of our earliest drivers had to > do a dance with checking a gpio to avoid stalling. That went away a long > time ago when we basically concluded that platform was too broken to support > interrupts on. > > Probably would have helped if try_reenable returned a bool as then people wouldn't > have returned error codes via it which is completely wrong as it stands (though > handy given what I suggest below) > > From a quick look. > bma180, bmc150, kxcjk1013, mxc4005m bmg180, kmx61, bmc150 > all only fail if an error occurs in an bus access. That's a bug. > > at91 never returns anything other than 0. > > So I think we can probably rip the iio_trigger_poll call out entirely > and replace it with an error message, or push an error message into > the drivers and drop the check of the value of try_reenable. > > > > > There is also the issue that iio_trigger_poll() effectively can end up > > calling itself recursively indefinitely via the > > iio_trigger_notify_done() chain, which might not be the best thing in > > hard IRQ context. > > > > :) Indeed unwise. I think the only driver that could in theory do this > that we had a long time back had a counter in the try_reen to give > up if it happened more than a few times. > A quick update on this one. I've sent out a fix for the try_reenable() patch. Short version is that we no longer let it fail as no one correctly used that path anyway anymore. https://lore.kernel.org/linux-iio/20200920132548.196452-2-jic23@kernel.org/T/#u My understanding is that resolves the open question on this series. I'd like to get both issues resolved, so if people have a chance to look that would be great. Thanks, Jonathan > Jonathan > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-20 18:15 ` Jonathan Cameron @ 2020-09-21 7:17 ` Christian Eggers 2020-09-21 9:57 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Christian Eggers @ 2020-09-21 7:17 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Sebastian Andrzej Siewior, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio Tested together with - iio: Fix: Do not poll the driver again if try_reenable() callback returns non 0. and - iio:trigger: rename try_reenable() to reenable() plus return void on latest mainline (without PREEMPT_RT). The original WARN_ONCE() in kernel/irq/handle.c:159 was not raised anymore. But even without the current patches, this warning is not shown (as this problem only applies to -RT). Currently I haven't ported a RT kernel > 5.4 for my board, so I cannot check with current RT. On 5.4. there the patches seem not to work fully as kernel/timer/hrtimer.c is not up to date enough. Sorry for being not very helpful... Best regards Christian On Sunday, 20 September 2020, 20:15:45 CEST, Jonathan Cameron wrote: > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > As we no longer support a try again if we cannot reenable the trigger > rename the function to reflect this. Also we don't do anything with > the value returned so stop it returning anything. For the few drivers > that didn't already print an error message in this patch, add such > a print. > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Christian Oder <me@myself5.de> > Cc: Eugen Hristev <eugen.hristev@microchip.com> > Cc: Nishant Malpani <nish.malpani25@gmail.com> > Cc: Daniel Baluta <daniel.baluta@oss.nxp.com> > --- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-21 7:17 ` Christian Eggers @ 2020-09-21 9:57 ` Jonathan Cameron 2020-09-21 12:27 ` Sebastian Andrzej Siewior 2020-10-02 14:10 ` Christian Eggers 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Cameron @ 2020-09-21 9:57 UTC (permalink / raw) To: Christian Eggers Cc: Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Sebastian Andrzej Siewior, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio On Mon, 21 Sep 2020 09:17:26 +0200 Christian Eggers <ceggers@arri.de> wrote: > Tested together with > > - iio: Fix: Do not poll the driver again if try_reenable() callback returns non 0. > and > - iio:trigger: rename try_reenable() to reenable() plus return void > > on latest mainline (without PREEMPT_RT). The original WARN_ONCE() in > kernel/irq/handle.c:159 was not raised anymore. But even without the current > patches, this warning is not shown (as this problem only applies to -RT). > > Currently I haven't ported a RT kernel > 5.4 for my board, so I cannot check > with current RT. On 5.4. there the patches seem not to work fully as > kernel/timer/hrtimer.c is not up to date enough. > > Sorry for being not very helpful... Thanks for at least trying! So looking at this the other way, are there any significant risks associated with this change? If not I'm tempted to queue them up and we have the rcX time to fix anything we've missed (just like every other patch!) Jonathan > > Best regards > Christian > > On Sunday, 20 September 2020, 20:15:45 CEST, Jonathan Cameron wrote: > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > > > As we no longer support a try again if we cannot reenable the trigger > > rename the function to reflect this. Also we don't do anything with > > the value returned so stop it returning anything. For the few drivers > > that didn't already print an error message in this patch, add such > > a print. > > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > > Cc: Christian Oder <me@myself5.de> > > Cc: Eugen Hristev <eugen.hristev@microchip.com> > > Cc: Nishant Malpani <nish.malpani25@gmail.com> > > Cc: Daniel Baluta <daniel.baluta@oss.nxp.com> > > --- > > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-21 9:57 ` Jonathan Cameron @ 2020-09-21 12:27 ` Sebastian Andrzej Siewior 2020-09-21 13:32 ` Jonathan Cameron 2020-10-02 14:10 ` Christian Eggers 1 sibling, 1 reply; 20+ messages in thread From: Sebastian Andrzej Siewior @ 2020-09-21 12:27 UTC (permalink / raw) To: Jonathan Cameron Cc: Christian Eggers, Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: > So looking at this the other way, are there any significant risks associated > with this change? If not I'm tempted to queue them up and we have the rcX > time to fix anything we've missed (just like every other patch!) I've been told that it only performs IRQ-thread wake-ups in hard-IRQ context. This is fine then. Looking at the other series where ->try_renable() got renamed. It still looks like bmc150_accel_trig_try_reen() may acquire a mutex. Is it still the case or do I miss something essential? > Jonathan Sebastian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-21 12:27 ` Sebastian Andrzej Siewior @ 2020-09-21 13:32 ` Jonathan Cameron 2020-09-22 2:51 ` Andy Duan 0 siblings, 1 reply; 20+ messages in thread From: Jonathan Cameron @ 2020-09-21 13:32 UTC (permalink / raw) To: Sebastian Andrzej Siewior Cc: Christian Eggers, Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio, Andy Duan On Mon, 21 Sep 2020 14:27:28 +0200 Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: > > So looking at this the other way, are there any significant risks associated > > with this change? If not I'm tempted to queue them up and we have the rcX > > time to fix anything we've missed (just like every other patch!) > > I've been told that it only performs IRQ-thread wake-ups in hard-IRQ > context. This is fine then. > > Looking at the other series where ->try_renable() got renamed. It still > looks like bmc150_accel_trig_try_reen() may acquire a mutex. Is it still > the case or do I miss something essential? True. We could safely drop the mutex there as it's not doing anything useful, but it can sleep anyway as it's doing a bus write. So question is whether we can actually hit that path. I think the reality is no (almost - see below), even though it looks like it from a high level. The path would be that we enter iio_trigger_poll() in interrupt context. That will call generic_handle_irq() to trigger individual devices that are using this trigger. It will also call iio_trigger_notify_done() to decrement the counter for spare outputs of the irq_chip(). It doesn't actually matter if the problem iio_trigger_notify_done() (the one that results in a count of 0 and hence reenable()) occurs as a result of generic_handle_irq() or the direct call of iio_trigger_notify_done() either way it can only happen if we have any drivers calling iio_trigger_notify_done() in interrupt context Someone who is better at coccinelle than me could probably automate checking this. Given this is always called after reading the data we should be safe for any device that requires sleeping. So that just leaves a few SoC ADCs to audit. Thankfully most of them don't implement triggered buffers. Of the ones that do only one doesn't call it from a threaded interrupt handler. drivers/iio/adc/vf610-adc.c However, there looks to be a lot more wrong in there than just this. So normally for a device with a data ready signal like this we would hook up as follows. Data ready #1 -> IRQ chip (trigger) -> Read sensor #1 + iio_trigger_notify_done() -> Read sensor #2 + iio_trigger_notify_done() (note that the read etc is normally in a thread - all we do in interrupt context is usually to grab a timestamp if that makes sense for a given sensor). This driver does both of. Data ready -> Read data from itself and call iio_trigger_notify_done() IRQ chip for a different trigger -> Take a timestamp and never call iio_trigger_notify_done() or read any data for that matter. Which won't do what we want at all. Andy, if I have a go at fixing this are you able to test the result? I think the simplest is probably to introduce a trigger to tie the two halves together. We can set it as the default trigger so things should keep on working for existing users. For more general case, we should probably have two functions. iio_trigger_notify_done() which is only called from places we can sleep. iio_trigger_notify_done_no_action() which only decrements the counter (or given this is only called inside industrialio-trigger.c could just replace with atomic_dec(&trig->use_count)). That change is about avoiding confusion rather than fixing a bug and I think tangential to both of the currently changes. Thanks Sebastian. You are certainly finding some rats holes in my code :) Jonathan > > > Jonathan > > Sebastian ^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-21 13:32 ` Jonathan Cameron @ 2020-09-22 2:51 ` Andy Duan 2020-09-24 6:41 ` Sanchayan Maity 0 siblings, 1 reply; 20+ messages in thread From: Andy Duan @ 2020-09-22 2:51 UTC (permalink / raw) To: Jonathan Cameron, Sebastian Andrzej Siewior, maitysanchayan Cc: Christian Eggers, Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > On Mon, 21 Sep 2020 14:27:28 +0200 > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > > On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: > > > So looking at this the other way, are there any significant risks > > > associated with this change? If not I'm tempted to queue them up > > > and we have the rcX time to fix anything we've missed (just like > > > every other patch!) > > > > I've been told that it only performs IRQ-thread wake-ups in hard-IRQ > > context. This is fine then. > > > drivers/iio/adc/vf610-adc.c > > However, there looks to be a lot more wrong in there than just this. > So normally for a device with a data ready signal like this we would hook up as > follows. > > Data ready #1 -> IRQ chip (trigger) -> Read sensor #1 + > iio_trigger_notify_done() > -> Read sensor #2 + > iio_trigger_notify_done() > > (note that the read etc is normally in a thread - all we do in interrupt context is > usually to grab a timestamp if that makes sense for a given sensor). > > This driver does both of. > Data ready -> Read data from itself and call iio_trigger_notify_done() IRQ chip > for a different trigger -> Take a timestamp and never call > iio_trigger_notify_done() > or read any data for that matter. > > Which won't do what we want at all. > > Andy, if I have a go at fixing this are you able to test the result? > I think the simplest is probably to introduce a trigger to tie the two halves > together. > We can set it as the default trigger so things should keep on working for existing > users. > > For more general case, we should probably have two functions. > > iio_trigger_notify_done() which is only called from places we can sleep. > iio_trigger_notify_done_no_action() which only decrements the counter (or > given this is only called inside industrialio-trigger.c could just replace with > atomic_dec(&trig->use_count)). > Sanchayan, can you help to verify the fixes that Jonathan will send out ? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-22 2:51 ` Andy Duan @ 2020-09-24 6:41 ` Sanchayan Maity 2020-09-24 8:54 ` Stefan Agner 0 siblings, 1 reply; 20+ messages in thread From: Sanchayan Maity @ 2020-09-24 6:41 UTC (permalink / raw) To: Andy Duan Cc: Jonathan Cameron, Sebastian Andrzej Siewior, Christian Eggers, Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio, Stefan Agner On 20-09-22 02:51:11, Andy Duan wrote: > From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > > On Mon, 21 Sep 2020 14:27:28 +0200 > > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > > > > > On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: > > > > So looking at this the other way, are there any significant risks > > > > associated with this change? If not I'm tempted to queue them up > > > > and we have the rcX time to fix anything we've missed (just like > > > > every other patch!) > > > > > > I've been told that it only performs IRQ-thread wake-ups in hard-IRQ > > > context. This is fine then. > > > > > drivers/iio/adc/vf610-adc.c > > > > However, there looks to be a lot more wrong in there than just this. > > So normally for a device with a data ready signal like this we would hook up as > > follows. > > > > Data ready #1 -> IRQ chip (trigger) -> Read sensor #1 + > > iio_trigger_notify_done() > > -> Read sensor #2 + > > iio_trigger_notify_done() > > > > (note that the read etc is normally in a thread - all we do in interrupt context is > > usually to grab a timestamp if that makes sense for a given sensor). > > > > This driver does both of. > > Data ready -> Read data from itself and call iio_trigger_notify_done() IRQ chip > > for a different trigger -> Take a timestamp and never call > > iio_trigger_notify_done() > > or read any data for that matter. > > > > Which won't do what we want at all. > > > > Andy, if I have a go at fixing this are you able to test the result? > > I think the simplest is probably to introduce a trigger to tie the two halves > > together. > > We can set it as the default trigger so things should keep on working for existing > > users. > > > > For more general case, we should probably have two functions. > > > > iio_trigger_notify_done() which is only called from places we can sleep. > > iio_trigger_notify_done_no_action() which only decrements the counter (or > > given this is only called inside industrialio-trigger.c could just replace with > > atomic_dec(&trig->use_count)). > > > > Sanchayan, can you help to verify the fixes that Jonathan will send out ? > Sorry for the delay in reply. Unfortunately can't as I do not access to the hardware having left Toradex. CCed Stefan Agner who might be able to help. @Stefan Hello Stefan :), may be you can help here? ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-24 6:41 ` Sanchayan Maity @ 2020-09-24 8:54 ` Stefan Agner 2020-09-25 12:42 ` Jonathan Cameron 0 siblings, 1 reply; 20+ messages in thread From: Stefan Agner @ 2020-09-24 8:54 UTC (permalink / raw) To: Sanchayan Maity Cc: Andy Duan, Jonathan Cameron, Sebastian Andrzej Siewior, Christian Eggers, Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio On 2020-09-24 08:41, Sanchayan Maity wrote: > On 20-09-22 02:51:11, Andy Duan wrote: >> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> >> > On Mon, 21 Sep 2020 14:27:28 +0200 >> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: >> > >> > > On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: >> > > > So looking at this the other way, are there any significant risks >> > > > associated with this change? If not I'm tempted to queue them up >> > > > and we have the rcX time to fix anything we've missed (just like >> > > > every other patch!) >> > > >> > > I've been told that it only performs IRQ-thread wake-ups in hard-IRQ >> > > context. This is fine then. >> > > >> > drivers/iio/adc/vf610-adc.c >> > >> > However, there looks to be a lot more wrong in there than just this. >> > So normally for a device with a data ready signal like this we would hook up as >> > follows. >> > >> > Data ready #1 -> IRQ chip (trigger) -> Read sensor #1 + >> > iio_trigger_notify_done() >> > -> Read sensor #2 + >> > iio_trigger_notify_done() >> > >> > (note that the read etc is normally in a thread - all we do in interrupt context is >> > usually to grab a timestamp if that makes sense for a given sensor). >> > >> > This driver does both of. >> > Data ready -> Read data from itself and call iio_trigger_notify_done() IRQ chip >> > for a different trigger -> Take a timestamp and never call >> > iio_trigger_notify_done() >> > or read any data for that matter. >> > >> > Which won't do what we want at all. >> > >> > Andy, if I have a go at fixing this are you able to test the result? >> > I think the simplest is probably to introduce a trigger to tie the two halves >> > together. >> > We can set it as the default trigger so things should keep on working for existing >> > users. >> > >> > For more general case, we should probably have two functions. >> > >> > iio_trigger_notify_done() which is only called from places we can sleep. >> > iio_trigger_notify_done_no_action() which only decrements the counter (or >> > given this is only called inside industrialio-trigger.c could just replace with >> > atomic_dec(&trig->use_count)). >> > >> >> Sanchayan, can you help to verify the fixes that Jonathan will send out ? >> > > Sorry for the delay in reply. Unfortunately can't as I do not access to the > hardware having left Toradex. > > CCed Stefan Agner who might be able to help. > > @Stefan > > Hello Stefan :), may be you can help here? Hi all, I do have access to the hardware and can run a test if required. I guess I would just need to test if ADC sampling still work with something like tools/iio/iio_generic_buffer.c? -- Stefan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-24 8:54 ` Stefan Agner @ 2020-09-25 12:42 ` Jonathan Cameron 0 siblings, 0 replies; 20+ messages in thread From: Jonathan Cameron @ 2020-09-25 12:42 UTC (permalink / raw) To: Stefan Agner Cc: Sanchayan Maity, Andy Duan, Jonathan Cameron, Sebastian Andrzej Siewior, Christian Eggers, Lars-Peter Clausen, Thomas Gleixner, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio On Thu, 24 Sep 2020 10:54:39 +0200 Stefan Agner <stefan@agner.ch> wrote: > On 2020-09-24 08:41, Sanchayan Maity wrote: > > On 20-09-22 02:51:11, Andy Duan wrote: > >> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > >> > On Mon, 21 Sep 2020 14:27:28 +0200 > >> > Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote: > >> > > >> > > On 2020-09-21 10:57:03 [+0100], Jonathan Cameron wrote: > >> > > > So looking at this the other way, are there any significant risks > >> > > > associated with this change? If not I'm tempted to queue them up > >> > > > and we have the rcX time to fix anything we've missed (just like > >> > > > every other patch!) > >> > > > >> > > I've been told that it only performs IRQ-thread wake-ups in hard-IRQ > >> > > context. This is fine then. > >> > > > >> > drivers/iio/adc/vf610-adc.c > >> > > >> > However, there looks to be a lot more wrong in there than just this. > >> > So normally for a device with a data ready signal like this we would hook up as > >> > follows. > >> > > >> > Data ready #1 -> IRQ chip (trigger) -> Read sensor #1 + > >> > iio_trigger_notify_done() > >> > -> Read sensor #2 + > >> > iio_trigger_notify_done() > >> > > >> > (note that the read etc is normally in a thread - all we do in interrupt context is > >> > usually to grab a timestamp if that makes sense for a given sensor). > >> > > >> > This driver does both of. > >> > Data ready -> Read data from itself and call iio_trigger_notify_done() IRQ chip > >> > for a different trigger -> Take a timestamp and never call > >> > iio_trigger_notify_done() > >> > or read any data for that matter. > >> > > >> > Which won't do what we want at all. > >> > > >> > Andy, if I have a go at fixing this are you able to test the result? > >> > I think the simplest is probably to introduce a trigger to tie the two halves > >> > together. > >> > We can set it as the default trigger so things should keep on working for existing > >> > users. > >> > > >> > For more general case, we should probably have two functions. > >> > > >> > iio_trigger_notify_done() which is only called from places we can sleep. > >> > iio_trigger_notify_done_no_action() which only decrements the counter (or > >> > given this is only called inside industrialio-trigger.c could just replace with > >> > atomic_dec(&trig->use_count)). > >> > > >> > >> Sanchayan, can you help to verify the fixes that Jonathan will send out ? > >> > > > > Sorry for the delay in reply. Unfortunately can't as I do not access to the > > hardware having left Toradex. > > > > CCed Stefan Agner who might be able to help. > > > > @Stefan > > > > Hello Stefan :), may be you can help here? > > Hi all, > > I do have access to the hardware and can run a test if required. I guess > I would just need to test if ADC sampling still work with something like > tools/iio/iio_generic_buffer.c? Yup. I haven't written the fix yet though. Will make sure to cc you! Thanks, Jonathan > > -- > Stefan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-09-21 9:57 ` Jonathan Cameron 2020-09-21 12:27 ` Sebastian Andrzej Siewior @ 2020-10-02 14:10 ` Christian Eggers 2020-10-10 13:23 ` Jonathan Cameron 1 sibling, 1 reply; 20+ messages in thread From: Christian Eggers @ 2020-10-02 14:10 UTC (permalink / raw) To: Jonathan Cameron Cc: Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Sebastian Andrzej Siewior, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio Hi Jonathan, On Monday, 21 September 2020, 11:57:03 CEST, Jonathan Cameron wrote: > On Mon, 21 Sep 2020 09:17:26 +0200 > > Christian Eggers <ceggers@arri.de> wrote: > > Tested together with > > > > - iio: Fix: Do not poll the driver again if try_reenable() callback > > returns non 0. and > > - iio:trigger: rename try_reenable() to reenable() plus return void > > > > on latest mainline (without PREEMPT_RT). The original WARN_ONCE() in > > kernel/irq/handle.c:159 was not raised anymore. But even without the > > current patches, this warning is not shown (as this problem only applies > > to -RT). > > > > Currently I haven't ported a RT kernel > 5.4 for my board, so I cannot > > check with current RT. On 5.4. there the patches seem not to work fully > > as kernel/timer/hrtimer.c is not up to date enough. > > > > Sorry for being not very helpful... > > Thanks for at least trying! I've just ported my BSP to v5.9-rc7-rt10. It looks like your patch misses one additional change in iio_trig_hrtimer_set_state(): - hrtimer_start(&trig_info->timer, trig_info->period, - HRTIMER_MODE_REL); + hrtimer_start(&trig_info->timer, trig_info->period, + HRTIMER_MODE_REL_HARD); Without this, WARN_ON_ONCE() in kernel/time/hrtimer.c:1133 will be hit: WARN_ON_ONCE(!(mode & HRTIMER_MODE_HARD) ^ !timer->is_hard); So the mode HRTIMER_MODE_REL_HARD is required for hrtimer_init() (will be stored in timer->is_hard) and for hrtimer_start(). Best regards Christian ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context 2020-10-02 14:10 ` Christian Eggers @ 2020-10-10 13:23 ` Jonathan Cameron 0 siblings, 0 replies; 20+ messages in thread From: Jonathan Cameron @ 2020-10-10 13:23 UTC (permalink / raw) To: Christian Eggers Cc: Jonathan Cameron, Lars-Peter Clausen, Thomas Gleixner, Sebastian Andrzej Siewior, Hartmut Knaack, Peter Meerwald-Stadler, linux-iio On Fri, 2 Oct 2020 16:10:09 +0200 Christian Eggers <ceggers@arri.de> wrote: > Hi Jonathan, Lars-Peter's patch! But I'm sure he'll do an updated version to deal with the point you make below. @Lars no great hurry as this isn't going to make the merge window anyway so we have a couple of weeks. Will have to hit after rc1 and get backported as relevant. Thanks, Jonathan > > On Monday, 21 September 2020, 11:57:03 CEST, Jonathan Cameron wrote: > > On Mon, 21 Sep 2020 09:17:26 +0200 > > > > Christian Eggers <ceggers@arri.de> wrote: > > > Tested together with > > > > > > - iio: Fix: Do not poll the driver again if try_reenable() callback > > > returns non 0. and > > > - iio:trigger: rename try_reenable() to reenable() plus return void > > > > > > on latest mainline (without PREEMPT_RT). The original WARN_ONCE() in > > > kernel/irq/handle.c:159 was not raised anymore. But even without the > > > current patches, this warning is not shown (as this problem only applies > > > to -RT). > > > > > > Currently I haven't ported a RT kernel > 5.4 for my board, so I cannot > > > check with current RT. On 5.4. there the patches seem not to work fully > > > as kernel/timer/hrtimer.c is not up to date enough. > > > > > > Sorry for being not very helpful... > > > > Thanks for at least trying! > > I've just ported my BSP to v5.9-rc7-rt10. It looks like your patch misses one > additional change in iio_trig_hrtimer_set_state(): > > - hrtimer_start(&trig_info->timer, trig_info->period, > - HRTIMER_MODE_REL); > + hrtimer_start(&trig_info->timer, trig_info->period, > + HRTIMER_MODE_REL_HARD); > > Without this, WARN_ON_ONCE() in kernel/time/hrtimer.c:1133 will be hit: > > WARN_ON_ONCE(!(mode & HRTIMER_MODE_HARD) ^ !timer->is_hard); > > So the mode HRTIMER_MODE_REL_HARD is required for hrtimer_init() (will be > stored in timer->is_hard) and for hrtimer_start(). > > Best regards > Christian > > > > ________________________________ > [http://assets.arri.com/media/sign/2020-04-03-E-mail-signature-Stellar2_V1.jpg] <https://microsites.arri.com/stellar/> > > Get all the latest information from www.arri.com<https://www.arri.com/>, Facebook<https://www.facebook.com/TeamARRI>, Twitter<https://twitter.com/ARRIChannel>, Instagram<https://instagram.com/arri> and YouTube<https://www.youtube.com/user/ARRIChannel>. > > Arnold & Richter Cine Technik GmbH & Co. Betriebs KG > Sitz: München - Registergericht: Amtsgericht München - Handelsregisternummer: HRA 57918 > Persönlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH > Sitz: München - Registergericht: Amtsgericht München - Handelsregisternummer: HRB 54477 > Geschäftsführer: Dr. Michael Neuhäuser; Stephan Schenk; Walter Trauninger; Markus Zeiler ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-10-10 22:57 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-13 7:53 [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Lars-Peter Clausen 2020-08-13 7:53 ` [PATCH 2/2] iio: sysfs-trigger: Mark irq_work to expire in hardirq context Lars-Peter Clausen 2020-08-13 9:11 ` [PATCH 1/2] iio: hrtimer-trigger: Mark hrtimer to expire in hard interrupt context Sebastian Andrzej Siewior 2020-08-13 9:46 ` Lars-Peter Clausen 2020-08-13 11:27 ` Sebastian Andrzej Siewior 2020-08-13 12:19 ` Thomas Gleixner 2020-08-13 14:55 ` Jonathan Cameron 2020-08-14 5:24 ` Lars-Peter Clausen 2020-08-14 10:30 ` Jonathan Cameron 2020-09-20 18:15 ` Jonathan Cameron 2020-09-21 7:17 ` Christian Eggers 2020-09-21 9:57 ` Jonathan Cameron 2020-09-21 12:27 ` Sebastian Andrzej Siewior 2020-09-21 13:32 ` Jonathan Cameron 2020-09-22 2:51 ` Andy Duan 2020-09-24 6:41 ` Sanchayan Maity 2020-09-24 8:54 ` Stefan Agner 2020-09-25 12:42 ` Jonathan Cameron 2020-10-02 14:10 ` Christian Eggers 2020-10-10 13:23 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).