Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 14 May 2023 16:56:01 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc:     maz@kernel.org, tsbogend@alpha.franken.de, tglx@linutronix.de,
        linux-mips@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org
Subject: Re: [PATCH 2/2] irqchip/mips-gic: Use raw spinlock for gic_lock
Message-ID: <20230514135601.5irhslf6tdv4tk5z@mobilestation>
References: <20230424103156.66753-1-jiaxun.yang@flygoat.com>
 <20230424103156.66753-3-jiaxun.yang@flygoat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230424103156.66753-3-jiaxun.yang@flygoat.com>
Precedence: bulk

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
>