rust: irq: make Registration compatible with lifetime-bound drivers
Subsystems (5)
Danilo Krummrich
maintainerPATCH 1/1 rust: irq: make Registration compatible with lifetime-bound drivers
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.
--- rust/kernel/irq/request.rs | 427 +++++++++++++++++-------------------- rust/kernel/pci/irq.rs | 26 ++- rust/kernel/platform.rs | 48 +++-- 3 files changed, 250 insertions(+), 251 deletions(-)
rust/kernel/irq/request.rs +200 -227
@@ -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) }; - } -} - -// SAFETY: We only use `inner` on drop, which called at most once with no -// concurrent access. -unsafe impl Sync for RegistrationInner {} - -// SAFETY: It is safe to send `RegistrationInner` across threads. -unsafe impl Send for RegistrationInner {} -/// A request for an IRQ line for a given device. /// /// # Invariants /// /// - `ìrq` is the number of an interrupt source of `dev`. -/// - `irq` has not been registered yet. +/// - `irq` has not been registered yet; this is consumed by [`Registration::new()`]. pub struct IrqRequest <'a> { - dev: &'aDevice <Bound >, irq:u32 , +/// Proves the device is bound at registration time and ties `'a` to the device's bound +/// lifetime, ensuring the [`Registration`] cannot outlive it. + _dev:PhantomData <&'aDevice <Bound >>, }impl <'a>IrqRequest <'a> {
@@ -111,9 +63,12 @@ impl<'a> IrqRequest<'a> {/// # Safety /// /// - `irq` should be a valid IRQ number for `dev`. -pub (crate )unsafe fn new (dev: &'aDevice <Bound >, irq:u32 ) ->Self { +pub (crate )unsafe fn new (_dev: &'aDevice <Bound >, irq:u32 ) ->Self {// INVARIANT: `irq` is a valid IRQ number for `dev`. -IrqRequest { dev, irq } +IrqRequest { + irq, + _dev:PhantomData , + } }/// Returns the IRQ number of an [`IrqRequest`].
@@ -139,10 +94,18 @@ pub fn irq(&self) -> u32 {/// [`Completion::wait_for_completion()`]: kernel::sync::Completion::wait_for_completion /// /// ``` -/// use kernel::device::{Bound, Device}; -/// use kernel::irq::{self, Flags, IrqRequest, IrqReturn, Registration}; -/// use kernel::prelude::*; -/// use kernel::sync::{Arc, Completion}; +/// use core::pin::Pin; +/// use kernel::{ +/// irq::{ +/// self, +/// Flags, +/// IrqRequest, +/// IrqReturn, +/// Registration, +/// }, +/// prelude::*, +/// sync::Completion, +/// }; /// /// // Data shared between process and IRQ context. /// #[pin_data]
@@ -153,7 +116,7 @@ pub fn irq(&self) -> u32 {/// /// impl irq::Handler for Data { /// // Executed in IRQ context. -/// fn handle(&self, _dev: &Device<Bound>) -> IrqReturn { +/// fn handle(&self) -> IrqReturn { /// self.completion.complete_all(); /// IrqReturn::Handled /// }
@@ -163,12 +126,21 @@ pub fn irq(&self) -> u32 {/// // /// // This runs in process context and assumes `request` was previously acquired from a device. /// fn register_irq( -/// handler: impl PinInit<Data, Error>, /// request: IrqRequest<'_>, -/// ) -> Result<Arc<Registration<Data>>> { -/// let registration = Registration::new(request, Flags::SHARED, c"my_device", handler); +/// ) -> Result<Pin<KBox<Registration<'_, Data>>>> { +/// // SAFETY: The returned Registration is not leaked. +/// let registration = unsafe { +/// Registration::new( +/// request, +/// Flags::SHARED, +/// c"my_device", +/// try_pin_init!(Data { +/// completion <- Completion::new(), +/// }? Error), +/// ) +/// }; /// -/// let registration = Arc::pin_init(registration, GFP_KERNEL)?; +/// let registration = KBox::pin_init(registration, GFP_KERNEL)?; /// /// registration.handler().completion.wait_for_completion(); ///
@@ -179,11 +151,10 @@ pub fn irq(&self) -> u32 {/// /// # Invariants /// -/// * We own an irq handler whose cookie is a pointer to `Self`. -#[pin_data] -pub struct Registration <T :Handler > { -#[pin] - inner:Devres <RegistrationInner >, +/// * We own an irq handler registered via `request_irq` whose cookie is a pointer to `Self`. +#[pin_data( PinnedDrop )] +pub struct Registration <'a,T :Handler > { + request:IrqRequest <'a>,#[pin] handler:T ,
@@ -194,43 +165,44 @@ pub struct Registration<T: Handler> { _pin: PhantomPinned, } -impl <T :Handler >Registration <T > { +impl <'a,T :Handler >Registration <'a,T > {/// Registers the IRQ handler with the system for the given IRQ number. -pub fn new <'a>( +/// +/// # Safety +/// +/// Callers must not `mem::forget()` the returned [`Registration`] or otherwise prevent its +/// [`Drop`] implementation from running. +pub unsafe fn new ( request:IrqRequest <'a>, flags:Flags , name: &'staticCStr , handler:impl PinInit <T ,Error > + 'a, - ) ->impl PinInit <Self ,Error > + 'a { + ) ->impl PinInit <Self ,Error > + 'a +where +T : 'a, + {try_pin_init! (&this inSelf { handler <- handler, - inner <-Devres ::new ( - request.dev, -try_pin_init! (RegistrationInner { -// INVARIANT: `this` is a valid pointer to the `Registration` instance - cookie: this.as_ptr ().cast ::<c_void >(), - irq: { -// SAFETY: -// - The callbacks are valid for use with request_irq. -// - If this succeeds, the slot is guaranteed to be valid until the -// destructor of Self runs, which will deregister the callbacks -// before the memory location becomes invalid. -// - When request_irq is called, everything that handle_irq_callback will -// touch has already been initialized, so it's safe for the callback to -// be called immediately. -to_result (unsafe { - bindings::request_irq ( - request.irq, -Some (handle_irq_callback ::<T >), - flags.into_inner (), - name.as_char_ptr (), - this.as_ptr ().cast ::<c_void >(), - ) - })?; - request.irq - } - }) - ), +request : { +// SAFETY: +// - The callbacks are valid for use with request_irq. +// - If this succeeds, the slot is guaranteed to be valid until the destructor of +// Self runs, which will deregister the callbacks before the memory location +// becomes invalid. +// - When request_irq is called, everything that handle_irq_callback will touch +// has already been initialized, so it's safe for the callback to be called +// immediately. + to_result(unsafe { + bindings::request_irq ( + request.irq, +Some (handle_irq_callback ::<T >), + flags.into_inner (), + name.as_char_ptr (), + this.as_ptr ().cast ::<c_void >(), + ) + })?; + request + }, _pin:PhantomPinned , }) }
@@ -241,19 +213,24 @@ pub fn handler(&self) -> &T { }/// Wait for pending IRQ handlers on other CPUs. -/// -/// This will attempt to access the inner [`Devres`] container. -pub fn try_synchronize (&self) ->Result { -let inner = self.inner.try_access ().ok_or (ENODEV )?; - inner.synchronize (); -Ok (()) +pub fn synchronize (&self) { +// SAFETY: `self.request.irq` is a valid registered IRQ number (type invariant). +unsafe { bindings::synchronize_irq (self.request.irq) }; } +} -/// Wait for pending IRQ handlers on other CPUs. -pub fn synchronize (&self, dev: &Device <Bound >) ->Result { -let inner = self.inner.access (dev)?; - inner.synchronize (); -Ok (()) +#[pinned_drop] +impl <T :Handler >PinnedDrop for Registration <'_,T > { +fn drop (self:Pin <&mut Self >) { +// SAFETY: The cookie was set to a pointer to `Self` in `Registration::new()`. This blocks +// until all in-flight handlers complete, so no references to `self` remain after this +// returns. +unsafe { + bindings::free_irq ( + self.request.irq, + core::ptr::from_mut ::<Self >(self.get_unchecked_mut ()).cast ::<c_void >(), + ) + }; } }
@@ -261,13 +238,11 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {/// /// This function should be only used as the callback in `request_irq`. unsafe extern "C" fn handle_irq_callback <T :Handler >(_irq:i32 , ptr: *mut c_void ) ->c_uint { -// SAFETY: `ptr` is a pointer to `Registration<T>` set in `Registration::new` -let registration =unsafe { &*(ptras *const Registration <T >) }; -// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq -// callback is running implies that the device has not yet been unbound. -let device =unsafe { registration.inner.device ().as_bound () }; +let ptr = ptr.cast_const ().cast ::<Registration <'_,T >>(); +// SAFETY: `ptr` is a pointer to `Registration<'_, T>` set in `Registration::new()`. +let registration =unsafe { &*ptr }; -T ::handle (®istration.handler, device)as c_uint +T ::handle (®istration.handler)as c_uint }/// The value that can be returned from [`ThreadedHandler::handle`].
@@ -284,7 +259,7 @@ pub enum ThreadedIrqReturn { }/// Callbacks for a threaded IRQ handler. -pub trait ThreadedHandler :Sync + 'static { +pub trait ThreadedHandler :Sync {/// The hard IRQ handler. /// /// This is executed in interrupt context, hence all corresponding
@@ -293,8 +268,7 @@ pub trait ThreadedHandler: Sync + 'static {/// handler, i.e. [`ThreadedHandler::handle_threaded`]. /// /// The default implementation returns [`ThreadedIrqReturn::WakeThread`]. -#[expect(unused_variables)] -fn handle (&self, device: &Device <Bound >) ->ThreadedIrqReturn { +fn handle (&self) ->ThreadedIrqReturn {ThreadedIrqReturn ::WakeThread }
@@ -302,27 +276,7 @@ fn handle(&self, device: &Device<Bound>) -> ThreadedIrqReturn {/// /// This is executed in process context. The kernel creates a dedicated /// `kthread` for this purpose. -fn handle_threaded (&self, device: &Device <Bound >) ->IrqReturn ; -} - -impl <T : ?Sized +ThreadedHandler +Send >ThreadedHandler for Arc <T > { -fn handle (&self, device: &Device <Bound >) ->ThreadedIrqReturn { -T ::handle (self, device) - } - -fn handle_threaded (&self, device: &Device <Bound >) ->IrqReturn { -T ::handle_threaded (self, device) - } -} - -impl <T : ?Sized +ThreadedHandler ,A :Allocator + 'static>ThreadedHandler for Box <T ,A > { -fn handle (&self, device: &Device <Bound >) ->ThreadedIrqReturn { -T ::handle (self, device) - } - -fn handle_threaded (&self, device: &Device <Bound >) ->IrqReturn { -T ::handle_threaded (self, device) - } +fn handle_threaded (&self) ->IrqReturn ; }/// A registration of a threaded IRQ handler for a given IRQ line.
@@ -339,13 +293,20 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {/// [`Mutex`](kernel::sync::Mutex) to provide interior mutability. /// /// ``` -/// use kernel::device::{Bound, Device}; -/// use kernel::irq::{ -/// self, Flags, IrqRequest, IrqReturn, ThreadedHandler, ThreadedIrqReturn, -/// ThreadedRegistration, +/// use core::pin::Pin; +/// use kernel::{ +/// irq::{ +/// self, +/// Flags, +/// IrqRequest, +/// IrqReturn, +/// ThreadedHandler, +/// ThreadedIrqReturn, +/// ThreadedRegistration, +/// }, +/// prelude::*, +/// sync::Mutex, /// }; -/// use kernel::prelude::*; -/// use kernel::sync::{Arc, Mutex}; /// /// // Declare a struct that will be passed in when the interrupt fires. The u32 /// // merely serves as an example of some internal data.
@@ -363,7 +324,7 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {/// // This will run (in a separate kthread) if and only if /// // [`ThreadedHandler::handle`] returns [`WakeThread`], which it does by /// // default. -/// fn handle_threaded(&self, _dev: &Device<Bound>) -> IrqReturn { +/// fn handle_threaded(&self) -> IrqReturn { /// let mut data = self.value.lock(); /// *data += 1; /// IrqReturn::Handled
@@ -375,13 +336,21 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {/// // This is executing in process context and assumes that `request` was /// // previously acquired from a device. /// fn register_threaded_irq( -/// handler: impl PinInit<Data, Error>, /// request: IrqRequest<'_>, -/// ) -> Result<Arc<ThreadedRegistration<Data>>> { -/// let registration = -/// ThreadedRegistration::new(request, Flags::SHARED, c"my_device", handler); +/// ) -> Result<Pin<KBox<ThreadedRegistration<'_, Data>>>> { +/// // SAFETY: The returned Registration is not leaked. +/// let registration = unsafe { +/// ThreadedRegistration::new( +/// request, +/// Flags::SHARED, +/// c"my_device", +/// try_pin_init!(Data { +/// value <- kernel::new_mutex!(0), +/// }? Error), +/// ) +/// }; /// -/// let registration = Arc::pin_init(registration, GFP_KERNEL)?; +/// let registration = KBox::pin_init(registration, GFP_KERNEL)?; /// /// { /// // The data can be accessed from process context too.
@@ -396,11 +365,11 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {/// /// # Invariants /// -/// * We own an irq handler whose cookie is a pointer to `Self`. -#[pin_data] -pub struct ThreadedRegistration <T :ThreadedHandler > { -#[pin] - inner:Devres <RegistrationInner >, +/// * We own an irq handler registered via `request_threaded_irq` whose cookie is a pointer to +/// `Self`. +#[pin_data( PinnedDrop )] +pub struct ThreadedRegistration <'a,T :ThreadedHandler > { + request:IrqRequest <'a>,#[pin] handler:T ,
@@ -411,44 +380,45 @@ pub struct ThreadedRegistration<T: ThreadedHandler> { _pin: PhantomPinned, } -impl <T :ThreadedHandler >ThreadedRegistration <T > { +impl <'a,T :ThreadedHandler >ThreadedRegistration <'a,T > {/// Registers the IRQ handler with the system for the given IRQ number. -pub fn new <'a>( +/// +/// # Safety +/// +/// Callers must not `mem::forget()` the returned [`ThreadedRegistration`] or otherwise prevent +/// its [`Drop`] implementation from running. +pub unsafe fn new ( request:IrqRequest <'a>, flags:Flags , name: &'staticCStr , handler:impl PinInit <T ,Error > + 'a, - ) ->impl PinInit <Self ,Error > + 'a { + ) ->impl PinInit <Self ,Error > + 'a +where +T : 'a, + {try_pin_init! (&this inSelf { handler <- handler, - inner <-Devres ::new ( - request.dev, -try_pin_init! (RegistrationInner { -// INVARIANT: `this` is a valid pointer to the `ThreadedRegistration` instance. - cookie: this.as_ptr ().cast ::<c_void >(), - irq: { -// SAFETY: -// - The callbacks are valid for use with request_threaded_irq. -// - If this succeeds, the slot is guaranteed to be valid until the -// destructor of Self runs, which will deregister the callbacks -// before the memory location becomes invalid. -// - When request_threaded_irq is called, everything that the two callbacks -// will touch has already been initialized, so it's safe for the -// callbacks to be called immediately. -to_result (unsafe { - bindings::request_threaded_irq ( - request.irq, -Some (handle_threaded_irq_callback ::<T >), -Some (thread_fn_callback ::<T >), - flags.into_inner (), - name.as_char_ptr (), - this.as_ptr ().cast ::<c_void >(), - ) - })?; - request.irq - } - }) - ), +request : { +// SAFETY: +// - The callbacks are valid for use with request_threaded_irq. +// - If this succeeds, the slot is guaranteed to be valid until the destructor of +// Self runs, which will deregister the callbacks before the memory location +// becomes invalid. +// - When request_threaded_irq is called, everything that the two callbacks will +// touch has already been initialized, so it's safe for the callbacks to be +// called immediately. + to_result(unsafe { + bindings::request_threaded_irq ( + request.irq, +Some (handle_threaded_irq_callback ::<T >), +Some (thread_fn_callback ::<T >), + flags.into_inner (), + name.as_char_ptr (), + this.as_ptr ().cast ::<c_void >(), + ) + })?; + request + }, _pin:PhantomPinned , }) }
@@ -459,19 +429,24 @@ pub fn handler(&self) -> &T { }/// Wait for pending IRQ handlers on other CPUs. -/// -/// This will attempt to access the inner [`Devres`] container. -pub fn try_synchronize (&self) ->Result { -let inner = self.inner.try_access ().ok_or (ENODEV )?; - inner.synchronize (); -Ok (()) +pub fn synchronize (&self) { +// SAFETY: `self.request.irq` is a valid registered IRQ number (type invariant). +unsafe { bindings::synchronize_irq (self.request.irq) }; } +} -/// Wait for pending IRQ handlers on other CPUs. -pub fn synchronize (&self, dev: &Device <Bound >) ->Result { -let inner = self.inner.access (dev)?; - inner.synchronize (); -Ok (()) +#[pinned_drop] +impl <T :ThreadedHandler >PinnedDrop for ThreadedRegistration <'_,T > { +fn drop (self:Pin <&mut Self >) { +// SAFETY: The cookie was set to a pointer to `Self` in `ThreadedRegistration::new()`. This +// blocks until all in-flight handlers complete, so no references to `self` remain after +// this returns. +unsafe { + bindings::free_irq ( + self.request.irq, + core::ptr::from_mut ::<Self >(self.get_unchecked_mut ()).cast ::<c_void >(), + ) + }; } }
@@ -482,24 +457,22 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result { _irq: i32, ptr: *mut c_void, ) -> c_uint { -// SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new` -let registration =unsafe { &*(ptras *const ThreadedRegistration <T >) }; -// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq -// callback is running implies that the device has not yet been unbound. -let device =unsafe { registration.inner.device ().as_bound () }; +let ptr = ptr.cast_const ().cast ::<ThreadedRegistration <'_,T >>(); +// SAFETY: `ptr` is a pointer to `ThreadedRegistration<'_, T>` set in +// `ThreadedRegistration::new()`. +let registration =unsafe { &*ptr }; -T ::handle (®istration.handler, device)as c_uint +T ::handle (®istration.handler)as c_uint }/// # Safety /// /// This function should be only used as the callback in `request_threaded_irq`. unsafe extern "C" fn thread_fn_callback <T :ThreadedHandler >(_irq:i32 , ptr: *mut c_void ) ->c_uint { -// SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new` -let registration =unsafe { &*(ptras *const ThreadedRegistration <T >) }; -// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq -// callback is running implies that the device has not yet been unbound. -let device =unsafe { registration.inner.device ().as_bound () }; +let ptr = ptr.cast_const ().cast ::<ThreadedRegistration <'_,T >>(); +// SAFETY: `ptr` is a pointer to `ThreadedRegistration<'_, T>` set in +// `ThreadedRegistration::new()`. +let registration =unsafe { &*ptr }; -T ::handle_threaded (®istration.handler, device)as c_uint +T ::handle_threaded (®istration.handler)as c_uint }
rust/kernel/pci/irq.rs +18 -8
@@ -173,34 +173,44 @@ fn drop(&mut self) {impl Device <device::Bound > {/// Returns a [`kernel::irq::Registration`] for the given IRQ vector. -pub fn request_irq <'a,T :crate ::irq::Handler + 'static>( +/// +/// # Safety +/// +/// Callers must not `mem::forget()` the resulting [`irq::Registration`] or otherwise prevent +/// its [`Drop`] implementation from running. +pub unsafe fn request_irq <'a,T :crate ::irq::Handler + 'a>( &'a self, vector:IrqVector <'a>, flags: irq::Flags , name: &'staticCStr , handler:impl PinInit <T ,Error > + 'a, - ) ->impl PinInit <irq::Registration <T >,Error > + 'a { + ) ->impl PinInit <irq::Registration <'a,T >,Error > + 'a { pin_init::pin_init_scope (move || {let request = vector.try_into ()?; -Ok (irq::Registration ::<T >::new (request, flags, name, handler)) +// SAFETY: Caller guarantees the Registration will not be leaked. +Ok (unsafe { irq::Registration ::<T >::new (request, flags, name, handler) }) }) }/// Returns a [`kernel::irq::ThreadedRegistration`] for the given IRQ vector. -pub fn request_threaded_irq <'a,T :crate ::irq::ThreadedHandler + 'static>( +/// +/// # Safety +/// +/// Callers must not `mem::forget()` the resulting [`irq::ThreadedRegistration`] or otherwise +/// prevent its [`Drop`] implementation from running. +pub unsafe fn request_threaded_irq <'a,T :crate ::irq::ThreadedHandler + 'a>( &'a self, vector:IrqVector <'a>, flags: irq::Flags , name: &'staticCStr , handler:impl PinInit <T ,Error > + 'a, - ) ->impl PinInit <irq::ThreadedRegistration <T >,Error > + 'a { + ) ->impl PinInit <irq::ThreadedRegistration <'a,T >,Error > + 'a { pin_init::pin_init_scope (move || {let request = vector.try_into ()?; -Ok (irq::ThreadedRegistration ::<T >::new ( - request, flags, name, handler, - )) +// SAFETY: Caller guarantees the Registration will not be leaked. +Ok (unsafe { irq::ThreadedRegistration ::<T >::new (request, flags, name, handler) }) }) }
rust/kernel/platform.rs +32 -16
76 lines not on this page · raw message
base-commit: e7e09a8546a9e41a164378979471a4b4bd166bc7
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [Medium] The doctests for Registration and ThreadedRegistration demonstrate wrapping the unsafe constructors in safe functions, violating soundness requirements.
- [Low] Missing // INVARIANT: comments when constructing structs with # Invariants documentation.
Pre-existing issues:
- [High] Creating a Rust reference to Registration or ThreadedRegistration before its request field is fully initialized causes Undefined Behavior during IRQ registration.
- [Low] Missing #[inline] annotations on small/forwarding functions.
--
commit 8e263235d0a19c8fc9b9f8c688ce7dc1f63f4165
Author: Danilo Krummrich <[email protected]>
rust: irq: make Registration compatible with lifetime-bound drivers
This commit adapts the IRQ registration to support Higher-Ranked Lifetime Types (HRT) device drivers by removing the static bound from IRQ handlers. It replaces Devres indirection with direct request_irq() and free_irq() calls during initialization and drop, tying the registration directly to the device binding scope.
Quoted 3 lines
rust/kernel/irq/request.rs +0
-0
[ ... ]
Quoted 10 lines
rust/kernel/irq/request.rs +2
-3@@ -163,12 +126,21 @@ pub fn irq(&self) -> u32 {/// // /// // This runs in process context and assumes `request` was previously acquired from a device. /// fn register_irq( -/// handler: impl PinInit<Data, Error>, /// request: IrqRequest<'_>, -/// ) -> Result<Arc<Registration<Data>>> { -/// let registration = Registration::new(request, Flags::SHARED, c"my_device", handler); +/// ) -> Result<Pin<KBox<Registration<'_, Data>>>> { +/// // SAFETY: The returned Registration is not leaked.
rust/kernel/irq/request.rs pub fn irq(&self) -> u32 { · line 141
Quoted 10 lines
rust/kernel/irq/request.rs +10
-0+/// let registration = unsafe { +/// Registration::new( +/// request, +/// Flags::SHARED, +/// c"my_device", +/// try_pin_init!(Data { +/// completion <- Completion::new(), +/// }? Error), +/// ) +/// };
[Severity: Medium]
Is it safe to expose this leak-sensitive object from a safe function?
The Registration::new() constructor is unsafe because callers must not leak the returned registration to guarantee proper drop semantics. Since register_irq() is a safe function returning a Pin<KBox<...>>, could a safe caller leak the registration and potentially trigger a use-after-free after the driver unbinds?
Should this example function be marked unsafe fn?
[ ... ]
Quoted 1 line
rust/kernel/irq/request.rs +0
-0@@ -194,43 +165,44 @@ pub struct Registration<T: Handler> {
[ ... ]
Quoted 1 line
+ pub unsafe fn new(
[ ... ]
Quoted 12 lines
rust/kernel/irq/request.rs +12
-0+try_pin_init! (&this inSelf { + handler <- handler, +request : { +// SAFETY: +// - The callbacks are valid for use with request_irq. +// - If this succeeds, the slot is guaranteed to be valid until the destructor of +// Self runs, which will deregister the callbacks before the memory location +// becomes invalid. +// - When request_irq is called, everything that handle_irq_callback will touch +// has already been initialized, so it's safe for the callback to be called +// immediately. + to_result(unsafe {
rust/kernel/irq/request.rs pub struct Registration<T: Handler> { · line 205
Quoted 10 lines
rust/kernel/irq/request.rs +10
-0+ bindings::request_irq ( + request.irq, +Some (handle_irq_callback ::<T >), + flags.into_inner (), + name.as_char_ptr (), + this.as_ptr ().cast ::<c_void >(), + ) + })?; + request + },
[Severity: Low]
This isn't a bug, but shouldn't there be an // INVARIANT: comment inside this initialization block to explain why the invariants for Registration are satisfied?
[Severity: High]
This is a pre-existing issue, but does this pattern trigger undefined behavior if the interrupt fires immediately?
During try_pin_init!, bindings::request_irq() is called while the request field is still being initialized. If the IRQ handler is invoked before request_irq() returns:
handle_irq_callback()
let ptr = ptr.cast_const().cast::<Registration<'_, T>>();
let registration = unsafe { &*ptr };Could this create a Rust reference to the Registration struct while the request field is not fully initialized?
Could the request_irq call be moved into a _: { ... } block to ensure the struct is fully initialized before the interrupt can fire?
[ ... ]
Quoted 4 lines
rust/kernel/irq/request.rs +0
-0@@ -241,19 +213,24 @@ pub fn handler(&self) -> &T { }/// Wait for pending IRQ handlers on other CPUs.
rust/kernel/irq/request.rs pub fn handler(&self) -> &T { · line 219
Quoted 10 lines
rust/kernel/irq/request.rs +3
-6-/// -/// This will attempt to access the inner [`Devres`] container. -pub fn try_synchronize (&self) ->Result { -let inner = self.inner.try_access ().ok_or (ENODEV )?; - inner.synchronize (); -Ok (()) +pub fn synchronize (&self) { +// SAFETY: `self.request.irq` is a valid registered IRQ number (type invariant). +unsafe { bindings::synchronize_irq (self.request.irq) }; }
[Severity: Low]
This is a pre-existing issue and not a bug, but should this forwarding function (and the small accessor IrqRequest::irq() in the same file) be annotated with #[inline] according to the Rust subsystem guidelines?
[ ... ]
Quoted 7 lines
rust/kernel/irq/request.rs +0
-2@@ -375,13 +336,21 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {/// // This is executing in process context and assumes that `request` was /// // previously acquired from a device. /// fn register_threaded_irq( -/// handler: impl PinInit<Data, Error>, /// request: IrqRequest<'_>, -/// ) -> Result<Arc<ThreadedRegistration<Data>>> {
[ ... ]
rust/kernel/irq/request.rs fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn { · line 343
Quoted 4 lines
rust/kernel/irq/request.rs +4
-0+/// ) -> Result<Pin<KBox<ThreadedRegistration<'_, Data>>>> { +/// // SAFETY: The returned Registration is not leaked. +/// let registration = unsafe { +/// ThreadedRegistration::new(
[Severity: Medium]
Is it safe to expose this leak-sensitive object from a safe function?
Similar to register_irq(), the ThreadedRegistration::new() constructor is unsafe to guarantee proper drop semantics. Does returning a Pin<KBox> from a safe function allow a caller to trivially leak the registration?
[ ... ]
Quoted 1 line
rust/kernel/irq/request.rs +0
-0@@ -411,44 +380,45 @@ pub struct ThreadedRegistration<T: ThreadedHandler> {
[ ... ]
Quoted 1 line
+ pub unsafe fn new(
[ ... ]
Quoted 5 lines
rust/kernel/irq/request.rs +5
-0+try_pin_init! (&this inSelf { + handler <- handler, +request : { +// SAFETY: +// - The callbacks are valid for use with request_threaded_irq.
[ ... ]
rust/kernel/irq/request.rs pub struct ThreadedRegistration<T: ThreadedHandler> { · line 421
Quoted 12 lines
rust/kernel/irq/request.rs +12
-0+to_result (unsafe { + bindings::request_threaded_irq ( + request.irq, +Some (handle_threaded_irq_callback ::<T >), +Some (thread_fn_callback ::<T >), + flags.into_inner (), + name.as_char_ptr (), + this.as_ptr ().cast ::<c_void >(), + ) + })?; + request + },
[Severity: Low]
This isn't a bug, but shouldn't there be an // INVARIANT: comment here explaining why the invariants are satisfied?
[Severity: High]
This is a pre-existing issue, but does this pattern also trigger undefined behavior if the interrupt fires immediately, similar to Registration::new()?
Could this request_threaded_irq call be moved into a _: { ... } block to avoid creating a reference to an uninitialized request field?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 37 lines · Danilo Krummrich
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); ... } }
Quoted 7 lines
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.
Signed-off-by Danilo Krummrich <[email protected]>
Alice Ryhl
maintainerrust/kernel/irq/request.rs pub trait Handler: Sync + 'static { · line 51
Quoted 10 lines · Danilo Krummrich
rust/kernel/irq/request.rs +0
-10-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) - }
Why are the implementations for Arc/Box being removed?
Alice
Danilo Krummrich
maintainerQuoted 1 line · Alice Ryhl
Why are the implementations for Arc/Box being removed?
The lifetime design enables us to just store references for shared ownership, rather the needing a reference count, so it seems reasonable to remove those blanket impls.
We could keep them a little longer until pin-init has self-referencial support, but on the other hand they don't enable anything and are just for covinience, so I'd rather drop them.