[PATCH 0/2] gpio: sim: lock simulated GPIOs as interrupts

[PATCH 0/2] gpio: sim: lock simulated GPIOs as interrupts

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

I realized that the gpio-sim module doesn't lock the GPIOs as interrupts
when they are requested from the irq_sim. This leads to users being able
to change the direction of GPIOs that should remain as inputs to output.
This series adds a notifier to the interrupt simulator that users can
register with to be informed about interrupts being requested and
released so that they can act accordingly. The gpio-sim is made to use
this notifier and lock GPIOs as interrupts when needed.

Thomas: if this is fine with you, can you Ack it so that I can take it
through the GPIO tree for the next merge window?

Bartosz Golaszewski (2):
  genirq/irq_sim: add a notifier for irqchip events
  gpio: sim: lock GPIOs as interrupts when they are requested

 drivers/gpio/gpio-sim.c | 30 +++++++++++++++++--
 include/linux/irq_sim.h | 11 +++++++
 kernel/irq/irq_sim.c    | 64 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 2 deletions(-)

-- 
2.40.1

[PATCH 1/2] genirq/irq_sim: add a notifier for irqchip events

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Currently users of the interrupt simulator don't have any way of being
notified about interrupts from the simulated domain being requested or
released. This causes a problem for one of the users - the GPIO
simulator - which is unable to lock the pins as interrupts.

Add a blocking notifier and provide interfaces to register with it, then
use it to notify users of the domain about interrupts being requested
and released while also leaving space for future extensions.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 include/linux/irq_sim.h | 11 +++++++
 kernel/irq/irq_sim.c    | 64 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/include/linux/irq_sim.h b/include/linux/irq_sim.h
index ab831e5ae748..462d1913bb27 100644
--- a/include/linux/irq_sim.h
+++ b/include/linux/irq_sim.h
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/fwnode.h>
 #include <linux/irqdomain.h>
+#include <linux/notifier.h>
 
 /*
  * Provides a framework for allocating simulated interrupts which can be
@@ -23,4 +24,14 @@ struct irq_domain *devm_irq_domain_create_sim(struct device *dev,
 					      unsigned int num_irqs);
 void irq_domain_remove_sim(struct irq_domain *domain);
 
+enum {
+	IRQ_SIM_DOMAIN_IRQ_REQUESTED = 1,
+	IRQ_SIM_DOMAIN_IRQ_RELEASED,
+};
+
+int irq_sim_domain_register_notifier(struct irq_domain *domain,
+				     struct notifier_block *nb);
+int irq_sim_domain_unregister_notifier(struct irq_domain *domain,
+				       struct notifier_block *nb);
+
 #endif /* _LINUX_IRQ_SIM_H */
diff --git a/kernel/irq/irq_sim.c b/kernel/irq/irq_sim.c
index 38d6ae651ac7..653b6ca869cb 100644
--- a/kernel/irq/irq_sim.c
+++ b/kernel/irq/irq_sim.c
@@ -17,6 +17,7 @@ struct irq_sim_work_ctx {
 	unsigned int		irq_count;
 	unsigned long		*pending;
 	struct irq_domain	*domain;
+	struct blocking_notifier_head notifier;
 };
 
 struct irq_sim_irq_ctx {
@@ -88,6 +89,28 @@ static int irq_sim_set_irqchip_state(struct irq_data *data,
 	return 0;
 }
 
+static int irq_sim_request_resources(struct irq_data *data)
+{
+	struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data);
+	struct irq_sim_work_ctx *work_ctx = irq_ctx->work_ctx;
+	irq_hw_number_t hwirq = irqd_to_hwirq(data);
+
+	blocking_notifier_call_chain(&work_ctx->notifier,
+				     IRQ_SIM_DOMAIN_IRQ_REQUESTED, &hwirq);
+
+	return 0;
+}
+
+static void irq_sim_release_resources(struct irq_data *data)
+{
+	struct irq_sim_irq_ctx *irq_ctx = irq_data_get_irq_chip_data(data);
+	struct irq_sim_work_ctx *work_ctx = irq_ctx->work_ctx;
+	irq_hw_number_t hwirq = irqd_to_hwirq(data);
+
+	blocking_notifier_call_chain(&work_ctx->notifier,
+				     IRQ_SIM_DOMAIN_IRQ_RELEASED, &hwirq);
+}
+
 static struct irq_chip irq_sim_irqchip = {
 	.name			= "irq_sim",
 	.irq_mask		= irq_sim_irqmask,
@@ -95,6 +118,8 @@ static struct irq_chip irq_sim_irqchip = {
 	.irq_set_type		= irq_sim_set_type,
 	.irq_get_irqchip_state	= irq_sim_get_irqchip_state,
 	.irq_set_irqchip_state	= irq_sim_set_irqchip_state,
+	.irq_request_resources	= irq_sim_request_resources,
+	.irq_release_resources	= irq_sim_release_resources,
 };
 
 static void irq_sim_handle_irq(struct irq_work *work)
@@ -183,6 +208,7 @@ struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
 	work_ctx->irq_count = num_irqs;
 	work_ctx->work = IRQ_WORK_INIT_HARD(irq_sim_handle_irq);
 	work_ctx->pending = no_free_ptr(pending);
+	BLOCKING_INIT_NOTIFIER_HEAD(&work_ctx->notifier);
 
 	return no_free_ptr(work_ctx)->domain;
 }
