spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

Hi!

During the development of a new spi driver on a raspberry pi CM1
I have seen an issue with the following code triggering strange behavior:

	ret = request_threaded_irq(spi->irq, NULL,
				   mcp2517fd_can_ist,
				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
				   DEVICE_NAME, priv);

This works fine the first time the module is loaded (spi->irq is not 0),
but as soon as the module gets removed and reinstalled spi->irq is 0 
and I get the message in dmesg:
[ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio@7e200000!

This does not happen when using the IRQF_TRIGGER_FALLING flag.

in spi_drv_probe spi core does sets spi->dev to 0 in case 
of_irq_get returns < 0;

The specific code that triggers this message and return 0 is 
irq_create_fwspec_mapping.

After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
and also checking for spi->irq == 0, I get:

[   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!
[   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0

The test for irq == 0 is the first thing that happens in the 
spi.driver.probe code of the module.

So to me it looks as if something deeper down the stack during
initialization.

As if the irq-type is not cleaned up during release of the irq on module
unload - which I can confirm calls free_irq(spi->irq, priv).

After modprobe the module the first time the following entry in
/proc/interrupts shows:
 88:          0  pinctrl-bcm2835  16 Level     mcp2517fd

Any ideas why this happens and what can done about it?

Thanks, Martin

P.s: This issue shows on 4.14.0-rc8, but also on the downstream 4.9.57.

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel at martin.sperl.org]

Hi!

During the development of a new spi driver on a raspberry pi CM1
I have seen an issue with the following code triggering strange behavior:

	ret = request_threaded_irq(spi->irq, NULL,
				   mcp2517fd_can_ist,
				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
				   DEVICE_NAME, priv);

This works fine the first time the module is loaded (spi->irq is not 0),
but as soon as the module gets removed and reinstalled spi->irq is 0 
and I get the message in dmesg:
[ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio at 7e200000!

This does not happen when using the IRQF_TRIGGER_FALLING flag.

in spi_drv_probe spi core does sets spi->dev to 0 in case 
of_irq_get returns < 0;

The specific code that triggers this message and return 0 is 
irq_create_fwspec_mapping.

After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
and also checking for spi->irq == 0, I get:

[   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!
[   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0

The test for irq == 0 is the first thing that happens in the 
spi.driver.probe code of the module.

So to me it looks as if something deeper down the stack during
initialization.

As if the irq-type is not cleaned up during release of the irq on module
unload - which I can confirm calls free_irq(spi->irq, priv).

After modprobe the module the first time the following entry in
/proc/interrupts shows:
 88:          0  pinctrl-bcm2835  16 Level     mcp2517fd

Any ideas why this happens and what can done about it?

Thanks, Martin

P.s: This issue shows on 4.14.0-rc8, but also on the downstream 4.9.57.

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, 12 Nov 2017 13:32:44 +0100
<kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:

Martin,

> Hi!
> 
> During the development of a new spi driver on a raspberry pi CM1
> I have seen an issue with the following code triggering strange behavior:
> 
> 	ret = request_threaded_irq(spi->irq, NULL,
> 				   mcp2517fd_can_ist,
> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> 				   DEVICE_NAME, priv);
> 
> This works fine the first time the module is loaded (spi->irq is not 0),
> but as soon as the module gets removed and reinstalled spi->irq is 0 
> and I get the message in dmesg:
> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio@7e200000!
> 
> This does not happen when using the IRQF_TRIGGER_FALLING flag.
> 
> in spi_drv_probe spi core does sets spi->dev to 0 in case 
> of_irq_get returns < 0;
> 
> The specific code that triggers this message and return 0 is 
> irq_create_fwspec_mapping.
> 
> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
> and also checking for spi->irq == 0, I get:
> 
> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!

Well, you have the answer here: The interrupt has been configured with
a falling edge trigger, while you're requesting a level low. Obviously,
something is changing it.

It would be interesting to see both the driver code and the DT file
where the interrupt is described...

> [   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0
> 
> The test for irq == 0 is the first thing that happens in the 
> spi.driver.probe code of the module.
> 
> So to me it looks as if something deeper down the stack during
> initialization.
> 
> As if the irq-type is not cleaned up during release of the irq on module
> unload - which I can confirm calls free_irq(spi->irq, priv).

I don't really understand this remark. The trigger type is a property
of the generating device, so "cleaning up" doesn't make much sense
(even if you;re not using the interrupt anymore, the device is still
there).

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, 12 Nov 2017 13:32:44 +0100
<kernel@martin.sperl.org> wrote:

Martin,

> Hi!
> 
> During the development of a new spi driver on a raspberry pi CM1
> I have seen an issue with the following code triggering strange behavior:
> 
> 	ret = request_threaded_irq(spi->irq, NULL,
> 				   mcp2517fd_can_ist,
> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> 				   DEVICE_NAME, priv);
> 
> This works fine the first time the module is loaded (spi->irq is not 0),
> but as soon as the module gets removed and reinstalled spi->irq is 0 
> and I get the message in dmesg:
> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio at 7e200000!
> 
> This does not happen when using the IRQF_TRIGGER_FALLING flag.
> 
> in spi_drv_probe spi core does sets spi->dev to 0 in case 
> of_irq_get returns < 0;
> 
> The specific code that triggers this message and return 0 is 
> irq_create_fwspec_mapping.
> 
> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
> and also checking for spi->irq == 0, I get:
> 
> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!

Well, you have the answer here: The interrupt has been configured with
a falling edge trigger, while you're requesting a level low. Obviously,
something is changing it.

It would be interesting to see both the driver code and the DT file
where the interrupt is described...

> [   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0
> 
> The test for irq == 0 is the first thing that happens in the 
> spi.driver.probe code of the module.
> 
> So to me it looks as if something deeper down the stack during
> initialization.
> 
> As if the irq-type is not cleaned up during release of the irq on module
> unload - which I can confirm calls free_irq(spi->irq, priv).

I don't really understand this remark. The trigger type is a property
of the generating device, so "cleaning up" doesn't make much sense
(even if you;re not using the interrupt anymore, the device is still
there).

Thanks,

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

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

Hi Marc!

> On 12.11.2017, at 15:13, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> On Sun, 12 Nov 2017 13:32:44 +0100
> <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> 
> Martin,
> 
>> Hi!
>> 
>> During the development of a new spi driver on a raspberry pi CM1
>> I have seen an issue with the following code triggering strange behavior:
>> 
>> 	ret = request_threaded_irq(spi->irq, NULL,
>> 				   mcp2517fd_can_ist,
>> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>> 				   DEVICE_NAME, priv);
>> 
>> This works fine the first time the module is loaded (spi->irq is not 0),
>> but as soon as the module gets removed and reinstalled spi->irq is 0 
>> and I get the message in dmesg:
>> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio@7e200000!
>> 
>> This does not happen when using the IRQF_TRIGGER_FALLING flag.
>> 
>> in spi_drv_probe spi core does sets spi->dev to 0 in case 
>> of_irq_get returns < 0;
>> 
>> The specific code that triggers this message and return 0 is 
>> irq_create_fwspec_mapping.
>> 
>> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
>> and also checking for spi->irq == 0, I get:
>> 
>> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!
> 
> Well, you have the answer here: The interrupt has been configured with
> a falling edge trigger, while you're requesting a level low. Obviously,
> something is changing it.

It was configured as level on the first install/request and the driver is
not changed between rmmod and insmod, so it again requests level on the
second request.

> 
> It would be interesting to see both the driver code and the DT file
> where the interrupt is described…

The relevant patch to the device tree I am using:
--- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
@@ -107,3 +107,38 @@
        pinctrl-0 = <&uart0_gpio14>;
        status = "okay";
 };
+
+&gpio {
+      can0_pins: can0_pins {
+                brcm,pins = <16>;
+                brcm,function = <0>; /* input */
+      };
+};
+
+/ {
+            can0_osc: can0_osc {
+                compatible = "fixed-clock";
+                #clock-cells = <0>;
+                clock-frequency  = <4000000>;
+            };
+};
+
+&spi {
+            status = "okay";
+
+            can0: mcp2517fd@0 {
+                reg = <0>;
+                compatible = "microchip,mcp2517fd";
+                pinctrl-names = "default";
+                pinctrl-0 = <&can0_pins>;
+                spi-max-frequency = <12500000>;
+                interrupt-parent = <&gpio>;
+                interrupts = <16 0x2>;
+                clocks = <&can0_osc>;
+                microchip,clock_div = <1>;
+                microchip,clock_out_div = <4>;
+                microchip,gpio0_mode = <1>;
+                microchip,gpio1_mode = <1>;
+                status = "okay";
+            };
+};

Here a very much trimmed down version of the driver 
(probably still contains too much code):
/*
 * CAN bus driver for Microchip 2517FD CAN Controller with SPI Interface
 *
 * Copyright 2017 Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
 *
 * Based on Microchip MCP251x CAN controller driver written by
 * David Vrabel, Copyright 2006 Arcom Control Systems Ltd.
 *
 */

#include <linux/device.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <linux/uaccess.h>

#define DEVICE_NAME "mcp2517fd"
#define CAN_MCP2517FD 0x2517f

static irqreturn_t mcp2517fd_can_ist(int irq, void *dev_id)
{
	return IRQ_HANDLED;
}

static const struct of_device_id mcp2517fd_of_match[] = {
	{ .compatible     = "microchip,mcp2517fd", },
	{ }
};
MODULE_DEVICE_TABLE(of, mcp2517fd_of_match);

static int mcp2517fd_can_probe(struct spi_device *spi)
{
	int ret;

	/* as irq_create_fwspec_mapping() can return 0, check for it */
	if (spi->irq <= 0) {
		dev_err(&spi->dev, "no valid irq line defined: irq = %i\n",
			spi->irq);
		return -EINVAL;
	}

	ret = request_threaded_irq(spi->irq, NULL,
				   mcp2517fd_can_ist,
				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
//				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
				   DEVICE_NAME, NULL);
	if (ret)
		dev_err(&spi->dev, "failed to acquire irq %d - %i\n",
			spi->irq, ret);

	return ret;
}

static int mcp2517fd_can_remove(struct spi_device *spi)
{
	free_irq(spi->irq, NULL);

	return 0;
}

static struct spi_driver mcp2517fd_can_driver = {
	.driver = {
		.name = DEVICE_NAME,
		.of_match_table = mcp2517fd_of_match,
	},
	.probe = mcp2517fd_can_probe,
	.remove = mcp2517fd_can_remove,
};
module_spi_driver(mcp2517fd_can_driver);

MODULE_AUTHOR("Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>");
MODULE_DESCRIPTION("Microchip 2517FD CAN driver");
MODULE_LICENSE("GPL v2");

It is severely cut down (and not 100% clean), but it shows the behaviur already!
in the real driver the request_irq happens in a later stage…
But this way you do not require the HW to replicate it and it reduces components...

Here the example right after a reboot (but on a downstream 4.9 Kernel) 
with TRIGGER_FALLING:
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd

Here the example right after a reboot with TRIGGER_LOW:
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   86.131543] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!
[   86.131579] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   86.131634] mcp2517fd: probe of spi0.0 failed with error -22
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   87.390516] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!
[   87.390554] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   87.390609] mcp2517fd: probe of spi0.0 failed with error -22
root@raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   88.085940] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!
[   88.085977] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   88.086032] mcp2517fd: probe of spi0.0 failed with error -22

