* [PATCH] irqchip/irq-mtk-sysirq: drop unnecessary spinlock
@ 2020-05-27 16:17 Bartosz Golaszewski
2020-05-27 16:38 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Bartosz Golaszewski @ 2020-05-27 16:17 UTC (permalink / raw)
To: Thomas Gleixner, Jason Cooper, Marc Zyngier, Matthias Brugger
Cc: Stephane Le Provost, Bartosz Golaszewski, Pedro Tsai,
linux-kernel, Fabien Parent, linux-mediatek, Andrew Perepech,
linux-arm-kernel
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This driver takes a regular spinlock when a raw spinlock is already
taken which results in the following lockdep splat:
=============================
[ BUG: Invalid wait context ]
5.7.0-rc6-02446-gb9827c0a9fe7-dirty #1 Not tainted
-----------------------------
swapper/0/0 is trying to lock:
ffffff800303b798 (&chip_data->lock){....}-{3:3}, at: mtk_sysirq_set_type+0x48/0xc0
other info that might help us debug this:
context-{5:5}
2 locks held by swapper/0/0:
#0: ffffff800302ee68 (&desc->request_mutex){....}-{4:4}, at: __setup_irq+0xc4/0x8a0
#1: ffffff800302ecf0 (&irq_desc_lock_class){....}-{2:2}, at: __setup_irq+0xe4/0x8a0
stack backtrace:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.7.0-rc6-02446-gb9827c0a9fe7-dirty #1
Hardware name: Pumpkin MT8516 (DT)
Call trace:
dump_backtrace+0x0/0x180
show_stack+0x14/0x20
dump_stack+0xd0/0x118
__lock_acquire+0x8c8/0x2270
lock_acquire+0xf8/0x470
_raw_spin_lock_irqsave+0x50/0x78
mtk_sysirq_set_type+0x48/0xc0
__irq_set_trigger+0x58/0x170
__setup_irq+0x420/0x8a0
request_threaded_irq+0xd8/0x190
timer_of_init+0x1e8/0x2c4
mtk_gpt_init+0x5c/0x1dc
timer_probe+0x74/0xf4
time_init+0x14/0x44
start_kernel+0x394/0x4f0
We don't need the spinlock here - the irq_set_type() callback is always
called with the irq_desc->lock taken. This removes the spinlock entirely.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/irqchip/irq-mtk-sysirq.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/irqchip/irq-mtk-sysirq.c b/drivers/irqchip/irq-mtk-sysirq.c
index 73eae5966a40..da2fc4809222 100644
--- a/drivers/irqchip/irq-mtk-sysirq.c
+++ b/drivers/irqchip/irq-mtk-sysirq.c
@@ -12,10 +12,8 @@
#include <linux/of_address.h>
#include <linux/io.h>
#include <linux/slab.h>
-#include <linux/spinlock.h>
struct mtk_sysirq_chip_data {
- spinlock_t lock;
u32 nr_intpol_bases;
void __iomem **intpol_bases;
u32 *intpol_words;
@@ -30,14 +28,12 @@ static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
u8 intpol_idx = chip_data->intpol_idx[hwirq];
void __iomem *base;
u32 offset, reg_index, value;
- unsigned long flags;
int ret;
base = chip_data->intpol_bases[intpol_idx];
reg_index = chip_data->which_word[hwirq];
offset = hwirq & 0x1f;
- spin_lock_irqsave(&chip_data->lock, flags);
value = readl_relaxed(base + reg_index * 4);
if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
if (type == IRQ_TYPE_LEVEL_LOW)
@@ -53,7 +49,6 @@ static int mtk_sysirq_set_type(struct irq_data *data, unsigned int type)
data = data->parent_data;
ret = data->chip->irq_set_type(data, type);
- spin_unlock_irqrestore(&chip_data->lock, flags);
return ret;
}
@@ -212,7 +207,6 @@ static int __init mtk_sysirq_of_init(struct device_node *node,
ret = -ENOMEM;
goto out_free_which_word;
}
- spin_lock_init(&chip_data->lock);
return 0;
--
2.25.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] irqchip/irq-mtk-sysirq: drop unnecessary spinlock
2020-05-27 16:17 [PATCH] irqchip/irq-mtk-sysirq: drop unnecessary spinlock Bartosz Golaszewski
@ 2020-05-27 16:38 ` Marc Zyngier
2020-05-27 17:06 ` Bartosz Golaszewski
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2020-05-27 16:38 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Jason Cooper, Stephane Le Provost, Bartosz Golaszewski,
Pedro Tsai, linux-kernel, Fabien Parent, linux-mediatek,
Andrew Perepech, Matthias Brugger, Thomas Gleixner,
linux-arm-kernel
On 2020-05-27 17:17, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> This driver takes a regular spinlock when a raw spinlock is already
> taken which results in the following lockdep splat:
>
> =============================
> [ BUG: Invalid wait context ]
> 5.7.0-rc6-02446-gb9827c0a9fe7-dirty #1 Not tainted
> -----------------------------
> swapper/0/0 is trying to lock:
> ffffff800303b798 (&chip_data->lock){....}-{3:3}, at:
> mtk_sysirq_set_type+0x48/0xc0
> other info that might help us debug this:
> context-{5:5}
> 2 locks held by swapper/0/0:
> #0: ffffff800302ee68 (&desc->request_mutex){....}-{4:4}, at:
> __setup_irq+0xc4/0x8a0
> #1: ffffff800302ecf0 (&irq_desc_lock_class){....}-{2:2}, at:
> __setup_irq+0xe4/0x8a0
> stack backtrace:
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted
> 5.7.0-rc6-02446-gb9827c0a9fe7-dirty #1
> Hardware name: Pumpkin MT8516 (DT)
> Call trace:
> dump_backtrace+0x0/0x180
> show_stack+0x14/0x20
> dump_stack+0xd0/0x118
> __lock_acquire+0x8c8/0x2270
> lock_acquire+0xf8/0x470
> _raw_spin_lock_irqsave+0x50/0x78
> mtk_sysirq_set_type+0x48/0xc0
> __irq_set_trigger+0x58/0x170
> __setup_irq+0x420/0x8a0
> request_threaded_irq+0xd8/0x190
> timer_of_init+0x1e8/0x2c4
> mtk_gpt_init+0x5c/0x1dc
> timer_probe+0x74/0xf4
> time_init+0x14/0x44
> start_kernel+0x394/0x4f0
>
> We don't need the spinlock here - the irq_set_type() callback is always
> called with the irq_desc->lock taken. This removes the spinlock
> entirely.
It looks really great. Not.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/irqchip/irq-mtk-sysirq.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mtk-sysirq.c
> b/drivers/irqchip/irq-mtk-sysirq.c
> index 73eae5966a40..da2fc4809222 100644
> --- a/drivers/irqchip/irq-mtk-sysirq.c
> +++ b/drivers/irqchip/irq-mtk-sysirq.c
> @@ -12,10 +12,8 @@
> #include <linux/of_address.h>
> #include <linux/io.h>
> #include <linux/slab.h>
> -#include <linux/spinlock.h>
>
> struct mtk_sysirq_chip_data {
> - spinlock_t lock;
> u32 nr_intpol_bases;
> void __iomem **intpol_bases;
> u32 *intpol_words;
> @@ -30,14 +28,12 @@ static int mtk_sysirq_set_type(struct irq_data
> *data, unsigned int type)
> u8 intpol_idx = chip_data->intpol_idx[hwirq];
> void __iomem *base;
> u32 offset, reg_index, value;
> - unsigned long flags;
> int ret;
>
> base = chip_data->intpol_bases[intpol_idx];
> reg_index = chip_data->which_word[hwirq];
> offset = hwirq & 0x1f;
>
> - spin_lock_irqsave(&chip_data->lock, flags);
> value = readl_relaxed(base + reg_index * 4);
> if (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING) {
> if (type == IRQ_TYPE_LEVEL_LOW)
> @@ -53,7 +49,6 @@ static int mtk_sysirq_set_type(struct irq_data
> *data, unsigned int type)
>
> data = data->parent_data;
> ret = data->chip->irq_set_type(data, type);
> - spin_unlock_irqrestore(&chip_data->lock, flags);
> return ret;
> }
>
> @@ -212,7 +207,6 @@ static int __init mtk_sysirq_of_init(struct
> device_node *node,
> ret = -ENOMEM;
> goto out_free_which_word;
> }
> - spin_lock_init(&chip_data->lock);
>
> return 0;
Sight... Do you realize that these two locks do not protect the same
thing at all? One protects the interrupt data, and the other protects
the MMIO register which is shared between multiple interrupts, and
on which the driver performs a RMW.
Thanks to the removal of this spinlock, two irq_set_type() can execute
in parallel and silently corrupt the register. Not exactly an
improvement.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] irqchip/irq-mtk-sysirq: drop unnecessary spinlock
2020-05-27 16:38 ` Marc Zyngier
@ 2020-05-27 17:06 ` Bartosz Golaszewski
0 siblings, 0 replies; 3+ messages in thread
From: Bartosz Golaszewski @ 2020-05-27 17:06 UTC (permalink / raw)
To: Marc Zyngier
Cc: Jason Cooper, Stephane Le Provost, Pedro Tsai,
Bartosz Golaszewski, LKML, Fabien Parent, linux-mediatek,
Andrew Perepech, Matthias Brugger, Thomas Gleixner, arm-soc
śr., 27 maj 2020 o 18:38 Marc Zyngier <maz@kernel.org> napisał(a):
>
> Sight... Do you realize that these two locks do not protect the same
> thing at all? One protects the interrupt data, and the other protects
> the MMIO register which is shared between multiple interrupts, and
> on which the driver performs a RMW.
>
> Thanks to the removal of this spinlock, two irq_set_type() can execute
> in parallel and silently corrupt the register. Not exactly an
> improvement.
>
Eek I missed the fact that the internal lock is tied to the chip, not
the interrupt. In that case I'll convert this spinlock to a raw one.
Bart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-27 17:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-27 16:17 [PATCH] irqchip/irq-mtk-sysirq: drop unnecessary spinlock Bartosz Golaszewski
2020-05-27 16:38 ` Marc Zyngier
2020-05-27 17:06 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).