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: [email protected]
Signed-off-by: Jiaxun Yang <[email protected]>
---
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);
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
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: [email protected]
> Signed-off-by: Jiaxun Yang <[email protected]>
LGTM especially in the RT-patch context. Feel free to add:
Reviewed-by: Serge Semin <[email protected]>
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 <[email protected]> 写道：
>
> 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: [email protected]
>> Signed-off-by: Jiaxun Yang <[email protected]>
>
> LGTM especially in the RT-patch context. Feel free to add:
> Reviewed-by: Serge Semin <[email protected]>
>
> 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)
>
The following commit has been merged into the irq/irqchip-fixes branch of irqchip:
Commit-ID: 3d6a0e4197c04599d75d85a608c8bb16a630a38c
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/3d6a0e4197c04599d75d85a608c8bb16a630a38c
Author: Jiaxun Yang <[email protected]>
AuthorDate: Mon, 24 Apr 2023 11:31:56 +01:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Tue, 16 May 2023 10:59:28 +01:00
irqchip/mips-gic: Use raw spinlock for gic_lock
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: [email protected]
Signed-off-by: Jiaxun Yang <[email protected]>
Reviewed-by: Serge Semin <[email protected]>
Tested-by: Serge Semin <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
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 b568d55..6d5ecc1 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;
}