On Sun, Jun 08, 2025 at 07:51:08PM -0300, Daniel Almeida wrote:
> +/// // This is running in process context.
> +/// fn register_irq(handler: Handler, dev: &platform::Device<Bound>) -> Result<Arc<Registration<Handler>>> {
> +/// let registration = dev.irq_by_index(0, flags::SHARED, c_str!("my-device"), handler)?;
> +///
> +/// // 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?
> +///
> +/// Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
> +///
> +#[pin_data]
> +pub struct Registration<T: Handler + '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,
> +}
> +
> +impl<T: Handler + 'static> Registration<T> {
> + /// Registers the IRQ handler with the system for the given IRQ number.
> + pub(crate) fn register<'a>(
I think we should call this Registration::new() instead. Except for
MiscDeviceRegistration, which is representing not *only* a registration, all
other Registration types just use new() and it'd be nice to be consistent.
> + dev: &'a Device<Bound>,
> + irq: u32,
> + flags: Flags,
> + name: &'static CStr,
> + handler: T,
> + ) -> impl PinInit<Self, Error> + 'a {
> + let closure = move |slot: *mut Self| {
> + // SAFETY: The slot passed to pin initializer is valid for writing.
> + unsafe {
> + slot.write(Self {
> + inner: Devres::new(
> + dev,
> + RegistrationInner {
> + irq,
> + cookie: slot.cast(),
> + },
> + GFP_KERNEL,
> + )?,
> + handler,
> + _pin: PhantomPinned,
> + })
> + };
> +
> + // 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.
> + let res = to_result(unsafe {
> + bindings::request_irq(
> + irq,
> + Some(handle_irq_callback::<T>),
> + flags.into_inner() as usize,
> + name.as_char_ptr(),
> + slot.cast(),
> + )
> + });
> +
> + if res.is_err() {
> + // SAFETY: We are returning an error, so we can destroy the slot.
> + unsafe { core::ptr::drop_in_place(&raw mut (*slot).handler) };
> + }
> +
> + res
> + };
> +
> + // SAFETY:
> + // - if this returns Ok, then every field of `slot` is fully
> + // initialized.
> + // - if this returns an error, then the slot does not need to remain
> + // valid.
> + unsafe { pin_init_from_closure(closure) }
Can't we use try_pin_init!() instead, move request_irq() into the initializer of
RegistrationInner and initialize inner last?
> + }
> +
> + /// Returns a reference to the handler that was registered with the system.
> + pub fn handler(&self) -> &T {
> + &self.handler
> + }
> +
> + /// 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(())
> + }
> +
> + /// 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(())
> + }
> +}
> +
> +/// # Safety
> +///
> +/// 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 core::ffi::c_void,
> +) -> core::ffi::c_uint {
> + // SAFETY: `ptr` is a pointer to Registration<T> set in `Registration::new`
> + let data = unsafe { &*(ptr as *const Registration<T>) };
> + T::handle_irq(&data.handler).into_inner()
> +}
>
> --
> 2.49.0
>
