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

[PATCH v2 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 specify the type of the fired interrupt so that
the configuration set by the set_type() callback can be taken into account.

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.

v1 -> v2:
- instead of providing the irq_sim_get_type() helper, move the irq type
  logic into the simulator and provide a helper that allows users to specify
  the type of the fired interrupt

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_fire_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 | 185 +++++++++++++++++++++++++++++++------
 include/linux/irq_sim.h    |  17 +++-
 kernel/irq/irq_sim.c       | 126 ++++++++++++++++++-------
 3 files changed, 263 insertions(+), 65 deletions(-)

-- 
2.19.1

[PATCH v2 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 v2 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. Use the legacy domain as we want to
preallocate the irq descriptors.

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 v2 3/9] irq/irq_sim: provide irq_sim_fire_type()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Provide a more specialized variant of irq_sim_fire() that allows to
specify the type of the fired interrupt. The type is stored in the
dummy irq context struct via the set_type callback.

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

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index b96c2f752320..647a6c8ffb31 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 {
@@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
 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_type(struct irq_sim *sim,
+		       unsigned int offset, unsigned int type);
 int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
 
+static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
+{
+	irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
+}
+
 #endif /* _LINUX_IRQ_SIM_H */
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 2bcdbab1bc5a..e3160b5e59b8 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) {
@@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
 }
 
 /**
- * irq_sim_fire - Enqueue an interrupt.
+ * irq_sim_fire_type - Enqueue an interrupt.
  *
  * @sim:        The interrupt simulator object.
  * @offset:     Offset of the simulated interrupt which should be fired.
+ * @type:	Type of the fired interrupt. Must be one of the following:
+ *		IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
+ *		IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
+ *		IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
  */
-void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
+void irq_sim_fire_type(struct irq_sim *sim,
+		       unsigned int offset, unsigned int type)
 {
 	struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
 
-	if (ctx->enabled) {
+	/* Only care about relevant flags. */
+	type &= IRQ_TYPE_SENSE_MASK;
+
+	if (ctx->enabled && (ctx->type & type)) {
 		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_type);
 
 /**
  * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
-- 
2.19.1

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

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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 <bgolaszewski@baylibre.com>
---
 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 v2 5/9] gpio: mockup: implement get_multiple()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 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 v2 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 v2 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 v2 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 v2 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 | 113 ++++++++++++++++++++++++++++++++-----
 1 file changed, 99 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index c498b0fbbec8..0b8de6e127eb 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,66 @@ 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);
 
-	gpiod_set_value_cansleep(desc, val);
-	irq_sim_fire(&chip->irqsim, priv->offset);
+	if (test_bit(FLAG_REQUESTED, &desc->flags) &&
+	    !test_bit(FLAG_IS_OUT, &desc->flags)) {
+		curr = __gpio_mockup_get(chip, priv->offset);
+		if (curr == val)
+			goto out;
+
+		irq_type = val == 0 ? IRQ_TYPE_EDGE_FALLING
+				    : IRQ_TYPE_EDGE_RISING;
+		irq_sim_fire_type(&chip->irqsim, priv->offset, irq_type);
+	}
+
+	/* Change the value unless we're actively driving the line. */
+	if (!test_bit(FLAG_REQUESTED, &desc->flags) ||
+	    !test_bit(FLAG_IS_OUT, &desc->flags))
+		__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 +342,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 +350,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 +426,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 +500,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 v2 3/9] irq/irq_sim: provide irq_sim_fire_type()

[Uwe Kleine-König]

Hello Bartosz,

On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote:
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> +void irq_sim_fire_type(struct irq_sim *sim,
> +		       unsigned int offset, unsigned int type)
>  {
>  	struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
>  
> -	if (ctx->enabled) {
> +	/* Only care about relevant flags. */
> +	type &= IRQ_TYPE_SENSE_MASK;
> +
> +	if (ctx->enabled && (ctx->type & type)) {
>  		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_type);

This looks better than the previous variant. I wonder if it would be
still more sensible to have type only in the mockup driver. But I don't
have the complete picture here and it might be easier this way.

Best regards
Uwe

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

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

[Bartosz Golaszewski]

wt., 29 sty 2019 o 10:07 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> Hello Bartosz,
>
> On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote:
> > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > +void irq_sim_fire_type(struct irq_sim *sim,
> > +                    unsigned int offset, unsigned int type)
> >  {
> >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> >
> > -     if (ctx->enabled) {
> > +     /* Only care about relevant flags. */
> > +     type &= IRQ_TYPE_SENSE_MASK;
> > +
> > +     if (ctx->enabled && (ctx->type & type)) {
> >               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_type);
>
> This looks better than the previous variant. I wonder if it would be
> still more sensible to have type only in the mockup driver. But I don't
> have the complete picture here and it might be easier this way.
>

I'm afraid I don't follow. Wasn't that the way it was done in v1?

Bart

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

[Uwe Kleine-König]

On Tue, Jan 29, 2019 at 12:01:37PM +0100, Bartosz Golaszewski wrote:
> wt., 29 sty 2019 o 10:07 Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> napisał(a):
> >
> > Hello Bartosz,
> >
> > On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote:
> > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > +void irq_sim_fire_type(struct irq_sim *sim,
> > > +                    unsigned int offset, unsigned int type)
> > >  {
> > >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> > >
> > > -     if (ctx->enabled) {
> > > +     /* Only care about relevant flags. */
> > > +     type &= IRQ_TYPE_SENSE_MASK;
> > > +
> > > +     if (ctx->enabled && (ctx->type & type)) {
> > >               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_type);
> >
> > This looks better than the previous variant. I wonder if it would be
> > still more sensible to have type only in the mockup driver. But I don't
> > have the complete picture here and it might be easier this way.
> >
> 
> I'm afraid I don't follow. Wasn't that the way it was done in v1?

No, in v1 you already had "type" in the irq_sim driver and the logic if
the irq should trigger in mockup. My wondering is about having both in
the mockup driver.

Then you have the tracking of the line's level and the logic if it should
trigger an irq in a single place.

But as I said, I'm not sure if this is better than your proposed
solution in v2.

Best regards
Uwe

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

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

[Bartosz Golaszewski]

wt., 29 sty 2019 o 13:55 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Tue, Jan 29, 2019 at 12:01:37PM +0100, Bartosz Golaszewski wrote:
> > wt., 29 sty 2019 o 10:07 Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> napisał(a):
> > >
> > > Hello Bartosz,
> > >
> > > On Tue, Jan 29, 2019 at 09:44:05AM +0100, Bartosz Golaszewski wrote:
> > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > > +void irq_sim_fire_type(struct irq_sim *sim,
> > > > +                    unsigned int offset, unsigned int type)
> > > >  {
> > > >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> > > >
> > > > -     if (ctx->enabled) {
> > > > +     /* Only care about relevant flags. */
> > > > +     type &= IRQ_TYPE_SENSE_MASK;
> > > > +
> > > > +     if (ctx->enabled && (ctx->type & type)) {
> > > >               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_type);
> > >
> > > This looks better than the previous variant. I wonder if it would be
> > > still more sensible to have type only in the mockup driver. But I don't
> > > have the complete picture here and it might be easier this way.
> > >
> >
> > I'm afraid I don't follow. Wasn't that the way it was done in v1?
>
> No, in v1 you already had "type" in the irq_sim driver and the logic if
> the irq should trigger in mockup. My wondering is about having both in
> the mockup driver.
>
> Then you have the tracking of the line's level and the logic if it should
> trigger an irq in a single place.
>
> But as I said, I'm not sure if this is better than your proposed
> solution in v2.
>

I think this a good compromise in case some other user would need it.
If there are no major objections to it, I'd like to keep it that way.

Bart

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

[Bartosz Golaszewski]

wt., 29 sty 2019 o 09:44 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
>
> 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 specify the type of the fired interrupt so that
> the configuration set by the set_type() callback can be taken into account.
>
> 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.
>
> v1 -> v2:
> - instead of providing the irq_sim_get_type() helper, move the irq type
>   logic into the simulator and provide a helper that allows users to specify
>   the type of the fired interrupt
>
> 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_fire_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 | 185 +++++++++++++++++++++++++++++++------
>  include/linux/irq_sim.h    |  17 +++-
>  kernel/irq/irq_sim.c       | 126 ++++++++++++++++++-------
>  3 files changed, 263 insertions(+), 65 deletions(-)
>
> --
> 2.19.1
>

Marc, Thomas,

if there are no objections - could you please ack the three first
patches and let me take them through the GPIO tree? I would really
appreciate if we could have them in v5.1.

Bart

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

[Marc Zyngier]

On Tue, 29 Jan 2019 09:44:04 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Delegate the offset to virq number mapping to the provided framework
> instead of handling it locally. Use the legacy domain as we want to
> preallocate the irq descriptors.

Why?

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

Consider using modern APIs such as irq_domain_set_info().

> +	irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);

Where is this requirement coming from?

> +
> +	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,

The intended use of the v2 API is to have alloc and free as a pair, and
no map. So please choose which version of the API you're using here.
The safest bet would be to move what you do on map into alloc.

> +};
> +
>  /**
>   * 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);

Why do you need a legacy domain? As far as I can tell, this is new
code, hence it has no legacy.

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


Thanks,

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

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

[Marc Zyngier]

On Tue, 29 Jan 2019 09:44:03 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

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

Why? There is nothing dynamic in an irqchip structure. It is all code
that is set in stone for the lifetime of the irqchip.

I've sure you have a use for such indirection, so why don't you explain
what you have in mind?

Thanks,

	M.

> 
> 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,



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

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

[Marc Zyngier]

On Wed, 6 Feb 2019 11:23:46 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> wt., 29 sty 2019 o 09:44 Bartosz Golaszewski <brgl@bgdev.pl> napisał(a):
> >
> > 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 specify the type of the fired interrupt so that
> > the configuration set by the set_type() callback can be taken into account.
> >
> > 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.
> >
> > v1 -> v2:
> > - instead of providing the irq_sim_get_type() helper, move the irq type
> >   logic into the simulator and provide a helper that allows users to specify
> >   the type of the fired interrupt
> >
> > 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_fire_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 | 185 +++++++++++++++++++++++++++++++------
> >  include/linux/irq_sim.h    |  17 +++-
> >  kernel/irq/irq_sim.c       | 126 ++++++++++++++++++-------
> >  3 files changed, 263 insertions(+), 65 deletions(-)
> >
> > --
> > 2.19.1
> >  
> 
> Marc, Thomas,
> 
> if there are no objections - could you please ack the three first
> patches and let me take them through the GPIO tree? I would really
> appreciate if we could have them in v5.1.

There are certainly a number of questions that have to be answered
before this can be merged. I can't find a rational for patch #1, and
patch #2 looks like a set of misuse of APIs.

At the very least, I'd like to see these questions answered, and the
code in patch #2 fixed.

Thanks,

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

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

[Bartosz Golaszewski]

pon., 11 lut 2019 o 23:28 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> On Tue, 29 Jan 2019 09:44:03 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > 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.
>
> Why? There is nothing dynamic in an irqchip structure. It is all code
> that is set in stone for the lifetime of the irqchip.
>
> I've sure you have a use for such indirection, so why don't you explain
> what you have in mind?
>

No you're right, this can be dropped.

Bart

> Thanks,
>
>         M.
>
> >
> > 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,
>
>
>
> --
> Without deviation from the norm, progress is not possible.

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

[Bartosz Golaszewski]

pon., 11 lut 2019 o 23:26 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> On Tue, 29 Jan 2019 09:44:04 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Delegate the offset to virq number mapping to the provided framework
> > instead of handling it locally. Use the legacy domain as we want to
> > preallocate the irq descriptors.
>
> Why?
>
> >
> > 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);
>
> Consider using modern APIs such as irq_domain_set_info().
>
> > +     irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
>
> Where is this requirement coming from?
>
> > +
> > +     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,
>
> The intended use of the v2 API is to have alloc and free as a pair, and
> no map. So please choose which version of the API you're using here.
> The safest bet would be to move what you do on map into alloc.
>
> > +};
> > +
> >  /**
> >   * 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);
>
> Why do you need a legacy domain? As far as I can tell, this is new
> code, hence it has no legacy.
>
> > +     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);
>
>
> Thanks,
>
>         M.
> --
> Without deviation from the norm, progress is not possible.

Hi Marc,

since we're so late in the release cycle and I really only need patch
3/9 from this series - I'll drop this patch and 1/9 as well in v3. We
can come back to this in the next cycle.

Bart

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

[Marc Zyngier]

Bartosz,

On Tue, 12 Feb 2019 08:30:32 +0000,
Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote:
> 
> pon., 11 lut 2019 o 23:26 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
> >
> > On Tue, 29 Jan 2019 09:44:04 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Delegate the offset to virq number mapping to the provided framework
> > > instead of handling it locally. Use the legacy domain as we want to
> > > preallocate the irq descriptors.
> >
> > Why?
> >
> > >
> > > 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);
> >
> > Consider using modern APIs such as irq_domain_set_info().
> >
> > > +     irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> >
> > Where is this requirement coming from?
> >
> > > +
> > > +     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,
> >
> > The intended use of the v2 API is to have alloc and free as a pair, and
> > no map. So please choose which version of the API you're using here.
> > The safest bet would be to move what you do on map into alloc.
> >
> > > +};
> > > +
> > >  /**
> > >   * 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);
> >
> > Why do you need a legacy domain? As far as I can tell, this is new
> > code, hence it has no legacy.
> >
> > > +     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);
> >
> >
> > Thanks,
> >
> >         M.
> > --
> > Without deviation from the norm, progress is not possible.
> 
> Hi Marc,
> 
> since we're so late in the release cycle and I really only need patch
> 3/9 from this series - I'll drop this patch and 1/9 as well in v3. We
> can come back to this in the next cycle.

Then this series doesn't even compile once you remove these two
patches (just removing patch #1 breaks everything). Please repost a
fixed series that can be reviewed in the light of your new, reduced
requirements.

I'd also appreciate if you could answer the questions I have above, as
it appears that you intend to bring this patch back in the future.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

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

[Bartosz Golaszewski]

wt., 12 lut 2019 o 09:53 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> Bartosz,
>
> On Tue, 12 Feb 2019 08:30:32 +0000,
> Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote:
> >
> > pon., 11 lut 2019 o 23:26 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
> > >
> > > On Tue, 29 Jan 2019 09:44:04 +0100
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Delegate the offset to virq number mapping to the provided framework
> > > > instead of handling it locally. Use the legacy domain as we want to
> > > > preallocate the irq descriptors.
> > >
> > > Why?
> > >
> > > >
> > > > 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);
> > >
> > > Consider using modern APIs such as irq_domain_set_info().
> > >
> > > > +     irq_modify_status(virq, IRQ_NOREQUEST | IRQ_NOAUTOEN, IRQ_NOPROBE);
> > >
> > > Where is this requirement coming from?
> > >
> > > > +
> > > > +     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,
> > >
> > > The intended use of the v2 API is to have alloc and free as a pair, and
> > > no map. So please choose which version of the API you're using here.
> > > The safest bet would be to move what you do on map into alloc.
> > >
> > > > +};
> > > > +
> > > >  /**
> > > >   * 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);
> > >
> > > Why do you need a legacy domain? As far as I can tell, this is new
> > > code, hence it has no legacy.
> > >
> > > > +     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);
> > >
> > >
> > > Thanks,
> > >
> > >         M.
> > > --
> > > Without deviation from the norm, progress is not possible.
> >
> > Hi Marc,
> >
> > since we're so late in the release cycle and I really only need patch
> > 3/9 from this series - I'll drop this patch and 1/9 as well in v3. We
> > can come back to this in the next cycle.
>
> Then this series doesn't even compile once you remove these two
> patches (just removing patch #1 breaks everything). Please repost a
> fixed series that can be reviewed in the light of your new, reduced
> requirements.
>

Yes, this is what I meant by "I'll drop this patch and 1/9 as well in
v3" - I meant I'll repost a new version without the first two and with
patch 3/9 fixed.

> I'd also appreciate if you could answer the questions I have above, as
> it appears that you intend to bring this patch back in the future.
>

Sure, will do.

Bart

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

[Marc Zyngier]

On 29/01/2019 08:44, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Provide a more specialized variant of irq_sim_fire() that allows to
> specify the type of the fired interrupt. The type is stored in the
> dummy irq context struct via the set_type callback.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/irq_sim.h |  9 ++++++++-
>  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
>  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> index b96c2f752320..647a6c8ffb31 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 {
> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
>  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_type(struct irq_sim *sim,
> +		       unsigned int offset, unsigned int type);
>  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
>  
> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> +{
> +	irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
> +}
> +
>  #endif /* _LINUX_IRQ_SIM_H */
> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> index 2bcdbab1bc5a..e3160b5e59b8 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) {
> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
>  }
>  
>  /**
> - * irq_sim_fire - Enqueue an interrupt.
> + * irq_sim_fire_type - Enqueue an interrupt.
>   *
>   * @sim:        The interrupt simulator object.
>   * @offset:     Offset of the simulated interrupt which should be fired.
> + * @type:	Type of the fired interrupt. Must be one of the following:
> + *		IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
> + *		IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
> + *		IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
>   */
> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> +void irq_sim_fire_type(struct irq_sim *sim,
> +		       unsigned int offset, unsigned int type)
>  {
>  	struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
>  
> -	if (ctx->enabled) {
> +	/* Only care about relevant flags. */
> +	type &= IRQ_TYPE_SENSE_MASK;
> +
> +	if (ctx->enabled && (ctx->type & type)) {

I wonder how realistic this is, given that you do not track the release
of a level. In short, mo matter what the type is, you treat everything
as edge.

What is the point of this?

>  		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_type);
>  
>  /**
>   * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Bartosz Golaszewski]

wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> On 29/01/2019 08:44, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Provide a more specialized variant of irq_sim_fire() that allows to
> > specify the type of the fired interrupt. The type is stored in the
> > dummy irq context struct via the set_type callback.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> >  include/linux/irq_sim.h |  9 ++++++++-
> >  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
> >  2 files changed, 30 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> > index b96c2f752320..647a6c8ffb31 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 {
> > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
> >  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_type(struct irq_sim *sim,
> > +                    unsigned int offset, unsigned int type);
> >  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> >
> > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > +{
> > +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
> > +}
> > +
> >  #endif /* _LINUX_IRQ_SIM_H */
> > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > index 2bcdbab1bc5a..e3160b5e59b8 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) {
> > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
> >  }
> >
> >  /**
> > - * irq_sim_fire - Enqueue an interrupt.
> > + * irq_sim_fire_type - Enqueue an interrupt.
> >   *
> >   * @sim:        The interrupt simulator object.
> >   * @offset:     Offset of the simulated interrupt which should be fired.
> > + * @type:    Type of the fired interrupt. Must be one of the following:
> > + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
> > + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
> > + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
> >   */
> > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > +void irq_sim_fire_type(struct irq_sim *sim,
> > +                    unsigned int offset, unsigned int type)
> >  {
> >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> >
> > -     if (ctx->enabled) {
> > +     /* Only care about relevant flags. */
> > +     type &= IRQ_TYPE_SENSE_MASK;
> > +
> > +     if (ctx->enabled && (ctx->type & type)) {
>
> I wonder how realistic this is, given that you do not track the release
> of a level. In short, mo matter what the type is, you treat everything
> as edge.
>
> What is the point of this?
>

When userspace wants to monitor GPIO line interrupts, the GPIO
framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
IRQF_TRIGGER_RISING or both. The testing module tries to act like real
hardware and so if we pass only one of the *_TRIGGER_* flags, we want
the simulated interrupt of corresponding type to be fired.

Another solution - if you don't like this one -  would be to have more
specialized functions: irq_sim_fire_rising() and
irq_sim_fire_falling(). How about that?

Bart

> >               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_type);
> >
> >  /**
> >   * irq_sim_irqnum - Get the allocated number of a dummy interrupt.
> >
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...

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

[Uwe Kleine-König]

On Tue, Feb 12, 2019 at 10:19:20AM +0100, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
> >
> > On 29/01/2019 08:44, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Provide a more specialized variant of irq_sim_fire() that allows to
> > > specify the type of the fired interrupt. The type is stored in the
> > > dummy irq context struct via the set_type callback.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > >  include/linux/irq_sim.h |  9 ++++++++-
> > >  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
> > >  2 files changed, 30 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> > > index b96c2f752320..647a6c8ffb31 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 {
> > > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
> > >  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_type(struct irq_sim *sim,
> > > +                    unsigned int offset, unsigned int type);
> > >  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> > >
> > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > +{
> > > +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
> > > +}
> > > +
> > >  #endif /* _LINUX_IRQ_SIM_H */
> > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > > index 2bcdbab1bc5a..e3160b5e59b8 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) {
> > > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
> > >  }
> > >
> > >  /**
> > > - * irq_sim_fire - Enqueue an interrupt.
> > > + * irq_sim_fire_type - Enqueue an interrupt.
> > >   *
> > >   * @sim:        The interrupt simulator object.
> > >   * @offset:     Offset of the simulated interrupt which should be fired.
> > > + * @type:    Type of the fired interrupt. Must be one of the following:
> > > + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
> > > + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
> > > + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
> > >   */
> > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > +void irq_sim_fire_type(struct irq_sim *sim,
> > > +                    unsigned int offset, unsigned int type)
> > >  {
> > >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> > >
> > > -     if (ctx->enabled) {
> > > +     /* Only care about relevant flags. */
> > > +     type &= IRQ_TYPE_SENSE_MASK;
> > > +
> > > +     if (ctx->enabled && (ctx->type & type)) {
> >
> > I wonder how realistic this is, given that you do not track the release
> > of a level. In short, mo matter what the type is, you treat everything
> > as edge.
> >
> > What is the point of this?
> >
> 
> When userspace wants to monitor GPIO line interrupts, the GPIO
> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> IRQF_TRIGGER_RISING or both.

I think the background of the question was: Why is there no support for
level sensitive irqs?

The userspace API has this limitation (also for no good reason I think).

Best regards
Uwe

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

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

[Bartosz Golaszewski]

wt., 12 lut 2019 o 11:06 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Tue, Feb 12, 2019 at 10:19:20AM +0100, Bartosz Golaszewski wrote:
> > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
> > >
> > > On 29/01/2019 08:44, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Provide a more specialized variant of irq_sim_fire() that allows to
> > > > specify the type of the fired interrupt. The type is stored in the
> > > > dummy irq context struct via the set_type callback.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > >  include/linux/irq_sim.h |  9 ++++++++-
> > > >  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
> > > >  2 files changed, 30 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> > > > index b96c2f752320..647a6c8ffb31 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 {
> > > > @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
> > > >  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_type(struct irq_sim *sim,
> > > > +                    unsigned int offset, unsigned int type);
> > > >  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> > > >
> > > > +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > > +{
> > > > +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
> > > > +}
> > > > +
> > > >  #endif /* _LINUX_IRQ_SIM_H */
> > > > diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> > > > index 2bcdbab1bc5a..e3160b5e59b8 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) {
> > > > @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
> > > >  }
> > > >
> > > >  /**
> > > > - * irq_sim_fire - Enqueue an interrupt.
> > > > + * irq_sim_fire_type - Enqueue an interrupt.
> > > >   *
> > > >   * @sim:        The interrupt simulator object.
> > > >   * @offset:     Offset of the simulated interrupt which should be fired.
> > > > + * @type:    Type of the fired interrupt. Must be one of the following:
> > > > + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
> > > > + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
> > > > + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
> > > >   */
> > > > -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> > > > +void irq_sim_fire_type(struct irq_sim *sim,
> > > > +                    unsigned int offset, unsigned int type)
> > > >  {
> > > >       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> > > >
> > > > -     if (ctx->enabled) {
> > > > +     /* Only care about relevant flags. */
> > > > +     type &= IRQ_TYPE_SENSE_MASK;
> > > > +
> > > > +     if (ctx->enabled && (ctx->type & type)) {
> > >
> > > I wonder how realistic this is, given that you do not track the release
> > > of a level. In short, mo matter what the type is, you treat everything
> > > as edge.
> > >
> > > What is the point of this?
> > >
> >
> > When userspace wants to monitor GPIO line interrupts, the GPIO
> > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> > IRQF_TRIGGER_RISING or both.
>
> I think the background of the question was: Why is there no support for
> level sensitive irqs?
>
> The userspace API has this limitation (also for no good reason I think).
>

Fair point. We also need support for programmable pull-up/down
resistors as it's something many users request. It's just a matter of
someone getting down to doing it. :)

Bart

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

[Marc Zyngier]

On 12/02/2019 09:19, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>>
>> On 29/01/2019 08:44, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> Provide a more specialized variant of irq_sim_fire() that allows to
>>> specify the type of the fired interrupt. The type is stored in the
>>> dummy irq context struct via the set_type callback.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>> ---
>>>  include/linux/irq_sim.h |  9 ++++++++-
>>>  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
>>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
>>> index b96c2f752320..647a6c8ffb31 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 {
>>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
>>>  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_type(struct irq_sim *sim,
>>> +                    unsigned int offset, unsigned int type);
>>>  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
>>>
>>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
>>> +{
>>> +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
>>> +}
>>> +
>>>  #endif /* _LINUX_IRQ_SIM_H */
>>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
>>> index 2bcdbab1bc5a..e3160b5e59b8 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) {
>>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
>>>  }
>>>
>>>  /**
>>> - * irq_sim_fire - Enqueue an interrupt.
>>> + * irq_sim_fire_type - Enqueue an interrupt.
>>>   *
>>>   * @sim:        The interrupt simulator object.
>>>   * @offset:     Offset of the simulated interrupt which should be fired.
>>> + * @type:    Type of the fired interrupt. Must be one of the following:
>>> + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
>>> + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
>>> + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
>>>   */
>>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
>>> +void irq_sim_fire_type(struct irq_sim *sim,
>>> +                    unsigned int offset, unsigned int type)
>>>  {
>>>       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
>>>
>>> -     if (ctx->enabled) {
>>> +     /* Only care about relevant flags. */
>>> +     type &= IRQ_TYPE_SENSE_MASK;
>>> +
>>> +     if (ctx->enabled && (ctx->type & type)) {
>>
>> I wonder how realistic this is, given that you do not track the release
>> of a level. In short, mo matter what the type is, you treat everything
>> as edge.
>>
>> What is the point of this?
>>
> 
> When userspace wants to monitor GPIO line interrupts, the GPIO
> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> IRQF_TRIGGER_RISING or both. The testing module tries to act like real
> hardware and so if we pass only one of the *_TRIGGER_* flags, we want
> the simulated interrupt of corresponding type to be fired.

Well, that's not how HW works.

> 
> Another solution - if you don't like this one -  would be to have more
> specialized functions: irq_sim_fire_rising() and
> irq_sim_fire_falling(). How about that?

I think you're missing the point. So far, your API has been "an
interrupt has fired", no matter what the trigger is, and that's fine.
That's just modeling the output of an abstract interrupt controller into
whatever the irqsim is simulating.

Now, what you're exposing is "this is how the line changed". Which is an
entirely different business, as you're now exposing the device output
line. Yes, you can model it with raising/falling, but you need at least
resampling for level interrupts, and actual edge detection (raising
followed by raising only generates a single interrupt, while
raising-falling-raising generates two).

At the moment, I don't see any of that so I seriously doubt the validity
of the approach.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Bartosz Golaszewski]

wt., 12 lut 2019 o 11:27 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>
> On 12/02/2019 09:19, Bartosz Golaszewski wrote:
> > wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
> >>
> >> On 29/01/2019 08:44, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>>
> >>> Provide a more specialized variant of irq_sim_fire() that allows to
> >>> specify the type of the fired interrupt. The type is stored in the
> >>> dummy irq context struct via the set_type callback.
> >>>
> >>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >>> ---
> >>>  include/linux/irq_sim.h |  9 ++++++++-
> >>>  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
> >>>  2 files changed, 30 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
> >>> index b96c2f752320..647a6c8ffb31 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 {
> >>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
> >>>  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_type(struct irq_sim *sim,
> >>> +                    unsigned int offset, unsigned int type);
> >>>  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
> >>>
> >>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> >>> +{
> >>> +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
> >>> +}
> >>> +
> >>>  #endif /* _LINUX_IRQ_SIM_H */
> >>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
> >>> index 2bcdbab1bc5a..e3160b5e59b8 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) {
> >>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
> >>>  }
> >>>
> >>>  /**
> >>> - * irq_sim_fire - Enqueue an interrupt.
> >>> + * irq_sim_fire_type - Enqueue an interrupt.
> >>>   *
> >>>   * @sim:        The interrupt simulator object.
> >>>   * @offset:     Offset of the simulated interrupt which should be fired.
> >>> + * @type:    Type of the fired interrupt. Must be one of the following:
> >>> + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
> >>> + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
> >>> + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
> >>>   */
> >>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
> >>> +void irq_sim_fire_type(struct irq_sim *sim,
> >>> +                    unsigned int offset, unsigned int type)
> >>>  {
> >>>       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
> >>>
> >>> -     if (ctx->enabled) {
> >>> +     /* Only care about relevant flags. */
> >>> +     type &= IRQ_TYPE_SENSE_MASK;
> >>> +
> >>> +     if (ctx->enabled && (ctx->type & type)) {
> >>
> >> I wonder how realistic this is, given that you do not track the release
> >> of a level. In short, mo matter what the type is, you treat everything
> >> as edge.
> >>
> >> What is the point of this?
> >>
> >
> > When userspace wants to monitor GPIO line interrupts, the GPIO
> > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> > IRQF_TRIGGER_RISING or both. The testing module tries to act like real
> > hardware and so if we pass only one of the *_TRIGGER_* flags, we want
> > the simulated interrupt of corresponding type to be fired.
>
> Well, that's not how HW works.
>
> >
> > Another solution - if you don't like this one -  would be to have more
> > specialized functions: irq_sim_fire_rising() and
> > irq_sim_fire_falling(). How about that?
>
> I think you're missing the point. So far, your API has been "an
> interrupt has fired", no matter what the trigger is, and that's fine.
> That's just modeling the output of an abstract interrupt controller into
> whatever the irqsim is simulating.
>
> Now, what you're exposing is "this is how the line changed". Which is an
> entirely different business, as you're now exposing the device output
> line. Yes, you can model it with raising/falling, but you need at least
> resampling for level interrupts, and actual edge detection (raising
> followed by raising only generates a single interrupt, while
> raising-falling-raising generates two).
>

This logic is later taken care of in the gpio-mockup driver in this
series. It checks the line state changes and decides if the interrupt
should be fired.

Bart

> At the moment, I don't see any of that so I seriously doubt the validity
> of the approach.
>

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

[Marc Zyngier]

On 12/02/2019 10:37, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 11:27 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>>
>> On 12/02/2019 09:19, Bartosz Golaszewski wrote:
>>> wt., 12 lut 2019 o 10:10 Marc Zyngier <marc.zyngier@arm.com> napisał(a):
>>>>
>>>> On 29/01/2019 08:44, Bartosz Golaszewski wrote:
>>>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>>>
>>>>> Provide a more specialized variant of irq_sim_fire() that allows to
>>>>> specify the type of the fired interrupt. The type is stored in the
>>>>> dummy irq context struct via the set_type callback.
>>>>>
>>>>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>>> ---
>>>>>  include/linux/irq_sim.h |  9 ++++++++-
>>>>>  kernel/irq/irq_sim.c    | 26 ++++++++++++++++++++++----
>>>>>  2 files changed, 30 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
>>>>> index b96c2f752320..647a6c8ffb31 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 {
>>>>> @@ -37,7 +38,13 @@ int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs);
>>>>>  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_type(struct irq_sim *sim,
>>>>> +                    unsigned int offset, unsigned int type);
>>>>>  int irq_sim_irqnum(struct irq_sim *sim, unsigned int offset);
>>>>>
>>>>> +static inline void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
>>>>> +{
>>>>> +     irq_sim_fire_type(sim, offset, IRQ_TYPE_DEFAULT);
>>>>> +}
>>>>> +
>>>>>  #endif /* _LINUX_IRQ_SIM_H */
>>>>> diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
>>>>> index 2bcdbab1bc5a..e3160b5e59b8 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) {
>>>>> @@ -192,21 +202,29 @@ irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset)
>>>>>  }
>>>>>
>>>>>  /**
>>>>> - * irq_sim_fire - Enqueue an interrupt.
>>>>> + * irq_sim_fire_type - Enqueue an interrupt.
>>>>>   *
>>>>>   * @sim:        The interrupt simulator object.
>>>>>   * @offset:     Offset of the simulated interrupt which should be fired.
>>>>> + * @type:    Type of the fired interrupt. Must be one of the following:
>>>>> + *           IRQ_TYPE_EDGE_RISING, IRQ_TYPE_EDGE_FALLING,
>>>>> + *           IRQ_TYPE_EDGE_BOTH, IRQ_TYPE_LEVEL_HIGH,
>>>>> + *           IRQ_TYPE_LEVEL_LOW, IRQ_TYPE_DEFAULT
>>>>>   */
>>>>> -void irq_sim_fire(struct irq_sim *sim, unsigned int offset)
>>>>> +void irq_sim_fire_type(struct irq_sim *sim,
>>>>> +                    unsigned int offset, unsigned int type)
>>>>>  {
>>>>>       struct irq_sim_irq_ctx *ctx = irq_sim_get_ctx(sim, offset);
>>>>>
>>>>> -     if (ctx->enabled) {
>>>>> +     /* Only care about relevant flags. */
>>>>> +     type &= IRQ_TYPE_SENSE_MASK;
>>>>> +
>>>>> +     if (ctx->enabled && (ctx->type & type)) {
>>>>
>>>> I wonder how realistic this is, given that you do not track the release
>>>> of a level. In short, mo matter what the type is, you treat everything
>>>> as edge.
>>>>
>>>> What is the point of this?
>>>>
>>>
>>> When userspace wants to monitor GPIO line interrupts, the GPIO
>>> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
>>> IRQF_TRIGGER_RISING or both. The testing module tries to act like real
>>> hardware and so if we pass only one of the *_TRIGGER_* flags, we want
>>> the simulated interrupt of corresponding type to be fired.
>>
>> Well, that's not how HW works.
>>
>>>
>>> Another solution - if you don't like this one -  would be to have more
>>> specialized functions: irq_sim_fire_rising() and
>>> irq_sim_fire_falling(). How about that?
>>
>> I think you're missing the point. So far, your API has been "an
>> interrupt has fired", no matter what the trigger is, and that's fine.
>> That's just modeling the output of an abstract interrupt controller into
>> whatever the irqsim is simulating.
>>
>> Now, what you're exposing is "this is how the line changed". Which is an
>> entirely different business, as you're now exposing the device output
>> line. Yes, you can model it with raising/falling, but you need at least
>> resampling for level interrupts, and actual edge detection (raising
>> followed by raising only generates a single interrupt, while
>> raising-falling-raising generates two).
>>
> 
> This logic is later taken care of in the gpio-mockup driver in this
> series. It checks the line state changes and decides if the interrupt
> should be fired.

But that's only for edge, right? I don't really see any level support.

Can you please trim this series to what you actually (1) need, (2)
support. So far, this series feels like a bunch of half-baked ideas.
Interesting ideas, but still half baked. And trying to rush me into
ACKing its of it is not improving the situation.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

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

[Uwe Kleine-König]

On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote:
> On 12/02/2019 09:19, Bartosz Golaszewski wrote:
> > When userspace wants to monitor GPIO line interrupts, the GPIO
> > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> > IRQF_TRIGGER_RISING or both. The testing module tries to act like real
> > hardware and so if we pass only one of the *_TRIGGER_* flags, we want
> > the simulated interrupt of corresponding type to be fired.
> 
> Well, that's not how HW works.

I cannot follow. I agree with Bartosz here. If you configure your SoC's
irq-controller to only fire on a raising edge, you don't get an event
when the line falls.
 
> > Another solution - if you don't like this one -  would be to have more
> > specialized functions: irq_sim_fire_rising() and
> > irq_sim_fire_falling(). How about that?
> 
> I think you're missing the point. So far, your API has been "an
> interrupt has fired", no matter what the trigger is, and that's fine.
> That's just modeling the output of an abstract interrupt controller into
> whatever the irqsim is simulating.
> 
> Now, what you're exposing is "this is how the line changed". Which is an
> entirely different business, as you're now exposing the device output
> line. Yes, you can model it with raising/falling, but you need at least
> resampling for level interrupts, and actual edge detection (raising
> followed by raising only generates a single interrupt, while
> raising-falling-raising generates two).

This matches my concern and that's why I suggested somewhere else in
this thread to put the configuration of the sensitiveness and the actual
tracking of the line in the same component (either irqsim or
gpio-mockup). Given that there are only two irqsim users and the other
one (something in iio) doesn't need that sensitiveness stuff (and I
cannot imagine another user of irqsim with the sensitiveness support) I
think it is best to move this to the mockup driver. That's how "normal"
hardware drivers have to do it, too.

Best regards
Uwe

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

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

[Bartosz Golaszewski]

wt., 12 lut 2019 o 12:05 Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> napisał(a):
>
> On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote:
> > On 12/02/2019 09:19, Bartosz Golaszewski wrote:
> > > When userspace wants to monitor GPIO line interrupts, the GPIO
> > > framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
> > > IRQF_TRIGGER_RISING or both. The testing module tries to act like real
> > > hardware and so if we pass only one of the *_TRIGGER_* flags, we want
> > > the simulated interrupt of corresponding type to be fired.
> >
> > Well, that's not how HW works.
>
> I cannot follow. I agree with Bartosz here. If you configure your SoC's
> irq-controller to only fire on a raising edge, you don't get an event
> when the line falls.
>
> > > Another solution - if you don't like this one -  would be to have more
> > > specialized functions: irq_sim_fire_rising() and
> > > irq_sim_fire_falling(). How about that?
> >
> > I think you're missing the point. So far, your API has been "an
> > interrupt has fired", no matter what the trigger is, and that's fine.
> > That's just modeling the output of an abstract interrupt controller into
> > whatever the irqsim is simulating.
> >
> > Now, what you're exposing is "this is how the line changed". Which is an
> > entirely different business, as you're now exposing the device output
> > line. Yes, you can model it with raising/falling, but you need at least
> > resampling for level interrupts, and actual edge detection (raising
> > followed by raising only generates a single interrupt, while
> > raising-falling-raising generates two).
>
> This matches my concern and that's why I suggested somewhere else in
> this thread to put the configuration of the sensitiveness and the actual
> tracking of the line in the same component (either irqsim or
> gpio-mockup). Given that there are only two irqsim users and the other
> one (something in iio) doesn't need that sensitiveness stuff (and I
> cannot imagine another user of irqsim with the sensitiveness support) I
> think it is best to move this to the mockup driver. That's how "normal"
> hardware drivers have to do it, too.
>

This is what v1 of this series did - it provided a function to
retrieve the type and the logic lived in gpio-mockup. Maybe we need to
get back to that solution.

Bart

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

[Marc Zyngier]

On 12/02/2019 11:05, Uwe Kleine-König wrote:
> On Tue, Feb 12, 2019 at 10:27:54AM +0000, Marc Zyngier wrote:
>> On 12/02/2019 09:19, Bartosz Golaszewski wrote:
>>> When userspace wants to monitor GPIO line interrupts, the GPIO
>>> framework requests a threaded interrupt with IRQF_TRIGGER_FALLING,
>>> IRQF_TRIGGER_RISING or both. The testing module tries to act like real
>>> hardware and so if we pass only one of the *_TRIGGER_* flags, we want
>>> the simulated interrupt of corresponding type to be fired.
>>
>> Well, that's not how HW works.
> 
> I cannot follow. I agree with Bartosz here. If you configure your SoC's
> irq-controller to only fire on a raising edge, you don't get an event
> when the line falls.

I was a bit quick here. It is more the fact that level gets treated as
"just another type of edge" that really irritated me (yes, I've wasted
way too much time implementing interrupt controllers).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...