Re: [PATCH] gpiolib: don't support irq sharing for gpio edge detection

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

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@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);

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