[PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

From: Linus Walleij <linus.walleij@linaro.org>

Currently the kernel is ambigously treating GPIOs and interrupts
from a GPIO controller: GPIOs and interrupts are treated as
orthogonal. This unfortunately makes it unclear how to actually
retrieve and request a GPIO line or interrupt from a GPIO
controller in the device tree probe path.

In the non-DT probe path it is clear that the GPIO number has to
be passed to the consuming device, and if it is to be used as
an interrupt, the consumer shall performa a gpio_to_irq() mapping
and request the resulting IRQ number.

In the DT boot path consumers may have been given one or more
interrupts from the interrupt-controller side of the GPIO chip
in an abstract way, such that the driver is not aware of the
fact that a GPIO chip is backing this interrupt, and the GPIO
side of the same line is never requested with gpio_request().
A typical case for this is ethernet chips which just take some
interrupt line which may be a "hard" interrupt line (such as an
external interrupt line from an SoC) or a cascaded interrupt
connected to a GPIO line.

This has the following undesired effects:

- The GPIOlib subsystem is not aware that the line is in use
  and willingly lets some other consumer perform gpio_request()
  on it, leading to a complex resource conflict if it occurs.

- The GPIO debugfs file claims this GPIO line is "free".

- The line direction of the interrupt GPIO line is not
  explicitly set as input, even though it is obvious that such
  a line need to be set up in this way, often making the system
  depend on boot-on defaults for this kind of settings.

To solve this dilemma, perform an interrupt consistency check
when adding a GPIO chip: if the chip is both gpio-controller and
interrupt-controller, walk all children of the device tree,
check if these in turn reference the interrupt-controller, and
if they do, loop over the interrupts used by that child and
perform gpio_request() and gpio_direction_input() on these,
making them unreachable from the GPIO side.

The patch has been devised by Linus Walleij and Lars Poeschel.

Changelog V2:
- To be able to parse custom interrupts properties from the
  device tree, get a reference to the drivers irq_domain
  and use the xlate function to parse the proptery and
  get the irq number. This is tested with
  #interrupt-cells = 1, 2, and 3 and multiple interrupts
  per property.

Cc: devicetree@vger.kernel.org
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Balaji T K <balajitk@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Jon Hunter <jgchunter@gmail.com>
Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 665f953..b42cdd7 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -10,7 +10,6 @@
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  */
-
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/module.h>
@@ -19,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_gpio.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/slab.h>
 
@@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
 EXPORT_SYMBOL(of_gpio_simple_xlate);
 
 /**
+ * of_gpio_scan_irq_lines() - internal function to recursively scan the device
+ * tree and request or free the GPIOs that are to be used as IRQ lines
+ * @node:	node to start the scanning at
+ * @gcn:	device node of the GPIO chip
+ * @irq_domain:	the irq_domain for the GPIO chip
+ * @intsize:	size of one single interrupt in the device tree for the GPIO
+ *		chip. It is the same as #interrupt-cells.
+ * @gc:		GPIO chip instantiated from same node
+ * @request:	wheter the function should request(true) or free(false) the
+ *		irq lines
+ *
+ * This is a internal function that calls itself to recursively scan the device
+ * tree. It scans for uses of the device_node gcn as an interrupt-controller.
+ * If it finds some, it requests the corresponding gpio lines that are to be
+ * used as irq lines and sets them as input.
+ *
+ * If the request parameter is 0 it frees the gpio lines.
+ * For more information see documentation of of_gpiochip_reserve_irq_lines
+ * function.
+ */
+static void of_gpio_scan_irq_lines(const struct device_node *const node,
+				   struct device_node *const gcn,
+				   struct irq_domain *const irq_domain,
+				   const u32 intsize,
+				   const struct gpio_chip * const gc,
+				   bool request)
+{
+	struct device_node *child;
+	struct device_node *irq_parent;
+	const __be32 *intspec;
+	u32 intlen;
+	int ret;
+	int i;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+
+	if (node == NULL)
+		return;
+
+	for_each_child_of_node(node, child) {
+		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
+				       request);
+		/* Check if we have an IRQ parent, else continue */
+		irq_parent = of_irq_find_parent(child);
+		if (!irq_parent)
+			continue;
+
+		/* Is it so that this very GPIO chip is the parent? */
+		if (irq_parent != gcn) {
+			of_node_put(irq_parent);
+			continue;
+		}
+		of_node_put(irq_parent);
+
+		pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
+				child->name);
+
+		/* Get the interrupts property */
+		intspec = of_get_property(child, "interrupts", &intlen);
+		if (intspec == NULL)
+			continue;
+		intlen /= sizeof(*intspec);
+
+		for (i = 0; i < intlen; i += intsize) {
+			/*
+			 * Find out the local IRQ number. This corresponds to
+			 * the GPIO line offset for a GPIO chip.
+			 */
+			if (irq_domain && irq_domain->ops->xlate)
+				irq_domain->ops->xlate(irq_domain, gcn,
+						       intspec + i, intsize,
+						       &hwirq, &type);
+			else
+				hwirq = intspec[0];
+
+			hwirq = be32_to_cpu(hwirq);
+			pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
+				child->name, gc->base + hwirq, hwirq);
+
+			if (request) {
+				/*
+				 * This child is making a reference to this
+				 * chip through the interrupts property, so
+				 * reserve these GPIO lines and set them as
+				 * input.
+				 */
+				ret = gpio_request(gc->base + hwirq,
+						   child->name);
+				if (ret)
+					pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node %s (%d)\n",
+						gc->base + hwirq, hwirq,
+						child->name, ret);
+				ret = gpio_direction_input(gc->base + hwirq);
+				if (ret)
+					pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for node %s (%d)\n",
+						gc->base + hwirq, hwirq,
+						child->name, ret);
+			} else {
+				gpio_free(gc->base + hwirq);
+			}
+		}
+	}
+}
+
+/**
+ * of_gpiochip_reserve_irq_lines() - request or free GPIO IRQ lines
+ * @np:		device node of the GPIO chip
+ * @gc:		GPIO chip instantiated from same node
+ * @request:	wheter the function should request(1) or free(0) the irq lines
+ *
+ * This function should not be used directly, use the macros
+ * of_gpiochip_request_irq_lines or of_gpiochip_free_gpio_lines instead.
+ *
+ * For the case of requesting the irq lines (request == 1) this function is
+ * called after instantiating a GPIO chip from a device tree node to assert
+ * that all interrupts derived from the chip are consistently requested as
+ * GPIO lines, if the GPIO chip is BOTH a gpio-controller AND an
+ * interrupt-controller.
+ *
+ * If another node in the device tree is referencing the interrupt-controller
+ * portions of the GPIO chip, such that it is using a GPIO line as some
+ * arbitrary interrupt source, the following holds:
+ *
+ * - That line must NOT be used anywhere else in the device tree as a
+ *   <&gpio N> reference, or GPIO and interrupt usage may conflict.
+ *
+ * Conversely, if a node is using a line as a direct reference to a GPIO line,
+ * no node in the tree may use that line as an interrupt.
+ *
+ * If another node is referencing a GPIO line, and also want to use that line
+ * as an interrupt source, it is necessary for this driver to use the
+ * gpio_to_irq() kernel interface.
+ *
+ * For the case of freeing the irq lines (request == 0) this function simply
+ * uses the same device tree information used to request the irq lines to call
+ * gpiochip_free on that GPIOs.
+ */
+static void of_gpiochip_reserve_irq_lines(struct device_node *np,
+					  struct gpio_chip *gc, bool request)
+{
+	struct device_node *root;
+	const __be32 *tmp;
+	struct irq_domain *irq_domain;
+	u32 intsize;
+
+	/*
+	 * If this chip is not tagged as interrupt-controller, there is
+	 * no problem so we just exit.
+	 */
+	if (!of_property_read_bool(np, "interrupt-controller"))
+		return;
+
+	/*
+	 * Proceed to check the consistency of all references to this
+	 * GPIO chip.
+	 */
+	root = of_find_node_by_path("/");
+	if (!root)
+		return;
+
+	tmp = of_get_property(np, "#interrupt-cells", NULL);
+	if (tmp == NULL)
+		intsize = 1;
+	else
+		intsize = be32_to_cpu(*tmp);
+
+	irq_domain = irq_find_host(np);
+
+	of_gpio_scan_irq_lines(root, np, irq_domain, intsize, gc, request);
+	of_node_put(root);
+}
+
+#define of_gpiochip_request_irq_lines(np, gc) \
+	of_gpiochip_reserve_irq_lines(np, gc, true)
+
+#define of_gpiochip_free_irq_lines(np, gc) \
+	of_gpiochip_reserve_irq_lines(np, gc, false)
+
+/**
  * of_mm_gpiochip_add - Add memory mapped GPIO chip (bank)
  * @np:		device node of the GPIO chip
  * @mm_gc:	pointer to the of_mm_gpio_chip allocated structure
@@ -170,6 +349,8 @@ int of_mm_gpiochip_add(struct device_node *np,
 	if (ret)
 		goto err2;
 
+	of_gpiochip_request_irq_lines(np, gc);
+
 	return 0;
 err2:
 	iounmap(mm_gc->regs);
@@ -231,12 +412,14 @@ void of_gpiochip_add(struct gpio_chip *chip)
 		chip->of_xlate = of_gpio_simple_xlate;
 	}
 
+	of_gpiochip_request_irq_lines(chip->of_node, chip);
 	of_gpiochip_add_pin_range(chip);
 	of_node_get(chip->of_node);
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
 {
+	of_gpiochip_free_irq_lines(chip->of_node, chip);
 	gpiochip_remove_pin_ranges(chip);
 
 	if (chip->of_node)
-- 
1.7.10.4

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

Hi Lars, Linus,

On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> Currently the kernel is ambigously treating GPIOs and interrupts
> from a GPIO controller: GPIOs and interrupts are treated as
> orthogonal. This unfortunately makes it unclear how to actually
> retrieve and request a GPIO line or interrupt from a GPIO
> controller in the device tree probe path.
> 
> In the non-DT probe path it is clear that the GPIO number has to
> be passed to the consuming device, and if it is to be used as
> an interrupt, the consumer shall performa a gpio_to_irq() mapping
> and request the resulting IRQ number.
> 
> In the DT boot path consumers may have been given one or more
> interrupts from the interrupt-controller side of the GPIO chip
> in an abstract way, such that the driver is not aware of the
> fact that a GPIO chip is backing this interrupt, and the GPIO
> side of the same line is never requested with gpio_request().
> A typical case for this is ethernet chips which just take some
> interrupt line which may be a "hard" interrupt line (such as an
> external interrupt line from an SoC) or a cascaded interrupt
> connected to a GPIO line.
> 
> This has the following undesired effects:
> 
> - The GPIOlib subsystem is not aware that the line is in use
>   and willingly lets some other consumer perform gpio_request()
>   on it, leading to a complex resource conflict if it occurs.
> 
> - The GPIO debugfs file claims this GPIO line is "free".
> 
> - The line direction of the interrupt GPIO line is not
>   explicitly set as input, even though it is obvious that such
>   a line need to be set up in this way, often making the system
>   depend on boot-on defaults for this kind of settings.
> 
> To solve this dilemma, perform an interrupt consistency check
> when adding a GPIO chip: if the chip is both gpio-controller and
> interrupt-controller, walk all children of the device tree,
> check if these in turn reference the interrupt-controller, and
> if they do, loop over the interrupts used by that child and
> perform gpio_request() and gpio_direction_input() on these,
> making them unreachable from the GPIO side.
> 
> The patch has been devised by Linus Walleij and Lars Poeschel.
> 
> Changelog V2:
> - To be able to parse custom interrupts properties from the
>   device tree, get a reference to the drivers irq_domain
>   and use the xlate function to parse the proptery and
>   get the irq number. This is tested with
>   #interrupt-cells = 1, 2, and 3 and multiple interrupts
>   per property.

This looks much better now, but I still can imagine potential problems.
See my comments inline.

> Cc: devicetree@vger.kernel.org
> Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Cc: Enric Balletbo i Serra <eballetbo@gmail.com>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Balaji T K <balajitk@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Jon Hunter <jgchunter@gmail.com>
> Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 665f953..b42cdd7 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -10,7 +10,6 @@
>   * the Free Software Foundation; either version 2 of the License, or
>   * (at your option) any later version.
>   */
> -
>  #include <linux/device.h>
>  #include <linux/errno.h>
>  #include <linux/module.h>
> @@ -19,6 +18,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_gpio.h>
> +#include <linux/of_irq.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/slab.h>
> 
> @@ -127,6 +127,185 @@ int of_gpio_simple_xlate(struct gpio_chip *gc,
>  EXPORT_SYMBOL(of_gpio_simple_xlate);
> 
>  /**
> + * of_gpio_scan_irq_lines() - internal function to recursively scan the
> device + * tree and request or free the GPIOs that are to be used as
> IRQ lines + * @node:	node to start the scanning at
> + * @gcn:	device node of the GPIO chip
> + * @irq_domain:	the irq_domain for the GPIO chip
> + * @intsize:	size of one single interrupt in the device tree for the
> GPIO + *		chip. It is the same as #interrupt-cells.
> + * @gc:		GPIO chip instantiated from same node
> + * @request:	wheter the function should request(true) or free(false)
> the + *		irq lines
> + *
> + * This is a internal function that calls itself to recursively scan
> the device + * tree. It scans for uses of the device_node gcn as an
> interrupt-controller. + * If it finds some, it requests the
> corresponding gpio lines that are to be + * used as irq lines and sets
> them as input.
> + *
> + * If the request parameter is 0 it frees the gpio lines.
> + * For more information see documentation of
> of_gpiochip_reserve_irq_lines + * function.
> + */
> +static void of_gpio_scan_irq_lines(const struct device_node *const
> node, +				   struct device_node *const gcn,
> +				   struct irq_domain *const irq_domain,
> +				   const u32 intsize,
> +				   const struct gpio_chip * const gc,
> +				   bool request)
> +{
> +	struct device_node *child;
> +	struct device_node *irq_parent;
> +	const __be32 *intspec;
> +	u32 intlen;
> +	int ret;
> +	int i;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +
> +	if (node == NULL)
> +		return;
> +
> +	for_each_child_of_node(node, child) {
> +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> +				       request);
> +		/* Check if we have an IRQ parent, else continue */
> +		irq_parent = of_irq_find_parent(child);
> +		if (!irq_parent)
> +			continue;
> +
> +		/* Is it so that this very GPIO chip is the parent? */
> +		if (irq_parent != gcn) {
> +			of_node_put(irq_parent);
> +			continue;
> +		}
> +		of_node_put(irq_parent);
> +
> +		pr_debug("gpiochip OF: node %s references GPIO interrupt lines\n",
> +				child->name);
> +
> +		/* Get the interrupts property */
> +		intspec = of_get_property(child, "interrupts", &intlen);
> +		if (intspec == NULL)
> +			continue;
> +		intlen /= sizeof(*intspec);
> +
> +		for (i = 0; i < intlen; i += intsize) {
> +			/*
> +			 * Find out the local IRQ number. This corresponds to
> +			 * the GPIO line offset for a GPIO chip.

I'm still not convinced that this assumption is correct. This code will
behave erraticaly in cases where it is not true, requesting innocent GPIO
pins.

> +			 */
> +			if (irq_domain && irq_domain->ops->xlate)
> +				irq_domain->ops->xlate(irq_domain, gcn,
> +						       intspec + i, intsize,
> +						       &hwirq, &type);
> +			else
> +				hwirq = intspec[0];

Is it a correct fallback when irq_domain is NULL?

> +
> +			hwirq = be32_to_cpu(hwirq);

Is this conversion correct? I don't think hwirq could be big endian here
(unless running on a big endian CPU).

> +			pr_debug("gpiochip OF: node %s references GPIO %lu (%lu)\n",
> +				child->name, gc->base + hwirq, hwirq);
> +
> +			if (request) {
> +				/*
> +				 * This child is making a reference to this
> +				 * chip through the interrupts property, so
> +				 * reserve these GPIO lines and set them as
> +				 * input.
> +				 */
> +				ret = gpio_request(gc->base + hwirq,
> +						   child->name);
> +				if (ret)
> +					pr_err("gpiolib OF: could not request IRQ GPIO %lu (%lu) for node
> %s (%d)\n", +						gc->base + hwirq, hwirq,
> +						child->name, ret);
> +				ret = gpio_direction_input(gc->base + hwirq);
> +				if (ret)
> +					pr_err("gpiolib OF: could not set IRQ GPIO %lu (%lu) as input for
> node %s (%d)\n", +						gc->base + hwirq, hwirq,
> +						child->name, ret);
> +			} else {
> +				gpio_free(gc->base + hwirq);
> +			}
> +		}
> +	}
> +}
> +
> +/**
> + * of_gpiochip_reserve_irq_lines() - request or free GPIO IRQ lines
> + * @np:		device node of the GPIO chip
> + * @gc:		GPIO chip instantiated from same node
> + * @request:	wheter the function should request(1) or free(0) the irq
> lines + *
> + * This function should not be used directly, use the macros
> + * of_gpiochip_request_irq_lines or of_gpiochip_free_gpio_lines
> instead. + *
> + * For the case of requesting the irq lines (request == 1) this
> function is + * called after instantiating a GPIO chip from a device
> tree node to assert + * that all interrupts derived from the chip are
> consistently requested as + * GPIO lines, if the GPIO chip is BOTH a
> gpio-controller AND an + * interrupt-controller.
> + *
> + * If another node in the device tree is referencing the
> interrupt-controller + * portions of the GPIO chip, such that it is
> using a GPIO line as some + * arbitrary interrupt source, the following
> holds:
> + *
> + * - That line must NOT be used anywhere else in the device tree as a
> + *   <&gpio N> reference, or GPIO and interrupt usage may conflict.
> + *
> + * Conversely, if a node is using a line as a direct reference to a
> GPIO line, + * no node in the tree may use that line as an interrupt.
> + *
> + * If another node is referencing a GPIO line, and also want to use
> that line + * as an interrupt source, it is necessary for this driver
> to use the + * gpio_to_irq() kernel interface.
> + *
> + * For the case of freeing the irq lines (request == 0) this function
> simply + * uses the same device tree information used to request the
> irq lines to call + * gpiochip_free on that GPIOs.
> + */
> +static void of_gpiochip_reserve_irq_lines(struct device_node *np,
> +					  struct gpio_chip *gc, bool request)
> +{
> +	struct device_node *root;
> +	const __be32 *tmp;
> +	struct irq_domain *irq_domain;
> +	u32 intsize;
> +
> +	/*
> +	 * If this chip is not tagged as interrupt-controller, there is
> +	 * no problem so we just exit.
> +	 */
> +	if (!of_property_read_bool(np, "interrupt-controller"))
> +		return;
> +
> +	/*
> +	 * Proceed to check the consistency of all references to this
> +	 * GPIO chip.
> +	 */
> +	root = of_find_node_by_path("/");
> +	if (!root)
> +		return;
> +
> +	tmp = of_get_property(np, "#interrupt-cells", NULL);
> +	if (tmp == NULL)
> +		intsize = 1;
> +	else
> +		intsize = be32_to_cpu(*tmp);
> +
> +	irq_domain = irq_find_host(np);

I'm not sure you can do too much if irq_find_host() fails to find the
domain you are looking for. I guess you can just bail out in this case.
However...

I believe this imposes some ordering requirement between GPIO and IRQ chip
initialization. For this code to work correctly, all GPIO/IRQ controller
drivers would have to register the IRQ controller part first and only then
the GPIO chip.

Best regards,
Tomasz

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> Hi Lars, Linus,
> 
> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>> From: Linus Walleij <linus.walleij@linaro.org>
>>
>> Currently the kernel is ambigously treating GPIOs and interrupts
>> from a GPIO controller: GPIOs and interrupts are treated as
>> orthogonal. This unfortunately makes it unclear how to actually
>> retrieve and request a GPIO line or interrupt from a GPIO
>> controller in the device tree probe path.
>>
>> In the non-DT probe path it is clear that the GPIO number has to
>> be passed to the consuming device, and if it is to be used as
>> an interrupt, the consumer shall performa a gpio_to_irq() mapping
>> and request the resulting IRQ number.
>>
>> In the DT boot path consumers may have been given one or more
>> interrupts from the interrupt-controller side of the GPIO chip
>> in an abstract way, such that the driver is not aware of the
>> fact that a GPIO chip is backing this interrupt, and the GPIO
>> side of the same line is never requested with gpio_request().
>> A typical case for this is ethernet chips which just take some
>> interrupt line which may be a "hard" interrupt line (such as an
>> external interrupt line from an SoC) or a cascaded interrupt
>> connected to a GPIO line.
>>
>> This has the following undesired effects:
>>
>> - The GPIOlib subsystem is not aware that the line is in use
>>   and willingly lets some other consumer perform gpio_request()
>>   on it, leading to a complex resource conflict if it occurs.
>>
>> - The GPIO debugfs file claims this GPIO line is "free".
>>
>> - The line direction of the interrupt GPIO line is not
>>   explicitly set as input, even though it is obvious that such
>>   a line need to be set up in this way, often making the system
>>   depend on boot-on defaults for this kind of settings.

That last point should simply be taken care of by the IRQ driver in the
relevant callbacks.

>> To solve this dilemma, perform an interrupt consistency check
>> when adding a GPIO chip: if the chip is both gpio-controller and
>> interrupt-controller, walk all children of the device tree,

It seems a little odd to solve this only for DT. What about the non-DT case?

>> check if these in turn reference the interrupt-controller, and
>> if they do, loop over the interrupts used by that child and
>> perform gpio_request() and gpio_direction_input() on these,
>> making them unreachable from the GPIO side.

What about bindings that require a GPIO to be specified, yet don't allow
an IRQ to be specified, and the driver internally does perform
gpio_to_irq() on it? I don't think one can detect that case.

Isn't it better to have the IRQ chip's .request() operation convert the
IRQ to a GPIO if relevant (which it can do since it has specific
knowledge of the HW) and take ownership of the GPIO at that level?

That would cover both the exceptions I pointed out above.

I vaguely recall seeing patches along those lines before, but there must
have been some other problem pointed out...

>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

>> +static void of_gpio_scan_irq_lines(const struct device_node *const

>> +		for (i = 0; i < intlen; i += intsize) {
>> +			/*
>> +			 * Find out the local IRQ number. This corresponds to
>> +			 * the GPIO line offset for a GPIO chip.
> 
> I'm still not convinced that this assumption is correct. This code will
> behave erraticaly in cases where it is not true, requesting innocent GPIO
> pins.
> 
>> +			 */
>> +			if (irq_domain && irq_domain->ops->xlate)
>> +				irq_domain->ops->xlate(irq_domain, gcn,
>> +						       intspec + i, intsize,
>> +						       &hwirq, &type);
>> +			else
>> +				hwirq = intspec[0];
> 
> Is it a correct fallback when irq_domain is NULL?

Indeed this fallback is dangerous. The /only/ way to parse an IRQ
specifier is with binding-specific knowledge, which is obtained by
calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
operation simply has to be deferred; we can't just guess and hope.

> 
>> +
>> +			hwirq = be32_to_cpu(hwirq);
> 
> Is this conversion correct? I don't think hwirq could be big endian here
> (unless running on a big endian CPU).

I think that should be inside the else branch above.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:

>>> To solve this dilemma, perform an interrupt consistency check
>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>> interrupt-controller, walk all children of the device tree,
>
> It seems a little odd to solve this only for DT. What about the non-DT case?

DT is the hardware configuration system that lets you request
the same resource in two ways, i.e. it allows one and the same
node to be both gpio-controller and interrupt-controller, and
start handing out the same line as both GPIO and IRQ
independently.

I asked if ACPI had this ambiguity, and the answer appears to
be either "no" (which I suspect) or just "nobody knows" :-/

In either way, checking the consistency of ACPI IRQs vs
GPIOs will be fundamentally different, should it have the same
problem, and does it appear we can certainly refactor this to
be shared, should there be something to gain from.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
[Me]
>>> check if these in turn reference the interrupt-controller, and
>>> if they do, loop over the interrupts used by that child and
>>> perform gpio_request() and gpio_direction_input() on these,
>>> making them unreachable from the GPIO side.
>
> What about bindings that require a GPIO to be specified, yet don't allow
> an IRQ to be specified, and the driver internally does perform
> gpio_to_irq() on it? I don't think one can detect that case.

This is still allowed. Consumers that prefer to have a GPIO
passed and convert it to IRQ by that call can still do so,
they will know what they're doing and will not cause the
double-command situation that we're trying to solve.

> Isn't it better to have the IRQ chip's .request() operation convert the
> IRQ to a GPIO if relevant (which it can do since it has specific
> knowledge of the HW) and take ownership of the GPIO at that level?

We tried this in the OMAP case, but apart from that the OMAP
driver blew up so we had to revert the patches, it also means
the same code needs to go into each and every driver
instead of solving the dilemma centrally like this.

> I vaguely recall seeing patches along those lines before, but there must
> have been some other problem pointed out...

You bet. It turns out these patches break the case which you
just described above, whereas this patch does not.

OMAP had drivers that used gpio_to_irq() *and* it had drivers
that used the GPIO controller node as interrupt parent.
So when they fixes .request() as per above, the latter started
working properly whereas the former started breaking.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >> 
> >> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >> 
> >> +		for (i = 0; i < intlen; i += intsize) {
> >> +			/*
> >> +			 * Find out the local IRQ number. This corresponds to
> >> +			 * the GPIO line offset for a GPIO chip.
> > 
> > I'm still not convinced that this assumption is correct. This code
> > will behave erraticaly in cases where it is not true, requesting
> > innocent GPIO pins.

Do you have an idea how we can destroy your doubts?
Either irq_chips nor irq_domains provide some sort of translation function 
for this.
Is there a driver in the kernel that has different gpio- vs. irq-namespaces 
where I can have a look at? How does platform code solve this translation? 
The irq_to_gpio functions in include/linux/gpio.h seem deprecated. They 
just return -EINVAL.
For me it seems, that there is no such device inside the kernel yet. 
Correct me if I'm wrong. If such a device comes to surface, we're in 
trouble. We will need some device-specific translation function then.
Is it the time to introduce an additional pointer for such a function now 
and nobody uses it? Or wait until such a device arises and introduce the 
pointer then?

> >> +			 */
> >> +			if (irq_domain && irq_domain->ops->xlate)
> >> +				irq_domain->ops->xlate(irq_domain, gcn,
> >> +						       intspec + i, intsize,
> >> +						       &hwirq, &type);
> >> +			else
> >> +				hwirq = intspec[0];
> > 
> > Is it a correct fallback when irq_domain is NULL?
> 
> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> specifier is with binding-specific knowledge, which is obtained by
> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
> operation simply has to be deferred; we can't just guess and hope.

At least the of irq mapping code make this assumption also: 
kernel/irq/irqdomain.c:483
It should be valid for us here too.
The additional assumption that I made is that if irq_domain == NULL (not 
only xlate), that we can use intspec[0] either.

> >> +
> >> +			hwirq = be32_to_cpu(hwirq);
> > 
> > Is this conversion correct? I don't think hwirq could be big endian
> > here (unless running on a big endian CPU).
> 
> I think that should be inside the else branch above.

No it has to be in both branches as it is. Device tree data is big endian. 
The conversion is converting big endian data (from device tree in both 
cases) to cpu endianess and not coverting TO big endian.
My test machine is a arm in little endian mode and it provided wrong values 
if I did not do the conversion.
What I am a bit unsure about is if the xlate function is expecting the 
intspec pointer to point to big endian device tree data or data already 
converted to cpu endianess. For the standard xlate functions 
irq_domain_xlate_[one|two|onetwo]cell it does not matter.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Andreas Larsson]

On 2013-08-21 15:38, Lars Poeschel wrote:
> +static void of_gpio_scan_irq_lines(const struct device_node *const node,
> +				   struct device_node *const gcn,
> +				   struct irq_domain *const irq_domain,
> +				   const u32 intsize,
> +				   const struct gpio_chip * const gc,
> +				   bool request)
> +{
> +	struct device_node *child;
> +	struct device_node *irq_parent;
> +	const __be32 *intspec;
> +	u32 intlen;
> +	int ret;
> +	int i;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +
> +	if (node == NULL)
> +		return;
> +
> +	for_each_child_of_node(node, child) {
> +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> +				       request);
> +		/* Check if we have an IRQ parent, else continue */
> +		irq_parent = of_irq_find_parent(child);

Hi!

This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the 
fact that the function is undefined when !defined(CONFIG_OF_IRQ) && 
defined(CONFIG_OF).

Defining the empty of_irq_find_parent in include/linux/of_irq.h when 
!defined(CONFIG_OF_IRQ) instead of the current case when 
!defined(CONFIG_OF) would solve the immediate compilation problem.

However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the 
whole tree walking will never accomplish anything, so it would be good 
if of_gpiochip_reserve_irq_lines is just an empty dummy or something 
like that when !defined(CONFIG_OF_IRQ).

Cheers,
Andreas Larsson

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/21/2013 05:27 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
> 
>>>> To solve this dilemma, perform an interrupt consistency check
>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>> interrupt-controller, walk all children of the device tree,
>>
>> It seems a little odd to solve this only for DT. What about the non-DT case?
> 
> DT is the hardware configuration system that lets you request
> the same resource in two ways, i.e. it allows one and the same
> node to be both gpio-controller and interrupt-controller, and
> start handing out the same line as both GPIO and IRQ
> independently.

Huh? What stops systems using board files and platform data from having
this issue?

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/22/2013 03:01 AM, Lars Poeschel wrote:
> On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
>> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
>>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c

>>>> +			 */
>>>> +			if (irq_domain && irq_domain->ops->xlate)
>>>> +				irq_domain->ops->xlate(irq_domain, gcn,
>>>> +						       intspec + i, intsize,
>>>> +						       &hwirq, &type);
>>>> +			else
>>>> +				hwirq = intspec[0];
>>>
>>> Is it a correct fallback when irq_domain is NULL?
>>
>> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
>> specifier is with binding-specific knowledge, which is obtained by
>> calling irq_domain->ops->xlate(). If the IRQ domain can't be found, this
>> operation simply has to be deferred; we can't just guess and hope.
> 
> At least the of irq mapping code make this assumption also: 
> kernel/irq/irqdomain.c:483
> It should be valid for us here too.
> The additional assumption that I made is that if irq_domain == NULL (not 
> only xlate), that we can use intspec[0] either.

OK, I guess it's likely this won't cause any additional issue then. I
suspect most IRQ domains use within the context of device tree already
provide an explicit xlate op anyway; for example irq_domain_simple_ops
points at the default irq_domain_xlate_onetwocell.

>>>> +
>>>> +			hwirq = be32_to_cpu(hwirq);
>>>
>>> Is this conversion correct? I don't think hwirq could be big endian
>>> here (unless running on a big endian CPU).
>>
>> I think that should be inside the else branch above.
> 
> No it has to be in both branches as it is. Device tree data is big endian. 
> The conversion is converting big endian data (from device tree in both 
> cases) to cpu endianess and not coverting TO big endian.
> My test machine is a arm in little endian mode and it provided wrong values 
> if I did not do the conversion.
> What I am a bit unsure about is if the xlate function is expecting the 
> intspec pointer to point to big endian device tree data or data already 
> converted to cpu endianess. For the standard xlate functions 
> irq_domain_xlate_[one|two|onetwo]cell it does not matter.

The xlate function assumes that data is already converted to CPU-endian.
See:

irq_of_parse_and_map() ->
    of_irq_map_one() ->
        of_irq_map_raw() ->
            out_irq->specifier[i] = of_read_number(intspec +i, 1);
    irq_create_of_mapping()

(of_read_number does the be32_to_cpu() internally)

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/21/2013 05:36 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> [Me]
>>>> check if these in turn reference the interrupt-controller, and
>>>> if they do, loop over the interrupts used by that child and
>>>> perform gpio_request() and gpio_direction_input() on these,
>>>> making them unreachable from the GPIO side.
>>
>> What about bindings that require a GPIO to be specified, yet don't allow
>> an IRQ to be specified, and the driver internally does perform
>> gpio_to_irq() on it? I don't think one can detect that case.
> 
> This is still allowed. Consumers that prefer to have a GPIO
> passed and convert it to IRQ by that call can still do so,
> they will know what they're doing and will not cause the
> double-command situation that we're trying to solve.

Why not? There are certainly drivers in the kernel which request a GPIO
as both a GPIO and as an (dual-edge) interrupt, so that they can read
the GPIO input whenever the IRQ goes off, in order to determine the pin
state. This is safer against high-latency or lost interrupts.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

On Thursday 22 of August 2013 15:08:12 Stephen Warren wrote:
> On 08/22/2013 03:01 AM, Lars Poeschel wrote:
> > On Thursday 22 August 2013 at 01:10:27, Stephen Warren wrote:
> >> On 08/21/2013 03:49 PM, Tomasz Figa wrote:
> >>>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> >>>> 
> >>>> +			 */
> >>>> +			if (irq_domain && irq_domain->ops->xlate)
> >>>> +				irq_domain->ops->xlate(irq_domain, 
gcn,
> >>>> +						       intspec + 
i, intsize,
> >>>> +						       &hwirq, 
&type);
> >>>> +			else
> >>>> +				hwirq = intspec[0];
> >>> 
> >>> Is it a correct fallback when irq_domain is NULL?
> >> 
> >> Indeed this fallback is dangerous. The /only/ way to parse an IRQ
> >> specifier is with binding-specific knowledge, which is obtained by
> >> calling irq_domain->ops->xlate(). If the IRQ domain can't be found,
> >> this operation simply has to be deferred; we can't just guess and
> >> hope.> 
> > At least the of irq mapping code make this assumption also:
> > kernel/irq/irqdomain.c:483
> > It should be valid for us here too.
> > The additional assumption that I made is that if irq_domain == NULL
> > (not only xlate), that we can use intspec[0] either.
> 
> OK, I guess it's likely this won't cause any additional issue then. I
> suspect most IRQ domains use within the context of device tree already
> provide an explicit xlate op anyway; for example irq_domain_simple_ops
> points at the default irq_domain_xlate_onetwocell.

We got away from the problem I pointed in my reply. If irq_domain == NULL, 
there is no way to translate specifier to hwirq (and in what domain such 
hwirq would be in anyway?).

> >>>> +
> >>>> +			hwirq = be32_to_cpu(hwirq);
> >>> 
> >>> Is this conversion correct? I don't think hwirq could be big endian
> >>> here (unless running on a big endian CPU).
> >> 
> >> I think that should be inside the else branch above.
> > 
> > No it has to be in both branches as it is. Device tree data is big
> > endian. The conversion is converting big endian data (from device
> > tree in both cases) to cpu endianess and not coverting TO big endian.
> > My test machine is a arm in little endian mode and it provided wrong
> > values if I did not do the conversion.
> > What I am a bit unsure about is if the xlate function is expecting the
> > intspec pointer to point to big endian device tree data or data
> > already
> > converted to cpu endianess. For the standard xlate functions
> > irq_domain_xlate_[one|two|onetwo]cell it does not matter.
> 
> The xlate function assumes that data is already converted to CPU-endian.
> See:
> 
> irq_of_parse_and_map() ->
>     of_irq_map_one() ->
>         of_irq_map_raw() ->
>             out_irq->specifier[i] = of_read_number(intspec +i, 1);
>     irq_create_of_mapping()
> 
> (of_read_number does the be32_to_cpu() internally)

So basically to be correct, the code here would need to read the specifier 
into internal buffer using a helper like of_read_number() or maybe even 
of_property_read_u32_array() and then pass this buffer to xlate().

Best regards,
Tomasz

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> > <swarren@wwwdotorg.org> wrote: [Me]
> > 
> >>>> check if these in turn reference the interrupt-controller, and
> >>>> if they do, loop over the interrupts used by that child and
> >>>> perform gpio_request() and gpio_direction_input() on these,
> >>>> making them unreachable from the GPIO side.
> >> 
> >> What about bindings that require a GPIO to be specified, yet don't
> >> allow an IRQ to be specified, and the driver internally does perform
> >> gpio_to_irq() on it? I don't think one can detect that case.
> > 
> > This is still allowed. Consumers that prefer to have a GPIO
> > passed and convert it to IRQ by that call can still do so,
> > they will know what they're doing and will not cause the
> > double-command situation that we're trying to solve.
> 
> Why not? There are certainly drivers in the kernel which request a GPIO
> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> the GPIO input whenever the IRQ goes off, in order to determine the pin
> state. This is safer against high-latency or lost interrupts.

This is the point! They REQUEST the GPIO. They can then do whatever they 
like with this GPIO then, even additionally use it as interrupt.
In the devicetree case the interrupts are not requested. This is what we 
are trying to address with the patch. The device using this interrupt has 
no idea where this interrupt comes from. Is it a gpio-interrupt or not? 
Does it have to request a gpio before using this interrupt or not?

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Thursday 22 August 2013 at 22:53:09, Stephen Warren wrote:
> On 08/21/2013 05:27 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> >>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
> >>>> To solve this dilemma, perform an interrupt consistency check
> >>>> when adding a GPIO chip: if the chip is both gpio-controller and
> >>>> interrupt-controller, walk all children of the device tree,
> >> 
> >> It seems a little odd to solve this only for DT. What about the
> >> non-DT case?
> > 
> > DT is the hardware configuration system that lets you request
> > the same resource in two ways, i.e. it allows one and the same
> > node to be both gpio-controller and interrupt-controller, and
> > start handing out the same line as both GPIO and IRQ
> > independently.
> 
> Huh? What stops systems using board files and platform data from having
> this issue?

I am not 100% sure, Linus knows better.
I think nothing stops them from having this issue, but board files are 
gentle and request the GPIO before doing gpio_to_irq, because they know 
that they are using a gpio based interrupt.

You can read the whole story here:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg91405.html
Things get interesting after the first mail from Alexander Holler, who is 
the first having a problem with the patch in the link.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/21/2013 05:27 PM, Linus Walleij wrote:
>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>>
>>>>> To solve this dilemma, perform an interrupt consistency check
>>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>>> interrupt-controller, walk all children of the device tree,
>>>
>>> It seems a little odd to solve this only for DT. What about the non-DT case?
>>
>> DT is the hardware configuration system that lets you request
>> the same resource in two ways, i.e. it allows one and the same
>> node to be both gpio-controller and interrupt-controller, and
>> start handing out the same line as both GPIO and IRQ
>> independently.
>
> Huh? What stops systems using board files and platform data from having
> this issue?

It can't be stopped but I consider it a bug if they do, as the proper
way to handle such GPIO lines is the sequence:

request_gpio(gpio);
request_irq(gpio_to_irq(gpio));

But I was mainly contrasting against ACPI, where the problem appears
to not exist.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> [Me]
>>>>> check if these in turn reference the interrupt-controller, and
>>>>> if they do, loop over the interrupts used by that child and
>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>> making them unreachable from the GPIO side.
>>>
>>> What about bindings that require a GPIO to be specified, yet don't allow
>>> an IRQ to be specified, and the driver internally does perform
>>> gpio_to_irq() on it? I don't think one can detect that case.
>>
>> This is still allowed. Consumers that prefer to have a GPIO
>> passed and convert it to IRQ by that call can still do so,
>> they will know what they're doing and will not cause the
>> double-command situation that we're trying to solve.
>
> Why not? There are certainly drivers in the kernel which request a GPIO
> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> the GPIO input whenever the IRQ goes off, in order to determine the pin
> state. This is safer against high-latency or lost interrupts.

Yes? Are we talking past each other here?

This is a perfectly OK thing to do as long as it is done like
this:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));

Pass only the GPIO in the device tree and this works just fine.

The use case after that we do not interfer with.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/23/2013 03:40 AM, Lars Poeschel wrote:
> On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>> <swarren@wwwdotorg.org> wrote: [Me]
>>>
>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>> if they do, loop over the interrupts used by that child and
>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>> making them unreachable from the GPIO side.
>>>>
>>>> What about bindings that require a GPIO to be specified, yet don't
>>>> allow an IRQ to be specified, and the driver internally does perform
>>>> gpio_to_irq() on it? I don't think one can detect that case.
>>>
>>> This is still allowed. Consumers that prefer to have a GPIO
>>> passed and convert it to IRQ by that call can still do so,
>>> they will know what they're doing and will not cause the
>>> double-command situation that we're trying to solve.
>>
>> Why not? There are certainly drivers in the kernel which request a GPIO
>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>> the GPIO input whenever the IRQ goes off, in order to determine the pin
>> state. This is safer against high-latency or lost interrupts.
> 
> This is the point! They REQUEST the GPIO. They can then do whatever they 
> like with this GPIO then, even additionally use it as interrupt.
> In the devicetree case the interrupts are not requested. This is what we 
> are trying to address with the patch. The device using this interrupt has 
> no idea where this interrupt comes from. Is it a gpio-interrupt or not? 
> Does it have to request a gpio before using this interrupt or not?

If the kernel automatically requests the GPIO because it's referenced as
an interrupt then surely if the driver also requests it, then it will
fail, because the GPIO is already requested. Or, is there an explicit
check for that?

Is the problem you're trying to solve really an actual problem? I'm not
convinced it's necessary for the GPIO to show up as requested if the pin
is used as an IRQ input?

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/23/2013 12:38 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 10:53 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/21/2013 05:27 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On Wednesday 21 of August 2013 15:38:54 Lars Poeschel wrote:
>>>
>>>>>> To solve this dilemma, perform an interrupt consistency check
>>>>>> when adding a GPIO chip: if the chip is both gpio-controller and
>>>>>> interrupt-controller, walk all children of the device tree,
>>>>
>>>> It seems a little odd to solve this only for DT. What about the non-DT case?
>>>
>>> DT is the hardware configuration system that lets you request
>>> the same resource in two ways, i.e. it allows one and the same
>>> node to be both gpio-controller and interrupt-controller, and
>>> start handing out the same line as both GPIO and IRQ
>>> independently.
>>
>> Huh? What stops systems using board files and platform data from having
>> this issue?
> 
> It can't be stopped but I consider it a bug if they do, as the proper
> way to handle such GPIO lines is the sequence:
> 
> request_gpio(gpio);
> request_irq(gpio_to_irq(gpio));

Back in the old days of ARM board files, there were many boards that
didn't do this. I guess that doesn't make it any less of a bug, but it
certainly implies to me that solving this in a way that caters to that
bug being present will be a lot more useful.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/23/2013 12:45 PM, Linus Walleij wrote:
> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> [Me]
>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>> if they do, loop over the interrupts used by that child and
>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>> making them unreachable from the GPIO side.
>>>>
>>>> What about bindings that require a GPIO to be specified, yet don't allow
>>>> an IRQ to be specified, and the driver internally does perform
>>>> gpio_to_irq() on it? I don't think one can detect that case.
>>>
>>> This is still allowed. Consumers that prefer to have a GPIO
>>> passed and convert it to IRQ by that call can still do so,
>>> they will know what they're doing and will not cause the
>>> double-command situation that we're trying to solve.
>>
>> Why not? There are certainly drivers in the kernel which request a GPIO
>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>> the GPIO input whenever the IRQ goes off, in order to determine the pin
>> state. This is safer against high-latency or lost interrupts.
> 
> Yes? Are we talking past each other here?
> 
> This is a perfectly OK thing to do as long as it is done like
> this:
> 
> request_gpio(gpio);
> gpio_direction_input(gpio);
> request_irq(gpio_to_irq(gpio));

But I'm not aware that there's a rule saying it's illegal to:

request_irq(gpio_to_irq(gpio));
request_gpio(gpio);
gpio_direction_input(gpio);

> Pass only the GPIO in the device tree and this works just fine.

And I wouldn't be surprised if there were DTs that had separate GPIO and
interrupt entries for the same pin. In fact, it's arguably technically
more correct to do that than just list the GPIO, and then hope the OS
will be able to convert it to the correct IRQ. Then, drivers wouldn't
have any reason to believe they needed a specific IRQ-vs-GPIO request
ordering.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Tomasz Figa]

On Friday 23 of August 2013 13:52:20 Stephen Warren wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren 
<swarren@wwwdotorg.org> wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <swarren@wwwdotorg.org> wrote: [Me]
> >>> 
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>> 
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>> 
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >> 
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO
> >> as both a GPIO and as an (dual-edge) interrupt, so that they can read
> >> the GPIO input whenever the IRQ goes off, in order to determine the
> >> pin
> >> state. This is safer against high-latency or lost interrupts.
> > 
> > Yes? Are we talking past each other here?
> > 
> > This is a perfectly OK thing to do as long as it is done like
> > this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> 
> But I'm not aware that there's a rule saying it's illegal to:
> 
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);

