[PATCH AUTOSEL 6.19-5.10] clocksource/drivers/sh_tmu: Always leave device running after probe

[PATCH AUTOSEL 6.19-5.10] clocksource/drivers/sh_tmu: Always leave device running after probe

[Sasha Levin]

From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

[ Upstream commit b1278972b08e480990e2789bdc6a7c918bc349be ]

The TMU device can be used as both a clocksource and a clockevent
provider. The driver tries to be smart and power itself on and off, as
well as enabling and disabling its clock when it's not in operation.
This behavior is slightly altered if the TMU is used as an early
platform device in which case the device is left powered on after probe,
but the clock is still enabled and disabled at runtime.

This has worked for a long time, but recent improvements in PREEMPT_RT
and PROVE_LOCKING have highlighted an issue. As the TMU registers itself
as a clockevent provider, clockevents_register_device(), it needs to use
raw spinlocks internally as this is the context of which the clockevent
framework interacts with the TMU driver. However in the context of
holding a raw spinlock the TMU driver can't really manage its power
state or clock with calls to pm_runtime_*() and clk_*() as these calls
end up in other platform drivers using regular spinlocks to control
power and clocks.

This mix of spinlock contexts trips a lockdep warning.

    =============================
    [ BUG: Invalid wait context ]
    6.18.0-arm64-renesas-09926-gee959e7c5e34 #1 Not tainted
    -----------------------------
    swapper/0/0 is trying to lock:
    ffff000008c9e180 (&dev->power.lock){-...}-{3:3}, at: __pm_runtime_resume+0x38/0x88
    other info that might help us debug this:
    context-{5:5}
    1 lock held by swapper/0/0:
    ccree e6601000.crypto: ARM CryptoCell 630P Driver: HW version 0xAF400001/0xDCC63000, Driver version 5.0
     #0: ffff8000817ec298
    ccree e6601000.crypto: ARM ccree device initialized
     (tick_broadcast_lock){-...}-{2:2}, at: __tick_broadcast_oneshot_control+0xa4/0x3a8
    stack backtrace:
    CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.18.0-arm64-renesas-09926-gee959e7c5e34 #1 PREEMPT
    Hardware name: Renesas Salvator-X 2nd version board based on r8a77965 (DT)
    Call trace:
     show_stack+0x14/0x1c (C)
     dump_stack_lvl+0x6c/0x90
     dump_stack+0x14/0x1c
     __lock_acquire+0x904/0x1584
     lock_acquire+0x220/0x34c
     _raw_spin_lock_irqsave+0x58/0x80
     __pm_runtime_resume+0x38/0x88
     sh_tmu_clock_event_set_oneshot+0x84/0xd4
     clockevents_switch_state+0xfc/0x13c
     tick_broadcast_set_event+0x30/0xa4
     __tick_broadcast_oneshot_control+0x1e0/0x3a8
     tick_broadcast_oneshot_control+0x30/0x40
     cpuidle_enter_state+0x40c/0x680
     cpuidle_enter+0x30/0x40
     do_idle+0x1f4/0x280
     cpu_startup_entry+0x34/0x40
     kernel_init+0x0/0x130
     do_one_initcall+0x0/0x230
     __primary_switched+0x88/0x90

For non-PREEMPT_RT builds this is not really an issue, but for
PREEMPT_RT builds where normal spinlocks can sleep this might be an
issue. Be cautious and always leave the power and clock running after
probe.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
Link: https://patch.msgid.link/20251202221341.1856773-1-niklas.soderlund+renesas@ragnatech.se
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have a complete picture. Let me compile my thorough analysis.

---

## Detailed Analysis

### 1. COMMIT MESSAGE ANALYSIS

The commit message is thorough and well-written. It clearly describes:
- **The problem**: An invalid wait context lockdep warning (`[ BUG:
  Invalid wait context ]`) on PREEMPT_RT kernels