Hope that shows the issue.

> 
>> [   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0
>> 
>> The test for irq == 0 is the first thing that happens in the 
>> spi.driver.probe code of the module.
>> 
>> So to me it looks as if something deeper down the stack during
>> initialization.
>> 
>> As if the irq-type is not cleaned up during release of the irq on module
>> unload - which I can confirm calls free_irq(spi->irq, priv).
> 
> I don't really understand this remark. The trigger type is a property
> of the generating device, so "cleaning up" doesn't make much sense
> (even if you;re not using the interrupt anymore, the device is still
> there).

As far as I see it the free_irq (or something else) does change something
in the info structure of the interrupt, so that it is now
configured as edge not level.

This means that it fails on the second attempt.

How/where this happens is unclear to me.

I darkly remember that there was something with regards to bcm2835 and
level interrupts, that had to be implemented as edge with a wrapper layer
to implement level or something…

But then I may be wrong here.

Ciao,
	Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel at martin.sperl.org]

Hi Marc!

> On 12.11.2017, at 15:13, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
> On Sun, 12 Nov 2017 13:32:44 +0100
> <kernel@martin.sperl.org> wrote:
> 
> Martin,
> 
>> Hi!
>> 
>> During the development of a new spi driver on a raspberry pi CM1
>> I have seen an issue with the following code triggering strange behavior:
>> 
>> 	ret = request_threaded_irq(spi->irq, NULL,
>> 				   mcp2517fd_can_ist,
>> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
>> 				   DEVICE_NAME, priv);
>> 
>> This works fine the first time the module is loaded (spi->irq is not 0),
>> but as soon as the module gets removed and reinstalled spi->irq is 0 
>> and I get the message in dmesg:
>> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio at 7e200000!
>> 
>> This does not happen when using the IRQF_TRIGGER_FALLING flag.
>> 
>> in spi_drv_probe spi core does sets spi->dev to 0 in case 
>> of_irq_get returns < 0;
>> 
>> The specific code that triggers this message and return 0 is 
>> irq_create_fwspec_mapping.
>> 
>> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
>> and also checking for spi->irq == 0, I get:
>> 
>> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!
> 
> Well, you have the answer here: The interrupt has been configured with
> a falling edge trigger, while you're requesting a level low. Obviously,
> something is changing it.