Well, at least on Samsung platforms it is illegal to do so, because 
gpio_direction_input() would override the interrupt+input function set by 
setup done in request_irq() with normal input function, thus breaking the 
interrupt.

We are still to implement some sanity check to disallow (or ignore) this 
if the pin is already configured as an interrupt.

Best regards,
Tomasz

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/23/2013 01:55 PM, Tomasz Figa wrote:
> On Friday 23 of August 2013 13:52:20 Stephen Warren wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren 
> <swarren@wwwdotorg.org> wrote:
>>>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>>>> <swarren@wwwdotorg.org> wrote: [Me]
>>>>>
>>>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>>>> if they do, loop over the interrupts used by that child and
>>>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>>>> making them unreachable from the GPIO side.
>>>>>>
>>>>>> What about bindings that require a GPIO to be specified, yet don't
>>>>>> allow an IRQ to be specified, and the driver internally does
>>>>>> perform gpio_to_irq() on it? I don't think one can detect that
>>>>>> case.
>>>>>
>>>>> This is still allowed. Consumers that prefer to have a GPIO
>>>>> passed and convert it to IRQ by that call can still do so,
>>>>> they will know what they're doing and will not cause the
>>>>> double-command situation that we're trying to solve.
>>>>
>>>> Why not? There are certainly drivers in the kernel which request a
>>>> GPIO
>>>> as both a GPIO and as an (dual-edge) interrupt, so that they can read
>>>> the GPIO input whenever the IRQ goes off, in order to determine the
>>>> pin
>>>> state. This is safer against high-latency or lost interrupts.
>>>
>>> Yes? Are we talking past each other here?
>>>
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
> 
> Well, at least on Samsung platforms it is illegal to do so, because 
> gpio_direction_input() would override the interrupt+input function set by 
> setup done in request_irq() with normal input function, thus breaking the 
> interrupt.