- **The root cause**: The TMU driver tries to manage PM runtime
  (`pm_runtime_get_sync`/`pm_runtime_put`) and clock state
  (`clk_enable`/`clk_disable`) at runtime, but these calls happen within
  a raw spinlock context from the clockevent framework
- **The fix strategy**: Leave the device and clock always running after
  probe
- **Full stack trace**: Reproduced on real hardware (Renesas Salvator-X
  board with r8a77965 SoC)
- **Tested-by**: Geert Uytterhoeven, a very well-known Renesas platform
  maintainer
- **Signed off by**: Daniel Lezcano, the clocksource subsystem
  maintainer

### 2. CODE CHANGE ANALYSIS - THE BUG MECHANISM

The bug is a **lock ordering / invalid wait context** issue. The precise
call chain is:

1. `cpuidle_enter_state` → `__tick_broadcast_oneshot_control` acquires
   `tick_broadcast_lock` (a **raw spinlock**, lock class `{-...}-{2:2}`)
2. Inside the raw spinlock, `clockevents_switch_state` →
   `sh_tmu_clock_event_set_oneshot` → `sh_tmu_clock_event_set_state` →
   `sh_tmu_enable`
3. `sh_tmu_enable` calls `pm_runtime_get_sync(&ch->tmu->pdev->dev)`
   which tries to acquire `dev->power.lock` (a **regular spinlock**,
   lock class `{-...}-{3:3}`)

I verified this call chain through the code:
- `___tick_broadcast_oneshot_control` (line 796 of `tick-broadcast.c`)
  does `raw_spin_lock(&tick_broadcast_lock)` at the top, then at line
  889 calls `clockevents_switch_state(dev, CLOCK_EVT_STATE_ONESHOT)`
- Similarly, `broadcast_shutdown_local` calls
  `clockevents_switch_state(dev, CLOCK_EVT_STATE_SHUTDOWN)` while
  tick_broadcast_lock is held
- `tick_broadcast_set_event` also calls `clockevents_switch_state`
  within the lock

On **PREEMPT_RT**, regular spinlocks are sleeping locks (they can
schedule). Acquiring a sleeping lock while holding a raw spinlock is
**illegal** - it can cause sleeping in atomic context or deadlock. The
lockdep annotation `{-...}-{2:2}` vs `{-...}-{3:3}` in the stack trace
confirms the context mismatch.

### 3. THE FIX

The patch is a **pure deletion** (18 lines removed, 0 added):

1. **`__sh_tmu_enable()`**: Removes `clk_enable()` call (clock stays
   enabled from probe)
2. **`sh_tmu_enable()`**: Removes `pm_runtime_get_sync()` call (PM
   runtime stays active from probe)
3. **`__sh_tmu_disable()`**: Removes `clk_disable()` call (clock never
   disabled)
4. **`sh_tmu_disable()`**: Removes `pm_runtime_put()` call (PM runtime
   never released)
5. **`sh_tmu_setup()`**: Removes `clk_disable()` after rate measurement
   (clock stays enabled)
6. **`sh_tmu_probe()`**: Removes `pm_runtime_idle()` else branch (PM
   runtime stays active)

The trade-off is slightly higher power consumption (the TMU hardware
stays powered/clocked when not actively timing), but this is acceptable
given the alternative is a hard bug.

### 4. SUSPEND/RESUME SAFETY

I verified the suspend/resume paths still work correctly:
- `sh_tmu_clocksource_suspend` calls `__sh_tmu_disable` (still stops the
  channel) + `dev_pm_genpd_suspend` (handles power domain)
- `sh_tmu_clocksource_resume` calls `dev_pm_genpd_resume` +
  `__sh_tmu_enable` (still restores registers)
- Since `clk_enable` count stays at 1 (never disabled), the clock
  framework correctly restores hardware state after genpd resume

### 5. SCOPE AND RISK

