Re: [PATCH v2] rust: irq: add support for request_irq()

[Alice Ryhl]

On Tue, Mar 04, 2025 at 02:43:20PM +0100, Andreas Hindborg wrote:
> "Daniel Almeida" <daniel.almeida@xxxxxxxxxxxxx> writes:
> > +    /// handler after suspending interrupts. For system wakeup devices users
> > +    /// need to implement wakeup detection in their interrupt handlers.
> > +    pub const COND_SUSPEND: Flags = Flags(bindings::IRQF_COND_SUSPEND as _);
> > +
> > +    /// Don't enable IRQ or NMI automatically when users request it. Users will
> > +    /// enable it explicitly by `enable_irq` or `enable_nmi` later.
> > +    pub const NO_AUTOEN: Flags = Flags(bindings::IRQF_NO_AUTOEN as _);
> > +
> > +    /// Exclude from runnaway detection for IPI and similar handlers, depends on
> > +    /// `PERCPU`.
> > +    pub const NO_DEBUG: Flags = Flags(bindings::IRQF_NO_DEBUG as _);
> > +}
> > +
> > +/// The value that can be returned from an IrqHandler or a ThreadedIrqHandler.
> > +pub enum IrqReturn {
> 
> I learned recently that if you choose the right representation here, you
> don't need to cast here and when you call `Handler::handle_irq`. I think
> `#[repr(u32)]` is the one to use here.

I wonder if we can get it to use the repr of the same size as
irqreturn_t?

> > +    /// The interrupt was not from this device or was not handled.
> > +    None = bindings::irqreturn_IRQ_NONE as _,
> > +
> > +    /// The interrupt was handled by this device.
> > +    Handled = bindings::irqreturn_IRQ_HANDLED as _,
> > +}
> > +
> > +/// Callbacks for an IRQ handler.
> > +pub trait Handler: Sync {
> > +    /// The actual handler function. As usual, sleeps are not allowed in IRQ
> > +    /// context.
> > +    fn handle_irq(&self) -> IrqReturn;
> > +}
> 
> What is the reason for moving away from the following:
> 
> 
>     pub trait Handler {
>         /// The context data associated with and made available to the handler.
>         type Data: ForeignOwnable;
> 
>         /// Called from interrupt context when the irq happens.
>         fn handle_irq(data: <Self::Data as ForeignOwnable>::Borrowed<'_>) -> Return;
>     }
> 
> 
> I think we will run into problems if we want to pass `Arc<Foo>` as the
> handler. I don't think we can `impl Handler for Arc<Foo>` in a driver
> crate, since both `Handler` and `Arc` are defined in external crates

My understanding is that since the data is not stored behind a
private_data void pointer, we don't need ForeignOwnable. I think we
should avoid using ForeignOwnable when it's not necessary.

We can support the Arc / Box case by adding

impl<T: ?Sized + Handler> Handler for Arc<T> {
    fn handle_irq(&self) -> IrqReturn {
        T::handle_irq(self)
    }
}

This way, the user implements it for their struct and then it works with
Arc<MyStruct> too.

This kind of blanket impl for Arc/Box is very common in userspace Rust
too. For example, the Tokio traits AsyncRead/AsyncWrite have blanket
impls for Box.

> > +#[pin_data(PinnedDrop)]
> > +pub struct ThreadedRegistration<T: ThreadedHandler> {
> > +    irq: u32,
> > +    #[pin]
> > +    handler: T,
> > +    #[pin]
> > +    /// Pinned because we need address stability so that we can pass a pointer
> > +    /// to the callback.
> > +    _pin: PhantomPinned,
> > +}
> 
> As others have mentioned, I wonder if we can avoid the code duplication
> that makes up most of the rest of this patch.

I'm worried that getting rid of duplication makes the code too complex
in this case. I could be wrong, but it seems difficult to deduplicate in
a simple way.

Alice