[PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[Marek Szyprowski]

The exynos-combiner driver uses a regular spinlock to protect access to
the combiner interrupt status register in combiner_handle_cascade_irq(),
which is invoked in hard IRQ context as a chained interrupt handler.

When PREEMPT_RT is enabled on ARM, regular spinlock is converted to a
sleeping lock (mutex-based), which must not be used in atomic context
such as hard interrupt handlers. Switch the irq_controller_lock to
raw_spinlock, which remains a true non-sleeping spinlock even under
PREEMPT_RT.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: a900e5d99718 ("ARM: exynos: move exynos4210-combiner to drivers/irqchip")
---
 drivers/irqchip/exynos-combiner.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/exynos-combiner.c b/drivers/irqchip/exynos-combiner.c
index 11d105457798..03cafcc5c835 100644
--- a/drivers/irqchip/exynos-combiner.c
+++ b/drivers/irqchip/exynos-combiner.c
@@ -24,7 +24,7 @@
 
 #define IRQ_IN_COMBINER		8
 
-static DEFINE_SPINLOCK(irq_controller_lock);
+static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 struct combiner_chip_data {
 	unsigned int hwirq_offset;
@@ -72,9 +72,9 @@ static void combiner_handle_cascade_irq(struct irq_desc *desc)
 
 	chained_irq_enter(chip, desc);
 
-	spin_lock(&irq_controller_lock);
+	raw_spin_lock(&irq_controller_lock);
 	status = readl_relaxed(chip_data->base + COMBINER_INT_STATUS);
-	spin_unlock(&irq_controller_lock);
+	raw_spin_unlock(&irq_controller_lock);
 	status &= chip_data->irq_mask;
 
 	if (status == 0)
-- 
2.34.1

Re: [PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[Sebastian Andrzej Siewior]

On 2026-05-21 00:04:22 [+0200], Marek Szyprowski wrote:
> The exynos-combiner driver uses a regular spinlock to protect access to
> the combiner interrupt status register in combiner_handle_cascade_irq(),
> which is invoked in hard IRQ context as a chained interrupt handler.
> 
> When PREEMPT_RT is enabled on ARM, regular spinlock is converted to a
> sleeping lock (mutex-based), which must not be used in atomic context
> such as hard interrupt handlers. Switch the irq_controller_lock to
> raw_spinlock, which remains a true non-sleeping spinlock even under
> PREEMPT_RT.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: a900e5d99718 ("ARM: exynos: move exynos4210-combiner to drivers/irqchip")

For the change based on the reasoning
Reviewed-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

but why do you need a lock around a single read of a register? 
As far as I can tell, it was introduced in commit
   84bbc16c1f621 ("ARM: S5PV310: Add IRQ support")

and then dragged through the kernel, merged, renamed, unmerged until it
got where it is today.

This
	https://lore.kernel.org/all/1277476037-8806-5-git-send-email-kgene.kim@samsung.com/

is v1, as you see the lock is used in multiple places. Then someone
asked "why locking"
	https://lore.kernel.org/all/20100628144743.GB3287@debian/

and in v2
	https://lore.kernel.org/all/1279270714-15146-5-git-send-email-kgene.kim@samsung.com/

it went down to a single user. 

It looks like a leftover from initial development. So it would make
sense to einer remove it or add a comment why it is still there after
all these years since it is not obvious.

Sebastian

Re: [PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[Thomas Gleixner]

On Thu, May 21 2026 at 00:04, Marek Szyprowski wrote:

> The exynos-combiner driver uses a regular spinlock to protect access to
> the combiner interrupt status register in combiner_handle_cascade_irq(),
> which is invoked in hard IRQ context as a chained interrupt handler.
>
> When PREEMPT_RT is enabled on ARM, regular spinlock is converted to a
> sleeping lock (mutex-based), which must not be used in atomic context
> such as hard interrupt handlers. Switch the irq_controller_lock to
> raw_spinlock, which remains a true non-sleeping spinlock even under
> PREEMPT_RT.

Mechanically this makes sense, but out of curiosity I have to ask:

> -static DEFINE_SPINLOCK(irq_controller_lock);
> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
>  struct combiner_chip_data {
>  	unsigned int hwirq_offset;
> @@ -72,9 +72,9 @@ static void combiner_handle_cascade_irq(struct irq_desc *desc)
>  
>  	chained_irq_enter(chip, desc);
>  
> -	spin_lock(&irq_controller_lock);
> +	raw_spin_lock(&irq_controller_lock);
>  	status = readl_relaxed(chip_data->base + COMBINER_INT_STATUS);
> -	spin_unlock(&irq_controller_lock);
> +	raw_spin_unlock(&irq_controller_lock);

What is this lock actually protecting?

Each combiner has it's own @base address, so there is no concurrency
problem between two cascade interrupts being handled at the same time.

That means the only possible problem would be that the same cascade
interrupt is handled on two CPUs concurrently. Is that even possible?

Thanks,

        tglx

Re: [PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[Marek Szyprowski]

On 21.05.2026 11:06, Thomas Gleixner wrote:
> On Thu, May 21 2026 at 00:04, Marek Szyprowski wrote:
>> The exynos-combiner driver uses a regular spinlock to protect access to
>> the combiner interrupt status register in combiner_handle_cascade_irq(),
>> which is invoked in hard IRQ context as a chained interrupt handler.
>>
>> When PREEMPT_RT is enabled on ARM, regular spinlock is converted to a
>> sleeping lock (mutex-based), which must not be used in atomic context
>> such as hard interrupt handlers. Switch the irq_controller_lock to
>> raw_spinlock, which remains a true non-sleeping spinlock even under
>> PREEMPT_RT.
> Mechanically this makes sense, but out of curiosity I have to ask:
>
>> -static DEFINE_SPINLOCK(irq_controller_lock);
>> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>>  
>>  struct combiner_chip_data {
>>  	unsigned int hwirq_offset;
>> @@ -72,9 +72,9 @@ static void combiner_handle_cascade_irq(struct irq_desc *desc)
>>  
>>  	chained_irq_enter(chip, desc);
>>  
>> -	spin_lock(&irq_controller_lock);
>> +	raw_spin_lock(&irq_controller_lock);
>>  	status = readl_relaxed(chip_data->base + COMBINER_INT_STATUS);
>> -	spin_unlock(&irq_controller_lock);
>> +	raw_spin_unlock(&irq_controller_lock);
> What is this lock actually protecting?
>
> Each combiner has it's own @base address, so there is no concurrency
> problem between two cascade interrupts being handled at the same time.
>
> That means the only possible problem would be that the same cascade
> interrupt is handled on two CPUs concurrently. Is that even possible?
Frankly speaking I did this conversion mechanically, late in the evening to fix
the bug warning I've spotted. Indeed this spinlock looks like a copy&paste or
development leftover. The only side-effect of it I see is a memory barrier,
which might affect how the register access happens, but this should not affect
cascade operation imho. Do You want me to send a patch removing it completely?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] irqchip/exynos-combiner: switch to raw_spinlock

[Thomas Gleixner]

On Thu, May 21 2026 at 13:26, Marek Szyprowski wrote:
> On 21.05.2026 11:06, Thomas Gleixner wrote:
>> What is this lock actually protecting?
>>
>> Each combiner has it's own @base address, so there is no concurrency
>> problem between two cascade interrupts being handled at the same time.
>>
>> That means the only possible problem would be that the same cascade
>> interrupt is handled on two CPUs concurrently. Is that even possible?
> Frankly speaking I did this conversion mechanically, late in the evening to fix
> the bug warning I've spotted. Indeed this spinlock looks like a copy&paste or
> development leftover. The only side-effect of it I see is a memory barrier,
> which might affect how the register access happens, but this should not affect
> cascade operation imho. Do You want me to send a patch removing it completely?

I've applied the fixup for now. But, yes please send a removal against

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git irq/urgent

Thanks,

        tglx