Re: [PATCH v2] rust: irq: make Registration compatible with lifetime-bound drivers

["Gary Guo" <gary@garyguo.net>]

On Tue Jul 21, 2026 at 3:57 PM BST, Alexandre Courbot wrote:
> On Sun Jul 19, 2026 at 8:36 AM PDT, Danilo Krummrich wrote:
>> Adapt the IRQ registration to work with the Higher-Ranked Lifetime Types
>> (HRT) device driver architecture introduced in commit 2c7c65933600
>> ("Merge patch series "rust: device: Higher-Ranked Lifetime Types for
>> device drivers"").
>>
>> With HRT, driver structs carry a lifetime parameter tied to the device
>> binding scope, allowing device resources such as pci::Bar<'bar> to be
>> held directly rather than through Devres indirection. However, the IRQ
>> abstraction required Handler: Sync + 'static, preventing handlers from
>> embedding lifetime-parameterized resources.
>>
>> Remove the 'static bound from Handler and ThreadedHandler and replace
>> the Devres<RegistrationInner> indirection with direct request_irq() /
>> free_irq() calls in the constructor and PinnedDrop.  Registration<'a, T>
>> stores the IrqRequest<'a>, which structurally ties it to the device
>> binding scope.
>>
>> Also remove the &Device<Bound> parameter from the handler callbacks,
>> since handlers that need device access can embed it in their own type.
>>
>> IRQ handlers can now directly own device resources:
>>
>> 	struct IrqHandler<'irq> {
>> 	    bar: pci::Bar<'irq, BAR_SIZE>,
>> 	}
>>
>> 	impl irq::Handler for IrqHandler<'_> {
>> 	    fn handle(&self) -> IrqReturn {
>> 	        let stat = self.bar.read(regs::STAT);
>> 	        ...
>> 	    }
>> 	}
>>
>> This eliminates the indirection previously required for IRQ handlers to
>> access device resources and aligns with the broader goal of expressing
>> every registration scoped to a driver binding through compile-time
>> lifetime bounds.
>>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>> Changes in v2:
>>   - Move request_irq() / request_threaded_irq() to _: blocks.
>>   - Add INVARIANT missing comments and missing #[inline] annotations.
>> ---
>>  rust/kernel/irq/request.rs | 432 ++++++++++++++++++-------------------
>>  rust/kernel/pci/irq.rs     |  26 ++-
>>  rust/kernel/platform.rs    |  48 +++--
>>  3 files changed, 255 insertions(+), 251 deletions(-)
>>
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> index f425fe12f7c8..c1c6525a676a 100644
>> --- a/rust/kernel/irq/request.rs
>> +++ b/rust/kernel/irq/request.rs
>> @@ -5,16 +5,21 @@
>>  //! [`ThreadedRegistration`], which allow users to register handlers for a given
>>  //! IRQ line.
>>  
>> -use core::marker::PhantomPinned;
>> -
>> -use crate::alloc::Allocator;
>> -use crate::device::{Bound, Device};
>> -use crate::devres::Devres;
>> -use crate::error::to_result;
>> -use crate::irq::flags::Flags;
>> -use crate::prelude::*;
>> -use crate::str::CStr;
>> -use crate::sync::Arc;
>> +use core::marker::{
>> +    PhantomData,
>> +    PhantomPinned, //
>> +};
>> +
>> +use crate::{
>> +    device::{
>> +        Bound,
>> +        Device, //
>> +    },
>> +    error::to_result,
>> +    irq::flags::Flags,
>> +    prelude::*,
>> +    str::CStr,
>> +};
>>  
>>  /// The value that can be returned from a [`Handler`] or a [`ThreadedHandler`].
>>  #[repr(u32)]
>> @@ -27,7 +32,7 @@ pub enum IrqReturn {
>>  }
>>  
>>  /// Callbacks for an IRQ handler.
>> -pub trait Handler: Sync + 'static {
>> +pub trait Handler: Sync {
>>      /// The hard IRQ handler.
>>      ///
>>      /// This is executed in interrupt context, hence all corresponding
>> @@ -36,73 +41,20 @@ pub trait Handler: Sync + 'static {
>>      /// All work that does not necessarily need to be executed from
>>      /// interrupt context, should be deferred to a threaded handler.
>>      /// See also [`ThreadedRegistration`].
>> -    fn handle(&self, device: &Device<Bound>) -> IrqReturn;
>> -}
>> -
>> -impl<T: ?Sized + Handler + Send> Handler for Arc<T> {
>> -    fn handle(&self, device: &Device<Bound>) -> IrqReturn {
>> -        T::handle(self, device)
>> -    }
>> -}
>> -
>> -impl<T: ?Sized + Handler, A: Allocator + 'static> Handler for Box<T, A> {
>> -    fn handle(&self, device: &Device<Bound>) -> IrqReturn {
>> -        T::handle(self, device)
>> -    }
>> +    fn handle(&self) -> IrqReturn;
>>  }
>>  
>> -/// # Invariants
>> -///
>> -/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
>> -/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It is guaranteed to be unique
>> -///   by the type system, since each call to `new` will return a different instance of
>> -///   `Registration`.
>> -#[pin_data(PinnedDrop)]
>> -struct RegistrationInner {
>> -    irq: u32,
>> -    cookie: *mut c_void,
>> -}
>> -
>> -impl RegistrationInner {
>> -    fn synchronize(&self) {
>> -        // SAFETY: safe as per the invariants of `RegistrationInner`
>> -        unsafe { bindings::synchronize_irq(self.irq) };
>> -    }
>> -}
>> -
>> -#[pinned_drop]
>> -impl PinnedDrop for RegistrationInner {
>> -    fn drop(self: Pin<&mut Self>) {
>> -        // SAFETY:
>> -        //
>> -        // Safe as per the invariants of `RegistrationInner` and:
>> -        //
>> -        // - The containing struct is `!Unpin` and was initialized using
>> -        // pin-init, so it occupied the same memory location for the entirety of
>> -        // its lifetime.
>> -        //
>> -        // Notice that this will block until all handlers finish executing,
>> -        // i.e.: at no point will &self be invalid while the handler is running.
>> -        unsafe { bindings::free_irq(self.irq, self.cookie) };
>
> Not directly related to this patch, but I think it would help to add a
> comment explaining why it is ok for `drop` to own what looks like a
> mutable reference to `Self` (and thus `Self::handler`) while a handler
> might be running holding a non-mutable reference to the latter.
>
> My first reaction upon seeing this was that this breaks the Rust
> aliasing rules, but looking at the feedback for the initial patch [1]
> revealed that the issue has been discussed and dismissed.
>
> Since this is quite subtle, and to avoid this being flagged again in the
> future it would be nice to explain why this is safe (IIUC
> `PhantomPinned` combined with the fact the destructor never forms a
> `&mut handler`, but this needs to be vetted by someone more familiar
> with these matters than I am).

If we want to play safe, we could wrap this inside `UnsafePinned` (or `Opaque`),
but that's actually problematic because doing so will change the variance of `T`
from covariance to invariance, and we actually want covariance here. There's no
way to opt out, so it's not good. (`T` is never mutated here and we only pin it
for self-reference purpose).

I left a comment on the `UnsafePinned` tracking issue about this:
https://github.com/rust-lang/rust/issues/125735#issuecomment-5038853925

Best,
Gary