rust/bindings/bindings_helper.h | 1 + rust/helpers/helpers.c | 1 + rust/helpers/irq.c | 9 + rust/kernel/irq.rs | 6 + rust/kernel/irq/request.rs | 450 ++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 6 files changed, 468 insertions(+)
Both regular and threaded versions are supported.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
---
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/irq.c | 9 +
rust/kernel/irq.rs | 6 +
rust/kernel/irq/request.rs | 450 ++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
6 files changed, 468 insertions(+)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ae82e9c941afa17c48737d2b2e49ac6d26f670b1..f1c35b334f5f7d2adcacb1def72182a09db2ac6c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -13,6 +13,7 @@
#include <linux/errname.h>
#include <linux/ethtool.h>
#include <linux/firmware.h>
+#include <linux/interrupt.h>
#include <linux/jiffies.h>
#include <linux/mdio.h>
#include <linux/phy.h>
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 30f40149f3a969f9e8eb747aabdc17a8cb06936b..0bb48df454bd87def8271444ea58c781b792e34c 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -12,6 +12,7 @@
#include "build_assert.c"
#include "build_bug.c"
#include "err.c"
+#include "irq.c"
#include "kunit.c"
#include "mutex.c"
#include "page.c"
diff --git a/rust/helpers/irq.c b/rust/helpers/irq.c
new file mode 100644
index 0000000000000000000000000000000000000000..1faca428e2c047a656dec3171855c1508d67e60b
--- /dev/null
+++ b/rust/helpers/irq.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/interrupt.h>
+
+int rust_helper_request_irq(unsigned int irq, irq_handler_t handler,
+ unsigned long flags, const char *name, void *dev)
+{
+ return request_irq(irq, handler, flags, name, dev);
+}
diff --git a/rust/kernel/irq.rs b/rust/kernel/irq.rs
new file mode 100644
index 0000000000000000000000000000000000000000..3ab83c5bdb831e84f5e035d9ef56f8e373fa572d
--- /dev/null
+++ b/rust/kernel/irq.rs
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IRQ abstractions
+
+/// IRQ allocation and handling
+pub mod request;
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4b5c5b80c3f43d482132423c2c52cfa5696b7661
--- /dev/null
+++ b/rust/kernel/irq/request.rs
@@ -0,0 +1,450 @@
+// SPDX-License-Identifier: GPL-2.0
+// SPDX-FileCopyrightText: Copyright 2019 Collabora ltd.
+
+//! IRQ allocation and handling
+
+use crate::error::to_result;
+use crate::prelude::*;
+use crate::str::CStr;
+use crate::types::Opaque;
+
+/// Flags to be used when registering IRQ handlers.
+///
+/// They can be combined with the operators `|`, `&`, and `!`.
+///
+/// Values can be used from the [`flags`] module.
+#[derive(Clone, Copy)]
+pub struct Flags(u64);
+
+impl core::ops::BitOr for Flags {
+ type Output = Self;
+ fn bitor(self, rhs: Self) -> Self::Output {
+ Self(self.0 | rhs.0)
+ }
+}
+
+impl core::ops::BitAnd for Flags {
+ type Output = Self;
+ fn bitand(self, rhs: Self) -> Self::Output {
+ Self(self.0 & rhs.0)
+ }
+}
+
+impl core::ops::Not for Flags {
+ type Output = Self;
+ fn not(self) -> Self::Output {
+ Self(!self.0)
+ }
+}
+
+/// The flags that can be used when registering an IRQ handler.
+pub mod flags {
+ use super::Flags;
+
+ use crate::bindings;
+
+ /// Use the interrupt line as already configured.
+ pub const TRIGGER_NONE: Flags = Flags(bindings::IRQF_TRIGGER_NONE as _);
+
+ /// The interrupt is triggered when the signal goes from low to high.
+ pub const TRIGGER_RISING: Flags = Flags(bindings::IRQF_TRIGGER_RISING as _);
+
+ /// The interrupt is triggered when the signal goes from high to low.
+ pub const TRIGGER_FALLING: Flags = Flags(bindings::IRQF_TRIGGER_FALLING as _);
+
+ /// The interrupt is triggered while the signal is held high.
+ pub const TRIGGER_HIGH: Flags = Flags(bindings::IRQF_TRIGGER_HIGH as _);
+
+ /// The interrupt is triggered while the signal is held low.
+ pub const TRIGGER_LOW: Flags = Flags(bindings::IRQF_TRIGGER_LOW as _);
+
+ /// Allow sharing the irq among several devices.
+ pub const SHARED: Flags = Flags(bindings::IRQF_SHARED as _);
+
+ /// Set by callers when they expect sharing mismatches to occur.
+ pub const PROBE_SHARED: Flags = Flags(bindings::IRQF_PROBE_SHARED as _);
+
+ /// Flag to mark this interrupt as timer interrupt.
+ pub const TIMER: Flags = Flags(bindings::IRQF_TIMER as _);
+
+ /// Interrupt is per cpu.
+ pub const PERCPU: Flags = Flags(bindings::IRQF_PERCPU as _);
+
+ /// Flag to exclude this interrupt from irq balancing.
+ pub const NOBALANCING: Flags = Flags(bindings::IRQF_NOBALANCING as _);
+
+ /// Interrupt is used for polling (only the interrupt that is registered
+ /// first in a shared interrupt is considered for performance reasons).
+ pub const IRQPOLL: Flags = Flags(bindings::IRQF_IRQPOLL as _);
+
+ /// Interrupt is not reenabled after the hardirq handler finished. Used by
+ /// threaded interrupts which need to keep the irq line disabled until the
+ /// threaded handler has been run.
+ pub const ONESHOT: Flags = Flags(bindings::IRQF_ONESHOT as _);
+
+ /// Do not disable this IRQ during suspend. Does not guarantee that this
+ /// interrupt will wake the system from a suspended state.
+ pub const NO_SUSPEND: Flags = Flags(bindings::IRQF_NO_SUSPEND as _);
+
+ /// Force enable it on resume even if [`NO_SUSPEND`] is set.
+ pub const FORCE_RESUME: Flags = Flags(bindings::IRQF_FORCE_RESUME as _);
+
+ /// Interrupt cannot be threaded.
+ pub const NO_THREAD: Flags = Flags(bindings::IRQF_NO_THREAD as _);
+
+ /// Resume IRQ early during syscore instead of at device resume time.
+ pub const EARLY_RESUME: Flags = Flags(bindings::IRQF_EARLY_RESUME as _);
+
+ /// If the IRQ is shared with a NO_SUSPEND user, execute this interrupt
+ /// 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;
+pub enum IrqReturn {
+ /// 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;
+}
+
+/// A registration of an IRQ handler for a given IRQ line.
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+/// # Examples
+///
+/// The following is an example of using `Registration`:
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq;
+/// use kernel::irq::Registration;
+/// use kernel::sync::Arc;
+/// use kernel::sync::lock::SpinLock;
+///
+/// // 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(u32);
+///
+/// // [`handle_irq`] returns &self. This example illustrates interior
+/// // mutability can be used when share the data between process context and IRQ
+/// // context.
+/// //
+/// // Ideally, this example would be using a version of SpinLock that is aware
+/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
+/// // implemented.
+///
+/// type Handler = SpinLock<Data>;
+///
+/// impl kernel::irq::Handler for Handler {
+/// // This is executing in IRQ context in some CPU. Other CPUs can still
+/// // try to access to data.
+/// fn handle_irq(&self) -> irq::IrqReturn {
+/// // We now have exclusive access to the data by locking the SpinLock.
+/// let mut handler = self.lock();
+/// handler.0 += 1;
+///
+/// IrqReturn::Handled
+/// }
+/// }
+///
+/// // This is running in process context.
+/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
+/// let registration = Registration::register(irq, irq::flags::SHARED, "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)?;
+///
+/// // 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().lock().0 = 42;
+///
+/// Ok(registration)
+/// }
+///
+/// # Ok::<(), Error>(())
+///```
+#[pin_data(PinnedDrop)]
+pub struct Registration<T: Handler> {
+ irq: u32,
+ #[pin]
+ handler: Opaque<T>,
+}
+
+impl<T: Handler> Registration<T> {
+ /// Registers the IRQ handler with the system for the given IRQ number. The
+ /// handler must be able to be called as soon as this function returns.
+ pub fn register(
+ irq: u32,
+ flags: Flags,
+ name: &'static CStr,
+ handler: T,
+ ) -> impl PinInit<Self, Error> {
+ try_pin_init!(Self {
+ irq,
+ handler: Opaque::new(handler)
+ })
+ .pin_chain(move |slot| {
+ // SAFETY:
+ // - `handler` points to a valid function defined below.
+ // - only valid flags can be constructed using the `flags` module.
+ // - `devname` is a nul-terminated string with a 'static lifetime.
+ // - `ptr` is a cookie used to identify the handler. The same cookie is
+ // passed back when the system calls the handler.
+ to_result(unsafe {
+ bindings::request_irq(
+ irq,
+ Some(handle_irq_callback::<T>),
+ flags.0,
+ name.as_char_ptr(),
+ &*slot as *const _ as *mut core::ffi::c_void,
+ )
+ })?;
+
+ Ok(())
+ })
+ }
+
+ /// Returns a reference to the handler that was registered with the system.
+ pub fn handler(&self) -> &T {
+ // SAFETY: `handler` is initialized in `register`.
+ unsafe { &*self.handler.get() }
+ }
+}
+
+#[pinned_drop]
+impl<T: Handler> PinnedDrop for Registration<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY:
+ // - `self.irq` is the same as the one passed to `reques_irq`.
+ // - `&self` was passed to `request_irq` as the cookie. It is
+ // guaranteed to be unique by the type system, since each call to
+ // `register` will return a different instance of `Registration`.
+ //
+ // Notice that this will block until all handlers finish executing, so,
+ // at no point will &self be invalid while the handler is running.
+ unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };
+ }
+}
+
+/// The value that can be returned from `ThreadedHandler::handle_irq`.
+pub enum ThreadedIrqReturn {
+ /// 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 _,
+
+ /// The handler wants the handler thread to wake up.
+ WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
+}
+
+/// The value that can be returned from `ThreadedFnHandler::thread_fn`.
+pub enum ThreadedFnReturn {
+ /// The thread function did not make any progress.
+ None = bindings::irqreturn_IRQ_NONE as _,
+
+ /// The thread function ran successfully.
+ Handled = bindings::irqreturn_IRQ_HANDLED as _,
+}
+
+/// 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) -> ThreadedFnReturn;
+}
+
+/// 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`.
+///
+/// # Invariants
+///
+/// * We own an irq handler using `&self` as its private data.
+///
+/// # Examples
+///
+/// The following is an example of using `ThreadedRegistration`:
+///
+/// ```
+/// use kernel::prelude::*;
+/// use kernel::irq;
+/// use kernel::irq::Registration;
+/// use kernel::sync::Arc;
+/// use kernel::sync::lock::SpinLock;
+///
+/// // 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(u32);
+///
+/// // [`handle_irq`] returns &self. This example illustrates interior
+/// // mutability can be used when share the data between process context and IRQ
+/// // context.
+/// //
+/// // Ideally, this example would be using a version of SpinLock that is aware
+/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
+/// // implemented.
+///
+/// type Handler = SpinLock<Data>;
+///
+/// impl kernel::irq::Handler for Handler {
+/// // This is executing in IRQ context in some CPU. Other CPUs can still
+/// // try to access to data.
+/// fn handle_irq(&self) -> irq::ThreadedIrqReturn {
+/// // We now have exclusive access to the data by locking the SpinLock.
+/// let mut handler = self.lock();
+/// handler.0 += 1;
+///
+/// // 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) iff `handle_irq` returns
+/// // `WakeThread`.
+/// fn thread_fn(&self) -> irq::ThreadedFnReturn {
+/// // We now have exclusive access to the data by locking the SpinLock.
+/// let mut handler = self.lock();
+/// handler.0 += 1;
+/// }
+///
+/// // This is running in process context.
+/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
+/// let registration = ThreadedRegistration::register(irq, irq::flags::SHARED, "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)?;
+///
+/// // 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().lock().0 = 42;
+///
+/// Ok(registration)
+/// }
+/// }
+///
+/// # Ok::<(), Error>(())
+///```
+#[pin_data(PinnedDrop)]
+pub struct ThreadedRegistration<T: ThreadedHandler> {
+ irq: u32,
+ #[pin]
+ handler: Opaque<T>,
+}
+
+impl<T: ThreadedHandler> ThreadedRegistration<T> {
+ /// Registers the IRQ handler with the system for the given IRQ number. The
+ /// handler must be able to be called as soon as this function returns.
+ pub fn register(
+ irq: u32,
+ flags: Flags,
+ name: &'static CStr,
+ handler: T,
+ ) -> impl PinInit<Self, Error> {
+ try_pin_init!(Self {
+ irq,
+ handler: Opaque::new(handler)
+ })
+ .pin_chain(move |slot| {
+ // SAFETY:
+ // - `handler` points to a valid function defined below.
+ // - only valid flags can be constructed using the `flags` module.
+ // - `devname` is a nul-terminated string with a 'static lifetime.
+ // - `ptr` is a cookie used to identify the handler. The same cookie is
+ // passed back when the system calls the handler.
+ to_result(unsafe {
+ bindings::request_threaded_irq(
+ irq,
+ Some(handle_threaded_irq_callback::<T>),
+ Some(thread_fn_callback::<T>),
+ flags.0,
+ name.as_char_ptr(),
+ &*slot as *const _ as *mut core::ffi::c_void,
+ )
+ })?;
+
+ Ok(())
+ })
+ }
+
+ /// Returns a reference to the handler that was registered with the system.
+ pub fn handler(&self) -> &T {
+ // SAFETY: `handler` is initialized in `register`.
+ unsafe { &*self.handler.get() }
+ }
+}
+
+#[pinned_drop]
+impl<T: ThreadedHandler> PinnedDrop for ThreadedRegistration<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY:
+ // - `self.irq` is the same as the one passed to `request_threaded_irq`.
+ // - `&self` was passed to `request_threaded_irq` as the cookie. It is
+ // guaranteed to be unique by the type system, since each call to
+ // `register` will return a different instance of
+ // `ThreadedRegistration`.
+ //
+ // Notice that this will block until all handlers finish executing, so,
+ // at no point will &self be invalid while the handler is running.
+ unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };
+ }
+}
+
+unsafe extern "C" fn handle_irq_callback<T: Handler>(
+ _irq: i32,
+ ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+ let data = unsafe { &*(ptr as *const T) };
+ T::handle_irq(data) as _
+}
+
+unsafe extern "C" fn handle_threaded_irq_callback<T: ThreadedHandler>(
+ _irq: i32,
+ ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+ let data = unsafe { &*(ptr as *const T) };
+ T::handle_irq(data) as _
+}
+
+unsafe extern "C" fn thread_fn_callback<T: ThreadedHandler>(
+ _irq: i32,
+ ptr: *mut core::ffi::c_void,
+) -> core::ffi::c_uint {
+ let data = unsafe { &*(ptr as *const T) };
+ T::thread_fn(data) as _
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 22a3bfa5a9e96a1dd9cf76bd34a8d992458870b4..0390bc0b114f60876414c82b5b8862401338d241 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -36,6 +36,7 @@
pub mod firmware;
pub mod init;
pub mod ioctl;
+pub mod irq;
#[cfg(CONFIG_KUNIT)]
pub mod kunit;
pub mod list;
---
base-commit: 33c255312660653cf54f8019896b5dca28e3c580
change-id: 20241023-topic-panthor-rs-request_irq-62008ccc82eb
Best regards,
--
Daniel Almeida <daniel.almeida@collabora.com>On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida <daniel.almeida@collabora.com> wrote: > > Both regular and threaded versions are supported. > > Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> Why implement request_irq over devm_request_irq? Alice
Hi Alice, > On 29 Oct 2024, at 08:59, Alice Ryhl <aliceryhl@google.com> wrote: > > On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida > <daniel.almeida@collabora.com> wrote: >> >> Both regular and threaded versions are supported. >> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com> > > Why implement request_irq over devm_request_irq? > > Alice > As we spoke earlier, and for the record, devm will automate the lifetime of a resource, taking care to relinquish it when the driver is unloaded. I feel better with an explicit `Drop` implementation, which is why the devm versions were not chosen. IMHO, the problem it was designed to solve often simply does not apply to Rust code. — Daniel
On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Both regular and threaded versions are supported.
>
> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
I left some comments below:
> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..4b5c5b80c3f43d482132423c2c52cfa5696b7661
> --- /dev/null
> +++ b/rust/kernel/irq/request.rs
> @@ -0,0 +1,450 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright 2019 Collabora ltd.
should this be 2024?
> +/// The value that can be returned from an IrqHandler;
> +pub enum IrqReturn {
> + /// 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;
> +}
> +
> +/// A registration of an IRQ handler for a given IRQ line.
> +///
> +/// # Invariants
> +///
> +/// * We own an irq handler using `&self` as its private data.
The invariants section is usually last.
> +/// # Examples
> +///
> +/// The following is an example of using `Registration`:
> +///
> +/// ```
> +/// use kernel::prelude::*;
> +/// use kernel::irq;
> +/// use kernel::irq::Registration;
> +/// use kernel::sync::Arc;
> +/// use kernel::sync::lock::SpinLock;
> +///
> +/// // 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(u32);
> +///
> +/// // [`handle_irq`] returns &self. This example illustrates interior
> +/// // mutability can be used when share the data between process context and IRQ
> +/// // context.
> +/// //
> +/// // Ideally, this example would be using a version of SpinLock that is aware
> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
> +/// // implemented.
> +///
> +/// type Handler = SpinLock<Data>;
I doubt this will compile outside of the kernel crate. It fails the
orphan rule because your driver neither owns the SpinLock type or the
Handler trait. You should move `SpinLock` inside `Data` instead.
> +/// impl kernel::irq::Handler for Handler {
> +/// // This is executing in IRQ context in some CPU. Other CPUs can still
> +/// // try to access to data.
> +/// fn handle_irq(&self) -> irq::IrqReturn {
> +/// // We now have exclusive access to the data by locking the SpinLock.
> +/// let mut handler = self.lock();
> +/// handler.0 += 1;
> +///
> +/// IrqReturn::Handled
> +/// }
> +/// }
> +///
> +/// // This is running in process context.
> +/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
Please try compiling the example. The return type should be
Result<Arc<irq::Registration<Handler>>>.
> +/// let registration = Registration::register(irq, irq::flags::SHARED, "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)?;
> +///
> +/// // 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().lock().0 = 42;
> +///
> +/// Ok(registration)
> +/// }
> +///
> +/// # Ok::<(), Error>(())
> +///```
> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: Handler> {
> + irq: u32,
> + #[pin]
> + handler: Opaque<T>,
> +}
> +
> +impl<T: Handler> Registration<T> {
> + /// Registers the IRQ handler with the system for the given IRQ number. The
> + /// handler must be able to be called as soon as this function returns.
> + pub fn register(
> + irq: u32,
> + flags: Flags,
> + name: &'static CStr,
Does the name need to be 'static?
> + handler: T,
> + ) -> impl PinInit<Self, Error> {
> + try_pin_init!(Self {
> + irq,
> + handler: Opaque::new(handler)
> + })
> + .pin_chain(move |slot| {
> + // SAFETY:
> + // - `handler` points to a valid function defined below.
> + // - only valid flags can be constructed using the `flags` module.
> + // - `devname` is a nul-terminated string with a 'static lifetime.
> + // - `ptr` is a cookie used to identify the handler. The same cookie is
> + // passed back when the system calls the handler.
> + to_result(unsafe {
> + bindings::request_irq(
> + irq,
> + Some(handle_irq_callback::<T>),
> + flags.0,
> + name.as_char_ptr(),
> + &*slot as *const _ as *mut core::ffi::c_void,
Can simplify to `slot as *mut c_void` or `slot.cast()`.
> + )
> + })?;
> +
> + Ok(())
> + })
> + }
> +
> + /// Returns a reference to the handler that was registered with the system.
> + pub fn handler(&self) -> &T {
> + // SAFETY: `handler` is initialized in `register`.
> + unsafe { &*self.handler.get() }
This relies on T being Sync as it could also get accessed by the irq
handler in parallel. You probably want the SAFETY comment to mention
that.
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T: Handler> PinnedDrop for Registration<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY:
> + // - `self.irq` is the same as the one passed to `reques_irq`.
> + // - `&self` was passed to `request_irq` as the cookie. It is
> + // guaranteed to be unique by the type system, since each call to
> + // `register` will return a different instance of `Registration`.
> + //
> + // Notice that this will block until all handlers finish executing, so,
> + // at no point will &self be invalid while the handler is running.
> + unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };
I can't tell if this creates a pointer to the Registration or a
pointer to a pointer to the Registration. Please spell out the type:
```
&*self as *const Self as *mut core::ffi::c_void
```
> + }
> +}
> +
> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
> +pub enum ThreadedIrqReturn {
> + /// 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 _,
> +
> + /// The handler wants the handler thread to wake up.
> + WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
> +}
> +
> +/// The value that can be returned from `ThreadedFnHandler::thread_fn`.
> +pub enum ThreadedFnReturn {
> + /// The thread function did not make any progress.
> + None = bindings::irqreturn_IRQ_NONE as _,
> +
> + /// The thread function ran successfully.
> + Handled = bindings::irqreturn_IRQ_HANDLED as _,
> +}
This is the same as IrqReturn?
> +/// 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) -> ThreadedFnReturn;
> +}
Most of my comments above also reply to ThreadedHandler.
Alice
Hi Alice, thanks for the review!
> On 28 Oct 2024, at 12:29, Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> <daniel.almeida@collabora.com> wrote:
>>
>> Both regular and threaded versions are supported.
>>
>> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
>
Yeah, as I was saying, my latest patches were sent with some provisional
commit messages. Sometimes these things slip through.
In fact, as this was my first time switching to b4, it took me a while to
realize I had sent the patches to myself only, so you can see I started off
with the “wrong foot” here.
> I left some comments below:
>
>> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..4b5c5b80c3f43d482132423c2c52cfa5696b7661
>> --- /dev/null
>> +++ b/rust/kernel/irq/request.rs
>> @@ -0,0 +1,450 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// SPDX-FileCopyrightText: Copyright 2019 Collabora ltd.
>
> should this be 2024?
>
>> +/// The value that can be returned from an IrqHandler;
>> +pub enum IrqReturn {
>> + /// 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;
>> +}
>> +
>> +/// A registration of an IRQ handler for a given IRQ line.
>> +///
>> +/// # Invariants
>> +///
>> +/// * We own an irq handler using `&self` as its private data.
>
> The invariants section is usually last.
>
>> +/// # Examples
>> +///
>> +/// The following is an example of using `Registration`:
>> +///
>> +/// ```
>> +/// use kernel::prelude::*;
>> +/// use kernel::irq;
>> +/// use kernel::irq::Registration;
>> +/// use kernel::sync::Arc;
>> +/// use kernel::sync::lock::SpinLock;
>> +///
>> +/// // 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(u32);
>> +///
>> +/// // [`handle_irq`] returns &self. This example illustrates interior
>> +/// // mutability can be used when share the data between process context and IRQ
>> +/// // context.
>> +/// //
>> +/// // Ideally, this example would be using a version of SpinLock that is aware
>> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
>> +/// // implemented.
>> +///
>> +/// type Handler = SpinLock<Data>;
>
> I doubt this will compile outside of the kernel crate. It fails the
> orphan rule because your driver neither owns the SpinLock type or the
> Handler trait. You should move `SpinLock` inside `Data` instead.
>
>> +/// impl kernel::irq::Handler for Handler {
>> +/// // This is executing in IRQ context in some CPU. Other CPUs can still
>> +/// // try to access to data.
>> +/// fn handle_irq(&self) -> irq::IrqReturn {
>> +/// // We now have exclusive access to the data by locking the SpinLock.
>> +/// let mut handler = self.lock();
>> +/// handler.0 += 1;
>> +///
>> +/// IrqReturn::Handled
>> +/// }
>> +/// }
>> +///
>> +/// // This is running in process context.
>> +/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
>
> Please try compiling the example. The return type should be
> Result<Arc<irq::Registration<Handler>>>.
Sorry, I was under the impression that `rustdoc` would compile the examples too.
>
>> +/// let registration = Registration::register(irq, irq::flags::SHARED, "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)?;
>> +///
>> +/// // 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().lock().0 = 42;
>> +///
>> +/// Ok(registration)
>> +/// }
>> +///
>> +/// # Ok::<(), Error>(())
>> +///```
>> +#[pin_data(PinnedDrop)]
>> +pub struct Registration<T: Handler> {
>> + irq: u32,
>> + #[pin]
>> + handler: Opaque<T>,
>> +}
>> +
>> +impl<T: Handler> Registration<T> {
>> + /// Registers the IRQ handler with the system for the given IRQ number. The
>> + /// handler must be able to be called as soon as this function returns.
>> + pub fn register(
>> + irq: u32,
>> + flags: Flags,
>> + name: &'static CStr,
>
> Does the name need to be 'static?
Actually, the lifetime relationship that we want to express here is that `name` should
live for at least as long as &self.
Most of the time in C, this is solved by having `name` point to a statically allocated string,
usually a string literal, so this version of the patch implemented that.
What about:
```
Registration<‘a> {
name: PhantomData<&‘a CStr>
}
```
Where calling register() with some c_str!(“foo”) would create a Registration<’static>
anyways?
>> + handler: T,
>> + ) -> impl PinInit<Self, Error> {
>> + try_pin_init!(Self {
>> + irq,
>> + handler: Opaque::new(handler)
>> + })
>> + .pin_chain(move |slot| {
>> + // SAFETY:
>> + // - `handler` points to a valid function defined below.
>> + // - only valid flags can be constructed using the `flags` module.
>> + // - `devname` is a nul-terminated string with a 'static lifetime.
>> + // - `ptr` is a cookie used to identify the handler. The same cookie is
>> + // passed back when the system calls the handler.
>> + to_result(unsafe {
>> + bindings::request_irq(
>> + irq,
>> + Some(handle_irq_callback::<T>),
>> + flags.0,
>> + name.as_char_ptr(),
>> + &*slot as *const _ as *mut core::ffi::c_void,
>
> Can simplify to `slot as *mut c_void` or `slot.cast()`.
>
>> + )
>> + })?;
>> +
>> + Ok(())
>> + })
>> + }
>> +
>> + /// Returns a reference to the handler that was registered with the system.
>> + pub fn handler(&self) -> &T {
>> + // SAFETY: `handler` is initialized in `register`.
>> + unsafe { &*self.handler.get() }
>
> This relies on T being Sync as it could also get accessed by the irq
> handler in parallel. You probably want the SAFETY comment to mention
> that.
>
>> + }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T: Handler> PinnedDrop for Registration<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY:
>> + // - `self.irq` is the same as the one passed to `reques_irq`.
>> + // - `&self` was passed to `request_irq` as the cookie. It is
>> + // guaranteed to be unique by the type system, since each call to
>> + // `register` will return a different instance of `Registration`.
>> + //
>> + // Notice that this will block until all handlers finish executing, so,
>> + // at no point will &self be invalid while the handler is running.
>> + unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };
>
> I can't tell if this creates a pointer to the Registration or a
> pointer to a pointer to the Registration. Please spell out the type:
> ```
> &*self as *const Self as *mut core::ffi::c_void
> ```
My thought process here is that Pin<Ptr<T>> dereferences to Ptr::Target, i.e. Self,
which is then borrowed, i.e. &Self.
I do not see how this can create a pointer to a pointer, but you’re right, it’s
always good to be more explicit by spelling out the full type. I will fix that.
>
>> + }
>> +}
>> +
>> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
>> +pub enum ThreadedIrqReturn {
>> + /// 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 _,
>> +
>> + /// The handler wants the handler thread to wake up.
>> + WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
>> +}
>> +
>> +/// The value that can be returned from `ThreadedFnHandler::thread_fn`.
>> +pub enum ThreadedFnReturn {
>> + /// The thread function did not make any progress.
>> + None = bindings::irqreturn_IRQ_NONE as _,
>> +
>> + /// The thread function ran successfully.
>> + Handled = bindings::irqreturn_IRQ_HANDLED as _,
>> +}
>
> This is the same as IrqReturn?
Thanks for noticing this. It indeed ended up as the same type after all.
>
>> +/// 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) -> ThreadedFnReturn;
>> +}
>
> Most of my comments above also reply to ThreadedHandler.
>
> Alice
— Daniel
On Mon, Nov 4, 2024 at 9:10 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Hi Alice, thanks for the review!
>
>
> > On 28 Oct 2024, at 12:29, Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
> > <daniel.almeida@collabora.com> wrote:
> >>
> >> Both regular and threaded versions are supported.
> >>
> >> Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
> >
>
> Yeah, as I was saying, my latest patches were sent with some provisional
> commit messages. Sometimes these things slip through.
>
> In fact, as this was my first time switching to b4, it took me a while to
> realize I had sent the patches to myself only, so you can see I started off
> with the “wrong foot” here.
>
> > I left some comments below:
> >
> >> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..4b5c5b80c3f43d482132423c2c52cfa5696b7661
> >> --- /dev/null
> >> +++ b/rust/kernel/irq/request.rs
> >> @@ -0,0 +1,450 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +// SPDX-FileCopyrightText: Copyright 2019 Collabora ltd.
> >
> > should this be 2024?
> >
> >> +/// The value that can be returned from an IrqHandler;
> >> +pub enum IrqReturn {
> >> + /// 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;
> >> +}
> >> +
> >> +/// A registration of an IRQ handler for a given IRQ line.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// * We own an irq handler using `&self` as its private data.
> >
> > The invariants section is usually last.
> >
> >> +/// # Examples
> >> +///
> >> +/// The following is an example of using `Registration`:
> >> +///
> >> +/// ```
> >> +/// use kernel::prelude::*;
> >> +/// use kernel::irq;
> >> +/// use kernel::irq::Registration;
> >> +/// use kernel::sync::Arc;
> >> +/// use kernel::sync::lock::SpinLock;
> >> +///
> >> +/// // 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(u32);
> >> +///
> >> +/// // [`handle_irq`] returns &self. This example illustrates interior
> >> +/// // mutability can be used when share the data between process context and IRQ
> >> +/// // context.
> >> +/// //
> >> +/// // Ideally, this example would be using a version of SpinLock that is aware
> >> +/// // of `spin_lock_irqsave` and `spin_lock_irqrestore`, but that is not yet
> >> +/// // implemented.
> >> +///
> >> +/// type Handler = SpinLock<Data>;
> >
> > I doubt this will compile outside of the kernel crate. It fails the
> > orphan rule because your driver neither owns the SpinLock type or the
> > Handler trait. You should move `SpinLock` inside `Data` instead.
> >
> >> +/// impl kernel::irq::Handler for Handler {
> >> +/// // This is executing in IRQ context in some CPU. Other CPUs can still
> >> +/// // try to access to data.
> >> +/// fn handle_irq(&self) -> irq::IrqReturn {
> >> +/// // We now have exclusive access to the data by locking the SpinLock.
> >> +/// let mut handler = self.lock();
> >> +/// handler.0 += 1;
> >> +///
> >> +/// IrqReturn::Handled
> >> +/// }
> >> +/// }
> >> +///
> >> +/// // This is running in process context.
> >> +/// fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
> >
> > Please try compiling the example. The return type should be
> > Result<Arc<irq::Registration<Handler>>>.
>
> Sorry, I was under the impression that `rustdoc` would compile the examples too.
>
> >
> >> +/// let registration = Registration::register(irq, irq::flags::SHARED, "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)?;
> >> +///
> >> +/// // 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().lock().0 = 42;
> >> +///
> >> +/// Ok(registration)
> >> +/// }
> >> +///
> >> +/// # Ok::<(), Error>(())
> >> +///```
> >> +#[pin_data(PinnedDrop)]
> >> +pub struct Registration<T: Handler> {
> >> + irq: u32,
> >> + #[pin]
> >> + handler: Opaque<T>,
> >> +}
> >> +
> >> +impl<T: Handler> Registration<T> {
> >> + /// Registers the IRQ handler with the system for the given IRQ number. The
> >> + /// handler must be able to be called as soon as this function returns.
> >> + pub fn register(
> >> + irq: u32,
> >> + flags: Flags,
> >> + name: &'static CStr,
> >
> > Does the name need to be 'static?
>
> Actually, the lifetime relationship that we want to express here is that `name` should
> live for at least as long as &self.
>
> Most of the time in C, this is solved by having `name` point to a statically allocated string,
> usually a string literal, so this version of the patch implemented that.
>
> What about:
>
> ```
> Registration<‘a> {
> name: PhantomData<&‘a CStr>
> }
> ```
>
> Where calling register() with some c_str!(“foo”) would create a Registration<’static>
> anyways?
Ah ... what you propose would be correct but it doesn't sound useful.
Let's just keep 'static.
> >> + handler: T,
> >> + ) -> impl PinInit<Self, Error> {
> >> + try_pin_init!(Self {
> >> + irq,
> >> + handler: Opaque::new(handler)
> >> + })
> >> + .pin_chain(move |slot| {
> >> + // SAFETY:
> >> + // - `handler` points to a valid function defined below.
> >> + // - only valid flags can be constructed using the `flags` module.
> >> + // - `devname` is a nul-terminated string with a 'static lifetime.
> >> + // - `ptr` is a cookie used to identify the handler. The same cookie is
> >> + // passed back when the system calls the handler.
> >> + to_result(unsafe {
> >> + bindings::request_irq(
> >> + irq,
> >> + Some(handle_irq_callback::<T>),
> >> + flags.0,
> >> + name.as_char_ptr(),
> >> + &*slot as *const _ as *mut core::ffi::c_void,
> >
> > Can simplify to `slot as *mut c_void` or `slot.cast()`.
> >
> >> + )
> >> + })?;
> >> +
> >> + Ok(())
> >> + })
> >> + }
> >> +
> >> + /// Returns a reference to the handler that was registered with the system.
> >> + pub fn handler(&self) -> &T {
> >> + // SAFETY: `handler` is initialized in `register`.
> >> + unsafe { &*self.handler.get() }
> >
> > This relies on T being Sync as it could also get accessed by the irq
> > handler in parallel. You probably want the SAFETY comment to mention
> > that.
> >
> >> + }
> >> +}
> >> +
> >> +#[pinned_drop]
> >> +impl<T: Handler> PinnedDrop for Registration<T> {
> >> + fn drop(self: Pin<&mut Self>) {
> >> + // SAFETY:
> >> + // - `self.irq` is the same as the one passed to `reques_irq`.
> >> + // - `&self` was passed to `request_irq` as the cookie. It is
> >> + // guaranteed to be unique by the type system, since each call to
> >> + // `register` will return a different instance of `Registration`.
> >> + //
> >> + // Notice that this will block until all handlers finish executing, so,
> >> + // at no point will &self be invalid while the handler is running.
> >> + unsafe { bindings::free_irq(self.irq, &*self as *const _ as *mut core::ffi::c_void) };
> >
> > I can't tell if this creates a pointer to the Registration or a
> > pointer to a pointer to the Registration. Please spell out the type:
> > ```
> > &*self as *const Self as *mut core::ffi::c_void
> > ```
>
> My thought process here is that Pin<Ptr<T>> dereferences to Ptr::Target, i.e. Self,
> which is then borrowed, i.e. &Self.
>
> I do not see how this can create a pointer to a pointer, but you’re right, it’s
> always good to be more explicit by spelling out the full type. I will fix that.
Ah, yes, I suppose the Pin wrapper does make it more clear. Still, I
think spelling out the type would improve the clarity in this case.
> >> + }
> >> +}
> >> +
> >> +/// The value that can be returned from `ThreadedHandler::handle_irq`.
> >> +pub enum ThreadedIrqReturn {
> >> + /// 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 _,
> >> +
> >> + /// The handler wants the handler thread to wake up.
> >> + WakeThread = bindings::irqreturn_IRQ_WAKE_THREAD as _,
> >> +}
> >> +
> >> +/// The value that can be returned from `ThreadedFnHandler::thread_fn`.
> >> +pub enum ThreadedFnReturn {
> >> + /// The thread function did not make any progress.
> >> + None = bindings::irqreturn_IRQ_NONE as _,
> >> +
> >> + /// The thread function ran successfully.
> >> + Handled = bindings::irqreturn_IRQ_HANDLED as _,
> >> +}
> >
> > This is the same as IrqReturn?
>
> Thanks for noticing this. It indeed ended up as the same type after all.
>
> >
> >> +/// 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) -> ThreadedFnReturn;
> >> +}
> >
> > Most of my comments above also reply to ThreadedHandler.
> >
> > Alice
>
> — Daniel
>
Hi Daniel,
kernel test robot noticed the following build errors:
[auto build test ERROR on 33c255312660653cf54f8019896b5dca28e3c580]
url: https://github.com/intel-lab-lkp/linux/commits/Daniel-Almeida/rust-irq-add-support-for-request_irq/20241024-222633
base: 33c255312660653cf54f8019896b5dca28e3c580
patch link: https://lore.kernel.org/r/20241024-topic-panthor-rs-request_irq-v1-1-7cbc51c182ca%40collabora.com
patch subject: [PATCH] rust: irq: add support for request_irq()
config: riscv-randconfig-001-20241027 (https://download.01.org/0day-ci/archive/20241027/202410271331.Kh0GAuXf-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241027/202410271331.Kh0GAuXf-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410271331.Kh0GAuXf-lkp@intel.com/
All errors (new ones prefixed by >>):
>> error[E0432]: unresolved import `kernel::irq::Registration`
--> rust/doctests_kernel_generated.rs:1837:5
|
1837 | use kernel::irq::Registration;
| ^^^^^^^^^^^^^^^^^^^^^^^^^ no `Registration` in `irq`
|
help: consider importing one of these items instead
|
1837 | use kernel::irq::request::Registration;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1837 | use kernel::net::phy::Registration;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
>> error[E0432]: unresolved import `kernel::sync::lock::SpinLock`
--> rust/doctests_kernel_generated.rs:1839:5
|
1839 | use kernel::sync::lock::SpinLock;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `SpinLock` in `sync::lock`
|
help: a similar name exists in the module
|
1839 | use kernel::sync::lock::spinlock;
| ~~~~~~~~
help: consider importing this type alias instead
|
1839 | use kernel::sync::SpinLock;
| ~~~~~~~~~~~~~~~~~~~~~~
--
>> error[E0412]: cannot find type `ThreadedIrqReturn` in module `irq`
--> rust/doctests_kernel_generated.rs:1955:34
|
1955 | fn handle_irq(&self) -> irq::ThreadedIrqReturn {
| ^^^^^^^^^^^^^^^^^ not found in `irq`
|
help: consider importing this enum
|
3 + use kernel::irq::request::ThreadedIrqReturn;
|
help: if you import `ThreadedIrqReturn`, refer to it directly
|
1955 - fn handle_irq(&self) -> irq::ThreadedIrqReturn {
1955 + fn handle_irq(&self) -> ThreadedIrqReturn {
|
--
>> error[E0433]: failed to resolve: use of undeclared type `ThreadedIrqReturn`
--> rust/doctests_kernel_generated.rs:1963:9
|
1963 | ThreadedIrqReturn::WakeThread
| ^^^^^^^^^^^^^^^^^ use of undeclared type `ThreadedIrqReturn`
|
help: consider importing this enum
|
3 + use kernel::irq::request::ThreadedIrqReturn;
|
--
>> error[E0412]: cannot find type `ThreadedFnReturn` in module `irq`
--> rust/doctests_kernel_generated.rs:1968:33
|
1968 | fn thread_fn(&self) -> irq::ThreadedFnReturn {
| ^^^^^^^^^^^^^^^^ not found in `irq`
|
help: consider importing this enum
|
3 + use kernel::irq::request::ThreadedFnReturn;
|
help: if you import `ThreadedFnReturn`, refer to it directly
|
1968 - fn thread_fn(&self) -> irq::ThreadedFnReturn {
1968 + fn thread_fn(&self) -> ThreadedFnReturn {
|
--
>> error[E0412]: cannot find type `Registration` in module `irq`
--> rust/doctests_kernel_generated.rs:1975:64
|
1975 | fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
| ^^^^^^^^^^^^ not found in `irq`
|
help: consider importing one of these items
|
3 + use kernel::irq::request::Registration;
|
3 + use kernel::net::phy::Registration;
|
help: if you import `Registration`, refer to it directly
|
1975 - fn register_irq(irq: u32, handler: Handler) -> Result<irq::Registration<Handler>> {
1975 + fn register_irq(irq: u32, handler: Handler) -> Result<Registration<Handler>> {
|
--
>> error[E0433]: failed to resolve: use of undeclared type `ThreadedRegistration`
--> rust/doctests_kernel_generated.rs:1976:28
|
1976 | let registration = ThreadedRegistration::register(irq, irq::flags::SHARED, "my-device", handler)?;
| ^^^^^^^^^^^^^^^^^^^^ use of undeclared type `ThreadedRegistration`
|
help: consider importing this struct
|
3 + use kernel::irq::request::ThreadedRegistration;
|
--
>> error[E0433]: failed to resolve: could not find `flags` in `irq`
--> rust/doctests_kernel_generated.rs:1976:69
|
1976 | let registration = ThreadedRegistration::register(irq, irq::flags::SHARED, "my-device", handler)?;
| ^^^^^ could not find `flags` in `irq`
|
help: consider importing this module
|
3 + use kernel::irq::request::flags;
|
help: if you import `flags`, refer to it directly
|
1976 - let registration = ThreadedRegistration::register(irq, irq::flags::SHARED, "my-device", handler)?;
1976 + let registration = ThreadedRegistration::register(irq, flags::SHARED, "my-device", handler)?;
|
--
>> error[E0432]: unresolved import `kernel::irq::Registration`
--> rust/doctests_kernel_generated.rs:1934:5
|
1934 | use kernel::irq::Registration;
| ^^^^^^^^^^^^^^^^^^^^^^^^^ no `Registration` in `irq`
|
help: consider importing one of these items instead
|
1934 | use kernel::irq::request::Registration;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1934 | use kernel::net::phy::Registration;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
>> error[E0432]: unresolved import `kernel::sync::lock::SpinLock`
--> rust/doctests_kernel_generated.rs:1936:5
|
1936 | use kernel::sync::lock::SpinLock;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `SpinLock` in `sync::lock`
|
help: a similar name exists in the module
|
1936 | use kernel::sync::lock::spinlock;
| ~~~~~~~~
help: consider importing this type alias instead
|
1936 | use kernel::sync::SpinLock;
| ~~~~~~~~~~~~~~~~~~~~~~
--
>> error[E0405]: cannot find trait `Handler` in module `kernel::irq`
--> rust/doctests_kernel_generated.rs:1855:19
|
1855 | impl kernel::irq::Handler for Handler {
| ^^^^^^^ not found in `kernel::irq`
|
help: consider importing this trait
|
3 + use kernel::irq::request::Handler;
|
help: if you import `Handler`, refer to it directly
|
1855 - impl kernel::irq::Handler for Handler {
1855 + impl Handler for Handler {
|
..
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Daniel,
A couple procedural things on your latest patches (no need to send new
versions right away).
On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida
<daniel.almeida@collabora.com> wrote:
>
> Both regular and threaded versions are supported.
I am not sure if the commit message was truncated, but it should
explain why the change is done and what is being changed. For
instance, it should mention who will need this upstream. Please see
other similar patches/series we have, e.g. one of the latest ones in Lore:
https://lore.kernel.org/rust-for-linux/20241022213221.2383-1-dakr@kernel.org/
> base-commit: 33c255312660653cf54f8019896b5dca28e3c580
Please try this one on top of `rust-next` -- you will get new Clippy
warnings about missing safety sections & comments.
Thanks!
Cheers,
Miguel
Hi Miguel, yes, my bad. > On 24 Oct 2024, at 12:05, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi Daniel, > > A couple procedural things on your latest patches (no need to send new > versions right away). > > On Thu, Oct 24, 2024 at 4:20 PM Daniel Almeida > <daniel.almeida@collabora.com> wrote: >> >> Both regular and threaded versions are supported. > > I am not sure if the commit message was truncated, but it should > explain why the change is done and what is being changed. For > instance, it should mention who will need this upstream. Please see > other similar patches/series we have, e.g. one of the latest ones in Lore: > > https://lore.kernel.org/rust-for-linux/20241022213221.2383-1-dakr@kernel.org/ > >> base-commit: 33c255312660653cf54f8019896b5dca28e3c580 > > Please try this one on top of `rust-next` -- you will get new Clippy > warnings about missing safety sections & comments. > > Thanks! > > Cheers, > Miguel — Daniel
© 2016 - 2024 Red Hat, Inc.