| Message ID | 20180820063253.5549-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: don't support irq sharing for gpio edge detection | expand |
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@pengutronix.de> > --- > 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.
On Mon, Aug 20, 2018 at 03:23:05PM +0800, Phil Reid wrote: > 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@pengutronix.de> > > --- > > 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. They are not. And with my patch there is always an event reported when the hardware detects one. That is the theoretical maximum that can be reached. > 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. /dev/gpiochipX doesn't support generating level-sensitive events, so this argument is moot. (And once this is implemented, I'm ok to readd IRQF_SHARED for the level-sensive case.) > If the gpio is on some kinda of i2c /spi bus then the service time can > be quite long. I read this as a reason to apply my patch. > 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. Just because a gpio is behind a slow bus this doesn't imply bad HW design. It also doesn't imply it changes slowly. If I have to choose between: - Interrupt reporting is broken if the line changes quicker than the latency of my machine allows to detect in software; or - I cannot detect interrupts on two gpios if they are connected to the same gpio controller that doesn't distinguish between events on different lines; it is obvious what to pick, isn't it? The current state is unreliable even for sane hardware. This is really ridiculous: My reliable hardware behind a slow bus (or on a loaded machine) reports an event, and because the driver tries to support a usecase that cannot be supported reliable chooses to drop my events. With my patch applied you get an error if you try to work with this unreliable setup. You seem to see this as a disadvantage, I say it is not. (And if you still want to go that path, you can teach the driver for this irq-sharing gpio chip to implement the needed logic and simulate different interrupts. That is without forcing sane hardware setups to drop events.) Do you know such a chip that supports generating an irq for 2 lines but is unable to tell afterwards which line triggered the irq? Best regards Uwe
Hello Linus, even with Phil's comment I'm convinced my patch is correct and an improvement. It throws out support for some hardware configuration (which IMHO are broken anyhow) and in return it makes the sane hardware cases actually work reliable. The current state is that even if my gpio's irq line isn't shared, a spike isn't detected as event because gpiolib suppresses it. And if there are really two gpios sharing the same irq line a level change of the first can result in an event for the second one. IMHO each of these issues alone would be bad enough to fix this. Do you still consider my patch? Best regards Uwe On Mon, Aug 20, 2018 at 08:32:53AM +0200, 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@pengutronix.de> > --- > 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);
On Mon, Aug 20, 2018 at 8:32 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Trying to work out the right event if it's not sure that the examined > gpio actually moved is impossible. I applied the patch with the following reasoning. 1) Uwe has a usecase and hardware that needs this so he can do what he needs to do. I.e. the patch corresponds to a practical need of a user. 2) Only theoretical counter-arguments. 3) Uwe clarified that it is fine to put IRQ sharing back if/when we support level-triggered IRQs from userspace. This makes a lot of sense to me. 4) Rough consensus and running code. I edited the subject and commit message to reflect that we are talking about the userspace events here, not in-kernel users. Edge triggers are tricky and I feel a bit uncertain about how we handle all kinds of electronics out there. I can think about a few design mistakes. At one point I had the feeling level interrupts on GPIO lines are completely bogus, but I guess they make some sense for an intelligent peripheral (like on I2C) with it own internal status register that you then go and handle in a thread. Yours, Linus Walleij
czw., 13 wrz 2018 o 11:13 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > On Mon, Aug 20, 2018 at 8:32 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > Trying to work out the right event if it's not sure that the examined > > gpio actually moved is impossible. > > I applied the patch with the following reasoning. > > 1) Uwe has a usecase and hardware that needs this so he can do > what he needs to do. I.e. the patch corresponds to a practical need > of a user. > > 2) Only theoretical counter-arguments. > > 3) Uwe clarified that it is fine to put IRQ sharing back if/when > we support level-triggered IRQs from userspace. This makes a > lot of sense to me. > > 4) Rough consensus and running code. > > I edited the subject and commit message to reflect that we are talking > about the userspace events here, not in-kernel users. > > Edge triggers are tricky and I feel a bit uncertain about how we handle > all kinds of electronics out there. I can think about a few design > mistakes. > > At one point I had the feeling level interrupts on GPIO lines are > completely bogus, but I guess they make some sense for an > intelligent peripheral (like on I2C) with it own internal status > register that you then go and handle in a thread. > Hi Linus, Uwe, just FYI - this patch broke existing libgpiod tests in 4.20-rc1. With this change, if user space requests a line for events of specific type, let's say rising edge, both falling and rising edge events will be emitted as rising edge. Uwe: was this expected? I haven't checked with real HW yet, but it seems strange. Best regards, Bartosz Golaszewski
Hello Bartosz, On Thu, Nov 08, 2018 at 06:07:43PM +0100, Bartosz Golaszewski wrote: > czw., 13 wrz 2018 o 11:13 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > On Mon, Aug 20, 2018 at 8:32 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > Trying to work out the right event if it's not sure that the examined > > > gpio actually moved is impossible. > > > > I applied the patch with the following reasoning. > > > > 1) Uwe has a usecase and hardware that needs this so he can do > > what he needs to do. I.e. the patch corresponds to a practical need > > of a user. > > > > 2) Only theoretical counter-arguments. > > > > 3) Uwe clarified that it is fine to put IRQ sharing back if/when > > we support level-triggered IRQs from userspace. This makes a > > lot of sense to me. > > > > 4) Rough consensus and running code. > > > > I edited the subject and commit message to reflect that we are talking > > about the userspace events here, not in-kernel users. > > > > Edge triggers are tricky and I feel a bit uncertain about how we handle > > all kinds of electronics out there. I can think about a few design > > mistakes. > > > > At one point I had the feeling level interrupts on GPIO lines are > > completely bogus, but I guess they make some sense for an > > intelligent peripheral (like on I2C) with it own internal status > > register that you then go and handle in a thread. > > > > Hi Linus, Uwe, > > just FYI - this patch broke existing libgpiod tests in 4.20-rc1. > > With this change, if user space requests a line for events of specific > type, let's say rising edge, both falling and rising edge events will > be emitted as rising edge. I bet this is using the gpio-mockup driver. The problem is that the mockup driver fires an irq for each write independent of the sensitivity configured by the consumer. Best regards Uwe
czw., 8 lis 2018 o 21:30 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > Hello Bartosz, > > On Thu, Nov 08, 2018 at 06:07:43PM +0100, Bartosz Golaszewski wrote: > > czw., 13 wrz 2018 o 11:13 Linus Walleij <linus.walleij@linaro.org> napisał(a): > > > > > > On Mon, Aug 20, 2018 at 8:32 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > Trying to work out the right event if it's not sure that the examined > > > > gpio actually moved is impossible. > > > > > > I applied the patch with the following reasoning. > > > > > > 1) Uwe has a usecase and hardware that needs this so he can do > > > what he needs to do. I.e. the patch corresponds to a practical need > > > of a user. > > > > > > 2) Only theoretical counter-arguments. > > > > > > 3) Uwe clarified that it is fine to put IRQ sharing back if/when > > > we support level-triggered IRQs from userspace. This makes a > > > lot of sense to me. > > > > > > 4) Rough consensus and running code. > > > > > > I edited the subject and commit message to reflect that we are talking > > > about the userspace events here, not in-kernel users. > > > > > > Edge triggers are tricky and I feel a bit uncertain about how we handle > > > all kinds of electronics out there. I can think about a few design > > > mistakes. > > > > > > At one point I had the feeling level interrupts on GPIO lines are > > > completely bogus, but I guess they make some sense for an > > > intelligent peripheral (like on I2C) with it own internal status > > > register that you then go and handle in a thread. > > > > > > > Hi Linus, Uwe, > > > > just FYI - this patch broke existing libgpiod tests in 4.20-rc1. > > > > With this change, if user space requests a line for events of specific > > type, let's say rising edge, both falling and rising edge events will > > be emitted as rising edge. > > I bet this is using the gpio-mockup driver. The problem is that the > mockup driver fires an irq for each write independent of the sensitivity > configured by the consumer. > Ah right, this normally resides in the interrupt controller. I'll see if I can modify the driver. Bart
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);
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@pengutronix.de> --- drivers/gpio/gpiolib.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)