Re: [PATCH v16 09/17] rust: sync: Add SpinLockIrq

["Benno Lossin" <lossin@kernel.org>]

On Mon Dec 15, 2025 at 6:57 PM CET, Lyude Paul wrote:
> A variant of SpinLock that is expected to be used in noirq contexts, so

You say in the next line that `lock` automatically disables interrupts,
so you also should be able to use the lock in contexts with interrupts,
right? Feel like this would be better summarized as

    A variant of `SpinLock` that ensures interrupts are disabled in the
    critical section. `lock()` will ensure that either interrupts are
    already disabled or disable them. `unlock()` will reverse the
    respective operation.

> lock() will disable interrupts and unlock() (i.e. `Guard::drop()` will
> undo the interrupt disable.
>
> [Boqun: Port to use spin_lock_irq_disable() and
> spin_unlock_irq_enable()]
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

> +/// A spinlock that may be acquired when local processor interrupts are disabled.

Similarly here, I would reuse the first sentence (or a variant of it)
that I gave above.

Aside from Gary's comments, this patch looks good:

Reviewed-by: Benno Lossin <lossin@kernel.org>

Cheers,
Benno

> +///
> +/// This is a version of [`SpinLock`] that can only be used in contexts where interrupts for the
> +/// local CPU are disabled. It can be acquired in two ways:
> +///
> +/// - Using [`lock()`] like any other type of lock, in which case the bindings will modify the
> +///   interrupt state to ensure that local processor interrupts remain disabled for at least as long
> +///   as the [`SpinLockIrqGuard`] exists.
> +/// - Using [`lock_with()`] in contexts where a [`LocalInterruptDisabled`] token is present and
> +///   local processor interrupts are already known to be disabled, in which case the local interrupt
> +///   state will not be touched. This method should be preferred if a [`LocalInterruptDisabled`]
> +///   token is present in the scope.