- **Files affected**: 1 file (`drivers/clocksource/sh_tmu.c`)
- **Lines changed**: 18 deletions, 0 additions
- **Risk**: Very low - only removes code that dynamically toggles
  power/clock; the conservative approach (always-on) is simpler and
  safer
- **Regression potential**: The only downside is marginally higher power
  consumption on Renesas platforms using TMU, which is negligible
- **Self-contained**: No dependencies on other patches; the companion
  sh_cmt fix (`62524f285c11`) is for a different driver

### 6. APPLICABILITY TO STABLE TREES

I verified the file is **identical** in stable trees 5.15, 6.1, 6.6,
6.12, and the current HEAD (6.19). The patch will apply cleanly to all
active stable trees without any modification.

### 7. USER IMPACT

- **Who is affected**: Users running PREEMPT_RT kernels on Renesas ARM64
  platforms with TMU timers
- **Severity without fix**: Invalid wait context → potential sleeping in
  atomic context → system instability/hang on PREEMPT_RT
- **PREEMPT_RT relevance**: PREEMPT_RT has been merged into mainline and
  is supported in stable trees (verified PREEMPT_RT fixes exist in
  6.12.y stable). This is increasingly used in embedded/industrial
  systems.

### 8. CLASSIFICATION

This is a **locking/synchronization bug fix**:
- Fixes invalid wait context (raw spinlock → regular spinlock
  acquisition)
- Prevents potential sleeping in atomic context on PREEMPT_RT
- Reproducible with lockdep enabled (PROVE_LOCKING)
- Real-world impact on PREEMPT_RT builds (not theoretical)
- Small, surgical, single-driver fix
- Tested on real hardware
- Reviewed and signed off by subsystem maintainer

**YES** signals:
- Fixes a real lockdep BUG warning (potential deadlock/sleep-in-atomic)
- Small, contained fix (18 line deletions in one file)
- Tested-by experienced maintainer
- Applies cleanly to all stable trees
- No dependencies
- Conservative approach (remove complexity, not add it)

**NO** signals: None identified.

**YES**

 drivers/clocksource/sh_tmu.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index beffff81c00f3..3fc6ed9b56300 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -143,16 +143,6 @@ static void sh_tmu_start_stop_ch(struct sh_tmu_channel *ch, int start)
 
 static int __sh_tmu_enable(struct sh_tmu_channel *ch)
 {
-	int ret;
-
-	/* enable clock */
-	ret = clk_enable(ch->tmu->clk);
-	if (ret) {
-		dev_err(&ch->tmu->pdev->dev, "ch%u: cannot enable clock\n",
-			ch->index);
-		return ret;
-	}
-
 	/* make sure channel is disabled */
 	sh_tmu_start_stop_ch(ch, 0);
 
@@ -174,7 +164,6 @@ static int sh_tmu_enable(struct sh_tmu_channel *ch)
 	if (ch->enable_count++ > 0)
 		return 0;
 
-	pm_runtime_get_sync(&ch->tmu->pdev->dev);
 	dev_pm_syscore_device(&ch->tmu->pdev->dev, true);
 
 	return __sh_tmu_enable(ch);
@@ -187,9 +176,6 @@ static void __sh_tmu_disable(struct sh_tmu_channel *ch)
 
 	/* disable interrupts in TMU block */
 	sh_tmu_write(ch, TCR, TCR_TPSC_CLK4);
-
-	/* stop clock */
-	clk_disable(ch->tmu->clk);
 }
 
 static void sh_tmu_disable(struct sh_tmu_channel *ch)
@@ -203,7 +189,6 @@ static void sh_tmu_disable(struct sh_tmu_channel *ch)
 	__sh_tmu_disable(ch);
 
 	dev_pm_syscore_device(&ch->tmu->pdev->dev, false);