It was configured as level on the first install/request and the driver is
not changed between rmmod and insmod, so it again requests level on the
second request.

> 
> It would be interesting to see both the driver code and the DT file
> where the interrupt is described?

The relevant patch to the device tree I am using:
--- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
@@ -107,3 +107,38 @@
        pinctrl-0 = <&uart0_gpio14>;
        status = "okay";
 };
+
+&gpio {
+      can0_pins: can0_pins {
+                brcm,pins = <16>;
+                brcm,function = <0>; /* input */
+      };
+};
+
+/ {
+            can0_osc: can0_osc {
+                compatible = "fixed-clock";
+                #clock-cells = <0>;
+                clock-frequency  = <4000000>;
+            };
+};
+
+&spi {
+            status = "okay";
+
+            can0: mcp2517fd at 0 {
+                reg = <0>;
+                compatible = "microchip,mcp2517fd";
+                pinctrl-names = "default";
+                pinctrl-0 = <&can0_pins>;
+                spi-max-frequency = <12500000>;
+                interrupt-parent = <&gpio>;
+                interrupts = <16 0x2>;
+                clocks = <&can0_osc>;
+                microchip,clock_div = <1>;
+                microchip,clock_out_div = <4>;
+                microchip,gpio0_mode = <1>;
+                microchip,gpio1_mode = <1>;
+                status = "okay";
+            };
+};

Here a very much trimmed down version of the driver 
(probably still contains too much code):
/*
 * CAN bus driver for Microchip 2517FD CAN Controller with SPI Interface
 *
 * Copyright 2017 Martin Sperl <kernel@martin.sperl.org>
 *
 * Based on Microchip MCP251x CAN controller driver written by
 * David Vrabel, Copyright 2006 Arcom Control Systems Ltd.
 *
 */

#include <linux/device.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spi/spi.h>
#include <linux/uaccess.h>

#define DEVICE_NAME "mcp2517fd"
#define CAN_MCP2517FD 0x2517f

static irqreturn_t mcp2517fd_can_ist(int irq, void *dev_id)
{
	return IRQ_HANDLED;
}

static const struct of_device_id mcp2517fd_of_match[] = {
	{ .compatible     = "microchip,mcp2517fd", },
	{ }
};
MODULE_DEVICE_TABLE(of, mcp2517fd_of_match);

