| Message ID | 20200527161743.15972-1-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | irqchip/irq-mtk-sysirq: drop unnecessary spinlock | expand |
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.
ś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
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;