Assuming that Linux has no general rule that requires a specific order,
isn't that simply a bug in the Samsung platforms? After all, a
completely generic cross-platform driver, which could touch both GPIO
and IRQ, would be written to any Linux-imposed rules, not any
Samsung-platform-imposed rules.

> We are still to implement some sanity check to disallow (or ignore) this 
> if the pin is already configured as an interrupt.

OK good, so it sounds like this is a temporary issue.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Friday 23 August 2013 at 21:48:43, Stephen Warren wrote:
> On 08/23/2013 03:40 AM, Lars Poeschel wrote:
> > On Thursday 22 August 2013 at 23:10:53, Stephen Warren wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <swarren@wwwdotorg.org> wrote: [Me]
> >>> 
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>> 
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>> 
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >> 
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
> >> can read the GPIO input whenever the IRQ goes off, in order to
> >> determine the pin state. This is safer against high-latency or lost
> >> interrupts.
> > 
> > This is the point! They REQUEST the GPIO. They can then do whatever
> > they like with this GPIO then, even additionally use it as interrupt.
> > In the devicetree case the interrupts are not requested. This is what
> > we are trying to address with the patch. The device using this
> > interrupt has no idea where this interrupt comes from. Is it a
> > gpio-interrupt or not? Does it have to request a gpio before using
> > this interrupt or not?
> 
> If the kernel automatically requests the GPIO because it's referenced as
> an interrupt then surely if the driver also requests it, then it will
> fail, because the GPIO is already requested. Or, is there an explicit
> check for that?

