[PATCH 0/2] irq/irq_sim: provide a specialized variant of irq_sim_fire()

[PATCH 0/2] irq/irq_sim: provide a specialized variant of irq_sim_fire()

[Bartosz Golaszewski]

The interrupts simulator has a limitation in that a call to irq_sim_fire()
always fires the dummy interrupt - irrespective of the flags passed to the
irq request function.

This series addresses a problem with certain libgpiod tests failing when
using gpio-mockup with simulated interrupts since commit fa38869b0161
("gpiolib: Don't support irq sharing for userspace"). The tests are
failing because irq_sim fires interrupts for falling edge events even if
gpiolib requests a threaded irq triggered by rising edge only (and vice
versa).

We're introducing a new function: irq_sim_fire_edge() which takes
another argument allowing to specify the trigger type for the dummy
interrupt. We then use it in gpio-mockup.

Patch 1 applies on top of my previous irq_sim changeset[1]. Patch 2
applies on top of the recent changes to gpio-mockup[2].

Since test breakage in user-space after 4.20-rc1 can be considered
a regression, it would be great if this could go into 4.20.

[1] https://lkml.org/lkml/2018/11/9/1418
[2] https://lkml.org/lkml/2018/11/8/1012

Bartosz Golaszewski (2):
  irq/irq_sim: provide irq_sim_fire_edge()
  gpio: mockup: use irq_sim_fire_edge()

 drivers/gpio/gpio-mockup.c |  5 +++--
 include/linux/irq_sim.h    |  2 ++
 kernel/irq/irq_sim.c       | 30 ++++++++++++++++++++++++++++--
 3 files changed, 33 insertions(+), 4 deletions(-)

-- 
2.19.1

[PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The irq_sim irqchip doesn't allow to configure the sensitivity so every
call to irq_sim_fire() fires a dummy interrupt. This used to not matter
for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
("gpiolib: Don't support irq sharing for userspace") which made it
impossible for gpio-mockup to ignore certain events (e.g. only receive
notifications about rising edge events).

Introduce a specialized variant of irq_sim_fire() which takes another
argument called edge. allowing to specify the trigger type for the
dummy interrupt.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/irq_sim.h |  2 ++
 kernel/irq/irq_sim.c    | 30 ++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index 4500d453a63e..f55148e77792 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -22,6 +22,7 @@ struct irq_sim_work_ctx {
 struct irq_sim_irq_ctx {
 	int			irqnum;
 	bool			enabled;
+	int			edge;
 };
 
 struct irq_sim {
@@ -36,6 +37,7 @@ int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
 		      unsigned int num_irqs);
 void irq_sim_fini(struct irq_sim *sim);
 void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
+void irq_sim_fire_edge(struct irq_sim *sim, unsigned int offset, int edge);
 int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
 
 #endif /* _LINUX_IRQ_SIM_H */
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 98a20e1594ce..a9330050f751 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -25,10 +25,20 @@ static void irq_sim_irqunmask(struct irq_data *data)
 	irq_ctx->enabled = true;
 }
 
+static int irq_sim_set_type(struct irq_data *data, unsigned int type)
+{
+	struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data);
+
+	irq_ctx->edge = type & IRQ_TYPE_EDGE_BOTH;
+
+	return 0;
+}
+
 static struct irq_chip irq_sim_irqchip = {
 	.name		= "irq_sim",
 	.irq_mask	= irq_sim_irqmask,
 	.irq_unmask	= irq_sim_irqunmask,
+	.irq_set_type	= irq_sim_set_type,
 };
 
 static void irq_sim_handle_irq(struct irq_work *work)
@@ -161,12 +171,28 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
  */
 void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
 {
-	if (sim->irqs[offset].enabled) {
+	/* Don't care about the edge. */
+	irq_sim_fire_edge(sim, offset, IRQ_TYPE_EDGE_BOTH);
+}
+EXPORT_SYMBOL_GPL(irq_sim_fire);
+
+/**
+ * irq_sim_fire_edge - Enqueue an interrupt, specify the edge.
+ *
+ * @sim:        The interrupt simulator object.
+ * @offset:     Offset of the simulated interrupt which should be fired.
+ * edge:        Edge of the interrupt (IRQ_TYPE_EDGE_RISING/FALLING).
+ */
+void irq_sim_fire_edge(struct irq_sim *sim, unsigned int offset, int edge)
+{
+	struct irq_sim_irq_ctx *irq = &sim->irqs[offset];
+
+	if (irq->enabled && (irq->edge & edge)) {
 		set_bit(offset, sim->work_ctx.pending);
 		irq_work_queue(&sim->work_ctx.work);
 	}
 }
-EXPORT_SYMBOL_GPL(irq_sim_fire);
+EXPORT_SYMBOL_GPL(irq_sim_fire_edge);
 
 /**
  * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
-- 
2.19.1

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The irq_sim irqchip doesn't allow to configure the sensitivity so every
> call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> ("gpiolib: Don't support irq sharing for userspace") which made it
> impossible for gpio-mockup to ignore certain events (e.g. only receive
> notifications about rising edge events).
> 
> Introduce a specialized variant of irq_sim_fire() which takes another
> argument called edge. allowing to specify the trigger type for the
> dummy interrupt.

I wonder if it's worth the effort to fix irq_sim. If you take a look in
my gpio-simulator patch, it is trivial to get it right without external
help with an amount of code that is usual for a driver that handles
irqs.

> @@ -161,12 +171,28 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
>   */
>  void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
>  {
> -	if (sim->irqs[offset].enabled) {
> +	/* Don't care about the edge. */
> +	irq_sim_fire_edge(sim, offset, IRQ_TYPE_EDGE_BOTH);

Conceptually irq_sim_fire_edge should be defined above irq_sim_fire.

> +EXPORT_SYMBOL_GPL(irq_sim_fire);
> +
> +/**
> + * irq_sim_fire_edge - Enqueue an interrupt, specify the edge.
> + *
> + * @sim:        The interrupt simulator object.
> + * @offset:     Offset of the simulated interrupt which should be fired.
> + * edge:        Edge of the interrupt (IRQ_TYPE_EDGE_RISING/FALLING).
> + */
> +void irq_sim_fire_edge(struct irq_sim *sim, unsigned int offset, int edge)
> +{
> +	struct irq_sim_irq_ctx *irq = &sim->irqs[offset];
> +
> +	if (irq->enabled && (irq->edge & edge)) {
>  		set_bit(offset, sim->work_ctx.pending);
>  		irq_work_queue(&sim->work_ctx.work);
>  	}
>  }
> -EXPORT_SYMBOL_GPL(irq_sim_fire);
> +EXPORT_SYMBOL_GPL(irq_sim_fire_edge);

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

wt., 20 lis 2018 o 18:17 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > ("gpiolib: Don't support irq sharing for userspace") which made it
> > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > notifications about rising edge events).
> >
> > Introduce a specialized variant of irq_sim_fire() which takes another
> > argument called edge. allowing to specify the trigger type for the
> > dummy interrupt.
>
> I wonder if it's worth the effort to fix irq_sim. If you take a look in
> my gpio-simulator patch, it is trivial to get it right without external
> help with an amount of code that is usual for a driver that handles
> irqs.
>

You're basically recommending handcrafting another local piece of code
for simulating interrupts - something that multiple users may be
interested in. You did that in your proposed gpio-simulator and I
still can't understand why you couldn't reuse the existing solution.
Even if it's broken for your use-case, it's surely easier to fix it
than to rewrite and duplicate it? There are very few cases where code
consolidation is not a good thing and I don't think this is one of
them.

The interrupt simulator exists and is used by two testing drivers
currently. One of them needs some more flexibility and this is what
this patch is trying to add. I believe it is worth the effort.

Best regards,
Bartosz

> > @@ -161,12 +171,28 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
> >   */
> >  void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> >  {
> > -     if (sim->irqs[offset].enabled) {
> > +     /* Don't care about the edge. */
> > +     irq_sim_fire_edge(sim, offset, IRQ_TYPE_EDGE_BOTH);
>
> Conceptually irq_sim_fire_edge should be defined above irq_sim_fire.
>
> > +EXPORT_SYMBOL_GPL(irq_sim_fire);
> > +
> > +/**
> > + * irq_sim_fire_edge - Enqueue an interrupt, specify the edge.
> > + *
> > + * @sim:        The interrupt simulator object.
> > + * @offset:     Offset of the simulated interrupt which should be fired.
> > + * edge:        Edge of the interrupt (IRQ_TYPE_EDGE_RISING/FALLING).
> > + */
> > +void irq_sim_fire_edge(struct irq_sim *sim, unsigned int offset, int edge)
> > +{
> > +     struct irq_sim_irq_ctx *irq = &sim->irqs[offset];
> > +
> > +     if (irq->enabled && (irq->edge & edge)) {
> >               set_bit(offset, sim->work_ctx.pending);
> >               irq_work_queue(&sim->work_ctx.work);
> >       }
> >  }
> > -EXPORT_SYMBOL_GPL(irq_sim_fire);
> > +EXPORT_SYMBOL_GPL(irq_sim_fire_edge);
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

Hello Bartosz,

On Wed, Nov 21, 2018 at 05:34:32PM +0100, Bartosz Golaszewski wrote:
> wt., 20 lis 2018 o 18:17 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > > ("gpiolib: Don't support irq sharing for userspace") which made it
> > > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > > notifications about rising edge events).
> > >
> > > Introduce a specialized variant of irq_sim_fire() which takes another
> > > argument called edge. allowing to specify the trigger type for the
> > > dummy interrupt.
> >
> > I wonder if it's worth the effort to fix irq_sim. If you take a look in
> > my gpio-simulator patch, it is trivial to get it right without external
> > help with an amount of code that is usual for a driver that handles
> > irqs.
> 
> You're basically recommending handcrafting another local piece of code
> for simulating interrupts - something that multiple users may be
> interested in. You did that in your proposed gpio-simulator and I
> still can't understand why you couldn't reuse the existing solution.
> Even if it's broken for your use-case, it's surely easier to fix it
> than to rewrite and duplicate it? There are very few cases where code
> consolidation is not a good thing and I don't think this is one of
> them.

I don't say that factoring out common stuff is bad. But if in the end
you call

	irq_sim_something(some, parameters, offset);

with the simulator and if you don't use the irq simulator you do

	irq = irq_find_mapping(irqdomain, offset);
	generic_handle_irq(irq);

I prefer the latter because it's only a single additional line and in
return it's more obvious what it does because it's the same that many
other drivers (for real hardware) also do.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

śr., 21 lis 2018 o 20:15 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> Hello Bartosz,
>
> On Wed, Nov 21, 2018 at 05:34:32PM +0100, Bartosz Golaszewski wrote:
> > wt., 20 lis 2018 o 18:17 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> napisał(a):
> > >
> > > On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > > > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > > > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > > > ("gpiolib: Don't support irq sharing for userspace") which made it
> > > > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > > > notifications about rising edge events).
> > > >
> > > > Introduce a specialized variant of irq_sim_fire() which takes another
> > > > argument called edge. allowing to specify the trigger type for the
> > > > dummy interrupt.
> > >
> > > I wonder if it's worth the effort to fix irq_sim. If you take a look in
> > > my gpio-simulator patch, it is trivial to get it right without external
> > > help with an amount of code that is usual for a driver that handles
> > > irqs.
> >
> > You're basically recommending handcrafting another local piece of code
> > for simulating interrupts - something that multiple users may be
> > interested in. You did that in your proposed gpio-simulator and I
> > still can't understand why you couldn't reuse the existing solution.
> > Even if it's broken for your use-case, it's surely easier to fix it
> > than to rewrite and duplicate it? There are very few cases where code
> > consolidation is not a good thing and I don't think this is one of
> > them.
>
> I don't say that factoring out common stuff is bad. But if in the end
> you call
>
>         irq_sim_something(some, parameters, offset);
>
> with the simulator and if you don't use the irq simulator you do
>
>         irq = irq_find_mapping(irqdomain, offset);
>         generic_handle_irq(irq);
>
> I prefer the latter because it's only a single additional line and in
> return it's more obvious what it does because it's the same that many
> other drivers (for real hardware) also do.
>

I'm not sure I'm following you. You still need to add ~150 LOC for the
gpio_simulator_irqtrigger() worker and gpio_simulator_irq_*() routines
locally as you did in your gpio-simulator patch. A generic simulator +
using the irq_work saves you that.

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Fri, Nov 23, 2018 at 04:59:46PM +0100, Bartosz Golaszewski wrote:
> śr., 21 lis 2018 o 20:15 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > Hello Bartosz,
> >
> > On Wed, Nov 21, 2018 at 05:34:32PM +0100, Bartosz Golaszewski wrote:
> > > wt., 20 lis 2018 o 18:17 Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> napisał(a):
> > > >
> > > > On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > > > > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > > > > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > > > > ("gpiolib: Don't support irq sharing for userspace") which made it
> > > > > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > > > > notifications about rising edge events).
> > > > >
> > > > > Introduce a specialized variant of irq_sim_fire() which takes another
> > > > > argument called edge. allowing to specify the trigger type for the
> > > > > dummy interrupt.
> > > >
> > > > I wonder if it's worth the effort to fix irq_sim. If you take a look in
> > > > my gpio-simulator patch, it is trivial to get it right without external
> > > > help with an amount of code that is usual for a driver that handles
> > > > irqs.
> > >
> > > You're basically recommending handcrafting another local piece of code
> > > for simulating interrupts - something that multiple users may be
> > > interested in. You did that in your proposed gpio-simulator and I
> > > still can't understand why you couldn't reuse the existing solution.
> > > Even if it's broken for your use-case, it's surely easier to fix it
> > > than to rewrite and duplicate it? There are very few cases where code
> > > consolidation is not a good thing and I don't think this is one of
> > > them.
> >
> > I don't say that factoring out common stuff is bad. But if in the end
> > you call
> >
> >         irq_sim_something(some, parameters, offset);
> >
> > with the simulator and if you don't use the irq simulator you do
> >
> >         irq = irq_find_mapping(irqdomain, offset);
> >         generic_handle_irq(irq);
> >
> > I prefer the latter because it's only a single additional line and in
> > return it's more obvious what it does because it's the same that many
> > other drivers (for real hardware) also do.
> 
> I'm not sure I'm following you. You still need to add ~150 LOC for the
> gpio_simulator_irqtrigger() worker and gpio_simulator_irq_*() routines
> locally as you did in your gpio-simulator patch. A generic simulator +
> using the irq_work saves you that.

If you teach the irq-sim driver everything that the gpio-simulator does
in the functions you pointed out then this is for sure functionality
that other users of the irq-sim code won't make use of. This is about
tracking the level of the gpio/irq line and the interrupt enable and raw
status bits that usually happen in hardware. The dummy iio driver won't
need that for sure as it only cares about triggering an irq and doesn't
even specify an irq type.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

niedz., 25 lis 2018 o 22:18 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Fri, Nov 23, 2018 at 04:59:46PM +0100, Bartosz Golaszewski wrote:
> > śr., 21 lis 2018 o 20:15 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> napisał(a):
> > >
> > > Hello Bartosz,
> > >
> > > On Wed, Nov 21, 2018 at 05:34:32PM +0100, Bartosz Golaszewski wrote:
> > > > wt., 20 lis 2018 o 18:17 Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> napisał(a):
> > > > >
> > > > > On Tue, Nov 20, 2018 at 02:40:31PM +0100, Bartosz Golaszewski wrote:
> > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > >
> > > > > > The irq_sim irqchip doesn't allow to configure the sensitivity so every
> > > > > > call to irq_sim_fire() fires a dummy interrupt. This used to not matter
> > > > > > for gpio-mockup (one of the irq_sim users) until commit fa38869b0161
> > > > > > ("gpiolib: Don't support irq sharing for userspace") which made it
> > > > > > impossible for gpio-mockup to ignore certain events (e.g. only receive
> > > > > > notifications about rising edge events).
> > > > > >
> > > > > > Introduce a specialized variant of irq_sim_fire() which takes another
> > > > > > argument called edge. allowing to specify the trigger type for the
> > > > > > dummy interrupt.
> > > > >
> > > > > I wonder if it's worth the effort to fix irq_sim. If you take a look in
> > > > > my gpio-simulator patch, it is trivial to get it right without external
> > > > > help with an amount of code that is usual for a driver that handles
> > > > > irqs.
> > > >
> > > > You're basically recommending handcrafting another local piece of code
> > > > for simulating interrupts - something that multiple users may be
> > > > interested in. You did that in your proposed gpio-simulator and I
> > > > still can't understand why you couldn't reuse the existing solution.
> > > > Even if it's broken for your use-case, it's surely easier to fix it
> > > > than to rewrite and duplicate it? There are very few cases where code
> > > > consolidation is not a good thing and I don't think this is one of
> > > > them.
> > >
> > > I don't say that factoring out common stuff is bad. But if in the end
> > > you call
> > >
> > >         irq_sim_something(some, parameters, offset);
> > >
> > > with the simulator and if you don't use the irq simulator you do
> > >
> > >         irq = irq_find_mapping(irqdomain, offset);
> > >         generic_handle_irq(irq);
> > >
> > > I prefer the latter because it's only a single additional line and in
> > > return it's more obvious what it does because it's the same that many
> > > other drivers (for real hardware) also do.
> >
> > I'm not sure I'm following you. You still need to add ~150 LOC for the
> > gpio_simulator_irqtrigger() worker and gpio_simulator_irq_*() routines
> > locally as you did in your gpio-simulator patch. A generic simulator +
> > using the irq_work saves you that.
>
> If you teach the irq-sim driver everything that the gpio-simulator does
> in the functions you pointed out then this is for sure functionality
> that other users of the irq-sim code won't make use of. This is about
> tracking the level of the gpio/irq line and the interrupt enable and raw
> status bits that usually happen in hardware. The dummy iio driver won't
> need that for sure as it only cares about triggering an irq and doesn't
> even specify an irq type.
>

We're getting too much into details of how to handle simulated
interrupts and we can continue discussing it, but meanwhile I'd like
to address a different thing:

Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
sharing for userspace") some libgpiod tests are failing because we can
no longer depend on reading the value of a dummy GPIO after detecting
an interrupt to know the edge of the interrupt. While these interrupts
are triggered from debugfs and debugfs is not required to maintain
compatibility, I thing having a working test suite for the GPIO
subsystem and uAPI is worth applying these two patches and also the
previous one[1].

Can we have them applied for 4.20 or are there any objections?

Best regards,
Bartosz Golaszewski

[1] https://lkml.org/lkml/2018/11/9/1418

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Linus Walleij]

On Thu, Nov 29, 2018 at 7:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> sharing for userspace") some libgpiod tests are failing because we can
> no longer depend on reading the value of a dummy GPIO after detecting
> an interrupt to know the edge of the interrupt. While these interrupts
> are triggered from debugfs and debugfs is not required to maintain
> compatibility, I thing having a working test suite for the GPIO
> subsystem and uAPI is worth applying these two patches and also the
> previous one[1].
>
> Can we have them applied for 4.20 or are there any objections?

I'm fine with applying them if I can just get an ACK from one of the IRQ
maintainers (Thomas, Marc).

Yours,
Linus Walleij

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Marc Zyngier]

On Fri, 30 Nov 2018 23:26:25 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Nov 29, 2018 at 7:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > sharing for userspace") some libgpiod tests are failing because we can
> > no longer depend on reading the value of a dummy GPIO after detecting
> > an interrupt to know the edge of the interrupt. While these interrupts
> > are triggered from debugfs and debugfs is not required to maintain
> > compatibility, I thing having a working test suite for the GPIO
> > subsystem and uAPI is worth applying these two patches and also the
> > previous one[1].
> >
> > Can we have them applied for 4.20 or are there any objections?  
> 
> I'm fine with applying them if I can just get an ACK from one of the IRQ
> maintainers (Thomas, Marc).

I'm fine with that patch, with the provision that (nitpick) the edge
field is turned into a bool instead of an int.

With that,

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 13:29 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> On Fri, 30 Nov 2018 23:26:25 +0100
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > On Thu, Nov 29, 2018 at 7:14 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > > sharing for userspace") some libgpiod tests are failing because we can
> > > no longer depend on reading the value of a dummy GPIO after detecting
> > > an interrupt to know the edge of the interrupt. While these interrupts
> > > are triggered from debugfs and debugfs is not required to maintain
> > > compatibility, I thing having a working test suite for the GPIO
> > > subsystem and uAPI is worth applying these two patches and also the
> > > previous one[1].
> > >
> > > Can we have them applied for 4.20 or are there any objections?
> >
> > I'm fine with applying them if I can just get an ACK from one of the IRQ
> > maintainers (Thomas, Marc).
>
> I'm fine with that patch, with the provision that (nitpick) the edge
> field is turned into a bool instead of an int.
>
> With that,
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>

I wanted to reuse the already existing IRQ_TYPE_EDGE_RISING/FALLING
defines which I think makes more sense here than a boolean.

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

Hello,

On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> We're getting too much into details of how to handle simulated
> interrupts and we can continue discussing it, but meanwhile I'd like
> to address a different thing:
> 
> Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> sharing for userspace") some libgpiod tests are failing because we can
> no longer depend on reading the value of a dummy GPIO after detecting
> an interrupt to know the edge of the interrupt. While these interrupts
> are triggered from debugfs and debugfs is not required to maintain
> compatibility, I thing having a working test suite for the GPIO
> subsystem and uAPI is worth applying these two patches and also the
> previous one[1].
> 
> Can we have them applied for 4.20 or are there any objections?

Just for the record: I objected the patch, Bartosz agrees to discuss
further and but because this is too much detail the patch should now be
applied anyhow to fix the test suite of an external project. This seems
wrong to me.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 22:56 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> Hello,
>
> On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> > We're getting too much into details of how to handle simulated
> > interrupts and we can continue discussing it, but meanwhile I'd like
> > to address a different thing:
> >
> > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > sharing for userspace") some libgpiod tests are failing because we can
> > no longer depend on reading the value of a dummy GPIO after detecting
> > an interrupt to know the edge of the interrupt. While these interrupts
> > are triggered from debugfs and debugfs is not required to maintain
> > compatibility, I thing having a working test suite for the GPIO
> > subsystem and uAPI is worth applying these two patches and also the
> > previous one[1].
> >
> > Can we have them applied for 4.20 or are there any objections?
>
> Just for the record: I objected the patch, Bartosz agrees to discuss
> further and but because this is too much detail the patch should now be
> applied anyhow to fix the test suite of an external project. This seems
> wrong to me.
>

Just to look at it from a different perspective: we have a project
whose tests rely on a behavior that was changed by Uwe's patch. While
the patch is fine, we need to find a correct way of testing the GPIO
user API. This may take a long time. In order to not break the tests
of an external project in 4.20 I propose to patch the interrupt
simulator (a component only used for testing) for now and to revisit
it later without time pressure.

Best regards,
Bartosz

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

niedz., 2 gru 2018 o 23:20 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> niedz., 2 gru 2018 o 22:56 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > Hello,
> >
> > On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> > > We're getting too much into details of how to handle simulated
> > > interrupts and we can continue discussing it, but meanwhile I'd like
> > > to address a different thing:
> > >
> > > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > > sharing for userspace") some libgpiod tests are failing because we can
> > > no longer depend on reading the value of a dummy GPIO after detecting
> > > an interrupt to know the edge of the interrupt. While these interrupts
> > > are triggered from debugfs and debugfs is not required to maintain
> > > compatibility, I thing having a working test suite for the GPIO
> > > subsystem and uAPI is worth applying these two patches and also the
> > > previous one[1].
> > >
> > > Can we have them applied for 4.20 or are there any objections?
> >
> > Just for the record: I objected the patch, Bartosz agrees to discuss
> > further and but because this is too much detail the patch should now be
> > applied anyhow to fix the test suite of an external project. This seems
> > wrong to me.
> >
>
> Just to look at it from a different perspective: we have a project
> whose tests rely on a behavior that was changed by Uwe's patch. While
> the patch is fine, we need to find a correct way of testing the GPIO
> user API. This may take a long time. In order to not break the tests
> of an external project in 4.20 I propose to patch the interrupt
> simulator (a component only used for testing) for now and to revisit
> it later without time pressure.
>
> Best regards,
> Bartosz

In fact after re-reading this conversation I'm still not sure what
your objection is exactly. You're proposing a solution that may well
be nicely engineered but it's specific to your gpio-simulator.
Meanwhile I'm trying to provide a more generalized API for more
testing modules to use.

Why exactly would you not merge this fix?

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 11:23:38AM +0100, Bartosz Golaszewski wrote:
> niedz., 2 gru 2018 o 23:20 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> >
> > niedz., 2 gru 2018 o 22:56 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> napisał(a):
> > >
> > > Hello,
> > >
> > > On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> > > > We're getting too much into details of how to handle simulated
> > > > interrupts and we can continue discussing it, but meanwhile I'd like
> > > > to address a different thing:
> > > >
> > > > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > > > sharing for userspace") some libgpiod tests are failing because we can
> > > > no longer depend on reading the value of a dummy GPIO after detecting
> > > > an interrupt to know the edge of the interrupt. While these interrupts
> > > > are triggered from debugfs and debugfs is not required to maintain
> > > > compatibility, I thing having a working test suite for the GPIO
> > > > subsystem and uAPI is worth applying these two patches and also the
> > > > previous one[1].
> > > >
> > > > Can we have them applied for 4.20 or are there any objections?
> > >
> > > Just for the record: I objected the patch, Bartosz agrees to discuss
> > > further and but because this is too much detail the patch should now be
> > > applied anyhow to fix the test suite of an external project. This seems
> > > wrong to me.
> > >
> >
> > Just to look at it from a different perspective: we have a project
> > whose tests rely on a behavior that was changed by Uwe's patch. While
> > the patch is fine, we need to find a correct way of testing the GPIO
> > user API. This may take a long time. In order to not break the tests
> > of an external project in 4.20 I propose to patch the interrupt
> > simulator (a component only used for testing) for now and to revisit
> > it later without time pressure.
> >
> > Best regards,
> > Bartosz
> 
> In fact after re-reading this conversation I'm still not sure what
> your objection is exactly. You're proposing a solution that may well
> be nicely engineered but it's specific to your gpio-simulator.
> Meanwhile I'm trying to provide a more generalized API for more
> testing modules to use.

I think you're generalizing something that won't find any user apart
from your mockup driver. So I'd say the generalisation is wrong and the
added code could better live in the mockup driver directly.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

pon., 3 gru 2018 o 11:49 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Mon, Dec 03, 2018 at 11:23:38AM +0100, Bartosz Golaszewski wrote:
> > niedz., 2 gru 2018 o 23:20 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> > >
> > > niedz., 2 gru 2018 o 22:56 Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> napisał(a):
> > > >
> > > > Hello,
> > > >
> > > > On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> > > > > We're getting too much into details of how to handle simulated
> > > > > interrupts and we can continue discussing it, but meanwhile I'd like
> > > > > to address a different thing:
> > > > >
> > > > > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > > > > sharing for userspace") some libgpiod tests are failing because we can
> > > > > no longer depend on reading the value of a dummy GPIO after detecting
> > > > > an interrupt to know the edge of the interrupt. While these interrupts
> > > > > are triggered from debugfs and debugfs is not required to maintain
> > > > > compatibility, I thing having a working test suite for the GPIO
> > > > > subsystem and uAPI is worth applying these two patches and also the
> > > > > previous one[1].
> > > > >
> > > > > Can we have them applied for 4.20 or are there any objections?
> > > >
> > > > Just for the record: I objected the patch, Bartosz agrees to discuss
> > > > further and but because this is too much detail the patch should now be
> > > > applied anyhow to fix the test suite of an external project. This seems
> > > > wrong to me.
> > > >
> > >
> > > Just to look at it from a different perspective: we have a project
> > > whose tests rely on a behavior that was changed by Uwe's patch. While
> > > the patch is fine, we need to find a correct way of testing the GPIO
> > > user API. This may take a long time. In order to not break the tests
> > > of an external project in 4.20 I propose to patch the interrupt
> > > simulator (a component only used for testing) for now and to revisit
> > > it later without time pressure.
> > >
> > > Best regards,
> > > Bartosz
> >
> > In fact after re-reading this conversation I'm still not sure what
> > your objection is exactly. You're proposing a solution that may well
> > be nicely engineered but it's specific to your gpio-simulator.
> > Meanwhile I'm trying to provide a more generalized API for more
> > testing modules to use.
>
> I think you're generalizing something that won't find any user apart
> from your mockup driver. So I'd say the generalisation is wrong and the
> added code could better live in the mockup driver directly.
>

It used to live in the gpio-mockup driver and I generalized it
precisely because there was another driver - iio evgen - which was
doing basically the same thing. While I don't know if there'll be more
users (I'd guess it would be useful for testing purposes of other
subsystems) having the same functionality implemented once is better
than twice.

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> pon., 3 gru 2018 o 11:49 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > On Mon, Dec 03, 2018 at 11:23:38AM +0100, Bartosz Golaszewski wrote:
> > > niedz., 2 gru 2018 o 23:20 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> > > >
> > > > niedz., 2 gru 2018 o 22:56 Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> napisał(a):
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Thu, Nov 29, 2018 at 07:14:45PM +0100, Bartosz Golaszewski wrote:
> > > > > > We're getting too much into details of how to handle simulated
> > > > > > interrupts and we can continue discussing it, but meanwhile I'd like
> > > > > > to address a different thing:
> > > > > >
> > > > > > Thomas, Linus: after commit fa38869b0161 ("gpiolib: Don't support irq
> > > > > > sharing for userspace") some libgpiod tests are failing because we can
> > > > > > no longer depend on reading the value of a dummy GPIO after detecting
> > > > > > an interrupt to know the edge of the interrupt. While these interrupts
> > > > > > are triggered from debugfs and debugfs is not required to maintain
> > > > > > compatibility, I thing having a working test suite for the GPIO
> > > > > > subsystem and uAPI is worth applying these two patches and also the
> > > > > > previous one[1].
> > > > > >
> > > > > > Can we have them applied for 4.20 or are there any objections?
> > > > >
> > > > > Just for the record: I objected the patch, Bartosz agrees to discuss
> > > > > further and but because this is too much detail the patch should now be
> > > > > applied anyhow to fix the test suite of an external project. This seems
> > > > > wrong to me.
> > > > >
> > > >
> > > > Just to look at it from a different perspective: we have a project
> > > > whose tests rely on a behavior that was changed by Uwe's patch. While
> > > > the patch is fine, we need to find a correct way of testing the GPIO
> > > > user API. This may take a long time. In order to not break the tests
> > > > of an external project in 4.20 I propose to patch the interrupt
> > > > simulator (a component only used for testing) for now and to revisit
> > > > it later without time pressure.
> > > >
> > > > Best regards,
> > > > Bartosz
> > >
> > > In fact after re-reading this conversation I'm still not sure what
> > > your objection is exactly. You're proposing a solution that may well
> > > be nicely engineered but it's specific to your gpio-simulator.
> > > Meanwhile I'm trying to provide a more generalized API for more
> > > testing modules to use.
> >
> > I think you're generalizing something that won't find any user apart
> > from your mockup driver. So I'd say the generalisation is wrong and the
> > added code could better live in the mockup driver directly.
> >
> 
> It used to live in the gpio-mockup driver and I generalized it
> precisely because there was another driver - iio evgen - which was
> doing basically the same thing. While I don't know if there'll be more
> users (I'd guess it would be useful for testing purposes of other
> subsystems) having the same functionality implemented once is better
> than twice.

The iio testing driver only needs the trigger and relies on an irq that
then calls the registerd handler. The iio driver doesn't need to tune
the edge sensitivity though and if your mockup driver just only calls
the fire routine if the configured sensitivity justifies that,
everything should work as expected.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Linus Walleij]

On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:

> > It used to live in the gpio-mockup driver and I generalized it
> > precisely because there was another driver - iio evgen - which was
> > doing basically the same thing. While I don't know if there'll be more
> > users (I'd guess it would be useful for testing purposes of other
> > subsystems) having the same functionality implemented once is better
> > than twice.
>
> The iio testing driver only needs the trigger and relies on an irq that
> then calls the registerd handler. The iio driver doesn't need to tune
> the edge sensitivity though and if your mockup driver just only calls
> the fire routine if the configured sensitivity justifies that,
> everything should work as expected.

Simulating edges in the generic IRQ simulator codes seems
generally useful to me, even if there is just one user now.

Certainly for any kind of IRQ testing, it could be interesting to
throw several low-to-high and high-to-low transitions
on a driver and see how it reacts.

But it is up to the irqchip maintainers to state whether they
agree.

Yours,
Linus Walleij

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

śr., 5 gru 2018 o 13:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
>
> > > It used to live in the gpio-mockup driver and I generalized it
> > > precisely because there was another driver - iio evgen - which was
> > > doing basically the same thing. While I don't know if there'll be more
> > > users (I'd guess it would be useful for testing purposes of other
> > > subsystems) having the same functionality implemented once is better
> > > than twice.
> >
> > The iio testing driver only needs the trigger and relies on an irq that
> > then calls the registerd handler. The iio driver doesn't need to tune
> > the edge sensitivity though and if your mockup driver just only calls
> > the fire routine if the configured sensitivity justifies that,
> > everything should work as expected.
>
> Simulating edges in the generic IRQ simulator codes seems
> generally useful to me, even if there is just one user now.
>
> Certainly for any kind of IRQ testing, it could be interesting to
> throw several low-to-high and high-to-low transitions
> on a driver and see how it reacts.
>
> But it is up to the irqchip maintainers to state whether they
> agree.
>

All that would be great, but at this point I just want to fix broken
tests in user-space. After that we can think about how to
improve/approach simulating interrupts all we want.

Marc: is my explanation for using an int instead of bool for
irq_sim_fire_edge() fine? Can Linus pick this up for fixes?

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Linus Walleij]

On Wed, Dec 5, 2018 at 1:38 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:

> All that would be great, but at this point I just want to fix broken
> tests in user-space. After that we can think about how to
> improve/approach simulating interrupts all we want.

That is true. The other Linus is known to steamroll
maintainers who knowingly break userspace, so we have
a broad consensus not to do that, and I think
that holds no matter if the ABI user is a test or not,
debugfs or not.

Yours,
Linus Walleij

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

śr., 5 gru 2018 o 13:38 Bartosz Golaszewski
<bgolaszewski@baylibre.com> napisał(a):
>
> śr., 5 gru 2018 o 13:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> >
> > On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> >
> > > > It used to live in the gpio-mockup driver and I generalized it
> > > > precisely because there was another driver - iio evgen - which was
> > > > doing basically the same thing. While I don't know if there'll be more
> > > > users (I'd guess it would be useful for testing purposes of other
> > > > subsystems) having the same functionality implemented once is better
> > > > than twice.
> > >
> > > The iio testing driver only needs the trigger and relies on an irq that
> > > then calls the registerd handler. The iio driver doesn't need to tune
> > > the edge sensitivity though and if your mockup driver just only calls
> > > the fire routine if the configured sensitivity justifies that,
> > > everything should work as expected.
> >
> > Simulating edges in the generic IRQ simulator codes seems
> > generally useful to me, even if there is just one user now.
> >
> > Certainly for any kind of IRQ testing, it could be interesting to
> > throw several low-to-high and high-to-low transitions
> > on a driver and see how it reacts.
> >
> > But it is up to the irqchip maintainers to state whether they
> > agree.
> >
>
> All that would be great, but at this point I just want to fix broken
> tests in user-space. After that we can think about how to
> improve/approach simulating interrupts all we want.
>
> Marc: is my explanation for using an int instead of bool for
> irq_sim_fire_edge() fine? Can Linus pick this up for fixes?
>

Ping. We have only this week left to fix the regression - can we get
your Ack Marc?

NOTE: this patch must go into v4.20, and will probably conflict with
the bitmap patch you picked up for 4.21. I can take it into my PR for
Linus Walleij and then fix the conflict in next.

Bart

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Mon, Dec 17, 2018 at 11:32:45AM +0100, Bartosz Golaszewski wrote:
> śr., 5 gru 2018 o 13:38 Bartosz Golaszewski
> <bgolaszewski@baylibre.com> napisał(a):
> >
> > śr., 5 gru 2018 o 13:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > >
> > > On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> > >
> > > > > It used to live in the gpio-mockup driver and I generalized it
> > > > > precisely because there was another driver - iio evgen - which was
> > > > > doing basically the same thing. While I don't know if there'll be more
> > > > > users (I'd guess it would be useful for testing purposes of other
> > > > > subsystems) having the same functionality implemented once is better
> > > > > than twice.
> > > >
> > > > The iio testing driver only needs the trigger and relies on an irq that
> > > > then calls the registerd handler. The iio driver doesn't need to tune
> > > > the edge sensitivity though and if your mockup driver just only calls
> > > > the fire routine if the configured sensitivity justifies that,
> > > > everything should work as expected.
> > >
> > > Simulating edges in the generic IRQ simulator codes seems
> > > generally useful to me, even if there is just one user now.
> > >
> > > Certainly for any kind of IRQ testing, it could be interesting to
> > > throw several low-to-high and high-to-low transitions
> > > on a driver and see how it reacts.
> > >
> > > But it is up to the irqchip maintainers to state whether they
> > > agree.
> > >
> >
> > All that would be great, but at this point I just want to fix broken
> > tests in user-space. After that we can think about how to
> > improve/approach simulating interrupts all we want.
> >
> > Marc: is my explanation for using an int instead of bool for
> > irq_sim_fire_edge() fine? Can Linus pick this up for fixes?
> >
> 
> Ping. We have only this week left to fix the regression - can we get
> your Ack Marc?

I don't understand the urge. The problem is that libgpiod's test is
failing. And that is because when userspace requested
IRQF_TRIGGER_FALLING the mockup driver also fires if the line rised and
with my change libgpiod now sees that and wonders about it. Apart from
the test failure both libgpiod and the gpio framework are entirely fine
(AFAICT).

The "fix" under discussion is to modify the mockup driver to only report
a falling irq if the output is set to 0. But it also fires if the value
is already 0 and is set to 0 again. So the problem isn't addressed
completely, but only enough to make libgpiod's test suite report
success.

In my eyes this is not how test-driven development works. Tests are
here to bring breakage into the light. That worked just fine here. And
now as a test fails, the goal is to fix the breakage, but not to change
just enough details to get the test to pass and then urge them through
because "we're already at -rc7 and userspace broke!"

And no, the right fix isn't hard. My concerns were expressed Tuesday
last week, and the problem wasn't important enough since then to fix the
patch set.

But maybe it's just me and the Linux development process changed since I
learned about it and today the demand on quality isn't that high any
more.

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Bartosz Golaszewski]

pon., 17 gru 2018 o 13:59 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Mon, Dec 17, 2018 at 11:32:45AM +0100, Bartosz Golaszewski wrote:
> > śr., 5 gru 2018 o 13:38 Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> napisał(a):
> > >
> > > śr., 5 gru 2018 o 13:20 Linus Walleij <linus.walleij@linaro.org> napisał(a):
> > > >
> > > > On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> > > >
> > > > > > It used to live in the gpio-mockup driver and I generalized it
> > > > > > precisely because there was another driver - iio evgen - which was
> > > > > > doing basically the same thing. While I don't know if there'll be more
> > > > > > users (I'd guess it would be useful for testing purposes of other
> > > > > > subsystems) having the same functionality implemented once is better
> > > > > > than twice.
> > > > >
> > > > > The iio testing driver only needs the trigger and relies on an irq that
> > > > > then calls the registerd handler. The iio driver doesn't need to tune
> > > > > the edge sensitivity though and if your mockup driver just only calls
> > > > > the fire routine if the configured sensitivity justifies that,
> > > > > everything should work as expected.
> > > >
> > > > Simulating edges in the generic IRQ simulator codes seems
> > > > generally useful to me, even if there is just one user now.
> > > >
> > > > Certainly for any kind of IRQ testing, it could be interesting to
> > > > throw several low-to-high and high-to-low transitions
> > > > on a driver and see how it reacts.
> > > >
> > > > But it is up to the irqchip maintainers to state whether they
> > > > agree.
> > > >
> > >
> > > All that would be great, but at this point I just want to fix broken
> > > tests in user-space. After that we can think about how to
> > > improve/approach simulating interrupts all we want.
> > >
> > > Marc: is my explanation for using an int instead of bool for
> > > irq_sim_fire_edge() fine? Can Linus pick this up for fixes?
> > >
> >
> > Ping. We have only this week left to fix the regression - can we get
> > your Ack Marc?
>
> I don't understand the urge. The problem is that libgpiod's test is
> failing. And that is because when userspace requested
> IRQF_TRIGGER_FALLING the mockup driver also fires if the line rised and
> with my change libgpiod now sees that and wonders about it. Apart from
> the test failure both libgpiod and the gpio framework are entirely fine
> (AFAICT).
>
> The "fix" under discussion is to modify the mockup driver to only report
> a falling irq if the output is set to 0. But it also fires if the value
> is already 0 and is set to 0 again. So the problem isn't addressed
> completely, but only enough to make libgpiod's test suite report
> success.
>

The problem with your approach is that you're treating gpio-mockup as
a regular driver the goal of which is self-contained correctness. I
treat it as a tool to test the userspace API.

Of course libgpiod works correctly - it requests and receives the
correct type of interrupts. So does the gpiolib in-kernel part. The
problem indeed lies with the kernel testing module. But it doesn't
matter - we want to make sure the uAPI works correctly i.e. it behaves
the same with gpio-mockup as it would with a real driver for actual HW
behind. We're not testing gpio-mockup(!) here.

> In my eyes this is not how test-driven development works. Tests are
> here to bring breakage into the light. That worked just fine here. And
> now as a test fails, the goal is to fix the breakage, but not to change
> just enough details to get the test to pass and then urge them through
> because "we're already at -rc7 and userspace broke!"
>

The tests here are to find regressions in a) libgpiod and b) gpiolib
kernel-to-userspace interface. The mockup module isn't part of either.

The breakage in gpio-mockup/irq_sim is really not all that important.
Whether the userspace API works is. And with a breakage like this
we're now unable to check if it behaves correctly for events of
specified types. So yes: for 4.20 I want to fix the gpio-mockup module
just enough to keep the tests passing.

> And no, the right fix isn't hard. My concerns were expressed Tuesday
> last week, and the problem wasn't important enough since then to fix the
> patch set.
>

I already told you on several occasions that I *will* address certain
issues in irq_sim. It will *not* make you happy however because it
will not use the mechanism you suggested in gpio-simulator as I still
want to keep this relatively generic for others to use. I will fix
other problems though - among others the one with multiple subsequent
events of the same edge. I don't want to be in a hurry to propose
something fast, so I want to patch the tests now and then have time to
come up with a better solution.

Linus agrees. I see no objections to that from neither Marc nor
Thomas. I would really like to stop discussing this over and over
again after my every e-mail in this thread.

> But maybe it's just me and the Linux development process changed since I
> learned about it and today the demand on quality isn't that high any
> more.
>

First rule has always been "don't break the userspace" and everything
else came after that.

Best regards,
Bartosz Golaszewski

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Wed, Dec 05, 2018 at 01:19:54PM +0100, Linus Walleij wrote:
> On Mon, Dec 3, 2018 at 12:06 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Dec 03, 2018 at 11:57:26AM +0100, Bartosz Golaszewski wrote:
> 
> > > It used to live in the gpio-mockup driver and I generalized it
> > > precisely because there was another driver - iio evgen - which was
> > > doing basically the same thing. While I don't know if there'll be more
> > > users (I'd guess it would be useful for testing purposes of other
> > > subsystems) having the same functionality implemented once is better
> > > than twice.
> >
> > The iio testing driver only needs the trigger and relies on an irq that
> > then calls the registerd handler. The iio driver doesn't need to tune
> > the edge sensitivity though and if your mockup driver just only calls
> > the fire routine if the configured sensitivity justifies that,
> > everything should work as expected.
> 
> Simulating edges in the generic IRQ simulator codes seems
> generally useful to me, even if there is just one user now.

I cannot imagine another potential user. Which kind of driver could use
that that is not a gpio simulator?

Best regards
Uwe

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

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Linus Walleij]

On Wed, Dec 5, 2018 at 2:20 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Dec 05, 2018 at 01:19:54PM +0100, Linus Walleij wrote:

> > > The iio testing driver only needs the trigger and relies on an irq that
> > > then calls the registerd handler. The iio driver doesn't need to tune
> > > the edge sensitivity though and if your mockup driver just only calls
> > > the fire routine if the configured sensitivity justifies that,
> > > everything should work as expected.
> >
> > Simulating edges in the generic IRQ simulator codes seems
> > generally useful to me, even if there is just one user now.
>
> I cannot imagine another potential user. Which kind of driver could use
> that that is not a gpio simulator?

