Re: [PATCH v2 1/3] rust: Introduce irq module

[Benno Lossin <benno.lossin@proton.me>]

On 01.08.24 18:44, Lyude Paul wrote:
> On Thu, 2024-08-01 at 09:51 +0000, Benno Lossin wrote:
>> On 01.08.24 00:35, Lyude Paul wrote:
>>> diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
>>> new file mode 100644
>>> index 0000000000000..e50110f92f3fa
>>> --- /dev/null
>>> +++ b/rust/kernel/irq.rs
>>> @@ -0,0 +1,87 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Interrupt controls
>>> +//!
>>> +//! This module allows Rust code to control processor interrupts. [`with_irqs_disabled()`] may be
>>> +//! used for nested disables of interrupts, whereas [`IrqDisabled`] can be used for annotating code
>>> +//! that requires that interrupts already be disabled.
>>
>> My intuition is telling me "requires that interrupts are already
>> disabled." sounds more natural, but I might be wrong.
> 
> Maybe "can be used for annotating code that requires interrupts to be
> disabled."?

SGTM

>>> +
>>> +use bindings;
>>> +use core::marker::*;
>>> +
>>> +/// A token that is only available in contexts where IRQs are disabled.
>>> +///
>>> +/// [`IrqDisabled`] is marker made available when interrupts are not active. Certain functions take
>>> +/// an `IrqDisabled` in order to indicate that they may only be run in IRQ-free contexts.
>>> +///
>>> +/// This is a marker type; it has no size, and is simply used as a compile-time guarantee that
>>> +/// interrupts are disabled where required.
>>> +///
>>> +/// This token can be created by [`with_irqs_disabled`]. See [`with_irqs_disabled`] for examples and
>>> +/// further information.
>>> +#[derive(Copy, Clone, Debug, Ord, Eq, PartialOrd, PartialEq, Hash)]
>>> +pub struct IrqDisabled<'a>(PhantomData<(&'a (), *mut ())>);
>>> +
>>> +impl IrqDisabled<'_> {
>>> +    /// Create a new [`IrqDisabled`] without disabling interrupts.
>>> +    ///
>>> +    /// This creates an [`IrqDisabled`] token, which can be passed to functions that must be run
>>> +    /// without interrupts. If debug assertions are enabled, this function will assert that
>>> +    /// interrupts are disabled upon creation. Otherwise, it has no size or cost at runtime.
>>> +    ///
>>> +    /// # Panics
>>> +    ///
>>> +    /// If debug assertions are enabled, this function will panic if interrupts are not disabled
>>> +    /// upon creation.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// This function must only be called in contexts where it is already known that interrupts have
>>> +    /// been disabled for the current CPU, as the user is making a promise that they will remain
>>> +    /// disabled at least until this [`IrqDisabled`] is dropped.
>>
>> This is a bit verbose for taste, what about:
>> "Must only be called in contexts where interrupts are disabled for the
>> current CPU. Additionally they must remain disabled at least until the
>> returned value is dropped."
> 
> sgtm
> 
>>
>> Importantly the second sentence is not 100% clear from your version.
>> Feel free to take mine (with modifications).
>>
>>> +    pub unsafe fn new() -> Self {
>>
>> Do we need this to be public? Ie do you (or someone you know) have a
>> usecase for this? If not, then we can start with this function being
>> private and make it public when necessary.
> 
> Yes - there's a few DRM callbacks, drm_crtc_funcs.{vblank_enable,
> vblank_disable, get_vblank_timestamp}, that happen with interrupts already
> disabled that will be using it:
> 
> https://gitlab.freedesktop.org/lyudess/linux/-/blob/rvkms-example-07312024/rust/kernel/drm/kms/vblank.rs?ref_type=heads#L24
> 
> It's also worth noting that if we weren't going to use this right away I think
> it would make more sense just to add the function later instead of having it
> private, since we don't actually use this anywhere in irq.rs.

I didn't want the good docs you wrote go to waste :)
But since you have a usage, we can just keep it public.

>>> +        // SAFETY: FFI call with no special requirements
>>> +        debug_assert!(unsafe { bindings::irqs_disabled() });
>>> +
>>> +        Self(PhantomData)
>>> +    }
>>> +}
>>> +
>>> +/// Run the closure `cb` with interrupts disabled on the local CPU.
>>> +///
>>> +/// This creates an [`IrqDisabled`] token, which can be passed to functions that must be run
>>> +/// without interrupts.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// Using [`with_irqs_disabled`] to call a function that can only be called with interrupts
>>> +/// disabled:
>>> +///
>>> +/// ```
>>> +/// use kernel::irq::{IrqDisabled, with_irqs_disabled};
>>> +///
>>> +/// // Requiring interrupts be disabled to call a function
>>> +/// fn dont_interrupt_me(_irq: IrqDisabled<'_>) {
>>> +///     /* When this token is available, IRQs are known to be disabled. Actions that rely on this
>>> +///      * can be safely performed
>>> +///      */
>>> +/// }
>>> +///
>>> +/// // Disabling interrupts. They'll be re-enabled once this closure completes.
>>> +/// with_irqs_disabled(|irq| dont_interrupt_me(irq));
>>> +/// ```
>>> +#[inline]
>>> +pub fn with_irqs_disabled<'a, T, F>(cb: F) -> T
>>> +where
>>> +    F: FnOnce(IrqDisabled<'a>) -> T,
>>
>> You can use this as the signature:
>>
>>     pub fn with_irqs_disabled<'a, T>(cb: impl FnOnce(IrqDisabled<'a>) -> T) -> T
>>
>> Not sure if we have any convention for this, but I personally think this
>> version is easier to parse.
> 
> sgtm

I hope that you saw my reply to myself, since what I wrote above is
unsound.

---
Cheers,
Benno