There is no explicit check, because it is not wanted! Drivers that only 
want the interrupt do not know that it is gpio-backed and therefore can not 
request it.
Drivers wanting to explicit use a gpio for interrupt, request the gpio in 
the device tree. They can then turn that gpio into an interrupt.

> Is the problem you're trying to solve really an actual problem? I'm not
> convinced it's necessary for the GPIO to show up as requested if the pin
> is used as an IRQ input?

It is definitely an actual problem. Please read the thread I already sent!
Surely it is not necessary for the GPIO to show up as requested but it does 
not harm it is even neat if it shows up. But it HAS to be requested so that 
no other entity can request it and change it's configuration i.e. turn it 
to an output.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> > On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren 
<swarren@wwwdotorg.org> wrote:
> >> On 08/21/2013 05:36 PM, Linus Walleij wrote:
> >>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
> >>> <swarren@wwwdotorg.org> wrote: [Me]
> >>> 
> >>>>>> check if these in turn reference the interrupt-controller, and
> >>>>>> if they do, loop over the interrupts used by that child and
> >>>>>> perform gpio_request() and gpio_direction_input() on these,
> >>>>>> making them unreachable from the GPIO side.
> >>>> 
> >>>> What about bindings that require a GPIO to be specified, yet don't
> >>>> allow an IRQ to be specified, and the driver internally does
> >>>> perform gpio_to_irq() on it? I don't think one can detect that
> >>>> case.
> >>> 
> >>> This is still allowed. Consumers that prefer to have a GPIO
> >>> passed and convert it to IRQ by that call can still do so,
> >>> they will know what they're doing and will not cause the
> >>> double-command situation that we're trying to solve.
> >> 
> >> Why not? There are certainly drivers in the kernel which request a
> >> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
> >> can read the GPIO input whenever the IRQ goes off, in order to
> >> determine the pin state. This is safer against high-latency or lost
> >> interrupts.
> > 
> > Yes? Are we talking past each other here?
> > 
> > This is a perfectly OK thing to do as long as it is done like
> > this:
> > 
> > request_gpio(gpio);
> > gpio_direction_input(gpio);
> > request_irq(gpio_to_irq(gpio));
> 
> But I'm not aware that there's a rule saying it's illegal to:
> 
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);