@@ -242,3 +268,41 @@ struct irq_domain *devm_irq_domain_create_sim(struct device *dev,
 	return domain;
 }
 EXPORT_SYMBOL_GPL(devm_irq_domain_create_sim);
+
+/**
+ * irq_sim_domain_register_notifier - Start listening for events on this
+ *                                    interrupt simulator.
+ *
+ * @domain:     The interrupt simulator domain to register with.
+ * @nb:         Notifier block called on domain events.
+ *
+ * On success: return 0.
+ * On failure: return negative error code.
+ */
+int irq_sim_domain_register_notifier(struct irq_domain *domain,
+				     struct notifier_block *nb)
+{
+	struct irq_sim_work_ctx *work_ctx = domain->host_data;
+
+	return blocking_notifier_chain_register(&work_ctx->notifier, nb);
+}
+EXPORT_SYMBOL_GPL(irq_sim_domain_register_notifier);
+
+/**
+ * irq_sim_domain_unregister_notifier - Stop listening for events on this
+ *                                      interrupt simulator.
+ *
+ * @domain:     The interrupt simulator domain to register with.
+ * @nb:         Notifier block called on domain events.
+ *
+ * On success: return 0.
+ * On failure: return negative error code.
+ */
+int irq_sim_domain_unregister_notifier(struct irq_domain *domain,
+				       struct notifier_block *nb)
+{
+	struct irq_sim_work_ctx *work_ctx = domain->host_data;
+
+	return blocking_notifier_chain_unregister(&work_ctx->notifier, nb);
+}
+EXPORT_SYMBOL_GPL(irq_sim_domain_unregister_notifier);
-- 
2.40.1

Re: [PATCH 1/2] genirq/irq_sim: add a notifier for irqchip events

[Thomas Gleixner]

On Wed, Jun 12 2024 at 13:52, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Currently users of the interrupt simulator don't have any way of being
> notified about interrupts from the simulated domain being requested or
> released. This causes a problem for one of the users - the GPIO
> simulator - which is unable to lock the pins as interrupts.
>
> Add a blocking notifier and provide interfaces to register with it, then
> use it to notify users of the domain about interrupts being requested
> and released while also leaving space for future extensions.

Why a notifier?

There is only one usage site per simulator domain. So there is no reason
to have a notifier with handwaving about future extensions.

The right thing to do is:

typedef void (*irq_sim_cb_t)(irq_hw_number_t hwirq, bool request, void *data)

struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
					 unsigned int num_irqs,
                                         irq_sim_cb_t *cb, void *cb_data);

You get the idea, right?

Thanks,

        tglx

Re: [PATCH 1/2] genirq/irq_sim: add a notifier for irqchip events

[Bartosz Golaszewski]

On Fri, 21 Jun 2024 17:40:00 +0200, Thomas Gleixner <tglx@linutronix.de> said:
> On Wed, Jun 12 2024 at 13:52, Bartosz Golaszewski wrote:
>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> Currently users of the interrupt simulator don't have any way of being
>> notified about interrupts from the simulated domain being requested or
>> released. This causes a problem for one of the users - the GPIO
>> simulator - which is unable to lock the pins as interrupts.
>>
>> Add a blocking notifier and provide interfaces to register with it, then
>> use it to notify users of the domain about interrupts being requested
>> and released while also leaving space for future extensions.
>
> Why a notifier?
>
> There is only one usage site per simulator domain. So there is no reason
> to have a notifier with handwaving about future extensions.
>
> The right thing to do is:
>
> typedef void (*irq_sim_cb_t)(irq_hw_number_t hwirq, bool request, void *data)
>
> struct irq_domain *irq_domain_create_sim(struct fwnode_handle *fwnode,
> 					 unsigned int num_irqs,
>                                          irq_sim_cb_t *cb, void *cb_data);
>
> You get the idea, right?
>


If you're opposed to the notifier, can we at least make it somewhat
future-proof and more elegant with the following?

