Message ID | 20230424103156.66753-3-jiaxun.yang@flygoat.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | irqchip/mips-gic fixes | expand |
On Mon, Apr 24, 2023 at 11:31:56AM +0100, Jiaxun Yang wrote: > Since we may hold gic_lock in hardirq context, use raw spinlock > makes more sense given that it is for low-level interrupt handling > routine and the critical section is small. > > Fixes BUG: > > [ 0.426106] ============================= > [ 0.426257] [ BUG: Invalid wait context ] > [ 0.426422] 6.3.0-rc7-next-20230421-dirty #54 Not tainted > [ 0.426638] ----------------------------- > [ 0.426766] swapper/0/1 is trying to lock: > [ 0.426954] ffffffff8104e7b8 (gic_lock){....}-{3:3}, at: gic_set_type+0x30/08 > > Fixes: 95150ae8b330 ("irqchip: mips-gic: Implement irq_set_type callback") > Cc: stable@vger.kernel.org > Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> LGTM especially in the RT-patch context. Feel free to add: Reviewed-by: Serge Semin <fancer.lancer@gmail.com> Please see a tiny nitpick below. > --- > drivers/irqchip/irq-mips-gic.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c > index b568d55ef7c5..6d5ecc10a870 100644 > --- a/drivers/irqchip/irq-mips-gic.c > +++ b/drivers/irqchip/irq-mips-gic.c > @@ -50,7 +50,7 @@ void __iomem *mips_gic_base; > > static DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks); > > -static DEFINE_SPINLOCK(gic_lock); > +static DEFINE_RAW_SPINLOCK(gic_lock); > static struct irq_domain *gic_irq_domain; > static int gic_shared_intrs; > static unsigned int gic_cpu_pin; > @@ -210,7 +210,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > > irq = GIC_HWIRQ_TO_SHARED(d->hwirq); > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); AFAICS this call can be moved way down to be after the switch-case block. -Serge(y) > switch (type & IRQ_TYPE_SENSE_MASK) { > case IRQ_TYPE_EDGE_FALLING: > pol = GIC_POL_FALLING_EDGE; > @@ -250,7 +250,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) > else > irq_set_chip_handler_name_locked(d, &gic_level_irq_controller, > handle_level_irq, NULL); > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > > return 0; > } > @@ -268,7 +268,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask, > return -EINVAL; > > /* Assumption : cpumask refers to a single CPU */ > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > > /* Re-route this IRQ */ > write_gic_map_vp(irq, BIT(mips_cm_vp_id(cpu))); > @@ -279,7 +279,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask, > set_bit(irq, per_cpu_ptr(pcpu_masks, cpu)); > > irq_data_update_effective_affinity(d, cpumask_of(cpu)); > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > > return IRQ_SET_MASK_OK; > } > @@ -357,12 +357,12 @@ static void gic_mask_local_irq_all_vpes(struct irq_data *d) > cd = irq_data_get_irq_chip_data(d); > cd->mask = false; > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > for_each_online_cpu(cpu) { > write_gic_vl_other(mips_cm_vp_id(cpu)); > write_gic_vo_rmask(BIT(intr)); > } > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > } > > static void gic_unmask_local_irq_all_vpes(struct irq_data *d) > @@ -375,12 +375,12 @@ static void gic_unmask_local_irq_all_vpes(struct irq_data *d) > cd = irq_data_get_irq_chip_data(d); > cd->mask = true; > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > for_each_online_cpu(cpu) { > write_gic_vl_other(mips_cm_vp_id(cpu)); > write_gic_vo_smask(BIT(intr)); > } > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > } > > static void gic_all_vpes_irq_cpu_online(void) > @@ -393,7 +393,7 @@ static void gic_all_vpes_irq_cpu_online(void) > unsigned long flags; > int i; > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > > for (i = 0; i < ARRAY_SIZE(local_intrs); i++) { > unsigned int intr = local_intrs[i]; > @@ -407,7 +407,7 @@ static void gic_all_vpes_irq_cpu_online(void) > write_gic_vl_smask(BIT(intr)); > } > > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > } > > static struct irq_chip gic_all_vpes_local_irq_controller = { > @@ -437,11 +437,11 @@ static int gic_shared_irq_domain_map(struct irq_domain *d, unsigned int virq, > > data = irq_get_irq_data(virq); > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > write_gic_map_pin(intr, GIC_MAP_PIN_MAP_TO_PIN | gic_cpu_pin); > write_gic_map_vp(intr, BIT(mips_cm_vp_id(cpu))); > irq_data_update_effective_affinity(data, cpumask_of(cpu)); > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > > return 0; > } > @@ -533,12 +533,12 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq, > if (!gic_local_irq_is_routable(intr)) > return -EPERM; > > - spin_lock_irqsave(&gic_lock, flags); > + raw_spin_lock_irqsave(&gic_lock, flags); > for_each_online_cpu(cpu) { > write_gic_vl_other(mips_cm_vp_id(cpu)); > write_gic_vo_map(mips_gic_vx_map_reg(intr), map); > } > - spin_unlock_irqrestore(&gic_lock, flags); > + raw_spin_unlock_irqrestore(&gic_lock, flags); > > return 0; > } > -- > 2.34.1 >
> 2023年5月14日 14:56,Serge Semin <fancer.lancer@gmail.com> 写道: > > On Mon, Apr 24, 2023 at 11:31:56AM +0100, Jiaxun Yang wrote: >> Since we may hold gic_lock in hardirq context, use raw spinlock >> makes more sense given that it is for low-level interrupt handling >> routine and the critical section is small. >> >> Fixes BUG: >> >> [ 0.426106] ============================= >> [ 0.426257] [ BUG: Invalid wait context ] >> [ 0.426422] 6.3.0-rc7-next-20230421-dirty #54 Not tainted >> [ 0.426638] ----------------------------- >> [ 0.426766] swapper/0/1 is trying to lock: >> [ 0.426954] ffffffff8104e7b8 (gic_lock){....}-{3:3}, at: gic_set_type+0x30/08 >> >> Fixes: 95150ae8b330 ("irqchip: mips-gic: Implement irq_set_type callback") >> Cc: stable@vger.kernel.org >> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> > > LGTM especially in the RT-patch context. Feel free to add: > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > Please see a tiny nitpick below. > >> --- >> drivers/irqchip/irq-mips-gic.c | 30 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c >> index b568d55ef7c5..6d5ecc10a870 100644 >> --- a/drivers/irqchip/irq-mips-gic.c >> +++ b/drivers/irqchip/irq-mips-gic.c >> @@ -50,7 +50,7 @@ void __iomem *mips_gic_base; >> >> static DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks); >> >> -static DEFINE_SPINLOCK(gic_lock); >> +static DEFINE_RAW_SPINLOCK(gic_lock); >> static struct irq_domain *gic_irq_domain; >> static int gic_shared_intrs; >> static unsigned int gic_cpu_pin; >> @@ -210,7 +210,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) >> >> irq = GIC_HWIRQ_TO_SHARED(d->hwirq); >> > >> - spin_lock_irqsave(&gic_lock, flags); >> + raw_spin_lock_irqsave(&gic_lock, flags); > > AFAICS this call can be moved way down to be after the switch-case > block. Thanks for the suggestion :-) Since it actually reduced critical section I think it should not be included in this patch which Cced stable. I’ll fix that in a new patch. Thanks - Jiaxun > > -Serge(y) >
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c index b568d55ef7c5..6d5ecc10a870 100644 --- a/drivers/irqchip/irq-mips-gic.c +++ b/drivers/irqchip/irq-mips-gic.c @@ -50,7 +50,7 @@ void __iomem *mips_gic_base; static DEFINE_PER_CPU_READ_MOSTLY(unsigned long[GIC_MAX_LONGS], pcpu_masks); -static DEFINE_SPINLOCK(gic_lock); +static DEFINE_RAW_SPINLOCK(gic_lock); static struct irq_domain *gic_irq_domain; static int gic_shared_intrs; static unsigned int gic_cpu_pin; @@ -210,7 +210,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) irq = GIC_HWIRQ_TO_SHARED(d->hwirq); - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); switch (type & IRQ_TYPE_SENSE_MASK) { case IRQ_TYPE_EDGE_FALLING: pol = GIC_POL_FALLING_EDGE; @@ -250,7 +250,7 @@ static int gic_set_type(struct irq_data *d, unsigned int type) else irq_set_chip_handler_name_locked(d, &gic_level_irq_controller, handle_level_irq, NULL); - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); return 0; } @@ -268,7 +268,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask, return -EINVAL; /* Assumption : cpumask refers to a single CPU */ - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); /* Re-route this IRQ */ write_gic_map_vp(irq, BIT(mips_cm_vp_id(cpu))); @@ -279,7 +279,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask, set_bit(irq, per_cpu_ptr(pcpu_masks, cpu)); irq_data_update_effective_affinity(d, cpumask_of(cpu)); - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); return IRQ_SET_MASK_OK; } @@ -357,12 +357,12 @@ static void gic_mask_local_irq_all_vpes(struct irq_data *d) cd = irq_data_get_irq_chip_data(d); cd->mask = false; - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); for_each_online_cpu(cpu) { write_gic_vl_other(mips_cm_vp_id(cpu)); write_gic_vo_rmask(BIT(intr)); } - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); } static void gic_unmask_local_irq_all_vpes(struct irq_data *d) @@ -375,12 +375,12 @@ static void gic_unmask_local_irq_all_vpes(struct irq_data *d) cd = irq_data_get_irq_chip_data(d); cd->mask = true; - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); for_each_online_cpu(cpu) { write_gic_vl_other(mips_cm_vp_id(cpu)); write_gic_vo_smask(BIT(intr)); } - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); } static void gic_all_vpes_irq_cpu_online(void) @@ -393,7 +393,7 @@ static void gic_all_vpes_irq_cpu_online(void) unsigned long flags; int i; - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); for (i = 0; i < ARRAY_SIZE(local_intrs); i++) { unsigned int intr = local_intrs[i]; @@ -407,7 +407,7 @@ static void gic_all_vpes_irq_cpu_online(void) write_gic_vl_smask(BIT(intr)); } - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); } static struct irq_chip gic_all_vpes_local_irq_controller = { @@ -437,11 +437,11 @@ static int gic_shared_irq_domain_map(struct irq_domain *d, unsigned int virq, data = irq_get_irq_data(virq); - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); write_gic_map_pin(intr, GIC_MAP_PIN_MAP_TO_PIN | gic_cpu_pin); write_gic_map_vp(intr, BIT(mips_cm_vp_id(cpu))); irq_data_update_effective_affinity(data, cpumask_of(cpu)); - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); return 0; } @@ -533,12 +533,12 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq, if (!gic_local_irq_is_routable(intr)) return -EPERM; - spin_lock_irqsave(&gic_lock, flags); + raw_spin_lock_irqsave(&gic_lock, flags); for_each_online_cpu(cpu) { write_gic_vl_other(mips_cm_vp_id(cpu)); write_gic_vo_map(mips_gic_vx_map_reg(intr), map); } - spin_unlock_irqrestore(&gic_lock, flags); + raw_spin_unlock_irqrestore(&gic_lock, flags); return 0; }
Since we may hold gic_lock in hardirq context, use raw spinlock makes more sense given that it is for low-level interrupt handling routine and the critical section is small. Fixes BUG: [ 0.426106] ============================= [ 0.426257] [ BUG: Invalid wait context ] [ 0.426422] 6.3.0-rc7-next-20230421-dirty #54 Not tainted [ 0.426638] ----------------------------- [ 0.426766] swapper/0/1 is trying to lock: [ 0.426954] ffffffff8104e7b8 (gic_lock){....}-{3:3}, at: gic_set_type+0x30/08 Fixes: 95150ae8b330 ("irqchip: mips-gic: Implement irq_set_type callback") Cc: stable@vger.kernel.org Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com> --- drivers/irqchip/irq-mips-gic.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)