But I'd consider this as a bug. What if the scheduler interrupts you right 
after you requested (and got assigned) the interrupt and another entity 
requests your gpio? Then you'd have a resource conflict, because you are 
not the owner of the gpio you requested an interrupt for.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Hi Andreas!

On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> On 2013-08-21 15:38, Lars Poeschel wrote:
> > +static void of_gpio_scan_irq_lines(const struct device_node *const
> > node, +				   struct device_node *const gcn,
> > +				   struct irq_domain *const irq_domain,
> > +				   const u32 intsize,
> > +				   const struct gpio_chip * const gc,
> > +				   bool request)
> > +{
> > +	struct device_node *child;
> > +	struct device_node *irq_parent;
> > +	const __be32 *intspec;
> > +	u32 intlen;
> > +	int ret;
> > +	int i;
> > +	irq_hw_number_t hwirq;
> > +	unsigned int type;
> > +
> > +	if (node == NULL)
> > +		return;
> > +
> > +	for_each_child_of_node(node, child) {
> > +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> > +				       request);
> > +		/* Check if we have an IRQ parent, else continue */
> > +		irq_parent = of_irq_find_parent(child);
> 
> Hi!
> 
> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
> fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
> defined(CONFIG_OF).
> 
> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> !defined(CONFIG_OF_IRQ) instead of the current case when
> !defined(CONFIG_OF) would solve the immediate compilation problem.

Is this a bug and should be fixed ?

> However, when !defined(CONFIG_OF_IRQ) (i.e. SPARC in this case) the
> whole tree walking will never accomplish anything, so it would be good
> if of_gpiochip_reserve_irq_lines is just an empty dummy or something
> like that when !defined(CONFIG_OF_IRQ).

You are right. I'll consider this in the next spin.

Thanks,
Lars

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Andreas Larsson]

