On 20/08/2018 14:32, Uwe Kleine-König wrote:
Trying to work out the right event if it's not sure that the examined
gpio actually moved is impossible.
Consider two gpios "gpioA" and "gpioB" that share an interrupt. gpioA's
irq should trigger on any edge, gpioB's on a falling edge. If now the
common irq fires and both gpio lines are high, there are several
possibilities that could have happend:
a) gpioA just had a low-to-high edge
b) gpioB just had a high-to-low-to-high spike
c) a combination of both a) and b)
While c) is unlikely (in most setups) a) and b) alone are bad enough.
Currently the code assumes case a) unconditionally and doesn't report an
event for gpioB. Note that even if there is no irq sharing involved a
spike for a gpio might not result in an event if it's configured to
trigger for a single edge only.
The only way to improve this is to drop support for interrupt sharing.
This way a spike results in an event for the right gpio at least.
Note that apart from dropping IRQF_SHARED this effectively undoes
commit df1e76f28ffe ("gpiolib: skip unwanted events, don't convert them
to opposite edge").
This obviously breaks setups that rely on interrupt sharing, but given
that this cannot be reliable, this is probably an acceptable trade-off.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
---
drivers/gpio/gpiolib.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..b43cc0f42e73 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -806,26 +806,26 @@ static irqreturn_t lineevent_irq_thread(int irq, void *p)
{
struct lineevent_state *le = p;
struct gpioevent_data ge;
- int ret, level;
+ int ret;
/* Do not leak kernel stack to userspace */
memset(&ge, 0, sizeof(ge));
ge.timestamp = le->timestamp;
- level = gpiod_get_value_cansleep(le->desc);
if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE
&& le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
+ int level = gpiod_get_value_cansleep(le->desc);
if (level)
/* Emit low-to-high event */
ge.id = GPIOEVENT_EVENT_RISING_EDGE;
else
/* Emit high-to-low event */
ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
- } else if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE && level) {
+ } else if (le->eflags & GPIOEVENT_REQUEST_RISING_EDGE) {
/* Emit low-to-high event */
ge.id = GPIOEVENT_EVENT_RISING_EDGE;
- } else if (le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE && !level) {
+ } else if (le->eflags & GPIOEVENT_REQUEST_FALLING_EDGE) {
/* Emit high-to-low event */
ge.id = GPIOEVENT_EVENT_FALLING_EDGE;
} else {
@@ -936,7 +936,6 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
if (eflags & GPIOEVENT_REQUEST_FALLING_EDGE)
irqflags |= IRQF_TRIGGER_FALLING;
irqflags |= IRQF_ONESHOT;
- irqflags |= IRQF_SHARED;
INIT_KFIFO(le->events);
init_waitqueue_head(&le->wait);
As Bartosz stated I also don't think this does much.
Edge triggered interrupts are always going to be subject to race.
This will also break the ability to share interrupts for those setups that are sane and use level triggered.
That is irq is asserted until they're serviced / cleared.
If the gpio is on some kinda of i2c /spi bus then the service time can be quite long.
and as you say need to keep gpiod_get_value_cansleep for these type of devices.
The only real fix is a proper HW design.
--
Regards
Phil Reid
