Re: [PATCH v2 2/2] irqchip/riscv-imsic: Complete vector moves without a timer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] irqchip/riscv-imsic: imsic_remote_kick() in
  __imsic_local_sync() wakes idle CPU
- [Low] irqchip/riscv-imsic: missing Fixes trailer for commit
  0f67911e821c
--

commit 41f6c7fe40c982e58c49f5640f84619e2a018221
Author: Troy Mitchell <troy.mitchell@linux.dev>

> irqchip/riscv-imsic: Complete vector moves without a timer
>
> When a vector moves, the new CPU clears move_prev only after the old CPU
> has released the old vector. If the new CPU syncs first, it arms a timer
> on itself and retries a jiffy later. That CPU is often the one entering
> idle and taking over the broadcast interrupt, with its tick already
> stopped, so the timer is not programmed. The move then completes only on
> the next interrupt of that CPU, which took up to 0.5s on an idle
> SpacemiT K3. Until then the IRQ is still marked as moving and further
> affinity changes of it return -EBUSY.
>
> Let the old CPU queue the sync work on the new CPU once it has released
> the old vector, and remove the retry timer. Wait only while the old
> vector still points to the new vector, since a released old vector may
> already belong to another move.
>
> IMSIC synchronization no longer depends on a timer.
>
> Signed-off-by: Troy Mitchell <troy.mitchell@linux.dev>

[Severity: Low]
This fixes a bug in the two-step vector move synchronization and local
retry timer.  Should it include a Fixes tag?

  Fixes: 0f67911e821c ("irqchip/riscv-imsic: Separate next and previous pointers in IMSIC vector")

> diff --git a/drivers/irqchip/irq-riscv-imsic-state.c b/drivers/irqchip/irq-riscv-imsic-state.c
> index f6373e4019a22..36f4b7ede76a5 100644
> --- a/drivers/irqchip/irq-riscv-imsic-state.c
> +++ b/drivers/irqchip/irq-riscv-imsic-state.c

[ ... ]

> @@ -151,13 +164,13 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
>  		mvec = READ_ONCE(vec->move_prev);
>  		if (mvec) {
>  			/*
> -			 * If the old vector has not been updated then
> -			 * try again in the next sync-up call.
> +			 * If the old vector has not been updated then keep
> +			 * this vector dirty. The old CPU kicks this CPU once
> +			 * it has updated the old vector. A released old vector
> +			 * may already belong to another move.
>  			 */
> -			if (READ_ONCE(mvec->move_next)) {
> -				ret = false;
> +			if (READ_ONCE(mvec->move_next) == vec)
>  				continue;
> -			}
>  
>  			WRITE_ONCE(vec->move_prev, NULL);
>  		}
> @@ -199,30 +212,15 @@ static bool __imsic_local_sync(struct imsic_local_priv *lpriv)
>  
>  			WRITE_ONCE(vec->move_next, NULL);
>  			imsic_vector_free(vec);
> +
> +			/* Let the new CPU finish the move it may be waiting for */
> +			imsic_remote_kick(mvec->cpu);
>  		}

[Severity: Medium]
Does calling imsic_remote_kick(mvec->cpu) here wake the destination CPU
out of cpuidle right after it takes over the broadcast timer?

When CPU1 enters a timer-stop idle state and moves the broadcast IRQ to
itself via cpuidle_enter_state() -> tick_broadcast_enter(), it runs
__imsic_local_sync() synchronously while CPU0 is still processing its
sync_work IPI:

CPU0                              CPU1
                                  cpuidle_enter_state()
                                    tick_broadcast_enter()
                                      ...
                                        imsic_vector_move(old_vec, new_vec)
                                          imsic_vector_move_update(old_lpriv, ...)
                                            imsic_remote_kick(CPU0)
                                          imsic_vector_move_update(new_lpriv, ...)
                                            __imsic_local_sync(new_lpriv)
                                              /* mvec->move_next == vec */
                                              /* leaves new_vec->move_prev set */
                                    target_state->enter()
imsic_local_sync_work()
  ...
    WRITE_ONCE(vec->move_next, NULL)
    imsic_vector_free(vec)
    imsic_remote_kick(CPU1)       <-- IPI wakes CPU1 from idle

CPU1 has already enabled new_vec->local_id in its local EIE CSR during its
own __imsic_local_sync(), so the IPI wakes CPU1 out of wfi or
riscv_sbi_hart_suspend() solely to clear new_vec->move_prev, forcing it
through tick_broadcast_exit() and another idle entry cycle.

In addition, cpuidle_enter_state() keeps local IRQs disabled on CPU1 from
before tick_broadcast_enter() until after target_state->enter() and
tick_broadcast_exit().  Until CPU1 re-enables IRQs and services the IPI,
new_vec->move_prev remains non-NULL, so if a third CPU enters
tick_broadcast_enter() with an earlier expiry during that window,
imsic_irq_set_affinity() still sees imsic_vector_get_move() != NULL and
returns -EBUSY.

Finally, when imsic_vector_move() runs on old_vec->cpu (such as from
tick_handle_oneshot_broadcast() or imsic_irq_ack() -> irq_move_irq()), the
first imsic_vector_move_update(old_lpriv, ...) runs __imsic_local_sync()
synchronously and calls imsic_remote_kick(new_vec->cpu) before the second
imsic_vector_move_update(new_lpriv, ...) has set new_vec->move_prev or
marked new_vec dirty, kicking new_vec->cpu twice.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-imsic-remote-sync-irqwork-v2-0-680f65b24ff6@linux.dev?part=2