On 2013-08-26 12:56, Lars Poeschel wrote:
> Hi Andreas!
>
> On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
>> On 2013-08-21 15:38, Lars Poeschel wrote:
>>> +static void of_gpio_scan_irq_lines(const struct device_node *const
>>> node, +				   struct device_node *const gcn,
>>> +				   struct irq_domain *const irq_domain,
>>> +				   const u32 intsize,
>>> +				   const struct gpio_chip * const gc,
>>> +				   bool request)
>>> +{
>>> +	struct device_node *child;
>>> +	struct device_node *irq_parent;
>>> +	const __be32 *intspec;
>>> +	u32 intlen;
>>> +	int ret;
>>> +	int i;
>>> +	irq_hw_number_t hwirq;
>>> +	unsigned int type;
>>> +
>>> +	if (node == NULL)
>>> +		return;
>>> +
>>> +	for_each_child_of_node(node, child) {
>>> +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
>>> +				       request);
>>> +		/* Check if we have an IRQ parent, else continue */
>>> +		irq_parent = of_irq_find_parent(child);
>>
>> Hi!
>>
>> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to the
>> fact that the function is undefined when !defined(CONFIG_OF_IRQ) &&
>> defined(CONFIG_OF).
>>
>> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
>> !defined(CONFIG_OF_IRQ) instead of the current case when
>> !defined(CONFIG_OF) would solve the immediate compilation problem.
>
> Is this a bug and should be fixed ?

Well, at least as soon as anyone tries to use in a context that does not 
exclude SPARC it creates a bug, so I would say so. There is no reason 
for SPARC to fall between the chairs. This is the first case I am aware 
of that triggers this.

Cheers,
Andreas

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
> On 2013-08-26 12:56, Lars Poeschel wrote:
> > Hi Andreas!
> > 
> > On Thursday 22 August 2013 at 15:16:18, Andreas Larsson wrote:
> >> On 2013-08-21 15:38, Lars Poeschel wrote:
> >>> +static void of_gpio_scan_irq_lines(const struct device_node *const
> >>> node, +				   struct device_node *const gcn,
> >>> +				   struct irq_domain *const irq_domain,
> >>> +				   const u32 intsize,
> >>> +				   const struct gpio_chip * const gc,
> >>> +				   bool request)
> >>> +{
> >>> +	struct device_node *child;
> >>> +	struct device_node *irq_parent;
> >>> +	const __be32 *intspec;
> >>> +	u32 intlen;
> >>> +	int ret;
> >>> +	int i;
> >>> +	irq_hw_number_t hwirq;
> >>> +	unsigned int type;
> >>> +
> >>> +	if (node == NULL)
> >>> +		return;
> >>> +
> >>> +	for_each_child_of_node(node, child) {
> >>> +		of_gpio_scan_irq_lines(child, gcn, irq_domain, intsize, gc,
> >>> +				       request);
> >>> +		/* Check if we have an IRQ parent, else continue */
> >>> +		irq_parent = of_irq_find_parent(child);
> >> 
> >> Hi!
> >> 
> >> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
> >> the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
> >> && defined(CONFIG_OF).
> >> 
> >> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
> >> !defined(CONFIG_OF_IRQ) instead of the current case when
> >> !defined(CONFIG_OF) would solve the immediate compilation problem.
> > 
> > Is this a bug and should be fixed ?
> 
> Well, at least as soon as anyone tries to use in a context that does not
> exclude SPARC it creates a bug, so I would say so. There is no reason
> for SPARC to fall between the chairs. This is the first case I am aware
> of that triggers this.

I also think this should be fixed.
Are you able to do a patch that fixes this and submit to the relevant 
people?

Regards,
Lars

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Andreas Larsson]

On 2013-08-26 16:04, Lars Poeschel wrote:
> On Monday 26 August 2013 at 13:29:07, Andreas Larsson wrote:
>> On 2013-08-26 12:56, Lars Poeschel wrote:
>>>> This call to of_irq_find_parent breaks gpiolib-of for SPARC due to
>>>> the fact that the function is undefined when !defined(CONFIG_OF_IRQ)
>>>> && defined(CONFIG_OF).
>>>>
>>>> Defining the empty of_irq_find_parent in include/linux/of_irq.h when
>>>> !defined(CONFIG_OF_IRQ) instead of the current case when
>>>> !defined(CONFIG_OF) would solve the immediate compilation problem.
>>>
>>> Is this a bug and should be fixed ?
>>
>> Well, at least as soon as anyone tries to use in a context that does not
>> exclude SPARC it creates a bug, so I would say so. There is no reason
>> for SPARC to fall between the chairs. This is the first case I am aware
>> of that triggers this.
>
> I also think this should be fixed.
> Are you able to do a patch that fixes this and submit to the relevant
> people?

Sure!

Cheers,
Andreas

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/26/2013 04:45 AM, Lars Poeschel wrote:
> On Friday 23 August 2013 at 21:52:20, Stephen Warren wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
>>> On Thu, Aug 22, 2013 at 11:10 PM, Stephen Warren 
> <swarren@wwwdotorg.org> wrote:
>>>> On 08/21/2013 05:36 PM, Linus Walleij wrote:
>>>>> On Thu, Aug 22, 2013 at 1:10 AM, Stephen Warren
>>>>> <swarren@wwwdotorg.org> wrote: [Me]
>>>>>
>>>>>>>> check if these in turn reference the interrupt-controller, and
>>>>>>>> if they do, loop over the interrupts used by that child and
>>>>>>>> perform gpio_request() and gpio_direction_input() on these,
>>>>>>>> making them unreachable from the GPIO side.
>>>>>>
>>>>>> What about bindings that require a GPIO to be specified, yet don't
>>>>>> allow an IRQ to be specified, and the driver internally does
>>>>>> perform gpio_to_irq() on it? I don't think one can detect that
>>>>>> case.
>>>>>
>>>>> This is still allowed. Consumers that prefer to have a GPIO
>>>>> passed and convert it to IRQ by that call can still do so,
>>>>> they will know what they're doing and will not cause the
>>>>> double-command situation that we're trying to solve.
>>>>
>>>> Why not? There are certainly drivers in the kernel which request a
>>>> GPIO as both a GPIO and as an (dual-edge) interrupt, so that they
>>>> can read the GPIO input whenever the IRQ goes off, in order to
>>>> determine the pin state. This is safer against high-latency or lost
>>>> interrupts.
>>>
>>> Yes? Are we talking past each other here?
>>>
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
> 
> But I'd consider this as a bug. What if the scheduler interrupts you right 
> after you requested (and got assigned) the interrupt and another entity 
> requests your gpio? Then you'd have a resource conflict, because you are 
> not the owner of the gpio you requested an interrupt for.

How is that any different from two drivers both attempting to claim a
GPIO, and there being a race to get their first?

Presumably, all this happens in the device's/driver's probe(), and hence
if the gpio_request() fails, then probe() fails, and undoes all allocations.

Your argument can equally be applied to the first case where the GPIO is
requested first, then the IRQ later. What if driver A requests the GPIO
and then attempts to request the IRQ, yet the scheduler causes driver B
to *just* request the (same) IRQ (because it only cares about using it
as an IRQ and doesn't even know it's a GPIO). Then, you have the exact
same problem in reverse.

The only possible way to solve this is for either request_irq() or
request_gpio() to take complete ownership of the GPIO that backs the IRQ
if there is one, yet allow request_gpio by the same driver on that same
GPIO to still succeed, so that the order doesn't matter; whatever the
driver first always claims the GPIO and disallows any other driver from
claiming "part of the GPIO".

Yet, in your other reply you explicitly said there isn't a check for
this (the same driver claiming both the IRQ and GPIO).

Perhaps it would help to lay out exactly the problem this is trying to
solve and why it solves it again.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 23, 2013 at 9:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/23/2013 12:38 PM, Linus Walleij wrote:

>> It can't be stopped but I consider it a bug if they do, as the proper
>> way to handle such GPIO lines is the sequence:
>>
>> request_gpio(gpio);
>> request_irq(gpio_to_irq(gpio));
>
> Back in the old days of ARM board files, there were many boards that
> didn't do this. I guess that doesn't make it any less of a bug, but it
> certainly implies to me that solving this in a way that caters to that
> bug being present will be a lot more useful.

I was more thinking along the lines of trying to avoid the
unpleasant lack of control when doing this with platform
data and hard-coded GPIO numbers by restricting the way
it can be done in the device tree.

Hoping that there were no offenders in there already ...
I guess it has to hit linux-next before we know that.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/23/2013 12:45 PM, Linus Walleij wrote:

>> This is a perfectly OK thing to do as long as it is done like
>> this:
>>
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
>> request_irq(gpio_to_irq(gpio));
>
> But I'm not aware that there's a rule saying it's illegal to:
>
> request_irq(gpio_to_irq(gpio));
> request_gpio(gpio);
> gpio_direction_input(gpio);

No but I think there should be one ... maybe I'm an oddball
but it seems natural to request a GPIO before tying
IRQs to fire off it.

Besides, the bug that this is trying to solve is due to the
fact that the OMAP driver require exactly the former order
for the IRQs to work. I think if the sequence matters it is
likely to be the first form, but it's a rough guess.

>> Pass only the GPIO in the device tree and this works just fine.
>
> And I wouldn't be surprised if there were DTs that had separate GPIO and
> interrupt entries for the same pin.

That is the situation we want to catch.

Don't you agree that it has some bit of ambiguity around it, like
the tree makes an assumtion that whether you ask for the GPIO
line or IRQ first does not matter, and leave it up to the driver to
"do something" if the order suddenly turns out the other way around,
but is important to the hardware?

I think we can't get away from the ambition to define this
semantic for all DT systems.

> In fact, it's arguably technically
> more correct to do that than just list the GPIO, and then hope the OS
> will be able to convert it to the correct IRQ. Then, drivers wouldn't
> have any reason to believe they needed a specific IRQ-vs-GPIO request
> ordering.

If that is more technically correct (hm I wonder what measure is
used for "correctness" here) I think we are back at the GPIO input
hogs to achive what OMAP needs. Using such hogs they can
let the gpiochip node hog a few pins and set them as input rather
than each and every driver having to do so.

Yours,
Linus Walleij

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Stephen Warren]

On 08/29/2013 01:00 PM, Linus Walleij wrote:
> On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> 
>>> This is a perfectly OK thing to do as long as it is done like
>>> this:
>>>
>>> request_gpio(gpio);
>>> gpio_direction_input(gpio);
>>> request_irq(gpio_to_irq(gpio));
>>
>> But I'm not aware that there's a rule saying it's illegal to:
>>
>> request_irq(gpio_to_irq(gpio));
>> request_gpio(gpio);
>> gpio_direction_input(gpio);
> 
> No but I think there should be one ... maybe I'm an oddball
> but it seems natural to request a GPIO before tying
> IRQs to fire off it.

What if there is no GPIO?

There are plenty of chips with dedicated IRQ input pins that can't be
read as GPIOs, or treated as GPIOs in any way.

If a driver only needs IRQ input functionality, it should just request
an IRQ and be done with it. There should be no need at all for the
driver to know that the IRQ might be routed into a GPIO controller, and
hence that the driver may (or may not) need to additionally request the
GPIO before requesting the IRQ.

In other words, request_irq() must do everything necessary for the input
signal to operate as an IRQ input, irrespective of whether it might be
possible to use that input signal as a GPIO.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Lars Poeschel]

