[PATCH 0/2] irqchip/mips-gic fixes

[PATCH 0/2] irqchip/mips-gic fixes

[Jiaxun Yang]

Hi all,

This patchset fixes two issues in the MIPS GIC driver that may block
booting on some systems.

Please review.

Thanks

Jiaxun Yang (2):
  irqchip/mips-gic: Don't touch vl_map if a local interrupt is not
    routable
  irqchip/mips-gic: Use raw spinlock for gic_lock

 drivers/irqchip/irq-mips-gic.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/2] irqchip/mips-gic: Don't touch vl_map if a local interrupt is not routable

[Jiaxun Yang]

When a GIC local interrupt is not routable, it's vl_map will be used
to control some internal states for core (providing IPTI, IPPCI, IPFDC
input signal for core). Overriding it will interfere core's intetrupt
controller.

Do not touch vl_map if a local interrupt is not routable, we are not
going to remap it.

Before dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on
irq_cpu_online()"), if a local interrupt is not routable, then it won't
be requested from GIC Local domain, and thus gic_all_vpes_irq_cpu_online
won't be called for that particular interrupt.

Fixes: dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on irq_cpu_online()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 drivers/irqchip/irq-mips-gic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index 046c355e120b..b568d55ef7c5 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -399,6 +399,8 @@ static void gic_all_vpes_irq_cpu_online(void)
 		unsigned int intr = local_intrs[i];
 		struct gic_all_vpes_chip_data *cd;
 
+		if (!gic_local_irq_is_routable(intr))
+			continue;
 		cd = &gic_all_vpes_chip_data[intr];
 		write_gic_vl_map(mips_gic_vx_map_reg(intr), cd->map);
 		if (cd->mask)
-- 
2.34.1

Re: [PATCH 1/2] irqchip/mips-gic: Don't touch vl_map if a local interrupt is not routable

[Serge Semin]

Hello Jiaxun

On Mon, Apr 24, 2023 at 11:31:55AM +0100, Jiaxun Yang wrote:
> When a GIC local interrupt is not routable, it's vl_map will be used
> to control some internal states for core (providing IPTI, IPPCI, IPFDC

> input signal for core). Overriding it will interfere core's intetrupt

s/intetrupt/interrupt

> controller.
> 
> Do not touch vl_map if a local interrupt is not routable, we are not
> going to remap it.
> 
> Before dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on
> irq_cpu_online()"), if a local interrupt is not routable, then it won't
> be requested from GIC Local domain, and thus gic_all_vpes_irq_cpu_online
> won't be called for that particular interrupt.
> 
> Fixes: dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on irq_cpu_online()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

My system doesn't have VPEs but 2x MIPS32 P5600 cores with GIC enabled as
EIC device so I can't fully test this change, but at the very least it
looks reasonable. Indeed performing the local IRQs routing setups for
the non-routable IRQs looks invalid. A similar change can be spotted
in the gic_irq_domain_map() method implementation.

> ---
>  drivers/irqchip/irq-mips-gic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 046c355e120b..b568d55ef7c5 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -399,6 +399,8 @@ static void gic_all_vpes_irq_cpu_online(void)
>  		unsigned int intr = local_intrs[i];
>  		struct gic_all_vpes_chip_data *cd;
>  
> +		if (!gic_local_irq_is_routable(intr))
> +			continue;

Please add newline here to distinguish the skip-step code chunk and
the setup code so the look would look a tiny bit more readable.

>  		cd = &gic_all_vpes_chip_data[intr];
>  		write_gic_vl_map(mips_gic_vx_map_reg(intr), cd->map);
>  		if (cd->mask)

Other than that the change looks good. Thanks.

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> -- 
> 2.34.1
> 

[irqchip: irq/irqchip-fixes] irqchip/mips-gic: Don't touch vl_map if a local interrupt is not routable

[irqchip-bot for Jiaxun Yang]

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID:     2c6c9c049510163090b979ea5f92a68ae8d93c45
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/2c6c9c049510163090b979ea5f92a68ae8d93c45
Author:        Jiaxun Yang <jiaxun.yang@flygoat.com>
AuthorDate:    Mon, 24 Apr 2023 11:31:55 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Tue, 16 May 2023 10:59:28 +01:00