struct irq_sim_ops {
	int (*irq_sim_irq_requested)(irq_hw_number_t hwirq , void *data);
	int (*irq_sim_irq_released)(irq_hw_number_t hwirq, void *data);
};

struct irq_domain *irq_domain_create_sim_ext(struct fwnode_handle *fwnode,
					     unsigned int num_irqs,
					     const struct irq_sim_ops *ops,
					     void *data);

This way we don't have to change the other call-site over at IIO at all nor
will need to change the prototype for irq_domain_create_sim_ext() if another
callback is needed.

Bart

Re: [PATCH 1/2] genirq/irq_sim: add a notifier for irqchip events

[Thomas Gleixner]

On Fri, Jun 21 2024 at 11:59, Bartosz Golaszewski wrote:
> On Fri, 21 Jun 2024 17:40:00 +0200, Thomas Gleixner <tglx@linutronix.de> said:
> If you're opposed to the notifier, can we at least make it somewhat
> future-proof and more elegant with the following?
>
> struct irq_sim_ops {
> 	int (*irq_sim_irq_requested)(irq_hw_number_t hwirq , void *data);
> 	int (*irq_sim_irq_released)(irq_hw_number_t hwirq, void *data);

release wants to be void.

> };
>
> struct irq_domain *irq_domain_create_sim_ext(struct fwnode_handle *fwnode,
> 					     unsigned int num_irqs,
> 					     const struct irq_sim_ops *ops,
> 					     void *data);
>
> This way we don't have to change the other call-site over at IIO at all nor
> will need to change the prototype for irq_domain_create_sim_ext() if another
> callback is needed.

I'm fine with that. It's at least well defined, while the notifier
business is not :)

Thanks,

        tglx

[PATCH 2/2] gpio: sim: lock GPIOs as interrupts when they are requested

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Use the notifier exposed by the interrupt simulator to be notified about
interrupts associated with simulated GPIO pins being requested or
released and lock/unlock them as interrupts accordingly.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/gpio-sim.c | 30 ++++++++++++++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-sim.c b/drivers/gpio/gpio-sim.c
index 2ed5cbe7c8a8..b5526f09b22b 100644
--- a/drivers/gpio/gpio-sim.c
+++ b/drivers/gpio/gpio-sim.c
@@ -53,6 +53,7 @@ struct gpio_sim_chip {
 	struct irq_domain *irq_sim;
 	struct mutex lock;
 	const struct attribute_group **attr_groups;
+	struct notifier_block nb;
 };
 
 struct gpio_sim_attribute {
@@ -227,6 +228,24 @@ static void gpio_sim_free(struct gpio_chip *gc, unsigned int offset)
 	}
 }
 
+static int gpio_sim_irq_domain_notify(struct notifier_block *nb,
+				      unsigned long action, void *data)
+{
+	struct gpio_sim_chip *chip = container_of(nb, struct gpio_sim_chip, nb);
+	irq_hw_number_t *offset = data;
+
+	switch (action) {
+	case IRQ_SIM_DOMAIN_IRQ_REQUESTED:
+		gpiochip_lock_as_irq(&chip->gc, *offset);
+		return NOTIFY_OK;
+	case IRQ_SIM_DOMAIN_IRQ_RELEASED:
+		gpiochip_unlock_as_irq(&chip->gc, *offset);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
 static void gpio_sim_dbg_show(struct seq_file *seq, struct gpio_chip *gc)
 {
 	struct gpio_sim_chip *chip = gpiochip_get_data(gc);
@@ -322,13 +341,15 @@ static void gpio_sim_put_device(void *data)
 	put_device(dev);
 }
 
-static void gpio_sim_dispose_mappings(void *data)
+static void gpio_sim_teardown_irq_sim(void *data)
 {
 	struct gpio_sim_chip *chip = data;
 	unsigned int i;
 
 	for (i = 0; i < chip->gc.ngpio; i++)
 		irq_dispose_mapping(irq_find_mapping(chip->irq_sim, i));
+
+	irq_sim_domain_unregister_notifier(chip->irq_sim, &chip->nb);
 }
 
 static void gpio_sim_sysfs_remove(void *data)
@@ -454,7 +475,12 @@ static int gpio_sim_add_bank(struct fwnode_handle *swnode, struct device *dev)
 	if (IS_ERR(chip->irq_sim))
 		return PTR_ERR(chip->irq_sim);
 
-	ret = devm_add_action_or_reset(dev, gpio_sim_dispose_mappings, chip);
+	chip->nb.notifier_call = gpio_sim_irq_domain_notify;
+	ret = irq_sim_domain_register_notifier(chip->irq_sim, &chip->nb);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, gpio_sim_teardown_irq_sim, chip);
 	if (ret)
 		return ret;
 
-- 
2.40.1