* WTF: patch "[PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup" was seriously submitted to be applied to the 4.13-stable tree?
@ 2017-09-21 10:59 gregkh
2017-09-21 11:41 ` Gregory CLEMENT
0 siblings, 1 reply; 3+ messages in thread
From: gregkh @ 2017-09-21 10:59 UTC (permalink / raw)
To: gregory.clement; +Cc: stable
The patch below was submitted to be applied to the 4.13-stable tree.
I fail to see how this patch meets the stable kernel rules as found at
Documentation/process/stable-kernel-rules.rst.
I could be totally wrong, and if so, please respond to
<stable@vger.kernel.org> and let me know why this patch should be
applied. Otherwise, it is now dropped from my patch queues, never to be
seen again.
thanks,
greg k-h
------------------ original commit in Linus's tree ------------------
>From a9a1a4833613b8a2e8dd79f860ea1871f17da02c Mon Sep 17 00:00:00 2001
From: Gregory CLEMENT <gregory.clement@free-electrons.com>
Date: Thu, 7 Sep 2017 16:54:07 +0200
Subject: [PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup
Since commit dc749a09ea5e ("gpiolib: allow gpio irqchip to map irqs
dynamically"), the irqs for gpio are not statically allocated during in
gpiochip_irqchip_add.
This driver was based on this assumption for initializing the mask
associated to each interrupt this led to a NULL pointer crash in the
kernel:
Unable to handle kernel NULL pointer dereference at virtual address 00000000
Mem abort info:
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x00000068
CM = 0, WnR = 1
[0000000000000000] user address but active_mm is swapper
Internal error: Oops: 96000044 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0-06657-g3b9f8ed25dbe #576
Hardware name: Marvell Armada 3720 Development Board DB-88F3720-DDR3 (DT)
task: ffff80001d908000 task.stack: ffff000008068000
PC is at armada_37xx_pinctrl_probe+0x5f8/0x670
LR is at armada_37xx_pinctrl_probe+0x5e8/0x670
pc : [<ffff000008e25cdc>] lr : [<ffff000008e25ccc>] pstate: 60000045
sp : ffff00000806bb80
x29: ffff00000806bb80 x28: 0000000000000024
x27: 000000000000000c x26: 0000000000000001
x25: ffff80001efee760 x24: 0000000000000000
x23: ffff80001db6f570 x22: ffff80001db6f438
x21: 0000000000000000 x20: ffff80001d9f4810
x19: ffff80001db6f418 x18: 0000000000000000
x17: 0000000000000001 x16: 0000000000000019
x15: ffffffffffffffff x14: 0140000000000000
x13: 0000000000000000 x12: 0000000000000030
x11: 0101010101010101 x10: 0000000000000040
x9 : ffff000009923580 x8 : ffff80001d400248
x7 : ffff80001d400270 x6 : 0000000000000000
x5 : ffff80001d400248 x4 : ffff80001d400270
x3 : 0000000000000000 x2 : 0000000000000001
x1 : 0000000000000001 x0 : 0000000000000000
Process swapper/0 (pid: 1, stack limit = 0xffff000008068000)
Call trace:
Exception stack(0xffff00000806ba40 to 0xffff00000806bb80)
ba40: 0000000000000000 0000000000000001 0000000000000001 0000000000000000
ba60: ffff80001d400270 ffff80001d400248 0000000000000000 ffff80001d400270
ba80: ffff80001d400248 ffff000009923580 0000000000000040 0101010101010101
baa0: 0000000000000030 0000000000000000 0140000000000000 ffffffffffffffff
bac0: 0000000000000019 0000000000000001 0000000000000000 ffff80001db6f418
bae0: ffff80001d9f4810 0000000000000000 ffff80001db6f438 ffff80001db6f570
bb00: 0000000000000000 ffff80001efee760 0000000000000001 000000000000000c
bb20: 0000000000000024 ffff00000806bb80 ffff000008e25ccc ffff00000806bb80
bb40: ffff000008e25cdc 0000000060000045 ffff00000806bb60 ffff0000081189b8
bb60: ffffffffffffffff ffff00000811cf1c ffff00000806bb80 ffff000008e25cdc
[<ffff000008e25cdc>] armada_37xx_pinctrl_probe+0x5f8/0x670
[<ffff00000859d8c8>] platform_drv_probe+0x58/0xb8
[<ffff00000859bb44>] driver_probe_device+0x22c/0x2d8
[<ffff00000859bcac>] __driver_attach+0xbc/0xc0
[<ffff000008599c84>] bus_for_each_dev+0x4c/0x98
[<ffff00000859b440>] driver_attach+0x20/0x28
[<ffff00000859af90>] bus_add_driver+0x1b8/0x228
[<ffff00000859c648>] driver_register+0x60/0xf8
[<ffff00000859df64>] __platform_driver_probe+0x74/0x130
[<ffff000008e256dc>] armada_37xx_pinctrl_driver_init+0x20/0x28
[<ffff000008083980>] do_one_initcall+0x38/0x128
[<ffff000008e00cf4>] kernel_init_freeable+0x188/0x22c
[<ffff0000089b56e8>] kernel_init+0x10/0x100
[<ffff000008084bb0>] ret_from_fork+0x10/0x18
Code: f9403fa2 12001341 1100075a 9ac12041 (b9000001)
---[ end trace 8b0f4e05e1603208 ]---
This patch moves the initialization of the mask field in the irq_startup
function. However some callbacks such as irq_set_type and irq_set_wake
could be called before irq_startup. For those functions the mask is
computed at each call which is not a issue as these functions are not
located in a hot path but are used sporadically for configuration.
Fixes: dc749a09ea5e ("gpiolib: allow gpio irqchip to map irqs
dynamically")
Cc: <stable@vger.kernel.org>
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
index b8b6ab072cd0..71b944748304 100644
--- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
+++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
@@ -550,9 +550,9 @@ static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on)
spin_lock_irqsave(&info->irq_lock, flags);
val = readl(info->base + reg);
if (on)
- val |= d->mask;
+ val |= (BIT(d->hwirq % GPIO_PER_REG));
else
- val &= ~d->mask;
+ val &= ~(BIT(d->hwirq % GPIO_PER_REG));
writel(val, info->base + reg);
spin_unlock_irqrestore(&info->irq_lock, flags);
@@ -571,10 +571,10 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
val = readl(info->base + reg);
switch (type) {
case IRQ_TYPE_EDGE_RISING:
- val &= ~d->mask;
+ val &= ~(BIT(d->hwirq % GPIO_PER_REG));
break;
case IRQ_TYPE_EDGE_FALLING:
- val |= d->mask;
+ val |= (BIT(d->hwirq % GPIO_PER_REG));
break;
default:
spin_unlock_irqrestore(&info->irq_lock, flags);
@@ -624,11 +624,27 @@ static void armada_37xx_irq_handler(struct irq_desc *desc)
chained_irq_exit(chip, desc);
}
+static unsigned int armada_37xx_irq_startup(struct irq_data *d)
+{
+ struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+ int irq = d->hwirq - chip->irq_base;
+ /*
+ * The mask field is a "precomputed bitmask for accessing the
+ * chip registers" which was introduced for the generic
+ * irqchip framework. As we don't use this framework, we can
+ * reuse this field for our own usage.
+ */
+ d->mask = BIT(irq % GPIO_PER_REG);
+
+ armada_37xx_irq_unmask(d);
+
+ return 0;
+}
+
static int armada_37xx_irqchip_register(struct platform_device *pdev,
struct armada_37xx_pinctrl *info)
{
struct device_node *np = info->dev->of_node;
- int nrirqs = info->data->nr_pins;
struct gpio_chip *gc = &info->gpio_chip;
struct irq_chip *irqchip = &info->irq_chip;
struct resource res;
@@ -666,8 +682,8 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev,
irqchip->irq_unmask = armada_37xx_irq_unmask;
irqchip->irq_set_wake = armada_37xx_irq_set_wake;
irqchip->irq_set_type = armada_37xx_irq_set_type;
+ irqchip->irq_startup = armada_37xx_irq_startup;
irqchip->name = info->data->name;
-
ret = gpiochip_irqchip_add(gc, irqchip, 0,
handle_edge_irq, IRQ_TYPE_NONE);
if (ret) {
@@ -680,19 +696,6 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev,
* controller. But we do not take advantage of this and use
* the chained irq with all of them.
*/
- for (i = 0; i < nrirqs; i++) {
- struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
-
- /*
- * The mask field is a "precomputed bitmask for
- * accessing the chip registers" which was introduced
- * for the generic irqchip framework. As we don't use
- * this framework, we can reuse this field for our own
- * usage.
- */
- d->mask = BIT(i % GPIO_PER_REG);
- }
-
for (i = 0; i < nr_irq_parent; i++) {
int irq = irq_of_parse_and_map(np, i);
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: WTF: patch "[PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup" was seriously submitted to be applied to the 4.13-stable tree?
2017-09-21 10:59 WTF: patch "[PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup" was seriously submitted to be applied to the 4.13-stable tree? gregkh
@ 2017-09-21 11:41 ` Gregory CLEMENT
2017-09-21 11:56 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Gregory CLEMENT @ 2017-09-21 11:41 UTC (permalink / raw)
To: gregkh; +Cc: stable
Hi Greg,
On jeu., sept. 21 2017, <gregkh@linuxfoundation.org> wrote:
> The patch below was submitted to be applied to the 4.13-stable tree.
>
> I fail to see how this patch meets the stable kernel rules as found at
> Documentation/process/stable-kernel-rules.rst.
When I submitted the patch, I thought it fixed a commit which was there
in 4.13 but actually it was introduced during the 4.14 merge window.
However I thought the "Fixes:" flag was used to automatically apply it
on the correct kernel (here it was 4.14).
Sorry for this,
Gregory
>
> I could be totally wrong, and if so, please respond to
> <stable@vger.kernel.org> and let me know why this patch should be
> applied. Otherwise, it is now dropped from my patch queues, never to be
> seen again.
>
> thanks,
>
> greg k-h
>
> ------------------ original commit in Linus's tree ------------------
>
> From a9a1a4833613b8a2e8dd79f860ea1871f17da02c Mon Sep 17 00:00:00 2001
> From: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Date: Thu, 7 Sep 2017 16:54:07 +0200
> Subject: [PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup
>
> Since commit dc749a09ea5e ("gpiolib: allow gpio irqchip to map irqs
> dynamically"), the irqs for gpio are not statically allocated during in
> gpiochip_irqchip_add.
>
> This driver was based on this assumption for initializing the mask
> associated to each interrupt this led to a NULL pointer crash in the
> kernel:
>
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> Mem abort info:
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x00000068
> CM = 0, WnR = 1
> [0000000000000000] user address but active_mm is swapper
> Internal error: Oops: 96000044 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.13.0-06657-g3b9f8ed25dbe #576
> Hardware name: Marvell Armada 3720 Development Board DB-88F3720-DDR3 (DT)
> task: ffff80001d908000 task.stack: ffff000008068000
> PC is at armada_37xx_pinctrl_probe+0x5f8/0x670
> LR is at armada_37xx_pinctrl_probe+0x5e8/0x670
> pc : [<ffff000008e25cdc>] lr : [<ffff000008e25ccc>] pstate: 60000045
> sp : ffff00000806bb80
> x29: ffff00000806bb80 x28: 0000000000000024
> x27: 000000000000000c x26: 0000000000000001
> x25: ffff80001efee760 x24: 0000000000000000
> x23: ffff80001db6f570 x22: ffff80001db6f438
> x21: 0000000000000000 x20: ffff80001d9f4810
> x19: ffff80001db6f418 x18: 0000000000000000
> x17: 0000000000000001 x16: 0000000000000019
> x15: ffffffffffffffff x14: 0140000000000000
> x13: 0000000000000000 x12: 0000000000000030
> x11: 0101010101010101 x10: 0000000000000040
> x9 : ffff000009923580 x8 : ffff80001d400248
> x7 : ffff80001d400270 x6 : 0000000000000000
> x5 : ffff80001d400248 x4 : ffff80001d400270
> x3 : 0000000000000000 x2 : 0000000000000001
> x1 : 0000000000000001 x0 : 0000000000000000
> Process swapper/0 (pid: 1, stack limit = 0xffff000008068000)
> Call trace:
> Exception stack(0xffff00000806ba40 to 0xffff00000806bb80)
> ba40: 0000000000000000 0000000000000001 0000000000000001 0000000000000000
> ba60: ffff80001d400270 ffff80001d400248 0000000000000000 ffff80001d400270
> ba80: ffff80001d400248 ffff000009923580 0000000000000040 0101010101010101
> baa0: 0000000000000030 0000000000000000 0140000000000000 ffffffffffffffff
> bac0: 0000000000000019 0000000000000001 0000000000000000 ffff80001db6f418
> bae0: ffff80001d9f4810 0000000000000000 ffff80001db6f438 ffff80001db6f570
> bb00: 0000000000000000 ffff80001efee760 0000000000000001 000000000000000c
> bb20: 0000000000000024 ffff00000806bb80 ffff000008e25ccc ffff00000806bb80
> bb40: ffff000008e25cdc 0000000060000045 ffff00000806bb60 ffff0000081189b8
> bb60: ffffffffffffffff ffff00000811cf1c ffff00000806bb80 ffff000008e25cdc
> [<ffff000008e25cdc>] armada_37xx_pinctrl_probe+0x5f8/0x670
> [<ffff00000859d8c8>] platform_drv_probe+0x58/0xb8
> [<ffff00000859bb44>] driver_probe_device+0x22c/0x2d8
> [<ffff00000859bcac>] __driver_attach+0xbc/0xc0
> [<ffff000008599c84>] bus_for_each_dev+0x4c/0x98
> [<ffff00000859b440>] driver_attach+0x20/0x28
> [<ffff00000859af90>] bus_add_driver+0x1b8/0x228
> [<ffff00000859c648>] driver_register+0x60/0xf8
> [<ffff00000859df64>] __platform_driver_probe+0x74/0x130
> [<ffff000008e256dc>] armada_37xx_pinctrl_driver_init+0x20/0x28
> [<ffff000008083980>] do_one_initcall+0x38/0x128
> [<ffff000008e00cf4>] kernel_init_freeable+0x188/0x22c
> [<ffff0000089b56e8>] kernel_init+0x10/0x100
> [<ffff000008084bb0>] ret_from_fork+0x10/0x18
> Code: f9403fa2 12001341 1100075a 9ac12041 (b9000001)
> ---[ end trace 8b0f4e05e1603208 ]---
>
> This patch moves the initialization of the mask field in the irq_startup
> function. However some callbacks such as irq_set_type and irq_set_wake
> could be called before irq_startup. For those functions the mask is
> computed at each call which is not a issue as these functions are not
> located in a hot path but are used sporadically for configuration.
>
> Fixes: dc749a09ea5e ("gpiolib: allow gpio irqchip to map irqs
> dynamically")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> index b8b6ab072cd0..71b944748304 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-37xx.c
> @@ -550,9 +550,9 @@ static int armada_37xx_irq_set_wake(struct irq_data *d, unsigned int on)
> spin_lock_irqsave(&info->irq_lock, flags);
> val = readl(info->base + reg);
> if (on)
> - val |= d->mask;
> + val |= (BIT(d->hwirq % GPIO_PER_REG));
> else
> - val &= ~d->mask;
> + val &= ~(BIT(d->hwirq % GPIO_PER_REG));
> writel(val, info->base + reg);
> spin_unlock_irqrestore(&info->irq_lock, flags);
>
> @@ -571,10 +571,10 @@ static int armada_37xx_irq_set_type(struct irq_data *d, unsigned int type)
> val = readl(info->base + reg);
> switch (type) {
> case IRQ_TYPE_EDGE_RISING:
> - val &= ~d->mask;
> + val &= ~(BIT(d->hwirq % GPIO_PER_REG));
> break;
> case IRQ_TYPE_EDGE_FALLING:
> - val |= d->mask;
> + val |= (BIT(d->hwirq % GPIO_PER_REG));
> break;
> default:
> spin_unlock_irqrestore(&info->irq_lock, flags);
> @@ -624,11 +624,27 @@ static void armada_37xx_irq_handler(struct irq_desc *desc)
> chained_irq_exit(chip, desc);
> }
>
> +static unsigned int armada_37xx_irq_startup(struct irq_data *d)
> +{
> + struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> + int irq = d->hwirq - chip->irq_base;
> + /*
> + * The mask field is a "precomputed bitmask for accessing the
> + * chip registers" which was introduced for the generic
> + * irqchip framework. As we don't use this framework, we can
> + * reuse this field for our own usage.
> + */
> + d->mask = BIT(irq % GPIO_PER_REG);
> +
> + armada_37xx_irq_unmask(d);
> +
> + return 0;
> +}
> +
> static int armada_37xx_irqchip_register(struct platform_device *pdev,
> struct armada_37xx_pinctrl *info)
> {
> struct device_node *np = info->dev->of_node;
> - int nrirqs = info->data->nr_pins;
> struct gpio_chip *gc = &info->gpio_chip;
> struct irq_chip *irqchip = &info->irq_chip;
> struct resource res;
> @@ -666,8 +682,8 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev,
> irqchip->irq_unmask = armada_37xx_irq_unmask;
> irqchip->irq_set_wake = armada_37xx_irq_set_wake;
> irqchip->irq_set_type = armada_37xx_irq_set_type;
> + irqchip->irq_startup = armada_37xx_irq_startup;
> irqchip->name = info->data->name;
> -
> ret = gpiochip_irqchip_add(gc, irqchip, 0,
> handle_edge_irq, IRQ_TYPE_NONE);
> if (ret) {
> @@ -680,19 +696,6 @@ static int armada_37xx_irqchip_register(struct platform_device *pdev,
> * controller. But we do not take advantage of this and use
> * the chained irq with all of them.
> */
> - for (i = 0; i < nrirqs; i++) {
> - struct irq_data *d = irq_get_irq_data(gc->irq_base + i);
> -
> - /*
> - * The mask field is a "precomputed bitmask for
> - * accessing the chip registers" which was introduced
> - * for the generic irqchip framework. As we don't use
> - * this framework, we can reuse this field for our own
> - * usage.
> - */
> - d->mask = BIT(i % GPIO_PER_REG);
> - }
> -
> for (i = 0; i < nr_irq_parent; i++) {
> int irq = irq_of_parse_and_map(np, i);
>
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: WTF: patch "[PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup" was seriously submitted to be applied to the 4.13-stable tree?
2017-09-21 11:41 ` Gregory CLEMENT
@ 2017-09-21 11:56 ` Greg KH
0 siblings, 0 replies; 3+ messages in thread
From: Greg KH @ 2017-09-21 11:56 UTC (permalink / raw)
To: Gregory CLEMENT; +Cc: stable
On Thu, Sep 21, 2017 at 01:41:09PM +0200, Gregory CLEMENT wrote:
> Hi Greg,
>
> On jeu., sept. 21 2017, <gregkh@linuxfoundation.org> wrote:
>
> > The patch below was submitted to be applied to the 4.13-stable tree.
> >
> > I fail to see how this patch meets the stable kernel rules as found at
> > Documentation/process/stable-kernel-rules.rst.
>
> When I submitted the patch, I thought it fixed a commit which was there
> in 4.13 but actually it was introduced during the 4.14 merge window.
>
> However I thought the "Fixes:" flag was used to automatically apply it
> on the correct kernel (here it was 4.14).
Fair enough, I wanted to check as 4.14 isn't released yet, and it seemed
a bit odd to see a stable tag for it just yet :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-09-21 11:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-21 10:59 WTF: patch "[PATCH] pinctrl: armada-37xx: Fix gpio interrupt setup" was seriously submitted to be applied to the 4.13-stable tree? gregkh
2017-09-21 11:41 ` Gregory CLEMENT
2017-09-21 11:56 ` Greg KH
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.