-	pm_runtime_put(&ch->tmu->pdev->dev);
 }
 
 static void sh_tmu_set_next(struct sh_tmu_channel *ch, unsigned long delta,
@@ -552,7 +537,6 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
 		goto err_clk_unprepare;
 
 	tmu->rate = clk_get_rate(tmu->clk) / 4;
-	clk_disable(tmu->clk);
 
 	/* Map the memory resource. */
 	ret = sh_tmu_map_memory(tmu);
@@ -626,8 +610,6 @@ static int sh_tmu_probe(struct platform_device *pdev)
  out:
 	if (tmu->has_clockevent || tmu->has_clocksource)
 		pm_runtime_irq_safe(&pdev->dev);
-	else
-		pm_runtime_idle(&pdev->dev);
 
 	return 0;
 }
-- 
2.51.0

[PATCH AUTOSEL 6.19-5.15] char: tpm: cr50: Remove IRQF_ONESHOT

[Sasha Levin]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

[ Upstream commit 1affd29ffbd50125a5492c6be1dbb1f04be18d4f ]

Passing IRQF_ONESHOT ensures that the interrupt source is masked until
the secondary (threaded) handler is done. If only a primary handler is
used then the flag makes no sense because the interrupt can not fire
(again) while its handler is running.

The flag also prevents force-threading of the primary handler and the
irq-core will warn about this.

Remove IRQF_ONESHOT from irqflags.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Link: https://patch.msgid.link/20260128095540.863589-10-bigeasy@linutronix.de
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Now I have a complete understanding. Let me write up the comprehensive
analysis.

---

## Comprehensive Analysis

### 1. Commit Message Analysis

The commit subject is "char: tpm: cr50: Remove IRQF_ONESHOT" and the
message explains three points:
1. `IRQF_ONESHOT` is for masking the interrupt source until the
   **threaded** (secondary) handler completes
2. These drivers only have primary handlers, so the flag is meaningless
3. The flag **prevents force-threading** of the primary handler, and the
   irq-core will warn about this

The commit is authored by **Sebastian Andrzej Siewior** (bigeasy), the
PREEMPT_RT maintainer, and signed off by **Thomas Gleixner**, the IRQ
subsystem maintainer and co-maintainer of the Linux kernel. It was
reviewed by **Jarkko Sakkinen**, the TPM subsystem maintainer.

### 2. Code Change Analysis

The change is minimal and surgical:
- **`tpm_tis_i2c_cr50.c`**: Removes `IRQF_ONESHOT` from the flags in
  `devm_request_irq()`, keeping `IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN`
- **`tpm_tis_spi_cr50.c`**: Removes `IRQF_ONESHOT` from the flags in
  `devm_request_irq()`, keeping `IRQF_TRIGGER_RISING`

Both interrupt handlers (`tpm_cr50_i2c_int_handler` at line 74 and
`cr50_spi_irq_handler` at line 65) are trivially simple - they just call
`complete()` and return `IRQ_HANDLED`. There is no thread_fn.
`devm_request_irq()` is a wrapper that calls
`devm_request_threaded_irq()` with `thread_fn = NULL`.

### 3. The Real Bug

