Re: [PATCH v3 2/5] irqchip/mips-gic: Fix recursive acquisition of gic_lock in gic_set_affinity()

[Thomas Bogendoerfer <tsbogend@alpha.franken.de>]

On Mon, Sep 07, 2026 at 02:46:36PM +0200, Benoît Monin wrote:
> Commit 322a90638768 ("irqchip/mips-gic: Multi-cluster support")
> made gic_set_affinity() call gic_set_type() when an interrupt's
> affinity is moved between clusters. gic_set_type() takes gic_lock with
> raw_spin_lock_irqsave(), but gic_set_affinity() holds gic_lock at that
> point, so the same raw spinlock is acquired twice, triggering a deadlock.
> 
> Split gic_set_type() into a gic_set_type_locked() helper that assumes
> gic_lock is already held. gic_set_type() takes gic_lock and calls the
> helper, and gic_set_affinity() calls gic_set_type_locked() directly,
> since it already holds gic_lock.
> 
> Fixes: 322a90638768 ("irqchip/mips-gic: Multi-cluster support")
> Signed-off-by: Benoît Monin <benoit.monin@bootlin.com>
> ---
>  drivers/irqchip/irq-mips-gic.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 3b31cbcbed6f..f2ae60d39d66 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -297,14 +297,14 @@ static void gic_ack_irq(struct irq_data *d)
>  	}
>  }
>  
> -static int gic_set_type(struct irq_data *d, unsigned int type)
> +static int gic_set_type_locked(struct irq_data *d, unsigned int type)
>  {
>  	unsigned int irq, pol, trig, dual;
> -	unsigned long flags;
> +
> +	lockdep_assert_held(&gic_lock);
>  
>  	irq = GIC_HWIRQ_TO_SHARED(d->hwirq);
>  
> -	raw_spin_lock_irqsave(&gic_lock, flags);
>  	switch (type & IRQ_TYPE_SENSE_MASK) {
>  	case IRQ_TYPE_EDGE_FALLING:
>  		pol = GIC_POL_FALLING_EDGE;
> @@ -351,11 +351,16 @@ 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);
> -	raw_spin_unlock_irqrestore(&gic_lock, flags);
>  
>  	return 0;
>  }
>  
> +static int gic_set_type(struct irq_data *d, unsigned int type)
> +{
> +	guard(raw_spinlock_irqsave)(&gic_lock);
> +	return gic_set_type_locked(d, type);
> +}
> +
>  #ifdef CONFIG_SMP
>  static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask,
>  			    bool force)
> @@ -407,7 +412,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *cpumask,
>  	 * trigger type in the new cluster.
>  	 */
>  	if (cl != old_cl)
> -		gic_set_type(d, irqd_get_trigger_type(d));
> +		gic_set_type_locked(d, irqd_get_trigger_type(d));
>  
>  	/* Route the interrupt to its new VP(E) */
>  	if (gic_irq_lock_cluster(d)) {
> 
> -- 
> 2.55.0

Reviewed-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]