Re: [PATCH v4 4/6] rust: irq: add support for threaded IRQs and handlers

[Daniel Almeida]

Hi Danilo,

> On 9 Jun 2025, at 09:27, Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
> 
> On Sun, Jun 08, 2025 at 07:51:09PM -0300, Daniel Almeida wrote:
>> +/// Callbacks for a threaded IRQ handler.
>> +pub trait ThreadedHandler: Sync {
>> +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
>> +    /// context.
>> +    fn handle_irq(&self) -> ThreadedIrqReturn;
>> +
>> +    /// The threaded handler function. This function is called from the irq
>> +    /// handler thread, which is automatically created by the system.
>> +    fn thread_fn(&self) -> IrqReturn;
>> +}
>> +
>> +impl<T: ?Sized + ThreadedHandler + Send> ThreadedHandler for Arc<T> {
>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>> +        T::handle_irq(self)
>> +    }
>> +
>> +    fn thread_fn(&self) -> IrqReturn {
>> +        T::thread_fn(self)
>> +    }
>> +}
> 
> In case you intend to be consistent with the function pointer names in
> request_threaded_irq(), it'd need to be handler() and thread_fn(). But I don't
> think there's a need for that, both aren't really nice for names of trait
> methods.
> 
> What about irq::Handler::handle() and irq::Handler::handle_threaded() for
> instance?
> 
> Alternatively, why not just
> 
> trait Handler {
>   fn handle(&self);
> }
> 
> trait ThreadedHandler {
>   fn handle(&self);
> }
> 
> and then we ask for `T: Handler + ThreadedHandler`.

Sure, I am totally OK with renaming things, but IIRC I've tried  Handler +
ThreadedHandler in the past and found it to be problematic. I don't recall why,
though, so maybe it's worth another attempt.

> 
>> +
>> +impl<T: ?Sized + ThreadedHandler, A: Allocator> ThreadedHandler for Box<T, A> {
>> +    fn handle_irq(&self) -> ThreadedIrqReturn {
>> +        T::handle_irq(self)
>> +    }
>> +
>> +    fn thread_fn(&self) -> IrqReturn {
>> +        T::thread_fn(self)
>> +    }
>> +}
>> +
>> +/// A registration of a threaded IRQ handler for a given IRQ line.
>> +///
>> +/// Two callbacks are required: one to handle the IRQ, and one to handle any
>> +/// other work in a separate thread.
>> +///
>> +/// The thread handler is only called if the IRQ handler returns `WakeThread`.
>> +///
>> +/// # Examples
>> +///
>> +/// The following is an example of using `ThreadedRegistration`. It uses a
>> +/// [`AtomicU32`](core::sync::AtomicU32) to provide the interior mutability.
>> +///
>> +/// ```
>> +/// use core::sync::atomic::AtomicU32;
>> +/// use core::sync::atomic::Ordering;
>> +///
>> +/// use kernel::prelude::*;
>> +/// use kernel::device::Bound;
>> +/// use kernel::irq::flags;
>> +/// use kernel::irq::ThreadedIrqReturn;
>> +/// use kernel::irq::ThreadedRegistration;
>> +/// use kernel::irq::IrqReturn;
>> +/// use kernel::platform;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::SpinLock;
>> +/// use kernel::alloc::flags::GFP_KERNEL;
>> +/// use kernel::c_str;
>> +///
>> +/// // Declare a struct that will be passed in when the interrupt fires. The u32
>> +/// // merely serves as an example of some internal data.
>> +/// struct Data(AtomicU32);
>> +///
>> +/// // [`handle_irq`] takes &self. This example illustrates interior
>> +/// // mutability can be used when share the data between process context and IRQ
>> +/// // context.
>> +///
>> +/// type Handler = Data;
>> +///
>> +/// impl kernel::irq::request::ThreadedHandler for Handler {
>> +///     // This is executing in IRQ context in some CPU. Other CPUs can still
>> +///     // try to access to data.
>> +///     fn handle_irq(&self) -> ThreadedIrqReturn {
>> +///         self.0.fetch_add(1, Ordering::Relaxed);
>> +///
>> +///         // By returning `WakeThread`, we indicate to the system that the
>> +///         // thread function should be called. Otherwise, return
>> +///         // ThreadedIrqReturn::Handled.
>> +///         ThreadedIrqReturn::WakeThread
>> +///     }
>> +///
>> +///     // This will run (in a separate kthread) if and only if `handle_irq`
>> +///     // returns `WakeThread`.
>> +///     fn thread_fn(&self) -> IrqReturn {
>> +///         self.0.fetch_add(1, Ordering::Relaxed);
>> +///
>> +///         IrqReturn::Handled
>> +///     }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_threaded_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<ThreadedRegistration<Handler>>> {
>> +///     let registration = dev.threaded_irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
> 
> This doesn't compile (yet). I think this should be a "raw" example, i.e. the
> function should take an IRQ number.
> 
> The example you sketch up here is for platform::Device::threaded_irq_by_index().

