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

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

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

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

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