Am Freitag, 30. August 2013, 14:08:41 schrieb Stephen Warren:
> On 08/29/2013 01:00 PM, Linus Walleij wrote:
> > On Fri, Aug 23, 2013 at 9:52 PM, Stephen Warren <swarren@wwwdotorg.org> 
wrote:
> >> On 08/23/2013 12:45 PM, Linus Walleij wrote:
> >>> This is a perfectly OK thing to do as long as it is done like
> >>> this:
> >>> 
> >>> request_gpio(gpio);
> >>> gpio_direction_input(gpio);
> >>> request_irq(gpio_to_irq(gpio));
> >> 
> >> But I'm not aware that there's a rule saying it's illegal to:
> >> 
> >> request_irq(gpio_to_irq(gpio));
> >> request_gpio(gpio);
> >> gpio_direction_input(gpio);
> > 
> > No but I think there should be one ... maybe I'm an oddball
> > but it seems natural to request a GPIO before tying
> > IRQs to fire off it.
> 
> What if there is no GPIO?

If there is no GPIO there is no gpio-controller and there is no problem.

> There are plenty of chips with dedicated IRQ input pins that can't be
> read as GPIOs, or treated as GPIOs in any way.
> 
> If a driver only needs IRQ input functionality, it should just request
> an IRQ and be done with it. There should be no need at all for the
> driver to know that the IRQ might be routed into a GPIO controller, and
> hence that the driver may (or may not) need to additionally request the
> GPIO before requesting the IRQ.