The companion commit **`aef30c8d569c`** ("genirq: Warn about using
IRQF_ONESHOT without a threaded handler") was merged on 2026-01-12 and
adds a `WARN_ON_ONCE()` in `__setup_irq()`:

```c
WARN_ON_ONCE(new->flags & IRQF_ONESHOT && !new->thread_fn);
```

This means that **without this cr50 fix**, every time the cr50 TPM
driver probes on a system with the updated IRQ core, it will emit a
`WARN_ON_ONCE` kernel warning at boot. This is a real runtime issue that
would affect all Chromebook and other systems using cr50/ti50 TPM chips.

More importantly, the core technical issue is that `IRQF_ONESHOT`
prevents force-threading of the primary handler. From
`irq_setup_forced_threading()` in `kernel/irq/manage.c`:

```c
if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
    return 0;  // Skip force-threading!
```

On **PREEMPT_RT kernels** (where `force_irqthreads()` returns `true`),
this means the cr50 interrupt handler runs in hardirq context instead of
being force-threaded. While the handler itself (`complete()`) is safe in
hardirq context, this defeats the PREEMPT_RT design goal of having all
interrupt handlers run in thread context. On non-RT systems with
`threadirqs` boot parameter, the same issue occurs.

### 4. Classification

This is a **bug fix** that addresses:
1. **A spurious kernel warning** triggered by the new `WARN_ON_ONCE`
   check added in `aef30c8d569c`
2. **Incorrect IRQ flags** - `IRQF_ONESHOT` has never been semantically
   correct for these drivers since they only use primary handlers
3. **PREEMPT_RT compatibility** - the flag prevents force-threading,
   which can be problematic

The fix is not a feature addition - it removes an incorrect flag that
was always wrong.

### 5. Scope and Risk Assessment

- **Lines changed**: 2 files, 2 lines total (removing `IRQF_ONESHOT`
  from two flag constants)
- **Risk**: Extremely low. `IRQF_ONESHOT` had no functional effect on
  these drivers since they use only primary handlers. The interrupt
  cannot fire while its primary handler is already running (hardware
  guarantee), so removing the flag changes nothing about interrupt
  masking behavior.
- **Subsystem**: TPM drivers, well-understood, mature code
- **Testing**: Reviewed by TPM maintainer (Jarkko Sakkinen), authored by
  PREEMPT_RT maintainer, signed off by IRQ core maintainer

### 6. User Impact

- **Who is affected**: All users with cr50/ti50 TPM chips (primarily
  Chromebooks and Google-based embedded devices)
- **Severity without fix**: Kernel `WARN_ON_ONCE` at boot if the
  `aef30c8d569c` warning commit is also backported; PREEMPT_RT
  degradation
- **Severity of regression risk**: Essentially zero - the flag had no
  functional purpose

### 7. Dependencies

This commit is **self-contained**. It does not require any other patches
to apply cleanly. However, the motivating warning comes from
`aef30c8d569c` - if that commit is backported to stable, this fix
becomes essential to avoid warnings. Even without the warning commit,
this fix is correct and beneficial (enables proper force-threading on RT
systems).

The i2c-spacemit driver already got a similar fix (`e351836a54e3`) with
`Cc: stable@vger.kernel.org` tag, establishing precedent that these
IRQF_ONESHOT removal patches are considered stable material.

### 8. Stability Indicators

- Author: Sebastian Andrzej Siewior (PREEMPT_RT maintainer) - domain
  expert
- Signed-off-by: Thomas Gleixner (IRQ subsystem maintainer) - domain
  expert
- Reviewed-by: Jarkko Sakkinen (TPM maintainer) - domain expert
- Part of a systematic cleanup pattern with prior art (i2c-exynos5,
  i2c-hix5hd2, i2c-spacemit, drm/msm)

### Conclusion

This is a small, surgical, obviously correct bug fix. The `IRQF_ONESHOT`
flag was always semantically wrong for these primary-only handlers. The
fix removes a flag that had no functional benefit but actively prevented
force-threading on PREEMPT_RT systems and will trigger a `WARN_ON_ONCE`
with the companion IRQ core warning check. The risk is near-zero, the
change is trivial (2 lines across 2 files), it was reviewed by all three
relevant subsystem maintainers, and there is clear precedent for
identical fixes being marked as stable material (the i2c-spacemit fix
had `Cc: stable@vger.kernel.org`).

**YES**

 drivers/char/tpm/tpm_tis_i2c_cr50.c | 3 +--
 drivers/char/tpm/tpm_tis_spi_cr50.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c b/drivers/char/tpm/tpm_tis_i2c_cr50.c
index fc6891a0b6936..b48cacacc0664 100644
--- a/drivers/char/tpm/tpm_tis_i2c_cr50.c
+++ b/drivers/char/tpm/tpm_tis_i2c_cr50.c
@@ -749,8 +749,7 @@ static int tpm_cr50_i2c_probe(struct i2c_client *client)
 
 	if (client->irq > 0) {
 		rc = devm_request_irq(dev, client->irq, tpm_cr50_i2c_int_handler,
-				      IRQF_TRIGGER_FALLING | IRQF_ONESHOT |
-				      IRQF_NO_AUTOEN,
+				      IRQF_TRIGGER_FALLING | IRQF_NO_AUTOEN,
 				      dev->driver->name, chip);
 		if (rc < 0) {
 			dev_err(dev, "Failed to probe IRQ %d\n", client->irq);
diff --git a/drivers/char/tpm/tpm_tis_spi_cr50.c b/drivers/char/tpm/tpm_tis_spi_cr50.c
index f4937280e9406..32920b4cecfb4 100644
--- a/drivers/char/tpm/tpm_tis_spi_cr50.c
+++ b/drivers/char/tpm/tpm_tis_spi_cr50.c
@@ -287,7 +287,7 @@ int cr50_spi_probe(struct spi_device *spi)
 	if (spi->irq > 0) {
 		ret = devm_request_irq(&spi->dev, spi->irq,
 				       cr50_spi_irq_handler,
-				       IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+				       IRQF_TRIGGER_RISING,
 				       "cr50_spi", cr50_phy);
 		if (ret < 0) {
 			if (ret == -EPROBE_DEFER)
-- 
2.51.0

[PATCH AUTOSEL 6.19-5.10] mailbox: bcm-ferxrm-mailbox: Use default primary handler

[Sasha Levin]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

[ Upstream commit 03843d95a4a4e0ba22ad4fcda65ccf21822b104c ]

request_threaded_irq() is invoked with a primary and a secondary handler
and no flags are passed. The primary handler is the same as
irq_default_primary_handler() so there is no need to have an identical
copy.

The lack of the IRQF_ONESHOT flag can be dangerous because the interrupt
source is not masked while the threaded handler is active. This means,
especially on LEVEL typed interrupt lines, the interrupt can fire again
before the threaded handler had a chance to run.

Use the default primary interrupt handler by specifying NULL and set
IRQF_ONESHOT so the interrupt source is masked until the secondary handler
is done.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
Link: https://patch.msgid.link/20260128095540.863589-5-bigeasy@linutronix.de
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Complete Analysis

### 1. COMMIT MESSAGE ANALYSIS

The subject says "Use default primary handler" which sounds like a
cleanup, but the commit body reveals a real bug:

> "The lack of the IRQF_ONESHOT flag can be dangerous because the
interrupt source is not masked while the threaded handler is active.
This means, especially on LEVEL typed interrupt lines, the interrupt can
fire again before the threaded handler had a chance to run."

The authors are Sebastian Andrzej Siewior (bigeasy, a core IRQ/RT
developer) and Thomas Gleixner (the IRQ subsystem maintainer and co-
author of the `__setup_irq` protection). This is coming from the very
people who understand threaded IRQ semantics best.

### 2. CODE CHANGE ANALYSIS

The change is small and surgical — two distinct modifications:

**A) Remove the redundant `flexrm_irq_event` primary handler:**

```1176:1182:drivers/mailbox/bcm-flexrm-mailbox.c
static irqreturn_t flexrm_irq_event(int irq, void *dev_id)
{
        /* We only have MSI for completions so just wakeup IRQ thread */
        /* Ring related errors will be informed via completion
descriptors */

        return IRQ_WAKE_THREAD;
}
```

This function is **identical** in behavior to
`irq_default_primary_handler()` in `kernel/irq/manage.c`:

```976:979:kernel/irq/manage.c
static irqreturn_t irq_default_primary_handler(int irq, void *dev_id)
{
        return IRQ_WAKE_THREAD;
}
```

Both simply return `IRQ_WAKE_THREAD`. There's zero functional
difference.

**B) Change the `request_threaded_irq()` call:**

Old code:
```
request_threaded_irq(ring->irq, flexrm_irq_event, flexrm_irq_thread, 0,
...)
```

New code:
```
request_threaded_irq(ring->irq, NULL, flexrm_irq_thread, IRQF_ONESHOT,
...)
```

### 3. THE BUG MECHANISM

This is a real bug with two dimensions:

**Dimension 1: Missing IRQF_ONESHOT on non-ONESHOT_SAFE interrupts**

The bcm-flexrm-mailbox driver uses **platform MSI** (via
`platform_device_msi_init_and_alloc_irqs()`), NOT PCI MSI. I verified
that while PCI MSI irqchips have `IRQCHIP_ONESHOT_SAFE` set (in
`drivers/pci/msi/irqdomain.c`), platform MSI does NOT. This means the
IRQ subsystem's safety net — automatically stripping `IRQF_ONESHOT` for
chips that don't need it — does not apply here.

Without `IRQF_ONESHOT`, the interrupt line is **not masked** while the
threaded handler (`flexrm_irq_thread`) runs. On a **level-triggered**
interrupt line, this creates an interrupt storm:
1. Interrupt fires → primary handler returns `IRQ_WAKE_THREAD`
2. Interrupt line is re-enabled immediately (no masking)
3. Device still has the line asserted → interrupt fires again
   immediately
4. Goto 1 — the thread never gets to run, the system is stuck in hard
   IRQ context

The commit message explicitly describes this: "especially on LEVEL typed
interrupt lines, the interrupt can fire again before the threaded
handler had a chance to run."

**Dimension 2: Forced threading bypass**

The old code provided an explicit primary handler (`flexrm_irq_event`),
even though it's functionally identical to
`irq_default_primary_handler`. This is problematic because
`irq_setup_forced_threading()` has a special check:

```1302:1303:kernel/irq/manage.c
        if (new->handler == irq_default_primary_handler)
                return 0;
```

When `handler != irq_default_primary_handler` (i.e., it's the driver's
custom `flexrm_irq_event`), forced threading proceeds and creates a
**secondary action** with the original thread handler, and converts the
primary handler to run in a thread too. This is wasteful and changes the
behavior on PREEMPT_RT kernels — instead of a simple wake-and-handle
flow, it creates an unnecessary secondary handler chain. But more
critically, with the old code and forced threading, the check at line
1295:

```1295:1296:kernel/irq/manage.c
        if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
                return 0;
```

Since flags=0 (no IRQF_ONESHOT), forced threading continues and sets
`IRQF_ONESHOT` at line 1305. But without forced threading (normal
kernels), IRQF_ONESHOT is never set, and the interrupt runs without
masking.

**Why the old code doesn't hit the __setup_irq safety check:**

The `__setup_irq` code at line 1664-1684 rejects `handler==NULL` without
`IRQF_ONESHOT` by checking `new->handler ==
irq_default_primary_handler`. But since the old code passes
`flexrm_irq_event` (a different function pointer that does the same
thing), this safety check is **bypassed**. The driver sneaks past the
protection that Thomas Gleixner himself added in commit 1c6c69525b40e
("genirq: Reject bogus threaded irq requests").

### 4. CLASSIFICATION

This is a **bug fix** — specifically fixing a potential interrupt storm
/ system hang on level-triggered interrupt configurations. It's
disguised as cleanup but addresses a real correctness issue.

### 5. SCOPE AND RISK ASSESSMENT

- **Lines changed:** ~10 lines removed, ~2 lines changed — extremely
  small
- **Files touched:** 1 file (`drivers/mailbox/bcm-flexrm-mailbox.c`)
- **Risk:** Very low. The change is:
  - Removing dead code (a function identical to
    `irq_default_primary_handler`)
  - Passing `NULL` + `IRQF_ONESHOT` instead of a custom handler +
    flags=0
  - This is the canonical correct way to request a threaded IRQ with no
    real primary handler
- **Could it break something?** No. The behavior with `IRQF_ONESHOT` is
  strictly safer — the interrupt is masked during threaded handler
  execution. The primary handler behavior is identical
  (`IRQ_WAKE_THREAD`).

### 6. USER IMPACT

- This driver is used on Broadcom iProc SoCs (embedded ARM) for FlexRM
  offload engine mailbox operations
- The bug manifests as an **interrupt storm causing system hang** on
  level-triggered interrupt configurations
- Even on edge-triggered (MSI) configurations, the missing
  `IRQF_ONESHOT` creates a window where the interrupt can re-fire before
  the thread handler runs, potentially causing lost completions or
  spurious interrupt warnings

### 7. STABILITY INDICATORS

- **Author:** Sebastian Andrzej Siewior — a core kernel developer,
  especially for PREEMPT_RT and IRQ subsystem
- **Acked by:** Thomas Gleixner — the IRQ subsystem maintainer and
  creator of the `__setup_irq` safety checks
- The fix follows a well-established pattern used across many drivers
- The patch is self-contained with no dependencies

### 8. DEPENDENCY CHECK

The code being modified exists in all stable trees that have this
driver. The driver `bcm-flexrm-mailbox.c` has been in the kernel since
at least v4.14. The `request_threaded_irq()` with `NULL` +
`IRQF_ONESHOT` pattern has been supported since the genirq safety check
was added in 2012 (commit 1c6c69525b40e). This patch applies cleanly to
any stable tree.

### CONCLUSION

This commit fixes a real bug: a missing `IRQF_ONESHOT` flag that can
cause an interrupt storm and system hang on level-triggered interrupt
lines. The old code also inadvertently bypassed the kernel's own safety
check for this exact scenario (by providing a custom handler identical
to the default one). The fix is small (net -10 lines), self-contained,
authored by core IRQ subsystem developers, and follows the canonical
pattern for threaded interrupts. It has zero risk of regression —
`IRQF_ONESHOT` is strictly correct and the removed handler was
functionally identical to the default.

**YES**

 drivers/mailbox/bcm-flexrm-mailbox.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/mailbox/bcm-flexrm-mailbox.c b/drivers/mailbox/bcm-flexrm-mailbox.c
index 41f79e51d9e5a..4255fefc3a5a0 100644
--- a/drivers/mailbox/bcm-flexrm-mailbox.c
+++ b/drivers/mailbox/bcm-flexrm-mailbox.c
@@ -1173,14 +1173,6 @@ static int flexrm_debugfs_stats_show(struct seq_file *file, void *offset)
 
 /* ====== FlexRM interrupt handler ===== */
 
-static irqreturn_t flexrm_irq_event(int irq, void *dev_id)
-{
-	/* We only have MSI for completions so just wakeup IRQ thread */
-	/* Ring related errors will be informed via completion descriptors */
-
-	return IRQ_WAKE_THREAD;
-}
-
 static irqreturn_t flexrm_irq_thread(int irq, void *dev_id)
 {
 	flexrm_process_completions(dev_id);
@@ -1271,10 +1263,8 @@ static int flexrm_startup(struct mbox_chan *chan)
 		ret = -ENODEV;
 		goto fail_free_cmpl_memory;
 	}
-	ret = request_threaded_irq(ring->irq,
-				   flexrm_irq_event,
-				   flexrm_irq_thread,
-				   0, dev_name(ring->mbox->dev), ring);
+	ret = request_threaded_irq(ring->irq, NULL, flexrm_irq_thread,
+				   IRQF_ONESHOT, dev_name(ring->mbox->dev), ring);
 	if (ret) {
 		dev_err(ring->mbox->dev,
 			"failed to request ring%d IRQ\n", ring->num);
-- 
2.51.0