gpio: mockup: improve the user-space testing interface
Subsystems (3)
v2 cover gpio: mockup: improve the user-space testing interface
From: Bartosz Golaszewski <[email protected]>
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(-)
Signature · 1 line
2.19.1
PATCH v2 1/9 irq/irq_sim: don't share the irq_chip structure between simulators
From: Bartosz Golaszewski <[email protected]>
We want to support multiple instances of irq_sim. Make struct irq_chip part of the irq_sim structure.
--- include/linux/irq_sim.h | 2 ++ kernel/irq/irq_sim.c | 12 +++++------- 2 files changed, 7 insertions(+), 7 deletions(-)
include/linux/irq_sim.h +2 -0
@@ -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;
kernel/irq/irq_sim.c +5 -7
@@ -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,
Signature · 1 line
2.19.1
Re: [PATCH v2 1/9] irq/irq_sim: don't share the irq_chip structure between simulators
Quoted 4 lines · On Tue, 29 Jan 2019 09:44:03 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
We want to support multiple instances of
irq_sim. Make structirq_chippart of theirq_simstructure.
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.
Quoted 64 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 2 ++ kernel/irq/irq_sim.c | 12 +++++------- 2 files changed, 7 insertions(+), 7 deletions(-)include/linux/irq_sim.h +2
-0@@ -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;kernel/irq/irq_sim.c +5
-7@@ -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,
Signature · 1 line
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
pon., 11 lut 2019 o 23:28 Marc Zyngier <[email protected]> napisał(a):
Quoted 6 lines
Quoted 2 lines · On Tue, 29 Jan 2019 09:44:03 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
Quoted 9 lines
Quoted 2 lines
We want to support multiple instances of
irq_sim. Make structirq_chippart of theirq_simstructure.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
Quoted 73 lines
Thanks,
M.Quoted 56 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 2 ++ kernel/irq/irq_sim.c | 12 +++++------- 2 files changed, 7 insertions(+), 7 deletions(-)include/linux/irq_sim.h +2
-0@@ -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;kernel/irq/irq_sim.c +4
-6@@ -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);kernel/irq/irq_sim.c int irq_sim_init(struct irq_sim *sim, unsigned int num_irqs) · line 88
Quoted 8 lines
kernel/irq/irq_sim.c +1
-1@@ -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.
PATCH v2 2/9 irq/irq_sim: use irq domain
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- include/linux/irq_sim.h | 6 +-- kernel/irq/irq_sim.c | 94 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 29 deletions(-)
include/linux/irq_sim.h +3 -3
@@ -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);
kernel/irq/irq_sim.c +68 -26
142 lines not on this page · raw message
Signature · 1 line
2.19.1
Re: [PATCH v2 2/9] irq/irq_sim: use irq domain
Quoted 5 lines · On Tue, 29 Jan 2019 09:44:04 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
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?
Quoted 49 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 6 +-- kernel/irq/irq_sim.c | 94 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 29 deletions(-)include/linux/irq_sim.h +3
-3@@ -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);kernel/irq/irq_sim.c +3
-0@@ -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) +{
kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 59
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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().
kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 60
Quoted 1 line
kernel/irq/irq_sim.c +1
-0+irq_modify_status (virq,IRQ_NOREQUEST |IRQ_NOAUTOEN ,IRQ_NOPROBE );
Where is this requirement coming from?
Quoted 11 lines
kernel/irq/irq_sim.c +11
-0+ +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; +
kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 81
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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.
Quoted 9 lines
kernel/irq/irq_sim.c +2
-0+}; +/** * 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) {
kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 101
Quoted 10 lines
kernel/irq/irq_sim.c +6
-4-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.
Quoted 84 lines
kernel/irq/irq_sim.c +24
-21+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); }
kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(irq_sim_fire); · line 222
Quoted 7 lines
kernel/irq/irq_sim.c +1
-1@@ -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.
Signature · 1 line
Without deviation from the norm, progress is not possible.
Re: [PATCH v2 2/9] irq/irq_sim: use irq domain
pon., 11 lut 2019 o 23:26 Marc Zyngier <[email protected]> napisał(a):
Quoted 222 lines
Quoted 5 lines · On Tue, 29 Jan 2019 09:44:04 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
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?
Quoted 49 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 6 +-- kernel/irq/irq_sim.c | 94 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 29 deletions(-)include/linux/irq_sim.h +3
-3@@ -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);kernel/irq/irq_sim.c +3
-0@@ -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) +{kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 59
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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().kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 60
Quoted 1 line
kernel/irq/irq_sim.c +1
-0+irq_modify_status (virq,IRQ_NOREQUEST |IRQ_NOAUTOEN ,IRQ_NOPROBE );Where is this requirement coming from?
Quoted 11 lines
kernel/irq/irq_sim.c +11
-0+ +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; +kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 81
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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.
Quoted 9 lines
kernel/irq/irq_sim.c +2
-0+}; +/** * 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) {kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 101
Quoted 10 lines
kernel/irq/irq_sim.c +6
-4-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.
Quoted 84 lines
kernel/irq/irq_sim.c +24
-21+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); }kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(irq_sim_fire); · line 222
Quoted 7 lines
kernel/irq/irq_sim.c +1
-1@@ -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,
Quoted 3 lines
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
Bartosz,
Quoted 228 lines · Bartosz Golaszewski
pon., 11 lut 2019 o 23:26 Marc Zyngier <[email protected]> napisał(a):
Quoted 222 lines
Quoted 5 lines · On Tue, 29 Jan 2019 09:44:04 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
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?
Quoted 49 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 6 +-- kernel/irq/irq_sim.c | 94 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 29 deletions(-)include/linux/irq_sim.h +3
-3@@ -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);kernel/irq/irq_sim.c +3
-0@@ -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) +{kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 59
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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().kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 60
Quoted 1 line
kernel/irq/irq_sim.c +1
-0+irq_modify_status (virq,IRQ_NOREQUEST |IRQ_NOAUTOEN ,IRQ_NOPROBE );Where is this requirement coming from?
Quoted 11 lines
kernel/irq/irq_sim.c +11
-0+ +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; +kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 81
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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.
Quoted 9 lines
kernel/irq/irq_sim.c +2
-0+}; +/** * 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) {kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 101
Quoted 10 lines
kernel/irq/irq_sim.c +6
-4-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.
Quoted 84 lines
kernel/irq/irq_sim.c +24
-21+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); }kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(irq_sim_fire); · line 222
Quoted 7 lines
kernel/irq/irq_sim.c +1
-1@@ -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,
Quoted 3 lines
M.--
Without deviation from the norm, progress is not possible.
Quoted 5 lines
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.
Signature · 1 line
Jazz is not dead, it just smell funny.
Re: [PATCH v2 2/9] irq/irq_sim: use irq domain
wt., 12 lut 2019 o 09:53 Marc Zyngier <[email protected]> napisał(a):
Quoted 235 lines
Bartosz,
Quoted 230 lines · Bartosz Golaszewski
pon., 11 lut 2019 o 23:26 Marc Zyngier <[email protected]> napisał(a):
Quoted 222 lines
Quoted 5 lines · On Tue, 29 Jan 2019 09:44:04 +0100 Bartosz Golaszewski <[email protected]> wrote:
From: Bartosz Golaszewski <[email protected]>
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?
Quoted 49 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 6 +-- kernel/irq/irq_sim.c | 94 +++++++++++++++++++++++++++++------------ 2 files changed, 71 insertions(+), 29 deletions(-)include/linux/irq_sim.h +3
-3@@ -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);kernel/irq/irq_sim.c +3
-0@@ -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) +{kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 59
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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().kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 60
Quoted 1 line
kernel/irq/irq_sim.c +1
-0+irq_modify_status (virq,IRQ_NOREQUEST |IRQ_NOAUTOEN ,IRQ_NOPROBE );Where is this requirement coming from?
Quoted 11 lines
kernel/irq/irq_sim.c +11
-0+ +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; +kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 81
Quoted 10 lines
kernel/irq/irq_sim.c +10
-0+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.
Quoted 9 lines
kernel/irq/irq_sim.c +2
-0+}; +/** * 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) {kernel/irq/irq_sim.c static void irq_sim_handle_irq(struct irq_work *work) · line 101
Quoted 10 lines
kernel/irq/irq_sim.c +6
-4-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.
Quoted 84 lines
kernel/irq/irq_sim.c +24
-21+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); }kernel/irq/irq_sim.c EXPORT_SYMBOL_GPL(irq_sim_fire); · line 222
Quoted 7 lines
kernel/irq/irq_sim.c +1
-1@@ -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,
Quoted 3 lines
M.--
Without deviation from the norm, progress is not possible.Hi Marc,
Quoted 9 lines
Quoted 3 lines
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.
Quoted 3 lines
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
PATCH v2 3/9 irq/irq_sim: provide irq_sim_fire_type()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)
include/linux/irq_sim.h +8 -1
@@ -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 */
kernel/irq/irq_sim.c +22 -4
@@ -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.
Signature · 1 line
2.19.1
Re: [PATCH v2 3/9] irq/irq_sim: provide irq_sim_fire_type()
Hello Bartosz,
Quoted 7 lines · Bartosz Golaszewski
kernel/irq/irq_sim.c +2
-2-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) {
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 227
Quoted 10 lines
kernel/irq/irq_sim.c +5
-1+/* 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
Signature · 2 lines
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()
wt., 29 sty 2019 o 10:07 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 10 lines
Hello Bartosz,
Quoted 6 lines · Bartosz Golaszewski
kernel/irq/irq_sim.c +2
-1-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);
Quoted 16 lines
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 227
Quoted 11 lines
kernel/irq/irq_sim.c +5
-2-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()
Quoted 29 lines · Bartosz Golaszewski
wt., 29 sty 2019 o 10:07 Uwe Kleine-König
<[email protected]> napisał(a):Quoted 10 lines
Hello Bartosz,
Quoted 6 lines · Bartosz Golaszewski
kernel/irq/irq_sim.c +2
-1-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);Quoted 16 lines
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 227
Quoted 11 lines
kernel/irq/irq_sim.c +5
-2-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.
Quoted 1 line
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
Signature · 2 lines
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()
wt., 29 sty 2019 o 13:55 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 37 lines · Bartosz Golaszewski
Quoted 29 lines
wt., 29 sty 2019 o 10:07 Uwe Kleine-König
<[email protected]> napisał(a):Quoted 10 lines
Hello Bartosz,
Quoted 6 lines · Bartosz Golaszewski
kernel/irq/irq_sim.c +2
-1-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);Quoted 16 lines
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 227
Quoted 11 lines
kernel/irq/irq_sim.c +5
-2-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.
Quoted 1 line
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_simdriver and the logic if the irq should trigger in mockup. My wondering is about having both in the mockup driver.
Quoted 6 lines
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 3/9] irq/irq_sim: provide irq_sim_fire_type()
In reply to Bartosz GolaszewskiQuoted 82 lines · Bartosz Golaszewski
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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?
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 230
Quoted 10 lines
kernel/irq/irq_sim.c +1
-1set_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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/9] irq/irq_sim: provide irq_sim_fire_type()
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 99 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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.
Quoted 2 lines
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
Quoted 16 lines
kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 230
Quoted 10 lines
kernel/irq/irq_sim.c +1
-1set_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()
Quoted 100 lines · Bartosz Golaszewski
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 99 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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.
Quoted 6 lines
Quoted 2 lines
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_RISINGor 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
Signature · 2 lines
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()
wt., 12 lut 2019 o 11:06 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 105 lines · Bartosz Golaszewski
Quoted 103 lines
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 101 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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?
Quoted 9 lines
Quoted 3 lines
When userspace wants to monitor GPIO line interrupts, the GPIO framework requests a threaded interrupt with
IRQF_TRIGGER_FALLING,IRQF_TRIGGER_RISINGor 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()
In reply to Bartosz GolaszewskiQuoted 100 lines · Bartosz Golaszewski
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 99 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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.
Quoted 8 lines
Quoted 2 lines
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_RISINGor 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.
Quoted 4 lines
Another solution - if you don't like this one - would be to have more specialized functions:
irq_sim_fire_rising()andirq_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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/9] irq/irq_sim: provide irq_sim_fire_type()
wt., 12 lut 2019 o 11:27 Marc Zyngier <[email protected]> napisał(a):
Quoted 123 lines · Bartosz Golaszewski
Quoted 100 lines
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 99 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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.
Quoted 8 lines
Quoted 2 lines
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_RISINGor 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.
Quoted 4 lines
Another solution - if you don't like this one - would be to have more specialized functions:
irq_sim_fire_rising()andirq_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.
Quoted 7 lines
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
Quoted 3 lines
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()
Quoted 132 lines · Bartosz Golaszewski
wt., 12 lut 2019 o 11:27 Marc Zyngier <[email protected]> napisał(a):
Quoted 123 lines · Bartosz Golaszewski
Quoted 100 lines
wt., 12 lut 2019 o 10:10 Marc Zyngier <[email protected]> napisał(a):
Quoted 99 lines · Bartosz Golaszewski
Quoted 82 lines
From: Bartosz Golaszewski <[email protected]>
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 theset_typecallback.Signed-off-by Bartosz Golaszewski <[email protected]>--- include/linux/irq_sim.h | 9 ++++++++- kernel/irq/irq_sim.c | 26 ++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-)include/linux/irq_sim.h +8
-1@@ -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 */ kernel/irq/irq_sim.c +15
-2@@ -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)kernel/irq/irq_sim.c irq_sim_get_ctx(struct irq_sim *sim, unsigned int offset) · line 222
Quoted 10 lines
kernel/irq/irq_sim.c +6
-1+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.
Quoted 8 lines
Quoted 2 lines
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_RISINGor 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.
Quoted 4 lines
Another solution - if you don't like this one - would be to have more specialized functions:
irq_sim_fire_rising()andirq_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.
Quoted 7 lines
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).
Quoted 3 lines
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.
Signature · 1 line
Jazz is not dead. It just smells funny...
Re: [PATCH v2 3/9] irq/irq_sim: provide irq_sim_fire_type()
In reply to Marc ZyngierQuoted 8 lines · Marc Zyngier
Quoted 5 lines · Bartosz Golaszewski
When userspace wants to monitor GPIO line interrupts, the GPIO framework requests a threaded interrupt with
IRQF_TRIGGER_FALLING,IRQF_TRIGGER_RISINGor 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.
Quoted 9 lines
Quoted 3 lines
Another solution - if you don't like this one - would be to have more specialized functions:
irq_sim_fire_rising()andirq_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.
Quoted 6 lines
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
Signature · 2 lines
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()
wt., 12 lut 2019 o 12:05 Uwe Kleine-König
<[email protected]> napisał(a):
Quoted 31 lines · Marc Zyngier
Quoted 8 lines · Bartosz Golaszewski
Quoted 5 lines
When userspace wants to monitor GPIO line interrupts, the GPIO framework requests a threaded interrupt with
IRQF_TRIGGER_FALLING,IRQF_TRIGGER_RISINGor 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.
Quoted 9 lines
Quoted 3 lines
Another solution - if you don't like this one - would be to have more specialized functions:
irq_sim_fire_rising()andirq_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.
Quoted 6 lines
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).
Quoted 9 lines
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()
In reply to Uwe Kleine-KönigQuoted 8 lines · Uwe Kleine-König
Quoted 7 lines · Marc Zyngier
Quoted 5 lines · Bartosz Golaszewski
When userspace wants to monitor GPIO line interrupts, the GPIO framework requests a threaded interrupt with
IRQF_TRIGGER_FALLING,IRQF_TRIGGER_RISINGor 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.
Quoted 5 lines
Quoted 1 line
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.
Signature · 1 line
Jazz is not dead. It just smells funny...
PATCH v2 4/9 gpio: mockup: add locking
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- drivers/gpio/gpio-mockup.c | 50 ++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 7 deletions(-)
drivers/gpio/gpio-mockup.c +43 -7
@@ -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;
Signature · 1 line
2.19.1
PATCH v2 5/9 gpio: mockup: implement get_multiple()
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
We already support set_multiple(). Implement get_multiple() as well.
--- drivers/gpio/gpio-mockup.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
drivers/gpio/gpio-mockup.c +17 -0
@@ -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;
Signature · 1 line
2.19.1
PATCH v2 6/9 gpio: mockup: don't create the debugfs link named after the label
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
User-space tests no longer use it and we're breaking the interface anyway.
--- drivers/gpio/gpio-mockup.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)
drivers/gpio/gpio-mockup.c +1 -5
@@ -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)
Signature · 1 line
2.19.1
PATCH v2 7/9 gpio: mockup: change the type of 'offset' to unsigned int
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
This field can never be negative.
--- drivers/gpio/gpio-mockup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpio/gpio-mockup.c +1 -1
@@ -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 ];
Signature · 1 line
2.19.1
PATCH v2 8/9 gpio: mockup: change the signature of unlocked get/set helpers
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- drivers/gpio/gpio-mockup.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mockup.c +8 -11
@@ -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 ;
Signature · 1 line
2.19.1
PATCH v2 9/9 gpio: mockup: rework debugfs interface
In reply to Bartosz GolaszewskiFrom: Bartosz Golaszewski <[email protected]>
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.
--- drivers/gpio/gpio-mockup.c | 113 ++++++++++++++++++++++++++++++++----- 1 file changed, 99 insertions(+), 14 deletions(-)
drivers/gpio/gpio-mockup.c +99 -14
@@ -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" );
Signature · 1 line
2.19.1
wt., 29 sty 2019 o 09:44 Bartosz Golaszewski <[email protected]> napisał(a):
Quoted 60 lines
From: Bartosz Golaszewski <[email protected]>
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_chippart of theirq_simstructure 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 theset_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 theirq_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 interruptBartosz Golaszewski (9):
irq/irq_sim: don't share theirq_chipstructure between simulators irq/irq_sim: use irq domain
irq/irq_sim: provideirq_sim_fire_type()
gpio: mockup: add locking
gpio: mockup: implementget_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
Quoted 8 lines
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
Quoted 70 lines · On Wed, 6 Feb 2019 11:23:46 +0100 Bartosz Golaszewski <[email protected]> wrote:
wt., 29 sty 2019 o 09:44 Bartosz Golaszewski <[email protected]> napisał(a):
Quoted 60 lines
From: Bartosz Golaszewski <[email protected]>
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_chippart of theirq_simstructure 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 theset_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 theirq_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 interruptBartosz Golaszewski (9):
irq/irq_sim: don't share theirq_chipstructure between simulators irq/irq_sim: use irq domain
irq/irq_sim: provideirq_sim_fire_type()
gpio: mockup: add locking
gpio: mockup: implementget_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 interfaceQuoted 8 lines
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
Quoted 5 lines
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.
Signature · 1 line
Without deviation from the norm, progress is not possible.