irqchip/mips-gic: Don't touch vl_map if a local interrupt is not routable

When a GIC local interrupt is not routable, it's vl_map will be used
to control some internal states for core (providing IPTI, IPPCI, IPFDC
input signal for core). Overriding it will interfere core's intetrupt
controller.

Do not touch vl_map if a local interrupt is not routable, we are not
going to remap it.

Before dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on
irq_cpu_online()"), if a local interrupt is not routable, then it won't
be requested from GIC Local domain, and thus gic_all_vpes_irq_cpu_online
won't be called for that particular interrupt.

Fixes: dd098a0e0319 (" irqchip/mips-gic: Get rid of the reliance on irq_cpu_online()")
Cc: stable@vger.kernel.org
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20230424103156.66753-2-jiaxun.yang@flygoat.com
---
 drivers/irqchip/irq-mips-gic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index 046c355..b568d55 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -399,6 +399,8 @@ static void gic_all_vpes_irq_cpu_online(void)
 		unsigned int intr = local_intrs[i];
 		struct gic_all_vpes_chip_data *cd;
 
+		if (!gic_local_irq_is_routable(intr))
+			continue;
 		cd = &gic_all_vpes_chip_data[intr];
 		write_gic_vl_map(mips_gic_vx_map_reg(intr), cd->map);
 		if (cd->mask)

[PATCH 2/2] irqchip/mips-gic: Use raw spinlock for gic_lock

[Jiaxun Yang]

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(-)

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

Re: [PATCH 2/2] irqchip/mips-gic: Use raw spinlock for gic_lock

[Serge Semin]

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
> 

Re: [PATCH 2/2] irqchip/mips-gic: Use raw spinlock for gic_lock

[Jiaxun Yang]

> 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)
> 

[irqchip: irq/irqchip-fixes] irqchip/mips-gic: Use raw spinlock for gic_lock

[irqchip-bot for Jiaxun Yang]

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 <jiaxun.yang@flygoat.com>
AuthorDate:    Mon, 24 Apr 2023 11:31:56 +01:00
Committer:     Marc Zyngier <maz@kernel.org>
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: stable@vger.kernel.org
Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20230424103156.66753-3-jiaxun.yang@flygoat.com
---
 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;
 }

Re: [PATCH 0/2] irqchip/mips-gic fixes

[Jiaxun Yang]

> 2023年4月24日 11:31，Jiaxun Yang <jiaxun.yang@flygoat.com> 写道：
> 
> Hi all,
> 
> This patchset fixes two issues in the MIPS GIC driver that may block
> booting on some systems.
> 
> Please review.

Ping?

I expect this series go through irqchip fixes tree.

Thanks
- Jiaxun

> 
> Thanks
> 
> Jiaxun Yang (2):
>  irqchip/mips-gic: Don't touch vl_map if a local interrupt is not
>    routable
>  irqchip/mips-gic: Use raw spinlock for gic_lock
> 
> drivers/irqchip/irq-mips-gic.c | 32 +++++++++++++++++---------------
> 1 file changed, 17 insertions(+), 15 deletions(-)
> 
> -- 
> 2.34.1
> 

Re: [PATCH 0/2] irqchip/mips-gic fixes

[Serge Semin]

Hi Jiaxun

On Mon, Apr 24, 2023 at 11:31:54AM +0100, Jiaxun Yang wrote:
> Hi all,
> 
> This patchset fixes two issues in the MIPS GIC driver that may block
> booting on some systems.
> 
> Please review.

Thanks for the series. Please find some comments sent in the framework
of the corresponding patches which don't though prevent from having
the patchset tested. No problems were found on the Baikal-T1 SoC with
2x MIPS32 P5600 core on board. So feel free to add for the entire
series:

Tested-by: Serge Semin <fancer.lancer@gmail.com>

-Serge(y)

> 
> Thanks
> 
> Jiaxun Yang (2):
>   irqchip/mips-gic: Don't touch vl_map if a local interrupt is not
>     routable
>   irqchip/mips-gic: Use raw spinlock for gic_lock
> 
>  drivers/irqchip/irq-mips-gic.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> -- 
> 2.34.1
>