Yes, you're right, but reality is different. Legacy drivers / board-files do:

request_gpio(gpio);
gpio_direction_input(gpio);
request_irq(gpio_to_irq(gpio));
 
> In other words, request_irq() must do everything necessary for the input
> signal to operate as an IRQ input, irrespective of whether it might be
> possible to use that input signal as a GPIO.

Re: [PATCH v2] gpio: interrupt consistency check for OF GPIO IRQs

[Linus Walleij]

On Fri, Aug 30, 2013 at 10:08 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/29/2013 01:00 PM, Linus Walleij wrote:

>> No but I think there should be one ... maybe I'm an oddball
>> but it seems natural to request a GPIO before tying
>> IRQs to fire off it.
>
> What if there is no GPIO?
>
> There are plenty of chips with dedicated IRQ input pins that can't be
> read as GPIOs, or treated as GPIOs in any way.

I'm not following this. In that case it is not tagged as
gpio-controller right? The patch is to gpiolib.c.

Do you mean chips where some part of it is GPIO and
another part is IRQ-only?

Should we not in that case create an MFD device that spawns
one GPIO device and then another irqchip device, or possibly
have the IRQ handling directly in the MFD device, as
ab8500-core.c does?

> If a driver only needs IRQ input functionality, it should just request
> an IRQ and be done with it. There should be no need at all for the
> driver to know that the IRQ might be routed into a GPIO controller, and
> hence that the driver may (or may not) need to additionally request the
> GPIO before requesting the IRQ.

This is what the patch is trying to achieve, for the DT use case.

> In other words, request_irq() must do everything necessary for the input
> signal to operate as an IRQ input, irrespective of whether it might be
> possible to use that input signal as a GPIO.

That is not the case for a bunch of OMAP drivers today, and their
attempt to fix that inside the irqchip callback backfired since it
was mutually exclusive with requesting the gpio first.

We have to encode some semantic into this, it's just a matter of
which one shall win, the APIs as they stand are ambiguous wrt
call sequence.

Yours,
Linus Walleij