Re: /dev/gpiochipX event filtering throws away wanted events

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

Hello Bartosz,

On Sun, Aug 19, 2018 at 07:22:03PM +0200, Bartosz Golaszewski wrote:
> 2018-08-16 14:39 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>:
> > I think there is a problem with event filtering, but didn't try to make
> > the problem trigger on hardware, so maybe this is only theoretical.
> >
> > Currently the code looks as follows (simplified):
> >
> >         level = gpiod_get_value_cansleep(le->desc);
> >
> >         if (triggering_on_both_edges)
> >                 ge.id = level ? GPIOEVENT_EVENT_RISING_EDGE : GPIOEVENT_EVENT_FALLING_EDGE;
> >         else if (triggering_on_rising_edge && level)
> >                 ge.id = GPIOEVENT_EVENT_RISING_EDGE;
> >         else if (triggering_on_falling_edge && !level)
> >                 ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
> >         else
> >                 return IRQ_NONE;
> >
> >         ...
> >
> > If now for example I trigger on falling edge and the line goes low and
> > up again before gpiod_get_value_cansleep is called, this event is
> > missed because level is 1.
> >
> > This was introduced by commit
> >
> >         df1e76f28ffe ("gpiolib: skip unwanted events, don't convert them to opposite edge")
> >
> > which was backported to 4.9.y and 4.12.y, too.
>
> While your concern is theoretically valid I don't see how this was
> introduced with this specific commit. We would run into this problem
> before, only the logic would be inverted (we would miss the first 1->0
> event and emit the 0->1).

You're talking about eflags having both RISING_EDGE and FALLING_EDGE. If
there is only one, the signaling is fine. And yeah, I'd miss an edge, but
the generated event still is right. This is a race that isn't completely
fixable in software. While you could track the gpio state and report a
falling and a rising edge if there was a spike, you cannot detect if
there might have been two spikes and so you might have to report 4
events.

re your "I don't see how this was introduced with this commit": Before
df1e76f28ffe with eflags = GPIOEVENT_REQUEST_FALLING_EDGE a quick
low-high reported an event. df1e76f28ffe filters that out even though it
is valid.

> > To fix this, we'd need to revert df1e76f28ffe and drop
> >
> >         irqflags |= IRQF_SHARED;
> >
> > to be sure that the irq really corresponds to the gpio we're interested
> > in. This however might result in regressions because userspace cannot
> > monitor more than 1 gpio per irq any more. (Not sure how relevant this
> > is.)
> 
> I think it's relevant for expanders that have a single interrupt line
> and multiple GPIOs.
> 
> > The commit log for df1e76f28ffe claims:
> >
> >         Instead of skipping the events we don't want, they are [without
> >         df1e76f28ffe] interpreted as events with opposing edge.
> >
> > I wonder if this was a theoretical issue, or a real one where irq
> > sharing was involved.

If it's about shared interrupts, the problem before df1e76f28ffe was
that gpioA's irq triggering resulted in an event for gpioB even though
gpioB didn't move. So I don't see how an opposing edge is relevant here.
Moreover we still report an event for gpioB if it triggers for both
edges, right?

These two together convince me that trying to support shared irq here is
wrong.

> This commit fixed an issue introduced with a previous change:
> ad537b822577 ("gpiolib: fix filtering out unwanted events") which
> fixed an actual issue with BOTH_EDGES flag being incorrectly
> interpreted.

> While your concern is real I think that our approach is mostly fine -
> we try to read the value as soon as we can in the interrupt handler.

I'd say being only "mostly fine" when doing irq handling isn't good
enough.

> What we could do however for some theoretical improvement is use
> gpiod_get_value() since we're in non-sleeping context and maybe even
> call it as the very first operation - even before calling
> ktime_get_real_ns().

No, the irq handler is threaded, to sleeping is allowed (otherwise the
might_sleep in gpiod_get_value_cansleep would have triggered a report
from a user for sure). When converting to gpiod_get_value you're
probably breaking some setups.

I'll prepare a patch.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |