[PATCH 0/9] gpio: mockup: improve the user-space testing interface

[PATCH 0/9] gpio: mockup: improve the user-space testing interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This series aims at reworking the gpio-mockup debugfs interface. The
reason for that is the fact that certain known problems with this
testing module exist and the user-space tests are broken anyway
after 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).

The first three patches improve the interrupt simulator. The first one
makes the struct irq_chip part of the irq_sim structure so that we can
support multiple instances at once. The second delegates the irq number
mapping to the irq domain subsystem. The third patch provides a helper
that will allow users to check the current configuration of a dummy
interrupt and decide whether it should be fired depending on external
logic.

Next six patches improve the gpio-mockup module. Patches 4-5 have been
reviewed before but missed the last merge window. Patch 6 is there
because we're already breaking the debugfs interface anyway and it
removes a link that has no users. Patches 7-8 are minor tweaks.

Last patch introduces a rework of the debugfs interface. With this
change each mockup chip is represented by a directory named after the
chip's device name under <debugfs mount point>/gpio-mockup/. Each line
is represented by a file using the line's offset as the name under the
chip's directory. Reading from the line's file yields the current
*value*, writing to the line's file changes the current "pull". Default
pull for mockup lines is down. More info on that can be found in the
comment added by this change to the gpio-mockup code.

This is somewhat inspired by the idea of the gpio-simulator by
Uwe Kleine-König except that I strongly belive that when testing
certain user API code paths we should not be using the same paths for
verification. That's why there's a separate interface (debugfs) sharing
as little as possible with the character device that allows to check if
various operations (reading and setting values, events) work as
expected instead of two connected dummy chips sharing the same
interface.

If accepted this will of course require major modification of user-space
tests in libgpiod once upstream.

Bartosz Golaszewski (9):
  irq/irq_sim: don't share the irq_chip structure between simulators
  irq/irq_sim: use irq domain
  irq/irq_sim: provide irq_sim_get_type()
  gpio: mockup: add locking
  gpio: mockup: implement get_multiple()
  gpio: mockup: don't create the debugfs link named after the label
  gpio: mockup: change the type of 'offset' to unsigned int
  gpio: mockup: change the signature of unlocked get/set helpers
  gpio: mockup: rework debugfs interface

 drivers/gpio/gpio-mockup.c | 186 +++++++++++++++++++++++++++++++------
 include/linux/irq_sim.h    |  10 +-
 kernel/irq/irq_sim.c       | 127 ++++++++++++++++++-------
 3 files changed, 262 insertions(+), 61 deletions(-)

-- 
2.19.1

[PATCH 1/9] irq/irq_sim: don't share the irq_chip structure between simulators

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We want to support multiple instances of irq_sim. Make struct irq_chip
part of the irq_sim structure.

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

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index 4500d453a63e..eda132c22b57 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -6,6 +6,7 @@
 #ifndef _LINUX_IRQ_SIM_H
 #define _LINUX_IRQ_SIM_H
 
+#include <linux/irq.h>
 #include <linux/irq_work.h>
 #include <linux/device.h>
 
@@ -25,6 +26,7 @@ struct irq_sim_irq_ctx {
 };
 
 struct irq_sim {
+	struct irq_chip		chip;
 	struct irq_sim_work_ctx	work_ctx;
 	int			irq_base;
 	unsigned int		irq_count;
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 98a20e1594ce..b732e4e2e45b 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -25,12 +25,6 @@ static void irq_sim_irqunmask(struct irq_data *data)
 	irq_ctx->enabled = true;
 }
 
-static struct irq_chip irq_sim_irqchip = {
-	.name		= "irq_sim",
-	.irq_mask	= irq_sim_irqmask,
-	.irq_unmask	= irq_sim_irqunmask,
-};
-
 static void irq_sim_handle_irq(struct irq_work *work)
 {
 	struct irq_sim_work_ctx *work_ctx;
@@ -74,6 +68,10 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 		return sim->irq_base;
 	}
 
+	sim->chip.name = "irq_sim";
+	sim->chip.irq_mask = irq_sim_irqmask;
+	sim->chip.irq_unmask = irq_sim_irqunmask;
+
 	sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
 	if (!sim->work_ctx.pending) {
 		kfree(sim->irqs);
@@ -84,7 +82,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 	for (i = 0; i < num_irqs; i++) {
 		sim->irqs[i].irqnum = sim->irq_base + i;
 		sim->irqs[i].enabled = false;
-		irq_set_chip(sim->irq_base + i, &irq_sim_irqchip);
+		irq_set_chip(sim->irq_base + i, &sim->chip);
 		irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]);
 		irq_set_handler(sim->irq_base + i, &handle_simple_irq);
 		irq_modify_status(sim->irq_base + i,
-- 
2.19.1

[PATCH 2/9] irq/irq_sim: use irq domain

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Delegate the offset to virq number mapping to the provided framework
instead of handling it locally.

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

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index eda132c22b57..b96c2f752320 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -8,6 +8,7 @@
 
 #include <linux/irq.h>
 #include <linux/irq_work.h>
+#include <linux/irqdomain.h>
 #include <linux/device.h>
 
 /*
@@ -21,16 +22,15 @@ struct irq_sim_work_ctx {
 };
 
 struct irq_sim_irq_ctx {
-	int			irqnum;
 	bool			enabled;
 };
 
 struct irq_sim {
 	struct irq_chip		chip;
+	struct irq_domain	*domain;
 	struct irq_sim_work_ctx	work_ctx;
-	int			irq_base;
+	int			virq_base;
 	unsigned int		irq_count;
-	struct irq_sim_irq_ctx	*irqs;
 };
 
 int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index b732e4e2e45b..2bcdbab1bc5a 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -44,6 +44,43 @@ static void irq_sim_handle_irq(struct irq_work *work)
 	}
 }
 
+static int irq_sim_domain_map(struct irq_domain *domain,
+			      unsigned int virq, irq_hw_number_t hw)
+{
+	struct irq_sim *sim = domain->host_data;
+	struct irq_sim_irq_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	irq_set_chip(virq, &sim->chip);
+	irq_set_chip_data(virq, ctx);
+	irq_set_handler(virq, handle_simple_irq);
+	irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
+
+	return 0;
+}
+
+static void irq_sim_domain_free(struct irq_domain *domain,
+				unsigned int virq, unsigned int num_irqs)
+{
+	struct irq_sim_irq_ctx *ctx;
+	struct irq_data *irq;
+	int i;
+
+	for (i = 0; i < num_irqs; i++) {
+		irq = irq_domain_get_irq_data(domain, virq + i);
+		ctx = irq_data_get_irq_chip_data(irq);
+		kfree(ctx);
+	}
+}
+
+static const struct irq_domain_ops irq_sim_domain_ops = {
+	.map = irq_sim_domain_map,
+	.free = irq_sim_domain_free,
+};
+
 /**
  * irq_sim_init - Initialize the interrupt simulator: allocate a range of
  *                dummy interrupts.
@@ -56,16 +93,15 @@ static void irq_sim_handle_irq(struct irq_work *work)
  */
 int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 {
-	int i;
-
-	sim->irqs = kmalloc_array(num_irqs, sizeof(*sim->irqs), GFP_KERNEL);
-	if (!sim->irqs)
+	sim->virq_base = irq_alloc_descs(-1, 0, num_irqs, 0);
+	if (sim->virq_base < 0)
+		return sim->virq_base;
+
+	sim->domain = irq_domain_add_legacy(NULL, num_irqs, sim->virq_base,
+					    0, &irq_sim_domain_ops, sim);
+	if (!sim->domain) {
+		irq_free_descs(sim->virq_base, num_irqs);
 		return -ENOMEM;
-
-	sim->irq_base = irq_alloc_descs(-1, 0, num_irqs, 0);
-	if (sim->irq_base < 0) {
-		kfree(sim->irqs);
-		return sim->irq_base;
 	}
 
 	sim->chip.name = "irq_sim";
@@ -74,25 +110,15 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 
 	sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
 	if (!sim->work_ctx.pending) {
-		kfree(sim->irqs);
-		irq_free_descs(sim->irq_base, num_irqs);
+		irq_domain_remove(sim->domain);
+		irq_free_descs(sim->virq_base, num_irqs);
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < num_irqs; i++) {
-		sim->irqs[i].irqnum = sim->irq_base + i;
-		sim->irqs[i].enabled = false;
-		irq_set_chip(sim->irq_base + i, &sim->chip);
-		irq_set_chip_data(sim->irq_base + i, &sim->irqs[i]);
-		irq_set_handler(sim->irq_base + i, &handle_simple_irq);
-		irq_modify_status(sim->irq_base + i,
-				  IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
-	}
-
 	init_irq_work(&sim->work_ctx.work, irq_sim_handle_irq);
 	sim->irq_count = num_irqs;
 
-	return sim->irq_base;
+	return sim->virq_base;
 }
 EXPORT_SYMBOL_GPL(irq_sim_init);
 
@@ -106,8 +132,8 @@ void irq_sim_fini(struct irq_sim *sim)
 {
 	irq_work_sync(&sim->work_ctx.work);
 	bitmap_free(sim->work_ctx.pending);
-	irq_free_descs(sim->irq_base, sim->irq_count);
-	kfree(sim->irqs);
+	irq_domain_free_irqs(sim->virq_base, sim->irq_count);
+	irq_domain_remove(sim->domain);
 }
 EXPORT_SYMBOL_GPL(irq_sim_fini);
 
@@ -151,6 +177,20 @@ int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
 }
 EXPORT_SYMBOL_GPL(devm_irq_sim_init);
 
+static struct irq_sim_irq_ctx *
+irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
+{
+	struct irq_sim_irq_ctx *ctx;
+	struct irq_data *irq;
+	int virq;
+
+	virq = irq_find_mapping(sim->domain, offset);
+	irq = irq_domain_get_irq_data(sim->domain, virq);
+	ctx = irq_data_get_irq_chip_data(irq);
+
+	return ctx;
+}
+
 /**
  * irq_sim_fire - Enqueue an interrupt.
  *
@@ -159,7 +199,9 @@ EXPORT_SYMBOL_GPL(devm_irq_sim_init);
  */
 void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
 {
-	if (sim->irqs[offset].enabled) {
+	struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
+
+	if (ctx->enabled) {
 		set_bit(offset, sim->work_ctx.pending);
 		irq_work_queue(&sim->work_ctx.work);
 	}
@@ -175,6 +217,6 @@ EXPORT_SYMBOL_GPL(irq_sim_fire);
  */
 int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset)
 {
-	return sim->irqs[offset].irqnum;
+	return irq_find_mapping(sim->domain, offset);
 }
 EXPORT_SYMBOL_GPL(irq_sim_irqnum);
-- 
2.19.1

[PATCH 3/9] irq/irq_sim: provide irq_sim_get_type()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide a helper that allows users to retrieve the configured flow type
of dummy interrupts. That allows certain users to decide whether an irq
needs to be fired depending on its edge/level/... configuration.

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

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index b96c2f752320..782dfc599632 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -23,6 +23,7 @@ struct irq_sim_work_ctx {
 
 struct irq_sim_irq_ctx {
 	bool			enabled;
+	unsigned int		type;
 };
 
 struct irq_sim {
@@ -39,5 +40,6 @@ int devm_irq_sim_init(struct device *dev, struct irq_sim *sim,
 void irq_sim_fini(struct irq_sim *sim);
 void irq_sim_fire(struct irq_sim *sim, unsigned int offset);
 int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
+int irq_sim_get_type(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 2bcdbab1bc5a..9168e7100559 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -25,6 +25,15 @@ 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->type = type;
+
+	return 0;
+}
+
 static void irq_sim_handle_irq(struct irq_work *work)
 {
 	struct irq_sim_work_ctx *work_ctx;
@@ -107,6 +116,7 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs)
 	sim->chip.name = "irq_sim";
 	sim->chip.irq_mask = irq_sim_irqmask;
 	sim->chip.irq_unmask = irq_sim_irqunmask;
+	sim->chip.irq_set_type = irq_sim_set_type;
 
 	sim->work_ctx.pending = bitmap_zalloc(num_irqs, GFP_KERNEL);
 	if (!sim->work_ctx.pending) {
@@ -220,3 +230,18 @@ int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset)
 	return irq_find_mapping(sim->domain, offset);
 }
 EXPORT_SYMBOL_GPL(irq_sim_irqnum);
+
+/**
+ * irq_sim_get_type - Get the configured flow type of a dummy interrupt.
+ *
+ * @sim:        The interrupt simulator object.
+ * @offset:     Offset of the simulated interrupt for which to retrieve
+ *              the type.
+ */
+int irq_sim_get_type(struct irq_sim *sim, unsigned int offset)
+{
+	struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
+
+	return ctx->type;
+}
+EXPORT_SYMBOL_GPL(irq_sim_get_type);
-- 
2.19.1

Re: [PATCH 3/9] irq/irq_sim: provide irq_sim_get_type()

[Uwe Kleine-König]

Hello Bartosz,

On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote:
> Provide a helper that allows users to retrieve the configured flow type
> of dummy interrupts. That allows certain users to decide whether an irq
> needs to be fired depending on its edge/level/... configuration.

You don't talk about the .set_type callback here; is this intended?

I wonder how you think this should be used. Assume the mockup-driver is
directed to pull up a certain line, does it do something like that:

	def mockup_setval(self, val):
		irqtype = irq_sim_get_type(...)
		if irqtype == LEVEL_HIGH:
			if val:
				fire_irq()

		else if irqtype == EDGE_RISING:
			if val and not prev_val:
				fire_irq()

		else if irqtype == LEVEL_LOW:
			if not val:
				fire_irq()

		else if irqtype == EDGE_FALLING:
			if not val and prev_val:
				fire_irq()

I wonder if that logic should be done in the same place as where the irq
type is stored. Otherwise that .type member is only a data store
provided by the irq simulator. So I suggest to either move the variable
that mirrors the current level of the line into the irq simulator, or
keep the irqtype variable in the mockup driver. Both approaches would
make it unnecessary to provide an accessor function for the type member.

Best regards
Uwe

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

Re: [PATCH 3/9] irq/irq_sim: provide irq_sim_get_type()

[Bartosz Golaszewski]

śr., 23 sty 2019 o 20:18 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> Hello Bartosz,
>
> On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote:
> > Provide a helper that allows users to retrieve the configured flow type
> > of dummy interrupts. That allows certain users to decide whether an irq
> > needs to be fired depending on its edge/level/... configuration.
>
> You don't talk about the .set_type callback here; is this intended?
>
> I wonder how you think this should be used. Assume the mockup-driver is
> directed to pull up a certain line, does it do something like that:
>
>         def mockup_setval(self, val):
>                 irqtype = irq_sim_get_type(...)
>                 if irqtype == LEVEL_HIGH:
>                         if val:
>                                 fire_irq()
>
>                 else if irqtype == EDGE_RISING:
>                         if val and not prev_val:
>                                 fire_irq()
>
>                 else if irqtype == LEVEL_LOW:
>                         if not val:
>                                 fire_irq()
>
>                 else if irqtype == EDGE_FALLING:
>                         if not val and prev_val:
>                                 fire_irq()
>
> I wonder if that logic should be done in the same place as where the irq
> type is stored. Otherwise that .type member is only a data store
> provided by the irq simulator. So I suggest to either move the variable
> that mirrors the current level of the line into the irq simulator, or
> keep the irqtype variable in the mockup driver. Both approaches would
> make it unnecessary to provide an accessor function for the type member.
>

Yeah, might be better to go back to my previous idea of adding
irq_sim_fire_edge(), but maybe it should be irq_sim_fire_type()
instead, so that irq_sim_fire() fires unconditionally and
irq_sim_fire_type() would fire only if the passed flag is the same as
the one previously configured by the set_type() callback.

Thanks,
Bartosz

Re: [PATCH 3/9] irq/irq_sim: provide irq_sim_get_type()

[Uwe Kleine-König]

On Thu, Jan 24, 2019 at 08:46:56AM +0100, Bartosz Golaszewski wrote:
> śr., 23 sty 2019 o 20:18 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > Hello Bartosz,
> >
> > On Wed, Jan 23, 2019 at 03:15:32PM +0100, Bartosz Golaszewski wrote:
> > > Provide a helper that allows users to retrieve the configured flow type
> > > of dummy interrupts. That allows certain users to decide whether an irq
> > > needs to be fired depending on its edge/level/... configuration.
> >
> > You don't talk about the .set_type callback here; is this intended?
> >
> > I wonder how you think this should be used. Assume the mockup-driver is
> > directed to pull up a certain line, does it do something like that:
> >
> >         def mockup_setval(self, val):
> >                 irqtype = irq_sim_get_type(...)
> >                 if irqtype == LEVEL_HIGH:
> >                         if val:
> >                                 fire_irq()
> >
> >                 else if irqtype == EDGE_RISING:
> >                         if val and not prev_val:
> >                                 fire_irq()
> >
> >                 else if irqtype == LEVEL_LOW:
> >                         if not val:
> >                                 fire_irq()
> >
> >                 else if irqtype == EDGE_FALLING:
> >                         if not val and prev_val:
> >                                 fire_irq()
> >
> > I wonder if that logic should be done in the same place as where the irq
> > type is stored. Otherwise that .type member is only a data store
> > provided by the irq simulator. So I suggest to either move the variable
> > that mirrors the current level of the line into the irq simulator, or
> > keep the irqtype variable in the mockup driver. Both approaches would
> > make it unnecessary to provide an accessor function for the type member.
> >
> 
> Yeah, might be better to go back to my previous idea of adding
> irq_sim_fire_edge(), but maybe it should be irq_sim_fire_type()
> instead, so that irq_sim_fire() fires unconditionally and
> irq_sim_fire_type() would fire only if the passed flag is the same as
> the one previously configured by the set_type() callback.

How (if at all) do you intend to support level sensitive irqs with this
interface? It probably works (but I didn't thought it through
completely), but firing a LEVEL_HIGH sensitive irq on

	irq_sim_fire_type(EDGE_RISING)

might look at least surprising and needs proper comments and thoughts.

Best regards
Uwe

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

[PATCH 4/9] gpio: mockup: add locking

[Bartosz Golaszewski]

While no user reported any race condition problems with gpio-mockup,
let's be on the safe side and use a mutex when performing any changes
on the dummy chip structures.

Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 6a50f9f59c90..b4c1de6acf74 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -54,6 +54,7 @@ struct gpio_mockup_chip {
 	struct gpio_mockup_line_status *lines;
 	struct irq_sim irqsim;
 	struct dentry *dbg_dir;
+	struct mutex lock;
 };
 
 struct gpio_mockup_dbgfs_private {
@@ -82,29 +83,53 @@ static int gpio_mockup_range_ngpio(unsigned int index)
 	return gpio_mockup_ranges[index * 2 + 1];
 }
 
-static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	return chip->lines[offset].value;
 }
 
-static void gpio_mockup_set(struct gpio_chip *gc,
-			    unsigned int offset, int value)
+static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	int val;
+
+	mutex_lock(&chip->lock);
+	val = __gpio_mockup_get(gc, offset);
+	mutex_unlock(&chip->lock);
+
+	return val;
+}
+
+static void __gpio_mockup_set(struct gpio_chip *gc,
+			      unsigned int offset, int value)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	chip->lines[offset].value = !!value;
 }
 
+static void gpio_mockup_set(struct gpio_chip *gc,
+			   unsigned int offset, int value)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+
+	mutex_lock(&chip->lock);
+	__gpio_mockup_set(gc, offset, value);
+	mutex_unlock(&chip->lock);
+}
+
 static void gpio_mockup_set_multiple(struct gpio_chip *gc,
 				     unsigned long *mask, unsigned long *bits)
 {
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 	unsigned int bit;
 
+	mutex_lock(&chip->lock);
 	for_each_set_bit(bit, mask, gc->ngpio)
-		gpio_mockup_set(gc, bit, test_bit(bit, bits));
-
+		__gpio_mockup_set(gc, bit, test_bit(bit, bits));
+	mutex_unlock(&chip->lock);
 }
 
 static int gpio_mockup_dirout(struct gpio_chip *gc,
@@ -112,8 +137,10 @@ static int gpio_mockup_dirout(struct gpio_chip *gc,
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
-	gpio_mockup_set(gc, offset, value);
+	mutex_lock(&chip->lock);
 	chip->lines[offset].dir = GPIO_MOCKUP_DIR_OUT;
+	__gpio_mockup_set(gc, offset, value);
+	mutex_unlock(&chip->lock);
 
 	return 0;
 }
@@ -122,7 +149,9 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
+	mutex_lock(&chip->lock);
 	chip->lines[offset].dir = GPIO_MOCKUP_DIR_IN;
+	mutex_unlock(&chip->lock);
 
 	return 0;
 }
@@ -130,8 +159,13 @@ static int gpio_mockup_dirin(struct gpio_chip *gc, unsigned int offset)
 static int gpio_mockup_get_direction(struct gpio_chip *gc, unsigned int offset)
 {
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	int direction;
 
-	return !chip->lines[offset].dir;
+	mutex_lock(&chip->lock);
+	direction = !chip->lines[offset].dir;
+	mutex_unlock(&chip->lock);
+
+	return direction;
 }
 
 static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)
@@ -283,6 +317,8 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 			return -ENOMEM;
 	}
 
+	mutex_init(&chip->lock);
+
 	gc = &chip->gc;
 	gc->base = base;
 	gc->ngpio = ngpio;
-- 
2.19.1

[PATCH 5/9] gpio: mockup: implement get_multiple()

[Bartosz Golaszewski]

We already support set_multiple(). Implement get_multiple() as well.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index b4c1de6acf74..1c945c967f60 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -102,6 +102,22 @@ static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 	return val;
 }
 
+static int gpio_mockup_get_multiple(struct gpio_chip *gc,
+				    unsigned long *mask, unsigned long *bits)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+	unsigned int bit, val;
+
+	mutex_lock(&chip->lock);
+	for_each_set_bit(bit, mask, gc->ngpio) {
+		val = __gpio_mockup_get(gc, bit);
+		__assign_bit(bit, bits, val);
+	}
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
 static void __gpio_mockup_set(struct gpio_chip *gc,
 			      unsigned int offset, int value)
 {
@@ -327,6 +343,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	gc->parent = dev;
 	gc->get = gpio_mockup_get;
 	gc->set = gpio_mockup_set;
+	gc->get_multiple = gpio_mockup_get_multiple;
 	gc->set_multiple = gpio_mockup_set_multiple;
 	gc->direction_output = gpio_mockup_dirout;
 	gc->direction_input = gpio_mockup_dirin;
-- 
2.19.1

[PATCH 6/9] gpio: mockup: don't create the debugfs link named after the label

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

User-space tests no longer use it and we're breaking the interface
anyway.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 1c945c967f60..0317917a3678 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -234,7 +234,7 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 				      struct gpio_mockup_chip *chip)
 {
 	struct gpio_mockup_dbgfs_private *priv;
-	struct dentry *evfile, *link;
+	struct dentry *evfile;
 	struct gpio_chip *gc;
 	const char *devname;
 	char *name;
@@ -247,10 +247,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 	if (IS_ERR_OR_NULL(chip->dbg_dir))
 		goto err;
 
-	link = debugfs_create_symlink(gc->label, gpio_mockup_dbg_dir, devname);
-	if (IS_ERR_OR_NULL(link))
-		goto err;
-
 	for (i = 0; i < gc->ngpio; i++) {
 		name = devm_kasprintf(dev, GFP_KERNEL, "%d", i);
 		if (!name)
-- 
2.19.1

[PATCH 7/9] gpio: mockup: change the type of 'offset' to unsigned int

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This field can never be negative.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 0317917a3678..433adb3b4617 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -60,7 +60,7 @@ struct gpio_mockup_chip {
 struct gpio_mockup_dbgfs_private {
 	struct gpio_mockup_chip *chip;
 	struct gpio_desc *desc;
-	int offset;
+	unsigned int offset;
 };
 
 static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_RANGES];
-- 
2.19.1

[PATCH 8/9] gpio: mockup: change the signature of unlocked get/set helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The unlocked variants only get called from places where we already have
the pointer to the underlying gpio_mockup_chip structure, so take it
as parameter instead of struct gpio_chip.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 433adb3b4617..c498b0fbbec8 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -83,10 +83,9 @@ static int gpio_mockup_range_ngpio(unsigned int index)
 	return gpio_mockup_ranges[index * 2 + 1];
 }
 
-static int __gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
+static int __gpio_mockup_get(struct gpio_mockup_chip *chip,
+			     unsigned int offset)
 {
-	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
-
 	return chip->lines[offset].value;
 }
 
@@ -96,7 +95,7 @@ static int gpio_mockup_get(struct gpio_chip *gc, unsigned int offset)
 	int val;
 
 	mutex_lock(&chip->lock);
-	val = __gpio_mockup_get(gc, offset);
+	val = __gpio_mockup_get(chip, offset);
 	mutex_unlock(&chip->lock);
 
 	return val;
@@ -110,7 +109,7 @@ static int gpio_mockup_get_multiple(struct gpio_chip *gc,
 
 	mutex_lock(&chip->lock);
 	for_each_set_bit(bit, mask, gc->ngpio) {
-		val = __gpio_mockup_get(gc, bit);
+		val = __gpio_mockup_get(chip, bit);
 		__assign_bit(bit, bits, val);
 	}
 	mutex_unlock(&chip->lock);
@@ -118,11 +117,9 @@ static int gpio_mockup_get_multiple(struct gpio_chip *gc,
 	return 0;
 }
 
-static void __gpio_mockup_set(struct gpio_chip *gc,
+static void __gpio_mockup_set(struct gpio_mockup_chip *chip,
 			      unsigned int offset, int value)
 {
-	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
-
 	chip->lines[offset].value = !!value;
 }
 
@@ -132,7 +129,7 @@ static void gpio_mockup_set(struct gpio_chip *gc,
 	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
 
 	mutex_lock(&chip->lock);
-	__gpio_mockup_set(gc, offset, value);
+	__gpio_mockup_set(chip, offset, value);
 	mutex_unlock(&chip->lock);
 }
 
@@ -144,7 +141,7 @@ static void gpio_mockup_set_multiple(struct gpio_chip *gc,
 
 	mutex_lock(&chip->lock);
 	for_each_set_bit(bit, mask, gc->ngpio)
-		__gpio_mockup_set(gc, bit, test_bit(bit, bits));
+		__gpio_mockup_set(chip, bit, test_bit(bit, bits));
 	mutex_unlock(&chip->lock);
 }
 
@@ -155,7 +152,7 @@ static int gpio_mockup_dirout(struct gpio_chip *gc,
 
 	mutex_lock(&chip->lock);
 	chip->lines[offset].dir = GPIO_MOCKUP_DIR_OUT;
-	__gpio_mockup_set(gc, offset, value);
+	__gpio_mockup_set(chip, offset, value);
 	mutex_unlock(&chip->lock);
 
 	return 0;
-- 
2.19.1

[PATCH 9/9] gpio: mockup: rework debugfs interface

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Modify the way the debugfs interface works in gpio-mockup. Introduce
the concept of dummy pull config which will keep the mockup lines in
known state. The pull values can be modified by writing to the debugfs
files corresponding to lines. Lines in input mode always report the
current pull value, lines in output mode change the line value but
it will revert back to the one specified by current pull when released.

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

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index c498b0fbbec8..a981ecd7c291 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -47,6 +47,7 @@ enum {
 struct gpio_mockup_line_status {
 	int dir;
 	int value;
+	int pull;
 };
 
 struct gpio_mockup_chip {
@@ -188,15 +189,56 @@ static int gpio_mockup_to_irq(struct gpio_chip *gc, unsigned int offset)
 	return irq_sim_irqnum(&chip->irqsim, offset);
 }
 
-static ssize_t gpio_mockup_event_write(struct file *file,
-				       const char __user *usr_buf,
-				       size_t size, loff_t *ppos)
+static void gpio_mockup_free(struct gpio_chip *gc, unsigned int offset)
+{
+	struct gpio_mockup_chip *chip = gpiochip_get_data(gc);
+
+	__gpio_mockup_set(chip, offset, chip->lines[offset].pull);
+}
+
+static ssize_t gpio_mockup_debugfs_read(struct file *file,
+					char __user *usr_buf,
+					size_t size, loff_t *ppos)
+{
+	struct gpio_mockup_dbgfs_private *priv;
+	struct gpio_mockup_chip *chip;
+	struct seq_file *sfile;
+	struct gpio_chip *gc;
+	char buf[3];
+	int val, rv;
+
+	if (*ppos != 0)
+		return 0;
+
+	sfile = file->private_data;
+	priv = sfile->private;
+	chip = priv->chip;
+	gc = &chip->gc;
+
+	val = gpio_mockup_get(gc, priv->offset);
+	snprintf(buf, sizeof(buf), "%d\n", val);
+
+	rv = copy_to_user(usr_buf, buf, sizeof(buf));
+	if (rv)
+		return rv;
+
+	return sizeof(buf) - 1;
+}
+
+static ssize_t gpio_mockup_debugfs_write(struct file *file,
+					 const char __user *usr_buf,
+					 size_t size, loff_t *ppos)
 {
 	struct gpio_mockup_dbgfs_private *priv;
 	struct gpio_mockup_chip *chip;
 	struct seq_file *sfile;
 	struct gpio_desc *desc;
-	int rv, val;
+	unsigned int irq_type;
+	struct gpio_chip *gc;
+	int rv, val, curr;
+
+	if (*ppos != 0)
+		return -EINVAL;
 
 	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
 	if (rv)
@@ -206,24 +248,67 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 
 	sfile = file->private_data;
 	priv = sfile->private;
-	desc = priv->desc;
 	chip = priv->chip;
+	gc = &chip->gc;
+	desc = &gc->gpiodev->descs[priv->offset];
+
+	mutex_lock(&chip->lock);
+
+	if (!test_bit(FLAG_IS_OUT, &desc->flags)) {
+		if (test_bit(FLAG_REQUESTED, &desc->flags)) {
+			curr = __gpio_mockup_get(chip, priv->offset);
+			if (curr == val)
+				goto out;
 
-	gpiod_set_value_cansleep(desc, val);
-	irq_sim_fire(&chip->irqsim, priv->offset);
+			irq_type = irq_sim_get_type(&chip->irqsim,
+						    priv->offset);
+
+			if ((val == 0 && (irq_type & IRQ_TYPE_EDGE_FALLING)) ||
+			    (val == 1 && (irq_type & IRQ_TYPE_EDGE_RISING)))
+				irq_sim_fire(&chip->irqsim, priv->offset);
+		}
+
+		__gpio_mockup_set(chip, priv->offset, val);
+	}
+
+out:
+	chip->lines[priv->offset].pull = val;
+	mutex_unlock(&chip->lock);
 
 	return size;
 }
 
-static int gpio_mockup_event_open(struct inode *inode, struct file *file)
+static int gpio_mockup_debugfs_open(struct inode *inode, struct file *file)
 {
 	return single_open(file, NULL, inode->i_private);
 }
 
-static const struct file_operations gpio_mockup_event_ops = {
+/*
+ * Each mockup chip is represented by a directory named after the chip's device
+ * name under /sys/kernel/debug/gpio-mockup/. Each line is represented by
+ * a file using the line's offset as the name under the chip's directory.
+ *
+ * Reading from the line's file yields the current *value*, writing to the
+ * line's file changes the current *pull*. Default pull for mockup lines is
+ * down.
+ *
+ * Examples:
+ * - when a line pulled down is requested in output mode and driven high, its
+ *   value will return to 0 once it's released
+ * - when the line is requested in output mode and driven high, writing 0 to
+ *   the corresponding debugfs file will change the pull to down but the
+ *   reported value will still be 1 until the line is released
+ * - line requested in input mode always reports the same value as its pull
+ *   configuration
+ * - when the line is requested in input mode and monitored for events, writing
+ *   the same value to the debugfs file will be a noop, while writing the
+ *   opposite value will generate a dummy interrupt with an appropriate edge
+ */
+static const struct file_operations gpio_mockup_debugfs_ops = {
 	.owner = THIS_MODULE,
-	.open = gpio_mockup_event_open,
-	.write = gpio_mockup_event_write,
+	.open = gpio_mockup_debugfs_open,
+	.read = gpio_mockup_debugfs_read,
+	.write = gpio_mockup_debugfs_write,
 	.llseek = no_llseek,
 };
 
@@ -258,7 +343,7 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 		priv->desc = &gc->gpiodev->descs[i];
 
 		evfile = debugfs_create_file(name, 0200, chip->dbg_dir, priv,
-					     &gpio_mockup_event_ops);
+					     &gpio_mockup_debugfs_ops);
 		if (IS_ERR_OR_NULL(evfile))
 			goto err;
 	}
@@ -266,7 +351,7 @@ static void gpio_mockup_debugfs_setup(struct device *dev,
 	return;
 
 err:
-	dev_err(dev, "error creating debugfs event files\n");
+	dev_err(dev, "error creating debugfs files\n");
 }
 
 static int gpio_mockup_name_lines(struct device *dev,
@@ -342,6 +427,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	gc->direction_input = gpio_mockup_dirin;
 	gc->get_direction = gpio_mockup_get_direction;
 	gc->to_irq = gpio_mockup_to_irq;
+	gc->free = gpio_mockup_free;
 
 	chip->lines = devm_kcalloc(dev, gc->ngpio,
 				   sizeof(*chip->lines), GFP_KERNEL);
@@ -415,7 +501,7 @@ static int __init gpio_mockup_init(void)
 			return -EINVAL;
 	}
 
-	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup-event", NULL);
+	gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL);
 	if (IS_ERR_OR_NULL(gpio_mockup_dbg_dir))
 		gpio_mockup_err("error creating debugfs directory\n");
 
-- 
2.19.1

Re: [PATCH 0/9] gpio: mockup: improve the user-space testing interface

[Linus Walleij]

On Wed, Jan 23, 2019 at 3:15 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This series aims at reworking the gpio-mockup debugfs interface. The
> reason for that is the fact that certain known problems with this
> testing module exist and the user-space tests are broken anyway
> after 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).
>
> The first three patches improve the interrupt simulator. The first one
> makes the struct irq_chip part of the irq_sim structure so that we can
> support multiple instances at once. The second delegates the irq number
> mapping to the irq domain subsystem. The third patch provides a helper
> that will allow users to check the current configuration of a dummy
> interrupt and decide whether it should be fired depending on external
> logic.
>
> Next six patches improve the gpio-mockup module. Patches 4-5 have been
> reviewed before but missed the last merge window. Patch 6 is there
> because we're already breaking the debugfs interface anyway and it
> removes a link that has no users. Patches 7-8 are minor tweaks.
>
> Last patch introduces a rework of the debugfs interface. With this
> change each mockup chip is represented by a directory named after the
> chip's device name under <debugfs mount point>/gpio-mockup/. Each line
> is represented by a file using the line's offset as the name under the
> chip's directory. Reading from the line's file yields the current
> *value*, writing to the line's file changes the current "pull". Default
> pull for mockup lines is down. More info on that can be found in the
> comment added by this change to the gpio-mockup code.
>
> This is somewhat inspired by the idea of the gpio-simulator by
> Uwe Kleine-König except that I strongly belive that when testing
> certain user API code paths we should not be using the same paths for
> verification. That's why there's a separate interface (debugfs) sharing
> as little as possible with the character device that allows to check if
> various operations (reading and setting values, events) work as
> expected instead of two connected dummy chips sharing the same
> interface.
>
> If accepted this will of course require major modification of user-space
> tests in libgpiod once upstream.

I like how this is developing and how I see the two major contributors
to simulated GPIOs cooperating on this :)

It'd be nice to get Marc's feedback on the irqchip changes so we
can solidify that part.

Yours,
Linus Walleij