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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

2018-08-16 14:39 GMT+02:00 Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>:
> Hello,
>
> while making use of /dev/gpiochipX to poll for a gpio irq I looked at
> lineevent_irq_thread while debugging a problem.
>
> 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.
>

Hi Uwe,

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).

> 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.
>

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.

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().

Best regards,
Bartosz Golaszewski