On Sun, 8 Nov 2020 19:27:28 +0100
Lorenzo Bianconi <lorenzo@xxxxxxxxxx> wrote:
> [...]
> > >
> > > I guess since edge interrupts run with the line unmasked a new interrupt can fire
> > > while the irq thread is still running (so wake_up_process() will just return) but
> > > the driver has already read fifo_status register and so it will not read new
> > > sample. This case should be fixed reading again the fifo_status register.
> >
> > It doesn't actually help because there is always a window after the fifo_status register
> > is read before we exit the thread.
> >
> > I 'think' what happens (it's been a while since I dug through this stuff) is
> > that you end up with the task being added to the runqueue, even though it's
> > already running. Upshot the thread gets scheduled gain.
> >
> > If this were not the case there would be a race with any edge based interrupt
> > as the thread has to reenable the interrupt and it will always be able to fire
> > whilst the thread is still running.
>
> I guess this is the case (race between irq-thread and edge interrupt) since afaik
> handle_edge_irq() runs with the irq-line unmasked.
> I agree with you this approach does not fix completely the issue but it reduces
> the race-surface since now the interrupt can fire while processing the previous one
> (the issue occurs if the interrupt fires between the end of hw->settings->fifo_ops.read_fifo()
> and the end of the irq-thread) while before the interrupt must always fire before
> reading the fifo status register (in fact with the patch applied I am not able
> to trigger the issue anymore).
So the thing I've been trying to say badly here is that I'm fairly sure the
issue isn't what you think it is at all. (Note I've spent a lot of
time with scopes on interrupt lines looking for similar issues - it's
not fun).
I think the actual condition here is that you have an interrupt that is not
guaranteed to go low for long enough between being cleared and set. Thus if you are
read the fifo at almost exactly the moment new data is written you may in theory
have the interrupt drop, but in practice analog electronics kicks in an you won't
get an interrupt detected at all. This why the sensor needs to put guarantees
on that drop time (some do - but I'm not seeing in datasheet for this one).
On a more mundane note, I'm not sure in this case that there is a guarantee
it will ever drop even in theory - this buffer could for this short period be
filling faster than we drain it.
The reason your change makes this much less likely to happen is that, by checking
again you are generally much closer to the time of the change of the level in
the fifo. Thus, unless you are preempted you should clear it long before it
would be set again, and thus get a nice clean drop on the interrupt.
So for some asci art
Previously we have
data samples | | |
_
Read of fifo ___________|_____
_______ _____________
interrupt line ____| | Interrupt stuck high as edge missed.
^
1
With your fix
data samples | | |
_
Read of fifo ___________|__|__
_______ __
interrupt line ____| | |____|
^ ^
1 2
So we would have missed 1, but because we check the fifo level again immediate
after we would have made it drop, if we hit this unfortunately timing we will
very quickly pull new data from the sensor and result in a drop well before the
next interrupt comes in.
>
> @denis, mario, armando: can you please confirm the hw does not support pulsed
> interrupts for fifo-watermark?
>
> If not one possible approach would be to disable the interrupt generation on
> the sensor at the beginning of st_lsm6dsx_handler_thread() and schedule a
> workqueue at the end of st_lsm6dsx_handler_thread() to re-enable the sensor
> interrupt generation. What do you think?
Reenabling it in the thread should work as well. It is a heavy weight solution
but it is what you are expected to do in cases like this.
I'd be very surprised if that doesn't work. The normal operation of edge
interrupt handlers is to reenable in the thread after we are sure we have
cleared the condition for the original interrupt. That will take long enough
(as bus transaction involved) that the interrupt will definitely have dropped
for long enough to be detected.
In some similar cases we've just concluded the right option is to not
support edge interrupts. Do we know if we have boards out there that are using
it in that mode and is there any chance they would support level interrupts
as that's going to be a lot simpler and more reliable for this?
Jonathan
>
> Regards,
> Lorenzo
>
> >
> > Jonathan
> >
> >
> >
> >
> > >
> > > Regards,
> > > Lorenzo
> > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Hmm. Having had a look at one of the datasheets, I'm far from convinced these
> > > > > > parts truely support edge interrupts. I can't see anything about minimum
> > > > > > off periods etc that you need for true edge interrupts. Otherwise they are
> > > > > > going to be prone to races.
> > > > >
> > > > > @mario, denis, armando: any pointer for this?
> > > > >
> > > > > >
> > > > > > So I think the following can happen.
> > > > > >
> > > > > > A) We drain the fifo and it stays under the limit. Hence once that
> > > > > > is crossed in future we will interrupt as normal.
> > > > > >
> > > > > > B) We drain the fifo but it either has a very low watermark, or is
> > > > > > filling very fast. We manage to drain enough to get the interrupt
> > > > > > to fire again, so all is fine if less than ideal. With you loop we
> > > > > > may up entering the interrupt handler when we don't actually need to.
> > > > > > If you want to avoid that you would need to disable the interrupt,
> > > > > > then drain the fifo and finally do a dance to successfully reenable
> > > > > > the interrupt, whilst ensuring no chance of missing by checking it
> > > > > > should not have fired (still below the threshold)
> > > > > >
> > > > > > C) We try to drain the fifo, but it is actually filling fast enough that
> > > > > > we never get it under the limit, so no interrupt ever fires.
> > > > > > With new code, we'll keep spinning to 0 so might eventually drain it.
> > > > > > That needs a timeout so we just give up eventually.
> > > > > >
> > > > > > D) watershed is one sample, we drain low enough to successfully get down
> > > > > > to zero at the moment of the read, but very very soon after that we get
> > > > > > one sample again. There is a window in which the interrupt line dropped
> > > > > > but analogue electronics etc being what they are, it may not have been
> > > > > > detectable. Hence we miss an interrupt... What you are doing is reducing
> > > > > > the chance of hitting this. It is nasty, but you might be able to ensure
> > > > > > a reasonable period by widening this window. Limit the watermark to 2
> > > > > > samples?
> > > > > >
> > > > > > Also needs a fixes tag :)
> > > > >
> > > > > ack, I will add them in v2
> > > > >
> > > > > Regards,
> > > > > Lorenzo
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx>
> > > > > > > ---
> > > > > > > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 33 +++++++++++++++-----
> > > > > > > 1 file changed, 25 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > index 5e584c6026f1..d43b08ceec01 100644
> > > > > > > --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> > > > > > > @@ -2457,22 +2457,36 @@ st_lsm6dsx_report_motion_event(struct st_lsm6dsx_hw *hw)
> > > > > > > return data & event_settings->wakeup_src_status_mask;
> > > > > > > }
> > > > > > >
> > > > > > > +static irqreturn_t st_lsm6dsx_handler_irq(int irq, void *private)
> > > > > > > +{
> > > > > > > + return IRQ_WAKE_THREAD;
> > > > > > > +}
> > > > > > > +
> > > > > > > static irqreturn_t st_lsm6dsx_handler_thread(int irq, void *private)
> > > > > > > {
> > > > > > > struct st_lsm6dsx_hw *hw = private;
> > > > > > > + int fifo_len = 0, len = 0;
> > > > > > > bool event;
> > > > > > > - int count;
> > > > > > >
> > > > > > > event = st_lsm6dsx_report_motion_event(hw);
> > > > > > >
> > > > > > > if (!hw->settings->fifo_ops.read_fifo)
> > > > > > > return event ? IRQ_HANDLED : IRQ_NONE;
> > > > > > >
> > > > > > > - mutex_lock(&hw->fifo_lock);
> > > > > > > - count = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > > - mutex_unlock(&hw->fifo_lock);
> > > > > > > + /*
> > > > > > > + * If we are using edge IRQs, new samples can arrive while
> > > > > > > + * processing current IRQ and those may be missed unless we
> > > > > > > + * pick them here, so let's try read FIFO status again
> > > > > > > + */
> > > > > > > + do {
> > > > > > > + mutex_lock(&hw->fifo_lock);
> > > > > > > + len = hw->settings->fifo_ops.read_fifo(hw);
> > > > > > > + mutex_unlock(&hw->fifo_lock);
> > > > > > > +
> > > > > > > + fifo_len += len;
> > > > > > > + } while (len > 0);
> > > > > > >
> > > > > > > - return count || event ? IRQ_HANDLED : IRQ_NONE;
> > > > > > > + return fifo_len || event ? IRQ_HANDLED : IRQ_NONE;
> > > > > > > }
> > > > > > >
> > > > > > > static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
> > > > > > > @@ -2488,10 +2502,14 @@ static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
> > > > > > >
> > > > > > > switch (irq_type) {
> > > > > > > case IRQF_TRIGGER_HIGH:
> > > > > > > + irq_type |= IRQF_ONESHOT;
> > > > > > > + fallthrough;
> > > > > > > case IRQF_TRIGGER_RISING:
> > > > > > > irq_active_low = false;
> > > > > > > break;
> > > > > > > case IRQF_TRIGGER_LOW:
> > > > > > > + irq_type |= IRQF_ONESHOT;
> > > > > > > + fallthrough;
> > > > > > > case IRQF_TRIGGER_FALLING:
> > > > > > > irq_active_low = true;
> > > > > > > break;
> > > > > > > @@ -2520,10 +2538,9 @@ static int st_lsm6dsx_irq_setup(struct st_lsm6dsx_hw *hw)
> > > > > > > }
> > > > > > >
> > > > > > > err = devm_request_threaded_irq(hw->dev, hw->irq,
> > > > > > > - NULL,
> > > > > > > + st_lsm6dsx_handler_irq,
> > > > > > > st_lsm6dsx_handler_thread,
> > > > > > > - irq_type | IRQF_ONESHOT,
> > > > > > > - "lsm6dsx", hw);
> > > > > > > + irq_type, "lsm6dsx", hw);
> > > > > > > if (err) {
> > > > > > > dev_err(hw->dev, "failed to request trigger irq %d\n",
> > > > > > > hw->irq);
> > > > > >
> > > > >
> > > >
> >