Yes, I originally had an example along the lines of what you mentioned. Except
that with the changes in register() from pub to pub(crate) they stopped
compiling.

I am not sure how the doctest to kunit machinery works, but I was expecting
tests to have access to everything within the module they're defined in, but
this is apparently not the case.

> 
>> +///
>> +///     // You can have as many references to the registration as you want, so
>> +///     // multiple parts of the driver can access it.
>> +///     let registration = Arc::pin_init(registration, GFP_KERNEL)?;
>> +///
>> +///     // The handler may be called immediately after the function above
>> +///     // returns, possibly in a different CPU.
>> +///
>> +///     {
>> +///         // The data can be accessed from the process context too.
>> +///         registration.handler().0.fetch_add(1, Ordering::Relaxed);
>> +///     }
> 
> Why the extra scope?

There used to be a lock here instead of AtomicU32. This extra scope was there to drop it.

It’s gone now so there’s no reason to have the extra scope indeed.

> 
>> +///
>> +///     Ok(registration)
>> +/// }
>> +///
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>> +///
>> +#[pin_data]
>> +pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
>> +    inner: Devres<RegistrationInner>,
>> +
>> +    #[pin]
>> +    handler: T,
>> +
>> +    /// Pinned because we need address stability so that we can pass a pointer
>> +    /// to the callback.
>> +    #[pin]
>> +    _pin: PhantomPinned,
>> +}
> 
> Most of the code in this file is a duplicate of the non-threaded registration.
> 
> I think this would greatly generalize with specialization and an HandlerInternal
> trait.
> 
> Without specialization I think we could use enums to generalize.
> 
> The most trivial solution would be to define the Handler trait as
> 
> trait Handler {
>   fn handle(&self);
>   fn handle_threaded(&self) {};
> }
> 
> but that's pretty dodgy.

A lot of the comments up until now have touched on somehow having threaded and
non-threaded versions implemented together. I personally see no problem in
having things duplicated here, because I think it's easier to reason about what
is going on this way. Alice has expressed a similar view in a previous iteration.

Can you expand a bit more on your suggestion? Perhaps there's a clean way to do
it (without macros and etc), but so far I don't see it.

> 
>> +impl<T: ThreadedHandler + 'static> ThreadedRegistration<T> {
>> +    /// Registers the IRQ handler with the system for the given IRQ number.
>> +    pub(crate) fn register<'a>(
>> +        dev: &'a Device<Bound>,
>> +        irq: u32,
>> +        flags: Flags,
>> +        name: &'static CStr,
>> +        handler: T,
>> +    ) -> impl PinInit<Self, Error> + 'a {
> 
> What happens if `dev`  does not match `irq`? The caller is responsible to only
> provide an IRQ number that was obtained from this device.
> 
> This should be a safety requirement and a type invariant.

This iteration converted register() from pub to pub(crate). The idea was to
force drivers to use the accessors. I assumed this was enough to make the API
safe, as the few users in the kernel crate (i.e.: so far platform and pci)
could be manually checked for correctness.

To summarize my point, there is still the possibility of misusing this from the
kernel crate itself, but that is no longer possible from a driver's
perspective.

— Daniel