static int mcp2517fd_can_probe(struct spi_device *spi)
{
	int ret;

	/* as irq_create_fwspec_mapping() can return 0, check for it */
	if (spi->irq <= 0) {
		dev_err(&spi->dev, "no valid irq line defined: irq = %i\n",
			spi->irq);
		return -EINVAL;
	}

	ret = request_threaded_irq(spi->irq, NULL,
				   mcp2517fd_can_ist,
				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
//				   IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
				   DEVICE_NAME, NULL);
	if (ret)
		dev_err(&spi->dev, "failed to acquire irq %d - %i\n",
			spi->irq, ret);

	return ret;
}

static int mcp2517fd_can_remove(struct spi_device *spi)
{
	free_irq(spi->irq, NULL);

	return 0;
}

static struct spi_driver mcp2517fd_can_driver = {
	.driver = {
		.name = DEVICE_NAME,
		.of_match_table = mcp2517fd_of_match,
	},
	.probe = mcp2517fd_can_probe,
	.remove = mcp2517fd_can_remove,
};
module_spi_driver(mcp2517fd_can_driver);

MODULE_AUTHOR("Martin Sperl <kernel@martin.sperl.org>");
MODULE_DESCRIPTION("Microchip 2517FD CAN driver");
MODULE_LICENSE("GPL v2");

It is severely cut down (and not 100% clean), but it shows the behaviur already!
in the real driver the request_irq happens in a later stage?
But this way you do not require the HW to replicate it and it reduces components...

Here the example right after a reboot (but on a downstream 4.9 Kernel) 
with TRIGGER_FALLING:
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Edge      mcp2517fd

Here the example right after a reboot with TRIGGER_LOW:
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   86.131543] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!
[   86.131579] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   86.131634] mcp2517fd: probe of spi0.0 failed with error -22
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   87.390516] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!
[   87.390554] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   87.390609] mcp2517fd: probe of spi0.0 failed with error -22
root at raspcm:~# dmesg -C; rmmod mcp2517fd; insmod /tmp/mcp2517fd.ko; dmesg; grep mcp2517fd /proc/interrupts
[   88.085940] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!
[   88.085977] mcp2517fd spi0.0: no valid irq line defined: irq = 0
[   88.086032] mcp2517fd: probe of spi0.0 failed with error -22

Hope that shows the issue.

> 
>> [   87.867512] mcp2517fd spi0.0: no valid irq line defined: irq = 0
>> 
>> The test for irq == 0 is the first thing that happens in the 
>> spi.driver.probe code of the module.
>> 
>> So to me it looks as if something deeper down the stack during
>> initialization.
>> 
>> As if the irq-type is not cleaned up during release of the irq on module
>> unload - which I can confirm calls free_irq(spi->irq, priv).
> 
> I don't really understand this remark. The trigger type is a property
> of the generating device, so "cleaning up" doesn't make much sense
> (even if you;re not using the interrupt anymore, the device is still
> there).

As far as I see it the free_irq (or something else) does change something
in the info structure of the interrupt, so that it is now
configured as edge not level.

This means that it fails on the second attempt.

How/where this happens is unclear to me.

I darkly remember that there was something with regards to bcm2835 and
level interrupts, that had to be implemented as edge with a wrapper layer
to implement level or something?

But then I may be wrong here.

Ciao,
	Martin

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, 12 Nov 2017 16:13:44 +0100
<kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:

> Hi Marc!
> 
> > On 12.11.2017, at 15:13, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> > 
> > On Sun, 12 Nov 2017 13:32:44 +0100
> > <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
> > 
> > Martin,
> >   
> >> Hi!
> >> 
> >> During the development of a new spi driver on a raspberry pi CM1
> >> I have seen an issue with the following code triggering strange behavior:
> >> 
> >> 	ret = request_threaded_irq(spi->irq, NULL,
> >> 				   mcp2517fd_can_ist,
> >> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> >> 				   DEVICE_NAME, priv);
> >> 
> >> This works fine the first time the module is loaded (spi->irq is not 0),
> >> but as soon as the module gets removed and reinstalled spi->irq is 0 
> >> and I get the message in dmesg:
> >> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio@7e200000!
> >> 
> >> This does not happen when using the IRQF_TRIGGER_FALLING flag.
> >> 
> >> in spi_drv_probe spi core does sets spi->dev to 0 in case 
> >> of_irq_get returns < 0;
> >> 
> >> The specific code that triggers this message and return 0 is 
> >> irq_create_fwspec_mapping.
> >> 
> >> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
> >> and also checking for spi->irq == 0, I get:
> >> 
> >> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio@7e200000!  
> > 
> > Well, you have the answer here: The interrupt has been configured with
> > a falling edge trigger, while you're requesting a level low. Obviously,
> > something is changing it.  
> 
> It was configured as level on the first install/request and the driver is
> not changed between rmmod and insmod, so it again requests level on the
> second request.
> 
> > 
> > It would be interesting to see both the driver code and the DT file
> > where the interrupt is described…  
> 
> The relevant patch to the device tree I am using:
> --- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
> +++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
> @@ -107,3 +107,38 @@
>         pinctrl-0 = <&uart0_gpio14>;
>         status = "okay";
>  };
> +
> +&gpio {
> +      can0_pins: can0_pins {
> +                brcm,pins = <16>;
> +                brcm,function = <0>; /* input */
> +      };
> +};
> +
> +/ {
> +            can0_osc: can0_osc {
> +                compatible = "fixed-clock";
> +                #clock-cells = <0>;
> +                clock-frequency  = <4000000>;
> +            };
> +};
> +
> +&spi {
> +            status = "okay";
> +
> +            can0: mcp2517fd@0 {
> +                reg = <0>;
> +                compatible = "microchip,mcp2517fd";
> +                pinctrl-names = "default";
> +                pinctrl-0 = <&can0_pins>;
> +                spi-max-frequency = <12500000>;
> +                interrupt-parent = <&gpio>;
> +                interrupts = <16 0x2>;

This indicates a falling edge. No wonder the kernel is confused (I
don't know why this isn't enforced the first time though, probably an
issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
allow you to make some progress.

Thanks,

	M.
-- 
Without deviation from the norm, progress is not possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, 12 Nov 2017 16:13:44 +0100
<kernel@martin.sperl.org> wrote:

> Hi Marc!
> 
> > On 12.11.2017, at 15:13, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > 
> > On Sun, 12 Nov 2017 13:32:44 +0100
> > <kernel@martin.sperl.org> wrote:
> > 
> > Martin,
> >   
> >> Hi!
> >> 
> >> During the development of a new spi driver on a raspberry pi CM1
> >> I have seen an issue with the following code triggering strange behavior:
> >> 
> >> 	ret = request_threaded_irq(spi->irq, NULL,
> >> 				   mcp2517fd_can_ist,
> >> 				   IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> >> 				   DEVICE_NAME, priv);
> >> 
> >> This works fine the first time the module is loaded (spi->irq is not 0),
> >> but as soon as the module gets removed and reinstalled spi->irq is 0 
> >> and I get the message in dmesg:
> >> [ 1282.311991] irq: type mismatch, failed to map hwirq-16 for /soc/gpio at 7e200000!
> >> 
> >> This does not happen when using the IRQF_TRIGGER_FALLING flag.
> >> 
> >> in spi_drv_probe spi core does sets spi->dev to 0 in case 
> >> of_irq_get returns < 0;
> >> 
> >> The specific code that triggers this message and return 0 is 
> >> irq_create_fwspec_mapping.
> >> 
> >> After implementing: https://www.spinics.net/lists/arm-kernel/msg528469.html
> >> and also checking for spi->irq == 0, I get:
> >> 
> >> [   87.867456] irq: type mismatch (2/8), failed to map hwirq-16 for /soc/gpio at 7e200000!  
> > 
> > Well, you have the answer here: The interrupt has been configured with
> > a falling edge trigger, while you're requesting a level low. Obviously,
> > something is changing it.  
> 
> It was configured as level on the first install/request and the driver is
> not changed between rmmod and insmod, so it again requests level on the
> second request.
> 
> > 
> > It would be interesting to see both the driver code and the DT file
> > where the interrupt is described?  
> 
> The relevant patch to the device tree I am using:
> --- a/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
> +++ b/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts
> @@ -107,3 +107,38 @@
>         pinctrl-0 = <&uart0_gpio14>;
>         status = "okay";
>  };
> +
> +&gpio {
> +      can0_pins: can0_pins {
> +                brcm,pins = <16>;
> +                brcm,function = <0>; /* input */
> +      };
> +};
> +
> +/ {
> +            can0_osc: can0_osc {
> +                compatible = "fixed-clock";
> +                #clock-cells = <0>;
> +                clock-frequency  = <4000000>;
> +            };
> +};
> +
> +&spi {
> +            status = "okay";
> +
> +            can0: mcp2517fd at 0 {
> +                reg = <0>;
> +                compatible = "microchip,mcp2517fd";
> +                pinctrl-names = "default";
> +                pinctrl-0 = <&can0_pins>;
> +                spi-max-frequency = <12500000>;
> +                interrupt-parent = <&gpio>;
> +                interrupts = <16 0x2>;

This indicates a falling edge. No wonder the kernel is confused (I
don't know why this isn't enforced the first time though, probably an
issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
allow you to make some progress.

Thanks,

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

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>> +
>> +            can0: mcp2517fd@0 {
>> +                reg = <0>;
>> +                compatible = "microchip,mcp2517fd";
>> +                pinctrl-names = "default";
>> +                pinctrl-0 = <&can0_pins>;
>> +                spi-max-frequency = <12500000>;
>> +                interrupt-parent = <&gpio>;
>> +                interrupts = <16 0x2>;
> 
> This indicates a falling edge. No wonder the kernel is confused (I
> don't know why this isn't enforced the first time though, probably an
> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
> allow you to make some progress.

Thanks for the clarification - with that change it works!

For a better understanding:
Isn’t the interrupt type to use more of a driver decision than a
HW implementation detail that needs to get defined in the device tree?

In my case I probably could write some more code that would allow edge
interrupts to work (without race-conditions on spi transfers - probably
by using spi_async to reenable interrupts on the HW device), but it
would not be as straight-forward and a bit more complex.

Summary: Essentially the driver has to match the interrupt type - 
otherwise it will fail (on second initialization).

Thanks,
	Martin






--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel at martin.sperl.org]

> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> +
>> +            can0: mcp2517fd at 0 {
>> +                reg = <0>;
>> +                compatible = "microchip,mcp2517fd";
>> +                pinctrl-names = "default";
>> +                pinctrl-0 = <&can0_pins>;
>> +                spi-max-frequency = <12500000>;
>> +                interrupt-parent = <&gpio>;
>> +                interrupts = <16 0x2>;
> 
> This indicates a falling edge. No wonder the kernel is confused (I
> don't know why this isn't enforced the first time though, probably an
> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
> allow you to make some progress.

Thanks for the clarification - with that change it works!

For a better understanding:
Isn?t the interrupt type to use more of a driver decision than a
HW implementation detail that needs to get defined in the device tree?

In my case I probably could write some more code that would allow edge
interrupts to work (without race-conditions on spi transfers - probably
by using spi_async to reenable interrupts on the HW device), but it
would not be as straight-forward and a bit more complex.

Summary: Essentially the driver has to match the interrupt type - 
otherwise it will fail (on second initialization).

Thanks,
	Martin

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Andrew Lunn]

> > +&spi {
> > +            status = "okay";
> > +
> > +            can0: mcp2517fd@0 {
> > +                reg = <0>;
> > +                compatible = "microchip,mcp2517fd";
> > +                pinctrl-names = "default";
> > +                pinctrl-0 = <&can0_pins>;
> > +                spi-max-frequency = <12500000>;
> > +                interrupt-parent = <&gpio>;
> > +                interrupts = <16 0x2>;
> 
> This indicates a falling edge. No wonder the kernel is confused (I
> don't know why this isn't enforced the first time though, probably an
> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
> allow you to make some progress.

And using IRQ_TYPE_LEVEL_LOW, from interrupt-controller/irq.h would
make it readable.

     Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Andrew Lunn]

> > +&spi {
> > +            status = "okay";
> > +
> > +            can0: mcp2517fd at 0 {
> > +                reg = <0>;
> > +                compatible = "microchip,mcp2517fd";
> > +                pinctrl-names = "default";
> > +                pinctrl-0 = <&can0_pins>;
> > +                spi-max-frequency = <12500000>;
> > +                interrupt-parent = <&gpio>;
> > +                interrupts = <16 0x2>;
> 
> This indicates a falling edge. No wonder the kernel is confused (I
> don't know why this isn't enforced the first time though, probably an
> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
> allow you to make some progress.

And using IRQ_TYPE_LEVEL_LOW, from interrupt-controller/irq.h would
make it readable.

     Andrew

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, Nov 12 2017 at  5:49:39 pm GMT, <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>>> +
>>> +            can0: mcp2517fd@0 {
>>> +                reg = <0>;
>>> +                compatible = "microchip,mcp2517fd";
>>> +                pinctrl-names = "default";
>>> +                pinctrl-0 = <&can0_pins>;
>>> +                spi-max-frequency = <12500000>;
>>> +                interrupt-parent = <&gpio>;
>>> +                interrupts = <16 0x2>;
>> 
>> This indicates a falling edge. No wonder the kernel is confused (I
>> don't know why this isn't enforced the first time though, probably an
>> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
>> allow you to make some progress.
>
> Thanks for the clarification - with that change it works!
>
> For a better understanding:
> Isn’t the interrupt type to use more of a driver decision than a
> HW implementation detail that needs to get defined in the device tree?

Absolutely *not*. The signalling of an interrupt is completely HW
dependent, and the driver *must* use abide by the HW rule. That's
because edge and level interrupts signal entirely different things:

- An edge interrupt signals a an event. Something has happened, and many
  of these events can be signalled independently without the CPU doing
  anything.

- A level interrupt indicates a change of state, and this state persist
  until the CPU has services the interrupt.

That's the difference between receiving a SMS each time you pay
something your bank card, and receiving a phone call from your bank
because your account is overdrawn.

> In my case I probably could write some more code that would allow edge
> interrupts to work (without race-conditions on spi transfers - probably
> by using spi_async to reenable interrupts on the HW device), but it
> would not be as straight-forward and a bit more complex.

And more or less wrong, given that the spec sheet calls out "active low"
interrupts.

> Summary: Essentially the driver has to match the interrupt type -
> otherwise it will fail (on second initialization).

And even that is not quite right. The driver should fail straight
away. on the first request. I don't have the HW to track it down, but
I'd appreciate it if you could have a look and enlighten me.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[Marc Zyngier]

On Sun, Nov 12 2017 at  5:49:39 pm GMT, <kernel@martin.sperl.org> wrote:
>> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> +
>>> +            can0: mcp2517fd at 0 {
>>> +                reg = <0>;
>>> +                compatible = "microchip,mcp2517fd";
>>> +                pinctrl-names = "default";
>>> +                pinctrl-0 = <&can0_pins>;
>>> +                spi-max-frequency = <12500000>;
>>> +                interrupt-parent = <&gpio>;
>>> +                interrupts = <16 0x2>;
>> 
>> This indicates a falling edge. No wonder the kernel is confused (I
>> don't know why this isn't enforced the first time though, probably an
>> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
>> allow you to make some progress.
>
> Thanks for the clarification - with that change it works!
>
> For a better understanding:
> Isn?t the interrupt type to use more of a driver decision than a
> HW implementation detail that needs to get defined in the device tree?

Absolutely *not*. The signalling of an interrupt is completely HW
dependent, and the driver *must* use abide by the HW rule. That's
because edge and level interrupts signal entirely different things:

- An edge interrupt signals a an event. Something has happened, and many
  of these events can be signalled independently without the CPU doing
  anything.

- A level interrupt indicates a change of state, and this state persist
  until the CPU has services the interrupt.

That's the difference between receiving a SMS each time you pay
something your bank card, and receiving a phone call from your bank
because your account is overdrawn.

> In my case I probably could write some more code that would allow edge
> interrupts to work (without race-conditions on spi transfers - probably
> by using spi_async to reenable interrupts on the HW device), but it
> would not be as straight-forward and a bit more complex.

And more or less wrong, given that the spec sheet calls out "active low"
interrupts.

> Summary: Essentially the driver has to match the interrupt type -
> otherwise it will fail (on second initialization).

And even that is not quite right. The driver should fail straight
away. on the first request. I don't have the HW to track it down, but
I'd appreciate it if you could have a look and enlighten me.

Thanks,

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

Re: spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

> On 13.11.2017, at 10:35, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> On Sun, Nov 12 2017 at  5:49:39 pm GMT, <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org> wrote:
>>> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> +
>>>> +            can0: mcp2517fd@0 {
>>>> +                reg = <0>;
>>>> +                compatible = "microchip,mcp2517fd";
>>>> +                pinctrl-names = "default";
>>>> +                pinctrl-0 = <&can0_pins>;
>>>> +                spi-max-frequency = <12500000>;
>>>> +                interrupt-parent = <&gpio>;
>>>> +                interrupts = <16 0x2>;
>>> 
>>> This indicates a falling edge. No wonder the kernel is confused (I
>>> don't know why this isn't enforced the first time though, probably an
>>> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
>>> allow you to make some progress.
>> 
>> Thanks for the clarification - with that change it works!
>> 
>> For a better understanding:
>> Isn’t the interrupt type to use more of a driver decision than a
>> HW implementation detail that needs to get defined in the device tree?
> 
> Absolutely *not*. The signalling of an interrupt is completely HW
> dependent, and the driver *must* use abide by the HW rule. That's
> because edge and level interrupts signal entirely different things:
> 
> - An edge interrupt signals a an event. Something has happened, and many
>  of these events can be signalled independently without the CPU doing
>  anything.
> 
> - A level interrupt indicates a change of state, and this state persist
>  until the CPU has services the interrupt.
> 
> That's the difference between receiving a SMS each time you pay
> something your bank card, and receiving a phone call from your bank
> because your account is overdrawn.

So for all practical purposes any typical active low interrupt line 
connected to a GPIO should be configured as TRIGGER_LOW in the
device tree.

Under this assumption:

There are actually quite a few drivers that are having a /INT
line, but are configured in the DT bindings as 2 - hence 
TRIGGER_FALLING.

Two of those are:
* Documentation/devicetree/bindings/net/can/microchip,mcp251x.txt
* Documentation/devicetree/bindings/net/microchip,enc28j60.txt

The enc28j60 does use flags = 0, which seems could be ok, as it is
not requesting some specific kind of irq, while the other
is using flags = IRQF_ONESHOT | IRQF_TRIGGER_FALLING, which is
incorrect.

That was one of the reasons why I was adding this subsequent question
for clarification.

So I wonder how one would go about correcting those devices...

> 
>> In my case I probably could write some more code that would allow edge
>> interrupts to work (without race-conditions on spi transfers - probably
>> by using spi_async to reenable interrupts on the HW device), but it
>> would not be as straight-forward and a bit more complex.
> 
> And more or less wrong, given that the spec sheet calls out "active low"
> interrupts.
It would have been a workaround which would not have been accepted
anyway.

>> Summary: Essentially the driver has to match the interrupt type -
>> otherwise it will fail (on second initialization).
> 
> And even that is not quite right. The driver should fail straight
> away. on the first request. I don't have the HW to track it down, but
> I'd appreciate it if you could have a look and enlighten me.

When I have finished this driver I am working on right now, then
I may look into it in detail.

But if I remember the bits and pieces I have been looking at before
asking this question: I remember that there is some code in 
irq_create_fwspec_mapping that allows for first time initialization.

The best I can do is:
* in __setup_irq:
  * the Trigger-type flags are set to “defaults” if none are set
  * otherwise the flags are used without matching them against
    what is configured in the device tree.
  * so there is no validation happening in case the types disagree.

Something like this patch would trigger an error on first try 
(unless flags = 0 or flags = “correct”):

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ea41820ab12e..c2dd1b4b879a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1136,8 +1136,15 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
         * If the trigger type is not specified by the caller,
         * then use the default for this interrupt.
         */
-       if (!(new->flags & IRQF_TRIGGER_MASK))
-               new->flags |= irqd_get_trigger_type(&desc->irq_data);
+       flags = irqd_get_trigger_type(&desc->irq_data);
+       if (!(new->flags & IRQF_TRIGGER_MASK)) {
+               new->flags |= flags;
+       } else if ((new->flags & IRQF_TRIGGER_MASK) != flags) {
+               pr_err("Requested trigger type %i does not match default %lu\n",
+                      new->flags & IRQF_TRIGGER_MASK,
+                      flags);
+               return -EINVAL;
+       }

        /*
         * Check whether the interrupt nests into another interrupt

Patch is against 4.9.57 - please ignore the whitespace issue as it
is a simple console cut/paste. 

It does not trigger anything else on my system, but there may
be other use-cases that would fail because of this.

Here some of the debugging output (see the irq_flags module parameter)
root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=2; dmesg -c; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
[   36.329522] genirq: Requested trigger type 2 does not match default 8
[   36.329556] mcp2517fd spi0.0: failed to acquire irq 176 - -22
[   36.329611] mcp2517fd: probe of spi0.0 failed with error -22
root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=0; dmesg -c; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd
root@raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=8; dmesg -c; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd

Hope this analysis helps...

Martin



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spi->irq == 0 on module reload of driver using IRQF_TRIGGER_LOW

[kernel at martin.sperl.org]

> On 13.11.2017, at 10:35, Marc Zyngier <marc.zyngier@arm.com> wrote:
> 
> On Sun, Nov 12 2017 at  5:49:39 pm GMT, <kernel@martin.sperl.org> wrote:
>>> On 12.11.2017, at 16:41, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> +
>>>> +            can0: mcp2517fd at 0 {
>>>> +                reg = <0>;
>>>> +                compatible = "microchip,mcp2517fd";
>>>> +                pinctrl-names = "default";
>>>> +                pinctrl-0 = <&can0_pins>;
>>>> +                spi-max-frequency = <12500000>;
>>>> +                interrupt-parent = <&gpio>;
>>>> +                interrupts = <16 0x2>;
>>> 
>>> This indicates a falling edge. No wonder the kernel is confused (I
>>> don't know why this isn't enforced the first time though, probably an
>>> issue in the GPIO irqchip driver...). Replacing this 2 with a 8 should
>>> allow you to make some progress.
>> 
>> Thanks for the clarification - with that change it works!
>> 
>> For a better understanding:
>> Isn?t the interrupt type to use more of a driver decision than a
>> HW implementation detail that needs to get defined in the device tree?
> 
> Absolutely *not*. The signalling of an interrupt is completely HW
> dependent, and the driver *must* use abide by the HW rule. That's
> because edge and level interrupts signal entirely different things:
> 
> - An edge interrupt signals a an event. Something has happened, and many
>  of these events can be signalled independently without the CPU doing
>  anything.
> 
> - A level interrupt indicates a change of state, and this state persist
>  until the CPU has services the interrupt.
> 
> That's the difference between receiving a SMS each time you pay
> something your bank card, and receiving a phone call from your bank
> because your account is overdrawn.

So for all practical purposes any typical active low interrupt line 
connected to a GPIO should be configured as TRIGGER_LOW in the
device tree.

Under this assumption:

There are actually quite a few drivers that are having a /INT
line, but are configured in the DT bindings as 2 - hence 
TRIGGER_FALLING.

Two of those are:
* Documentation/devicetree/bindings/net/can/microchip,mcp251x.txt
* Documentation/devicetree/bindings/net/microchip,enc28j60.txt

The enc28j60 does use flags = 0, which seems could be ok, as it is
not requesting some specific kind of irq, while the other
is using flags = IRQF_ONESHOT | IRQF_TRIGGER_FALLING, which is
incorrect.

That was one of the reasons why I was adding this subsequent question
for clarification.

So I wonder how one would go about correcting those devices...

> 
>> In my case I probably could write some more code that would allow edge
>> interrupts to work (without race-conditions on spi transfers - probably
>> by using spi_async to reenable interrupts on the HW device), but it
>> would not be as straight-forward and a bit more complex.
> 
> And more or less wrong, given that the spec sheet calls out "active low"
> interrupts.
It would have been a workaround which would not have been accepted
anyway.

>> Summary: Essentially the driver has to match the interrupt type -
>> otherwise it will fail (on second initialization).
> 
> And even that is not quite right. The driver should fail straight
> away. on the first request. I don't have the HW to track it down, but
> I'd appreciate it if you could have a look and enlighten me.

When I have finished this driver I am working on right now, then
I may look into it in detail.

But if I remember the bits and pieces I have been looking at before
asking this question: I remember that there is some code in 
irq_create_fwspec_mapping that allows for first time initialization.

The best I can do is:
* in __setup_irq:
  * the Trigger-type flags are set to ?defaults? if none are set
  * otherwise the flags are used without matching them against
    what is configured in the device tree.
  * so there is no validation happening in case the types disagree.

Something like this patch would trigger an error on first try 
(unless flags = 0 or flags = ?correct?):

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index ea41820ab12e..c2dd1b4b879a 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1136,8 +1136,15 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
         * If the trigger type is not specified by the caller,
         * then use the default for this interrupt.
         */
-       if (!(new->flags & IRQF_TRIGGER_MASK))
-               new->flags |= irqd_get_trigger_type(&desc->irq_data);
+       flags = irqd_get_trigger_type(&desc->irq_data);
+       if (!(new->flags & IRQF_TRIGGER_MASK)) {
+               new->flags |= flags;
+       } else if ((new->flags & IRQF_TRIGGER_MASK) != flags) {
+               pr_err("Requested trigger type %i does not match default %lu\n",
+                      new->flags & IRQF_TRIGGER_MASK,
+                      flags);
+               return -EINVAL;
+       }

        /*
         * Check whether the interrupt nests into another interrupt

Patch is against 4.9.57 - please ignore the whitespace issue as it
is a simple console cut/paste. 

It does not trigger anything else on my system, but there may
be other use-cases that would fail because of this.

Here some of the debugging output (see the irq_flags module parameter)
root at raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=2; dmesg -c; grep mcp2517fd /proc/interrupts
rmmod: ERROR: Module mcp2517fd is not currently loaded
[   36.329522] genirq: Requested trigger type 2 does not match default 8
[   36.329556] mcp2517fd spi0.0: failed to acquire irq 176 - -22
[   36.329611] mcp2517fd: probe of spi0.0 failed with error -22
root at raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=0; dmesg -c; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd
root at raspcm:~# rmmod mcp2517fd; dmesg -c; modprobe mcp2517fd irq_flags=8; dmesg -c; grep mcp2517fd /proc/interrupts
176:          0  pinctrl-bcm2835  16 Level     mcp2517fd

Hope this analysis helps...

Martin