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

[Jiaxun Yang]

> 2023年5月14日 14:56，Serge Semin <fancer.lancer@xxxxxxxxx> 写道：
> 
> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Jiaxun Yang <jiaxun.yang@xxxxxxxxxxx>
> 
> LGTM especially in the RT-patch context. Feel free to add:
> Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> 
> 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)
>