I suppose anything that can generate an IRQ and wants to generate
some test IRQs where edge matters, drivers/irqchips/?

Regmap IRQ drivers/base/regmap/regmap-irq.c does support
edges, and regmap already expose all registers it handles in
debugfs, so it'd be really neat of you can also use debugfs
to insert IRQs from a device using regmap.

Yours,
Linus Walleij

Re: [PATCH 1/2] irq/irq_sim: provide irq_sim_fire_edge()

[Uwe Kleine-König]

On Fri, Dec 14, 2018 at 03:07:37PM +0100, Linus Walleij wrote:
> On Wed, Dec 5, 2018 at 2:20 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Dec 05, 2018 at 01:19:54PM +0100, Linus Walleij wrote:
> 
> > > > The iio testing driver only needs the trigger and relies on an irq that
> > > > then calls the registerd handler. The iio driver doesn't need to tune
> > > > the edge sensitivity though and if your mockup driver just only calls
> > > > the fire routine if the configured sensitivity justifies that,
> > > > everything should work as expected.
> > >
> > > Simulating edges in the generic IRQ simulator codes seems
> > > generally useful to me, even if there is just one user now.
> >
> > I cannot imagine another potential user. Which kind of driver could use
> > that that is not a gpio simulator?
> 
> I suppose anything that can generate an IRQ and wants to generate
> some test IRQs where edge matters, drivers/irqchips/?

Should the irqchip be the consumer or the provider of the irq? I would
have said the provider as it is an irqchip. What would be the gain of
such an irq chip as all users could use a (flexible enough) gpio
simulator instead?

Best regards
Uwe

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

[PATCH 2/2] gpio: mockup: use irq_sim_fire_edge()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We now have a specialized variant of irq_sim_fire() - use it in
gpio-mockup so that we only generate events of types that were
requested with the LINEEVENT ioctl().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index a4c054cf9c5f..18563d2c7876 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -199,7 +199,7 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct gpio_mockup_chip *chip;
 	struct seq_file *sfile;
 	struct gpio_desc *desc;
-	int rv, val;
+	int rv, val, edge;
 
 	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
 	if (rv)
@@ -213,7 +213,8 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	chip = priv->chip;
 
 	gpiod_set_value_cansleep(desc, val);
-	irq_sim_fire(&chip->irqsim, priv->offset);
+	edge = val == 0 ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
+	irq_sim_fire_edge(&chip->irqsim, priv->offset, edge);
 
 	return size;
 }
-- 
2.19.1

Re: [PATCH 2/2] gpio: mockup: use irq_sim_fire_edge()

[Uwe Kleine-König]

On Tue, Nov 20, 2018 at 02:40:32PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> We now have a specialized variant of irq_sim_fire() - use it in
> gpio-mockup so that we only generate events of types that were
> requested with the LINEEVENT ioctl().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/gpio/gpio-mockup.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
> index a4c054cf9c5f..18563d2c7876 100644
> --- a/drivers/gpio/gpio-mockup.c
> +++ b/drivers/gpio/gpio-mockup.c
> @@ -199,7 +199,7 @@ static ssize_t gpio_mockup_event_write(struct file *file,
>  	struct gpio_mockup_chip *chip;
>  	struct seq_file *sfile;
>  	struct gpio_desc *desc;
> -	int rv, val;
> +	int rv, val, edge;
>  
>  	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
>  	if (rv)
> @@ -213,7 +213,8 @@ static ssize_t gpio_mockup_event_write(struct file *file,
>  	chip = priv->chip;
>  
>  	gpiod_set_value_cansleep(desc, val);
> -	irq_sim_fire(&chip->irqsim, priv->offset);
> +	edge = val == 0 ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> +	irq_sim_fire_edge(&chip->irqsim, priv->offset, edge);

If I write 0 twice into the debugfs file, does it fire two irqs or only
one? I think it fires two but only one would be the right behaviour?!

Best regards
Uwe

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

Re: [PATCH 2/2] gpio: mockup: use irq_sim_fire_edge()

[Uwe Kleine-König]

Hello Bartosz,

On Mon, Dec 03, 2018 at 12:09:16PM +0100, Uwe Kleine-König wrote:
> On Tue, Nov 20, 2018 at 02:40:32PM +0100, Bartosz Golaszewski wrote:
> > @@ -213,7 +213,8 @@ static ssize_t gpio_mockup_event_write(struct file *file,
> >  	chip = priv->chip;
> >  
> >  	gpiod_set_value_cansleep(desc, val);
> > -	irq_sim_fire(&chip->irqsim, priv->offset);
> > +	edge = val == 0 ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > +	irq_sim_fire_edge(&chip->irqsim, priv->offset, edge);
> 
> If I write 0 twice into the debugfs file, does it fire two irqs or only
> one? I think it fires two but only one would be the right behaviour?!

If you still think that patch 1 of this series is the way to go, I think
this objection is still valid. Then you need to check the state of the
line by at least calling (something like) .get_value to determine if the
previous value was different.

Best regards
Uwe

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

Re: [PATCH 2/2] gpio: mockup: use irq_sim_fire_edge()

[Bartosz Golaszewski]

wt., 11 gru 2018 o 15:15 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> Hello Bartosz,
>
> On Mon, Dec 03, 2018 at 12:09:16PM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 20, 2018 at 02:40:32PM +0100, Bartosz Golaszewski wrote:
> > > @@ -213,7 +213,8 @@ static ssize_t gpio_mockup_event_write(struct file *file,
> > >     chip = priv->chip;
> > >
> > >     gpiod_set_value_cansleep(desc, val);
> > > -   irq_sim_fire(&chip->irqsim, priv->offset);
> > > +   edge = val == 0 ? IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
> > > +   irq_sim_fire_edge(&chip->irqsim, priv->offset, edge);
> >
> > If I write 0 twice into the debugfs file, does it fire two irqs or only
> > one? I think it fires two but only one would be the right behaviour?!
>
> If you still think that patch 1 of this series is the way to go, I think
> this objection is still valid. Then you need to check the state of the
> line by at least calling (something like) .get_value to determine if the
> previous value was different.
>

Hi Uwe,

I've already started working on a series improving the entire concept.
I've taken some of your suggestions into account.

Since we're already at rc-6 I'd like to get those upstream despite
there being some disagreements to keep the userspace intact.

For now it will generate two falling edge interrupts when you write 0 twice.

Bart