* [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA)
@ 2026-06-14 15:59 Maurice Hieronymus
2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Maurice Hieronymus @ 2026-06-14 15:59 UTC (permalink / raw)
To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
Onur Özkan
Cc: linux-pci, rust-for-linux, linux-kernel, Maurice Hieronymus
The Rust sample drivers currently exercise PCI facilities in isolation:
rust_driver_pci covers MMIO and rust_dma covers DMA, but there is no
single in-tree example that combines memory-mapped I/O, interrupts and
DMA in one driver.
This series adds one. It targets QEMU's `edu` device -- a small,
well-documented educational PCI device that supports exactly these three
facilities and ships with any recent QEMU (`-device edu`), so the sample
runs without special hardware.
The sample maps BAR0 and runs a set of MMIO self-tests (identification,
liveness, factorial), allocates an MSI vector and registers an IRQ
handler, and performs a DMA round-trip -- each stage waiting on a
Completion that the IRQ handler signals.
Prerequisites, that had to be implemented:
- pci: make Vendor::from_raw() public, so a driver can match a device
whose vendor ID has no symbolic name in pci_ids.h (QEMU's 0x1234),
matching what C drivers already do.
- pci: add a managed enable_device() wrapping pcim_enable_device(), so
the enable count stays balanced across unbind/rebind.
- completion: add complete(), so a single Completion can be reused to
wait for consecutive events (e.g. back-to-back DMA transfers).
Tested with QEMU `-device edu`;
Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
Maurice Hieronymus (4):
rust: pci: make Vendor::from_raw() public
rust: pci: add managed Device::enable_device()
rust: completion: add complete()
rust: samples: add EDU PCI driver sample
rust/kernel/pci.rs | 16 ++
rust/kernel/pci/id.rs | 2 +-
rust/kernel/sync/completion.rs | 14 ++
samples/rust/Kconfig | 11 ++
samples/rust/Makefile | 1 +
samples/rust/rust_driver_edu.rs | 379 ++++++++++++++++++++++++++++++++++++++++
6 files changed, 422 insertions(+), 1 deletion(-)
---
base-commit: 48b375e482027ba6566107cec40c1b21b453fa4e
change-id: 20260614-b4-rust-pci-edu-driver-3e50db2dda0f
Best regards,
--
Maurice Hieronymus <mhi@mailbox.org>
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/4] rust: pci: make Vendor::from_raw() public 2026-06-14 15:59 [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA) Maurice Hieronymus @ 2026-06-14 15:59 ` Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot 2026-06-14 16:47 ` Gary Guo 2026-06-14 15:59 ` [PATCH 2/4] rust: pci: add managed Device::enable_device() Maurice Hieronymus ` (2 subsequent siblings) 3 siblings, 2 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 15:59 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel, Maurice Hieronymus Vendor::from_raw() is currently pub(super), so a Vendor can only be obtained through the named constants generated from the PCI_VENDOR_ID_* defines in <linux/pci_ids.h>. A driver therefore cannot match a device whose vendor ID has no symbolic name. Such devices exist. QEMU's "edu" educational device and the legacy qemu/Bochs stdvga both use vendor ID 0x1234, which is not registered in pci_ids.h. Per the policy stated at the top of that header, IDs are only added there when shared between multiple drivers; a single-driver ID is expected to be open-coded in the driver instead. C drivers already do this -- see drivers/gpu/drm/tiny/bochs.c, which matches with a bare ".vendor = 0x1234". The Rust abstraction has no equivalent escape hatch: there is no public way to express an unregistered vendor. Make Vendor::from_raw() public (and const, so it can be used in the const device-ID tables built by pci_device_table!) so that drivers can construct a Vendor from a raw ID, matching what C drivers can already do. Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> --- rust/kernel/pci/id.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs index 50005d176561..bd3cf17fd8de 100644 --- a/rust/kernel/pci/id.rs +++ b/rust/kernel/pci/id.rs @@ -156,7 +156,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { impl Vendor { /// Create a Vendor from a raw 16-bit vendor ID. #[inline] - pub(super) fn from_raw(vendor_id: u16) -> Self { + pub const fn from_raw(vendor_id: u16) -> Self { Self(vendor_id) } -- 2.51.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] rust: pci: make Vendor::from_raw() public 2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus @ 2026-06-14 16:04 ` sashiko-bot 2026-06-14 16:47 ` Gary Guo 1 sibling, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-06-14 16:04 UTC (permalink / raw) To: Maurice Hieronymus; +Cc: linux-pci, ojeda > Vendor::from_raw() is currently pub(super), so a Vendor can only be > obtained through the named constants generated from the PCI_VENDOR_ID_* > defines in <linux/pci_ids.h>. A driver therefore cannot match a device > whose vendor ID has no symbolic name. > > Such devices exist. QEMU's "edu" educational device and the legacy > qemu/Bochs stdvga both use vendor ID 0x1234, which is not registered in > pci_ids.h. Per the policy stated at the top of that header, IDs are only > added there when shared between multiple drivers; a single-driver ID is > expected to be open-coded in the driver instead. C drivers already do > this -- see drivers/gpu/drm/tiny/bochs.c, which matches with a bare > ".vendor = 0x1234". > > The Rust abstraction has no equivalent escape hatch: there is no public > way to express an unregistered vendor. Make Vendor::from_raw() public (and > const, so it can be used in the const device-ID tables built by > pci_device_table!) so that drivers can construct a Vendor from a raw ID, > matching what C drivers can already do. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> Sashiko has reviewed this patch and found no issues. It looks great! -- Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-rust-pci-edu-driver-v1-0-e3f2471b595c@mailbox.org?part=1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] rust: pci: make Vendor::from_raw() public 2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot @ 2026-06-14 16:47 ` Gary Guo 1 sibling, 0 replies; 14+ messages in thread From: Gary Guo @ 2026-06-14 16:47 UTC (permalink / raw) To: Maurice Hieronymus, Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel On Sun Jun 14, 2026 at 4:59 PM BST, Maurice Hieronymus wrote: > Vendor::from_raw() is currently pub(super), so a Vendor can only be > obtained through the named constants generated from the PCI_VENDOR_ID_* > defines in <linux/pci_ids.h>. A driver therefore cannot match a device > whose vendor ID has no symbolic name. nit: quote these code blocks inside commit text for visual clarity > > Such devices exist. QEMU's "edu" educational device and the legacy > qemu/Bochs stdvga both use vendor ID 0x1234, which is not registered in > pci_ids.h. Per the policy stated at the top of that header, IDs are only > added there when shared between multiple drivers; a single-driver ID is > expected to be open-coded in the driver instead. C drivers already do > this -- see drivers/gpu/drm/tiny/bochs.c, which matches with a bare > ".vendor = 0x1234". > > The Rust abstraction has no equivalent escape hatch: there is no public > way to express an unregistered vendor. Make Vendor::from_raw() public (and > const, so it can be used in the const device-ID tables built by > pci_device_table!) so that drivers can construct a Vendor from a raw ID, > matching what C drivers can already do. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> Reviewed-by: Gary Guo <gary@garyguo.net> > --- > rust/kernel/pci/id.rs | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs > index 50005d176561..bd3cf17fd8de 100644 > --- a/rust/kernel/pci/id.rs > +++ b/rust/kernel/pci/id.rs > @@ -156,7 +156,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { > impl Vendor { > /// Create a Vendor from a raw 16-bit vendor ID. > #[inline] > - pub(super) fn from_raw(vendor_id: u16) -> Self { > + pub const fn from_raw(vendor_id: u16) -> Self { > Self(vendor_id) > } > ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] rust: pci: add managed Device::enable_device() 2026-06-14 15:59 [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA) Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus @ 2026-06-14 15:59 ` Maurice Hieronymus 2026-06-14 16:11 ` sashiko-bot 2026-06-14 19:06 ` Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 4/4] rust: samples: add EDU PCI driver sample Maurice Hieronymus 3 siblings, 2 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 15:59 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel, Maurice Hieronymus Add a managed counterpart to Device::enable_device_mem() that wraps pcim_enable_device(). In addition to enabling the device, it registers a pci_disable_device() cleanup that runs automatically when the device is unbound from its driver, keeping the device's enable count balanced across unbind/rebind cycles. The existing enable_device_mem() wraps the unmanaged pci_enable_device_mem() and has no disable counterpart, so the enable count is leaked on unbind. On the next probe pci_enable_device_flags() sees a non-zero enable count and returns early, skipping the power-state transition back to D0. For a device without a PCI power management capability the power state cannot be re-read from hardware and stays PCI_UNKNOWN, which makes __pci_enable_msi_range() reject the subsequent MSI allocation with -EINVAL. Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> --- rust/kernel/pci.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index af74ddff6114..ca04548c82c3 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -452,6 +452,22 @@ pub fn enable_device_mem(&self) -> Result { to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }) } + /// Enable I/O and memory resources for this device, with automatic cleanup. + /// + /// This is the managed version of `pci_enable_device()`: it enables the device's I/O and + /// memory resources and registers a `pci_disable_device()` call that runs automatically + /// when the device is unbound from its driver. In contrast, [`Device::enable_device_mem`] + /// is unmanaged and only enables memory resources. + /// + /// The automatic cleanup keeps the device's enable count balanced across driver + /// unbind/rebind cycles. With an unbalanced (leaked) enable count, a re-probe skips the + /// power-state transition back to `D0`, which makes subsequent MSI allocation fail with + /// `EINVAL`. + pub fn enable_device(&self) -> Result { + // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`. + to_result(unsafe { bindings::pcim_enable_device(self.as_raw()) }) + } + /// Enable bus-mastering for this device. #[inline] pub fn set_master(&self) { -- 2.51.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] rust: pci: add managed Device::enable_device() 2026-06-14 15:59 ` [PATCH 2/4] rust: pci: add managed Device::enable_device() Maurice Hieronymus @ 2026-06-14 16:11 ` sashiko-bot 2026-06-14 19:06 ` Maurice Hieronymus 1 sibling, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-06-14 16:11 UTC (permalink / raw) To: Maurice Hieronymus; +Cc: linux-pci, ojeda Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: New issues: - [High] Wrapping `pcim_enable_device()` in `Device::enable_device()` triggers a legacy C-side side effect that conflicts with Rust's explicit IRQ management, causing a double-free of IRQ vectors on driver unbind. - [Low] Missing `#[inline]` annotation on a small forwarding function in an abstraction crate. Pre-existing issues: - [High] The existing `Device::enable_device_mem()` method leaks the PCI device's enable count because it lacks a devres cleanup action or safe disable method. -- commit 418855d3e0788282fef9f3b016a6200435cea33d Author: Maurice Hieronymus <mhi@mailbox.org> rust: pci: add managed Device::enable_device() Add a managed counterpart to Device::enable_device_mem() that wraps pcim_enable_device(). In addition to enabling the device, it registers a pci_disable_device() cleanup that runs automatically when the device is unbound from its driver, keeping the device's enable count balanced across unbind/rebind cycles. > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index af74ddff6114d..ca04548c82c31 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -452,6 +452,22 @@ pub fn enable_device_mem(&self) -> Result { > to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) }) > } [Severity: High] This isn't a bug introduced by this patch, but does the existing Device::enable_device_mem() method leak the PCI device's enable count? Since this method wraps the unmanaged pci_enable_device_mem(), it increments the device's internal enable count. Safe Rust currently exposes no disable_device_mem method, and enable_device_mem does not register a devres cleanup action. It appears that any driver using this safe method will permanently leak the enable count on unbind. As noted in the commit message, this prevents the device from transitioning back to the D0 power state on re-probe, causing subsequent MSI allocations to fail. Should this be refactored to use devres for automatic cleanup, or be marked unsafe? > + > + /// Enable I/O and memory resources for this device, with automatic cleanup. > + /// [ ... ] > + pub fn enable_device(&self) -> Result { [Severity: Low] Should this function have an #[inline] annotation? The Rust guidelines for the kernel state that for abstractions, functions that are small or forwarding to a binding call should be annotated with #[inline]. Since Device::enable_device() simply forwards to the C binding pcim_enable_device(), it seems to fit this criteria. > + // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`. > + to_result(unsafe { bindings::pcim_enable_device(self.as_raw()) }) > + } [Severity: High] Does wrapping pcim_enable_device() here trigger a legacy C-side side effect that conflicts with Rust's explicit IRQ management? The function pcim_enable_device() sets pdev->is_managed = true. When a driver later allocates IRQs via the Rust method Device::alloc_irq_vectors(), the underlying C function pci_alloc_irq_vectors() detects this flag and automatically registers a C-side devres action (pcim_msi_release) to free the IRQ vectors. However, the Rust method unconditionally registers its own devres action which also drops them: rust/kernel/pci/irq.rs:IrqVectorRegistration::drop() { unsafe { bindings::pci_free_irq_vectors(self.dev.as_raw()) }; } During driver unbind, both devres actions would execute, causing pci_free_irq_vectors() to be called twice. Could this be resolved by having Device::enable_device() wrap the unmanaged pci_enable_device() instead, and explicitly register a Rust-side devres cleanup handler that calls pci_disable_device()? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-rust-pci-edu-driver-v1-0-e3f2471b595c@mailbox.org?part=2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] rust: pci: add managed Device::enable_device() 2026-06-14 15:59 ` [PATCH 2/4] rust: pci: add managed Device::enable_device() Maurice Hieronymus 2026-06-14 16:11 ` sashiko-bot @ 2026-06-14 19:06 ` Maurice Hieronymus 1 sibling, 0 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 19:06 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel On Sun, 2026-06-14 at 17:59 +0200, Maurice Hieronymus wrote: > Add a managed counterpart to Device::enable_device_mem() that wraps > pcim_enable_device(). In addition to enabling the device, it > registers a > pci_disable_device() cleanup that runs automatically when the device > is > unbound from its driver, keeping the device's enable count balanced > across unbind/rebind cycles. > > The existing enable_device_mem() wraps the unmanaged > pci_enable_device_mem() and has no disable counterpart, so the enable > count is leaked on unbind. On the next probe > pci_enable_device_flags() > sees a non-zero enable count and returns early, skipping the power- > state > transition back to D0. For a device without a PCI power management > capability the power state cannot be re-read from hardware and stays > PCI_UNKNOWN, which makes __pci_enable_msi_range() reject the > subsequent > MSI allocation with -EINVAL. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> > --- > rust/kernel/pci.rs | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index af74ddff6114..ca04548c82c3 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -452,6 +452,22 @@ pub fn enable_device_mem(&self) -> Result { > to_result(unsafe { > bindings::pci_enable_device_mem(self.as_raw()) }) > } > > + /// Enable I/O and memory resources for this device, with > automatic cleanup. > + /// > + /// This is the managed version of `pci_enable_device()`: it > enables the device's I/O and > + /// memory resources and registers a `pci_disable_device()` call > that runs automatically > + /// when the device is unbound from its driver. In contrast, > [`Device::enable_device_mem`] > + /// is unmanaged and only enables memory resources. > + /// > + /// The automatic cleanup keeps the device's enable count > balanced across driver > + /// unbind/rebind cycles. With an unbalanced (leaked) enable > count, a re-probe skips the > + /// power-state transition back to `D0`, which makes subsequent > MSI allocation fail with > + /// `EINVAL`. Will add #[inline] in V2. > + pub fn enable_device(&self) -> Result { > + // SAFETY: `self.as_raw` is guaranteed to be a pointer to a > valid `struct pci_dev`. > + to_result(unsafe { > bindings::pcim_enable_device(self.as_raw()) }) > + } > + > /// Enable bus-mastering for this device. > #[inline] > pub fn set_master(&self) { ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/4] rust: completion: add complete() 2026-06-14 15:59 [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA) Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 2/4] rust: pci: add managed Device::enable_device() Maurice Hieronymus @ 2026-06-14 15:59 ` Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot ` (2 more replies) 2026-06-14 15:59 ` [PATCH 4/4] rust: samples: add EDU PCI driver sample Maurice Hieronymus 3 siblings, 3 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 15:59 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel, Maurice Hieronymus The initial completion abstraction only added complete_all() and wait_for_completion(). complete_all() marks the completion permanently done, which makes a single Completion unsuitable for signalling the same event repeatedly: once complete_all() has run, every subsequent wait_for_completion() returns immediately without waiting. Add complete(), which wakes a single waiter and increments the internal counter by one. Paired one-to-one with wait_for_completion(), it allows the same completion to be reused across multiple cycles, e.g. to wait for consecutive DMA transfers to finish. Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> --- rust/kernel/sync/completion.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs index c50012a940a3..b386b84222f9 100644 --- a/rust/kernel/sync/completion.rs +++ b/rust/kernel/sync/completion.rs @@ -90,6 +90,20 @@ fn as_raw(&self) -> *mut bindings::completion { self.inner.get() } + /// Signal one task waiting on this completion. + /// + /// This method wakes up a single task waiting on this completion and increments the + /// internal counter by one. If no task is currently waiting, the next + /// [`Completion::wait_for_completion`] returns immediately. + /// + /// Unlike [`Completion::complete_all`], the completion is not marked permanently done, so a + /// single [`Completion`] paired one-to-one with [`Completion::wait_for_completion`] can be + /// reused to signal the same event repeatedly. + pub fn complete(&self) { + // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`. + unsafe { bindings::complete(self.as_raw()) }; + } + /// Signal all tasks waiting on this completion. /// /// This method wakes up all tasks waiting on this completion; after this operation the -- 2.51.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] rust: completion: add complete() 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus @ 2026-06-14 16:04 ` sashiko-bot 2026-06-14 17:38 ` Gary Guo 2026-06-14 19:07 ` Maurice Hieronymus 2 siblings, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-06-14 16:04 UTC (permalink / raw) To: Maurice Hieronymus; +Cc: linux-pci, ojeda Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] Missing `#[inline]` annotation on the `complete()` abstraction wrapper. -- commit 566fae28c64ca5359e88bcd074eba624bc6951cb Author: Maurice Hieronymus <mhi@mailbox.org> rust: completion: add complete() The initial completion abstraction only added complete_all() and wait_for_completion(). Add complete(), which wakes a single waiter and increments the internal counter by one. Paired one-to-one with wait_for_completion(), it allows the same completion to be reused across multiple cycles. > diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs > index c50012a940a3c..b386b84222f97 100644 > --- a/rust/kernel/sync/completion.rs > +++ b/rust/kernel/sync/completion.rs > @@ -90,6 +90,20 @@ fn as_raw(&self) -> *mut bindings::completion { > self.inner.get() > } > > + /// Signal one task waiting on this completion. > + /// > + /// This method wakes up a single task waiting on this completion and increments the > + /// internal counter by one. If no task is currently waiting, the next > + /// [`Completion::wait_for_completion`] returns immediately. > + /// > + /// Unlike [`Completion::complete_all`], the completion is not marked permanently done, so a > + /// single [`Completion`] paired one-to-one with [`Completion::wait_for_completion`] can be > + /// reused to signal the same event repeatedly. > + pub fn complete(&self) { [Severity: Low] This isn't a bug, but should this function have an #[inline] annotation? Since complete() is a small wrapper in an abstraction that merely forwards to the C binding bindings::complete(), adding #[inline] would match the guidelines for such functions. > + // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`. > + unsafe { bindings::complete(self.as_raw()) }; > + } > + > /// Signal all tasks waiting on this completion. > /// > /// This method wakes up all tasks waiting on this completion; after this operation the -- Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-rust-pci-edu-driver-v1-0-e3f2471b595c@mailbox.org?part=3 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] rust: completion: add complete() 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot @ 2026-06-14 17:38 ` Gary Guo 2026-06-14 19:07 ` Maurice Hieronymus 2 siblings, 0 replies; 14+ messages in thread From: Gary Guo @ 2026-06-14 17:38 UTC (permalink / raw) To: Maurice Hieronymus, Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel On Sun Jun 14, 2026 at 4:59 PM BST, Maurice Hieronymus wrote: > The initial completion abstraction only added complete_all() and > wait_for_completion(). complete_all() marks the completion permanently > done, which makes a single Completion unsuitable for signalling the same > event repeatedly: once complete_all() has run, every subsequent > wait_for_completion() returns immediately without waiting. > > Add complete(), which wakes a single waiter and increments the internal > counter by one. Paired one-to-one with wait_for_completion(), it allows > the same completion to be reused across multiple cycles, e.g. to wait for > consecutive DMA transfers to finish. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> > --- > rust/kernel/sync/completion.rs | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/rust/kernel/sync/completion.rs b/rust/kernel/sync/completion.rs > index c50012a940a3..b386b84222f9 100644 > --- a/rust/kernel/sync/completion.rs > +++ b/rust/kernel/sync/completion.rs > @@ -90,6 +90,20 @@ fn as_raw(&self) -> *mut bindings::completion { > self.inner.get() > } > > + /// Signal one task waiting on this completion. "Signal a single task" to put some emphasis on "single". > + /// > + /// This method wakes up a single task waiting on this completion and increments the > + /// internal counter by one. If no task is currently waiting, the next Why does this need to mention about its implementation detail? The user of the API doesn't need to care. > + /// [`Completion::wait_for_completion`] returns immediately. > + /// > + /// Unlike [`Completion::complete_all`], the completion is not marked permanently done, so a > + /// single [`Completion`] paired one-to-one with [`Completion::wait_for_completion`] can be > + /// reused to signal the same event repeatedly. Hmm, I don't think the behaviour of `complete_all` needs to be in the documentation of `complete`. On the other note, you probably should update documentation of `wait_for_completion` to also mention `complete`. Best, Gary > + pub fn complete(&self) { > + // SAFETY: `self.as_raw()` is a pointer to a valid `struct completion`. > + unsafe { bindings::complete(self.as_raw()) }; > + } > + > /// Signal all tasks waiting on this completion. > /// > /// This method wakes up all tasks waiting on this completion; after this operation the ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] rust: completion: add complete() 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot 2026-06-14 17:38 ` Gary Guo @ 2026-06-14 19:07 ` Maurice Hieronymus 2 siblings, 0 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 19:07 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel On Sun, 2026-06-14 at 17:59 +0200, Maurice Hieronymus wrote: > The initial completion abstraction only added complete_all() and > wait_for_completion(). complete_all() marks the completion > permanently > done, which makes a single Completion unsuitable for signalling the > same > event repeatedly: once complete_all() has run, every subsequent > wait_for_completion() returns immediately without waiting. > > Add complete(), which wakes a single waiter and increments the > internal > counter by one. Paired one-to-one with wait_for_completion(), it > allows > the same completion to be reused across multiple cycles, e.g. to wait > for > consecutive DMA transfers to finish. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> > --- > rust/kernel/sync/completion.rs | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/rust/kernel/sync/completion.rs > b/rust/kernel/sync/completion.rs > index c50012a940a3..b386b84222f9 100644 > --- a/rust/kernel/sync/completion.rs > +++ b/rust/kernel/sync/completion.rs > @@ -90,6 +90,20 @@ fn as_raw(&self) -> *mut bindings::completion { > self.inner.get() > } > > + /// Signal one task waiting on this completion. > + /// > + /// This method wakes up a single task waiting on this > completion and increments the > + /// internal counter by one. If no task is currently waiting, > the next > + /// [`Completion::wait_for_completion`] returns immediately. > + /// > + /// Unlike [`Completion::complete_all`], the completion is not > marked permanently done, so a > + /// single [`Completion`] paired one-to-one with > [`Completion::wait_for_completion`] can be > + /// reused to signal the same event repeatedly. Will add #[inline] in V2. > + pub fn complete(&self) { > + // SAFETY: `self.as_raw()` is a pointer to a valid `struct > completion`. > + unsafe { bindings::complete(self.as_raw()) }; > + } > + > /// Signal all tasks waiting on this completion. > /// > /// This method wakes up all tasks waiting on this completion; > after this operation the ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/4] rust: samples: add EDU PCI driver sample 2026-06-14 15:59 [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA) Maurice Hieronymus ` (2 preceding siblings ...) 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus @ 2026-06-14 15:59 ` Maurice Hieronymus 2026-06-14 16:16 ` sashiko-bot 2026-06-15 10:12 ` Ewan Chorynski 3 siblings, 2 replies; 14+ messages in thread From: Maurice Hieronymus @ 2026-06-14 15:59 UTC (permalink / raw) To: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan Cc: linux-pci, rust-for-linux, linux-kernel, Maurice Hieronymus Add a Rust sample driver for the QEMU EDU device, wired up via a new SAMPLE_RUST_DRIVER_EDU Kconfig option and the samples Makefile. Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> --- samples/rust/Kconfig | 11 ++ samples/rust/Makefile | 1 + samples/rust/rust_driver_edu.rs | 379 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 391 insertions(+) diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig index c49ab9106345..742c42262e9b 100644 --- a/samples/rust/Kconfig +++ b/samples/rust/Kconfig @@ -118,6 +118,17 @@ config SAMPLE_RUST_DRIVER_PCI If unsure, say N. +config SAMPLE_RUST_DRIVER_EDU + tristate "EDU Driver" + depends on PCI + help + This option builds the Rust EDU driver sample. + + To compile this as a module, choose M here: + the module will be called rust_driver_edu. + + If unsure, say N. + config SAMPLE_RUST_DRIVER_PLATFORM tristate "Platform Driver" help diff --git a/samples/rust/Makefile b/samples/rust/Makefile index 6c0aaa58cccc..c24a328243b1 100644 --- a/samples/rust/Makefile +++ b/samples/rust/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_SAMPLE_RUST_DMA) += rust_dma.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_I2C) += rust_driver_i2c.o obj-$(CONFIG_SAMPLE_RUST_I2C_CLIENT) += rust_i2c_client.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o +obj-$(CONFIG_SAMPLE_RUST_DRIVER_EDU) += rust_driver_edu.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB) += rust_driver_usb.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o diff --git a/samples/rust/rust_driver_edu.rs b/samples/rust/rust_driver_edu.rs new file mode 100644 index 000000000000..c092117473d3 --- /dev/null +++ b/samples/rust/rust_driver_edu.rs @@ -0,0 +1,379 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Rust EDU driver sample (based on QEMU's `edu`). +//! +//! To make this driver probe, QEMU must be run with `-device edu`. + +use kernel::{ + device::Bound, + devres::Devres, + dma::{Coherent, Device, DmaMask}, + io::{ + poll::read_poll_timeout, + register, + Io, // + }, + irq::{self, Flags}, + pci::{self, IrqTypes}, + prelude::*, + sync::{aref::ARef, Arc, Completion}, + time::Delta, // +}; + +const QEMU_VENDOR_ID: u16 = 0x1234; +const QEMU_EDU_DEVICE_ID: u32 = 0x11e8; +const QEMU_EDU_DEVICE_MAGIC: u8 = 0xed; +const QEMU_DMA_BASE: u64 = 0x40000; + +const IRQ_MAGIC_VALUE: u32 = 42; + +/// Bit set in `IRQ_STATUS` when a DMA transfer has completed. +const DMA_IRQ: u32 = 0x100; + +mod regs { + use super::*; + + register! { + pub(super) IDENTIFICATION(u32) @ 0x0 { + 31:24 major; + 23:16 minor; + 7:0 magic; + } + + pub(super) LIVENESS_CHECK(u32) @ 0x04 {} + + pub(super) FACTORIAL(u32) @ 0x08 {} + + pub(super) STATUS(u32) @ 0x20 { + 0:0 computing; + 7:7 raise_interrupt; + } + + pub(super) IRQ_STATUS(u32) @ 0x24 {} + pub(super) IRQ_RAISE(u32) @ 0x60 {} + pub(super) IRQ_ACK(u32) @ 0x64 {} + + pub(super) DMA_SRC(u64) @ 0x80 {} + pub(super) DMA_DST(u64) @ 0x88 {} + pub(super) DMA_COUNT(u64) @ 0x90 {} + pub(super) DMA_COMMAND(u64) @ 0x98 { + 0:0 start_transfer; + 1:1 direction; + 2:2 raise_irq; + } + } + + pub(super) const END: usize = 0xA0; +} + +type Bar0 = pci::Bar<{ regs::END }>; + +#[pin_data(PinnedDrop)] +struct EduDriver { + pdev: ARef<pci::Device>, + data: Arc<EduDriverData>, + #[pin] + irq_handler: irq::Registration<Arc<EduDriverData>>, +} + +#[pin_data] +struct EduDriverData { + #[pin] + bar: Devres<Bar0>, + #[pin] + irq_test_completion: Completion, + #[pin] + irq_dma_completion: Completion, + dma: Coherent<u64>, +} + +impl EduDriver { + fn init(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { + Self::magic(pdev, bar)?; + Self::liveness_check(pdev, bar)?; + Self::factorial(pdev, bar)?; + Self::test_irq(pdev, bar, data)?; + Self::test_dma(pdev, bar, data)?; + Ok(()) + } + + fn magic(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { + let identification = bar.read(regs::IDENTIFICATION); + + let magic: u8 = identification.magic().into(); + + if magic != QEMU_EDU_DEVICE_MAGIC { + dev_err!( + pdev, + "magic mismatch: expected {:#x} got {:#x}\n", + QEMU_EDU_DEVICE_MAGIC, + magic + ); + return Err(ENODEV); + } + + dev_info!( + pdev, + "major: {:#x} minor: {:#x}\n", + identification.major(), + identification.minor() + ); + Ok(()) + } + + fn liveness_check(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { + let test_value = 0xabcd; + + bar.write(regs::LIVENESS_CHECK, test_value.into()); + + let inverse_value = bar.read(regs::LIVENESS_CHECK).into_raw(); + + if inverse_value != !test_value { + dev_err!( + pdev, + "inverse mismatch: expected {:#x} got {:#x}\n", + !test_value, + inverse_value + ); + return Err(ENODEV); + } + + dev_info!(pdev, "inverse test successful\n"); + Ok(()) + } + + fn factorial(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { + Self::wait_until_compute_has_finished(pdev, bar)?; + + bar.write(regs::FACTORIAL, 4.into()); + + Self::wait_until_compute_has_finished(pdev, bar)?; + + let result: u32 = bar.read(regs::FACTORIAL).into(); + + let expected = 24; + + if result != expected { + dev_err!( + pdev, + "factorial result wrong: expected {} got {}\n", + expected, + result + ); + return Err(ENODEV); + } + + dev_info!(pdev, "factorial test successful\n"); + Ok(()) + } + + fn test_irq(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { + dev_dbg!(pdev, "raising irq\n"); + + bar.write(regs::IRQ_RAISE, IRQ_MAGIC_VALUE.into()); + + data.irq_test_completion.wait_for_completion(); + Ok(()) + } + + fn test_dma(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { + dev_dbg!(pdev, "testing dma\n"); + + let dma = &data.dma; + + const DMA_VALUE: u64 = 42; + + kernel::dma_write!(dma, , DMA_VALUE); + + bar.write(regs::DMA_SRC, dma.dma_handle().into()); + bar.write(regs::DMA_DST, QEMU_DMA_BASE.into()); + bar.write(regs::DMA_COUNT, (dma.size() as u64).into()); + bar.write( + regs::DMA_COMMAND, + regs::DMA_COMMAND::zeroed() + .with_start_transfer(true) + .with_direction(false) + .with_raise_irq(true), + ); + + data.irq_dma_completion.wait_for_completion(); + + // Destroy previous value to test roundtrip + kernel::dma_write!(dma, , 0); + + bar.write(regs::DMA_SRC, QEMU_DMA_BASE.into()); + bar.write(regs::DMA_DST, dma.dma_handle().into()); + bar.write(regs::DMA_COUNT, (dma.size() as u64).into()); + bar.write( + regs::DMA_COMMAND, + regs::DMA_COMMAND::zeroed() + .with_start_transfer(true) + .with_direction(true) + .with_raise_irq(true), + ); + + data.irq_dma_completion.wait_for_completion(); + + let result = kernel::dma_read!(dma,); + + if result != DMA_VALUE { + dev_err!( + pdev, + "dma result wrong: expected {} got {}\n", + DMA_VALUE, + result + ); + return Err(ENODEV); + } + + dev_info!(pdev, "dma test successful\n"); + Ok(()) + } + + fn wait_until_compute_has_finished(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { + if read_poll_timeout( + || Ok(bar.read(regs::STATUS)), + |status| status.computing() == 0, + Delta::from_millis(10), + Delta::from_millis(100), + ) + .is_err() + { + dev_err!(pdev, "computation bit did not clear before timeout\n"); + return Err(ETIMEDOUT); + } + Ok(()) + } +} + +impl pci::Driver for EduDriver { + type IdInfo = (); + + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; + + fn probe( + pdev: &pci::Device<kernel::device::Core>, + _id_info: &Self::IdInfo, + ) -> impl PinInit<Self, Error> { + pin_init::pin_init_scope(move || { + let vendor = pdev.vendor_id(); + dev_dbg!( + pdev, + "Probe Rust EDU driver sample (PCI ID: {}, 0x{:x}).\n", + vendor, + pdev.device_id() + ); + + pdev.enable_device()?; + pdev.set_master(); + + let mask = DmaMask::new::<28>(); + + // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives. + unsafe { pdev.dma_set_mask_and_coherent(mask)? }; + + let ca: Coherent<u64> = Coherent::zeroed(pdev.as_ref(), GFP_KERNEL)?; + + let irq = pdev + .alloc_irq_vectors(1, 1, IrqTypes::default().with(pci::IrqType::Msi)) + .inspect_err(|e| dev_err!(pdev, "alloc_irq_vectors failed: {:?}\n", e))?; + + // State shared with the IRQ handler (the BAR and the completion the + // handler signals) lives in an `Arc<EduDriverData>`. `EduDriverData` + // itself implements `irq::Handler`, and the registration takes an + // `Arc<T>` via the `impl Handler for Arc<T>` blanket impl. This keeps + // the handler's state out of `EduDriver` and avoids a self-reference. + let data = Arc::pin_init( + try_pin_init!(EduDriverData { + bar <- pdev.iomap_region_sized(0, c"rust_driver_edu"), + irq_test_completion <- Completion::new(), + irq_dma_completion <- Completion::new(), + dma: ca, + }), + GFP_KERNEL, + )?; + + let req = irq::Registration::new( + (*irq.start()).try_into()?, + Flags::TRIGGER_NONE, + c"rust_edu_irq", + Ok(data.clone()), + ); + + // Ordering matters: the handler is registered (`irq_handler <- req`) + // *before* the `_:` block runs the self-tests, one of which raises an + // interrupt and waits for the handler. Raising before the handler is + // registered would hang (the completion is never signalled). + Ok(try_pin_init!(Self { + irq_handler <- req, + // Side-effect block: run the staged self-tests against the mapped + // BAR now that the handler is live. A failure here aborts probe. + _: { + let bar = data.bar.access(pdev.as_ref())?; + EduDriver::init(pdev, bar, &data)?; + dev_info!( + pdev, + "rust_driver_edu successfully initialized\n", + ); + }, + data, + pdev: pdev.into() + })) + }) + } +} + +impl irq::Handler for EduDriverData { + fn handle(&self, pdev: &kernel::device::Device<Bound>) -> irq::IrqReturn { + dev_dbg!(pdev, "irq handler called\n"); + // `access()` only fails on device mismatch, so this branch is + // structurally unreachable here, but it must be handled. + let Ok(bar) = self.bar.access(pdev.as_ref()) else { + dev_err!(pdev, "cannot access bar register inside irq handler\n"); + return irq::IrqReturn::None; + }; + let status: u32 = bar.read(regs::IRQ_STATUS).into(); + + // DMA_IRQ + if status & DMA_IRQ != 0 { + dev_dbg!(pdev, "handling dma completion in irq\n"); + bar.write(regs::IRQ_ACK, DMA_IRQ.into()); + self.irq_dma_completion.complete(); + } + + // TEST_IRQ + let magic = status & !DMA_IRQ; + if magic == IRQ_MAGIC_VALUE { + dev_dbg!(pdev, "handling test completion in irq\n"); + bar.write(regs::IRQ_ACK, magic.into()); + self.irq_test_completion.complete(); + } + + irq::IrqReturn::Handled + } +} + +#[pinned_drop] +impl PinnedDrop for EduDriver { + fn drop(self: Pin<&mut Self>) { + dev_dbg!(self.pdev, "Remove Rust EDU driver sample.\n"); + } +} + +kernel::pci_device_table!( + PCI_TABLE, + MODULE_PCI_TABLE, + <EduDriver as pci::Driver>::IdInfo, + [( + pci::DeviceId::from_id(pci::Vendor::from_raw(QEMU_VENDOR_ID), QEMU_EDU_DEVICE_ID), + () + )] +); + +kernel::module_pci_driver! { + type: EduDriver, + name: "rust_driver_edu", + authors: ["Maurice Hieronymus"], + description: "Rust EDU driver", + license: "GPL v2", +} -- 2.51.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] rust: samples: add EDU PCI driver sample 2026-06-14 15:59 ` [PATCH 4/4] rust: samples: add EDU PCI driver sample Maurice Hieronymus @ 2026-06-14 16:16 ` sashiko-bot 2026-06-15 10:12 ` Ewan Chorynski 1 sibling, 0 replies; 14+ messages in thread From: sashiko-bot @ 2026-06-14 16:16 UTC (permalink / raw) To: Maurice Hieronymus; +Cc: linux-pci, ojeda Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] The IRQ handler unconditionally returns `irq::IrqReturn::Handled` even if no interrupt was serviced. - [High] The driver blocks indefinitely in `probe()` waiting for hardware interrupts, risking a system hang. - [High] An unacknowledged factorial interrupt causes the subsequent IRQ test to fail and hang indefinitely. - [High] Leftover interrupts or spurious IRQs trigger premature completions, causing a data race with active DMA transfers. - [High] The allocated PCI IRQ vectors are prematurely freed before the driver is fully initialized, permanently breaking interrupts. -- commit 028e22726596441f18ba4b799ddf50110e879fa5 Author: Maurice Hieronymus <mhi@mailbox.org> rust: samples: add EDU PCI driver sample Add a Rust sample driver for the QEMU EDU device, wired up via a new SAMPLE_RUST_DRIVER_EDU Kconfig option and the samples Makefile. > diff --git a/samples/rust/rust_driver_edu.rs b/samples/rust/rust_driver_edu.rs > new file mode 100644 > index 0000000000000..c092117473d3d > --- /dev/null > +++ b/samples/rust/rust_driver_edu.rs [ ... ] > + fn factorial(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { > + Self::wait_until_compute_has_finished(pdev, bar)?; > + > + bar.write(regs::FACTORIAL, 4.into()); > + > + Self::wait_until_compute_has_finished(pdev, bar)?; > + > + let result: u32 = bar.read(regs::FACTORIAL).into(); [Severity: High] Does this code leave the FACT_IRQ bit set in IRQ_STATUS without acknowledging it? Since IRQ_ACK is never written to clear this completion, could the leftover unacknowledged interrupt cause the strict equality check magic == IRQ_MAGIC_VALUE to fail later in the IRQ handler, and block test_irq() indefinitely? > + > + let expected = 24; [ ... ] > + fn test_irq(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { > + dev_dbg!(pdev, "raising irq\n"); > + > + bar.write(regs::IRQ_RAISE, IRQ_MAGIC_VALUE.into()); > + > + data.irq_test_completion.wait_for_completion(); [Severity: High] Can this block probe() indefinitely? If the hardware fails to raise the interrupt, wouldn't waiting without a timeout stall the driver probe sequence and potentially lock up subsystem initialization? > + Ok(()) > + } > + > + fn test_dma(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { [ ... ] > + bar.write( > + regs::DMA_COMMAND, > + regs::DMA_COMMAND::zeroed() > + .with_start_transfer(true) > + .with_direction(false) > + .with_raise_irq(true), > + ); > + > + data.irq_dma_completion.wait_for_completion(); [Severity: High] As with test_irq(), can this unbounded wait hang the system if the DMA interrupt never arrives? > + > + // Destroy previous value to test roundtrip > + kernel::dma_write!(dma, , 0); > + > + bar.write(regs::DMA_SRC, QEMU_DMA_BASE.into()); > + bar.write(regs::DMA_DST, dma.dma_handle().into()); > + bar.write(regs::DMA_COUNT, (dma.size() as u64).into()); > + bar.write( > + regs::DMA_COMMAND, > + regs::DMA_COMMAND::zeroed() > + .with_start_transfer(true) > + .with_direction(true) > + .with_raise_irq(true), > + ); > + > + data.irq_dma_completion.wait_for_completion(); [Severity: High] Could this return immediately due to stale completion counts? If there are leftover interrupts from a previous module load or a spurious IRQ inflates the completion count before this second transfer, wait_for_completion() might not wait for the active transfer. Could this result in a data race with the active DMA transfer? > + > + let result = kernel::dma_read!(dma,); [ ... ] > + fn probe( > + pdev: &pci::Device<kernel::device::Core>, > + _id_info: &Self::IdInfo, > + ) -> impl PinInit<Self, Error> { > + pin_init::pin_init_scope(move || { [ ... ] > + let irq = pdev > + .alloc_irq_vectors(1, 1, IrqTypes::default().with(pci::IrqType::Msi)) > + .inspect_err(|e| dev_err!(pdev, "alloc_irq_vectors failed: {:?}\n", e))?; > + > + // State shared with the IRQ handler (the BAR and the completion the > + // handler signals) lives in an `Arc<EduDriverData>`. `EduDriverData` [ ... ] > + Ok(try_pin_init!(Self { > + irq_handler <- req, > + // Side-effect block: run the staged self-tests against the mapped > + // BAR now that the handler is live. A failure here aborts probe. > + _: { > + let bar = data.bar.access(pdev.as_ref())?; > + EduDriver::init(pdev, bar, &data)?; [Severity: High] Are the allocated IRQ vectors prematurely freed before this block executes? Since irq is a local variable in the closure passed to pin_init_scope(), won't its Drop implementation run and free the vectors when the closure returns? This would happen before the initializer is pinned and the side-effect block runs EduDriver::init(), causing the self-tests to trigger device events on freed IRQ vectors. > + dev_info!( > + pdev, > + "rust_driver_edu successfully initialized\n", > + ); > + }, > + data, > + pdev: pdev.into() > + })) > + }) > + } > +} > + > +impl irq::Handler for EduDriverData { > + fn handle(&self, pdev: &kernel::device::Device<Bound>) -> irq::IrqReturn { [ ... ] > + // TEST_IRQ > + let magic = status & !DMA_IRQ; > + if magic == IRQ_MAGIC_VALUE { > + dev_dbg!(pdev, "handling test completion in irq\n"); > + bar.write(regs::IRQ_ACK, magic.into()); > + self.irq_test_completion.complete(); > + } > + > + irq::IrqReturn::Handled [Severity: High] Will returning irq::IrqReturn::Handled unconditionally circumvent the kernel's spurious interrupt detection mechanism? If IRQ_STATUS is 0 or no specific interrupt source is found, wouldn't claiming to handle the interrupt cause an infinite IRQ storm instead of correctly returning irq::IrqReturn::None? > + } > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/20260614-b4-rust-pci-edu-driver-v1-0-e3f2471b595c@mailbox.org?part=4 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] rust: samples: add EDU PCI driver sample 2026-06-14 15:59 ` [PATCH 4/4] rust: samples: add EDU PCI driver sample Maurice Hieronymus 2026-06-14 16:16 ` sashiko-bot @ 2026-06-15 10:12 ` Ewan Chorynski 1 sibling, 0 replies; 14+ messages in thread From: Ewan Chorynski @ 2026-06-15 10:12 UTC (permalink / raw) To: Maurice Hieronymus Cc: Danilo Krummrich, Bjorn Helgaas, Krzysztof Wilczyński, Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot, Onur Özkan, linux-pci, rust-for-linux, linux-kernel Le 2026-06-14T17:59:48.000+02:00, Maurice Hieronymus <mhi@mailbox.org> a écrit : > Add a Rust sample driver for the QEMU EDU device, wired up via a new > SAMPLE_RUST_DRIVER_EDU Kconfig option and the samples Makefile. > > Signed-off-by: Maurice Hieronymus <mhi@mailbox.org> > --- > samples/rust/Kconfig | 11 ++ > samples/rust/Makefile | 1 + > samples/rust/rust_driver_edu.rs | 379 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 391 insertions(+) > > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > index c49ab9106345..742c42262e9b 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -118,6 +118,17 @@ config SAMPLE_RUST_DRIVER_PCI > > If unsure, say N. > > +config SAMPLE_RUST_DRIVER_EDU > + tristate "EDU Driver" > + depends on PCI > + help > + This option builds the Rust EDU driver sample. > + > + To compile this as a module, choose M here: > + the module will be called rust_driver_edu. > + > + If unsure, say N. > + > config SAMPLE_RUST_DRIVER_PLATFORM > tristate "Platform Driver" > help > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > index 6c0aaa58cccc..c24a328243b1 100644 > --- a/samples/rust/Makefile > +++ b/samples/rust/Makefile > @@ -10,6 +10,7 @@ obj-$(CONFIG_SAMPLE_RUST_DMA) += rust_dma.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_I2C) += rust_driver_i2c.o > obj-$(CONFIG_SAMPLE_RUST_I2C_CLIENT) += rust_i2c_client.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_EDU) += rust_driver_edu.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_USB) += rust_driver_usb.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o > diff --git a/samples/rust/rust_driver_edu.rs b/samples/rust/rust_driver_edu.rs > new file mode 100644 > index 000000000000..c092117473d3 > --- /dev/null > +++ b/samples/rust/rust_driver_edu.rs > @@ -0,0 +1,379 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Rust EDU driver sample (based on QEMU's `edu`). > +//! > +//! To make this driver probe, QEMU must be run with `-device edu`. > + > +use kernel::{ > + device::Bound, > + devres::Devres, > + dma::{Coherent, Device, DmaMask}, > + io::{ > + poll::read_poll_timeout, > + register, > + Io, // > + }, > + irq::{self, Flags}, > + pci::{self, IrqTypes}, > + prelude::*, > + sync::{aref::ARef, Arc, Completion}, > + time::Delta, // > +}; > + > +const QEMU_VENDOR_ID: u16 = 0x1234; > +const QEMU_EDU_DEVICE_ID: u32 = 0x11e8; > +const QEMU_EDU_DEVICE_MAGIC: u8 = 0xed; > +const QEMU_DMA_BASE: u64 = 0x40000; > + > +const IRQ_MAGIC_VALUE: u32 = 42; > + > +/// Bit set in `IRQ_STATUS` when a DMA transfer has completed. > +const DMA_IRQ: u32 = 0x100; > + > +mod regs { > + use super::*; > + > + register! { > + pub(super) IDENTIFICATION(u32) @ 0x0 { > + 31:24 major; > + 23:16 minor; > + 7:0 magic; > + } > + > + pub(super) LIVENESS_CHECK(u32) @ 0x04 {} > + > + pub(super) FACTORIAL(u32) @ 0x08 {} > + > + pub(super) STATUS(u32) @ 0x20 { > + 0:0 computing; > + 7:7 raise_interrupt; > + } > + > + pub(super) IRQ_STATUS(u32) @ 0x24 {} > + pub(super) IRQ_RAISE(u32) @ 0x60 {} > + pub(super) IRQ_ACK(u32) @ 0x64 {} > + > + pub(super) DMA_SRC(u64) @ 0x80 {} > + pub(super) DMA_DST(u64) @ 0x88 {} > + pub(super) DMA_COUNT(u64) @ 0x90 {} > + pub(super) DMA_COMMAND(u64) @ 0x98 { > + 0:0 start_transfer; > + 1:1 direction; > + 2:2 raise_irq; > + } > + } > + > + pub(super) const END: usize = 0xA0; > +} > + > +type Bar0 = pci::Bar<{ regs::END }>; > + > +#[pin_data(PinnedDrop)] > +struct EduDriver { > + pdev: ARef<pci::Device>, > + data: Arc<EduDriverData>, > + #[pin] > + irq_handler: irq::Registration<Arc<EduDriverData>>, > +} > + > +#[pin_data] > +struct EduDriverData { > + #[pin] > + bar: Devres<Bar0>, > + #[pin] > + irq_test_completion: Completion, > + #[pin] > + irq_dma_completion: Completion, > + dma: Coherent<u64>, > +} > + > +impl EduDriver { > + fn init(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { You can pass &EduDriverData instead > + Self::magic(pdev, bar)?; > + Self::liveness_check(pdev, bar)?; > + Self::factorial(pdev, bar)?; > + Self::test_irq(pdev, bar, data)?; > + Self::test_dma(pdev, bar, data)?; > + Ok(()) > + } > + > + fn magic(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { > + let identification = bar.read(regs::IDENTIFICATION); > + > + let magic: u8 = identification.magic().into(); > + > + if magic != QEMU_EDU_DEVICE_MAGIC { > + dev_err!( > + pdev, > + "magic mismatch: expected {:#x} got {:#x}\n", > + QEMU_EDU_DEVICE_MAGIC, > + magic > + ); > + return Err(ENODEV); > + } > + > + dev_info!( > + pdev, > + "major: {:#x} minor: {:#x}\n", > + identification.major(), > + identification.minor() > + ); > + Ok(()) > + } > + > + fn liveness_check(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { > + let test_value = 0xabcd; > + > + bar.write(regs::LIVENESS_CHECK, test_value.into()); > + > + let inverse_value = bar.read(regs::LIVENESS_CHECK).into_raw(); > + > + if inverse_value != !test_value { > + dev_err!( > + pdev, > + "inverse mismatch: expected {:#x} got {:#x}\n", > + !test_value, > + inverse_value > + ); > + return Err(ENODEV); > + } > + > + dev_info!(pdev, "inverse test successful\n"); > + Ok(()) > + } > + > + fn factorial(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { > + Self::wait_until_compute_has_finished(pdev, bar)?; > + > + bar.write(regs::FACTORIAL, 4.into()); > + > + Self::wait_until_compute_has_finished(pdev, bar)?; > + > + let result: u32 = bar.read(regs::FACTORIAL).into(); > + > + let expected = 24; > + > + if result != expected { > + dev_err!( > + pdev, > + "factorial result wrong: expected {} got {}\n", > + expected, > + result > + ); > + return Err(ENODEV); > + } > + > + dev_info!(pdev, "factorial test successful\n"); > + Ok(()) > + } > + > + fn test_irq(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { Same here > + dev_dbg!(pdev, "raising irq\n"); > + > + bar.write(regs::IRQ_RAISE, IRQ_MAGIC_VALUE.into()); > + > + data.irq_test_completion.wait_for_completion(); > + Ok(()) > + } > + > + fn test_dma(pdev: &pci::Device<Bound>, bar: &Bar0, data: &Arc<EduDriverData>) -> Result { And here > + dev_dbg!(pdev, "testing dma\n"); > + > + let dma = &data.dma; > + > + const DMA_VALUE: u64 = 42; > + > + kernel::dma_write!(dma, , DMA_VALUE); > + > + bar.write(regs::DMA_SRC, dma.dma_handle().into()); > + bar.write(regs::DMA_DST, QEMU_DMA_BASE.into()); > + bar.write(regs::DMA_COUNT, (dma.size() as u64).into()); > + bar.write( > + regs::DMA_COMMAND, > + regs::DMA_COMMAND::zeroed() > + .with_start_transfer(true) > + .with_direction(false) > + .with_raise_irq(true), > + ); > + > + data.irq_dma_completion.wait_for_completion(); > + > + // Destroy previous value to test roundtrip > + kernel::dma_write!(dma, , 0); > + > + bar.write(regs::DMA_SRC, QEMU_DMA_BASE.into()); > + bar.write(regs::DMA_DST, dma.dma_handle().into()); > + bar.write(regs::DMA_COUNT, (dma.size() as u64).into()); > + bar.write( > + regs::DMA_COMMAND, > + regs::DMA_COMMAND::zeroed() > + .with_start_transfer(true) > + .with_direction(true) > + .with_raise_irq(true), > + ); > + > + data.irq_dma_completion.wait_for_completion(); > + > + let result = kernel::dma_read!(dma,); > + > + if result != DMA_VALUE { > + dev_err!( > + pdev, > + "dma result wrong: expected {} got {}\n", > + DMA_VALUE, > + result > + ); > + return Err(ENODEV); > + } > + > + dev_info!(pdev, "dma test successful\n"); > + Ok(()) > + } > + > + fn wait_until_compute_has_finished(pdev: &pci::Device<Bound>, bar: &Bar0) -> Result { > + if read_poll_timeout( > + || Ok(bar.read(regs::STATUS)), > + |status| status.computing() == 0, > + Delta::from_millis(10), > + Delta::from_millis(100), > + ) > + .is_err() > + { > + dev_err!(pdev, "computation bit did not clear before timeout\n"); > + return Err(ETIMEDOUT); > + } You can replace the if block with read_poll_timeout(...).inspect_err(|_| dev_err!(pdev, ...))?; > + Ok(()) > + } > +} > + > +impl pci::Driver for EduDriver { > + type IdInfo = (); > + > + const ID_TABLE: pci::IdTable<Self::IdInfo> = &PCI_TABLE; > + > + fn probe( > + pdev: &pci::Device<kernel::device::Core>, > + _id_info: &Self::IdInfo, > + ) -> impl PinInit<Self, Error> { > + pin_init::pin_init_scope(move || { > + let vendor = pdev.vendor_id(); > + dev_dbg!( > + pdev, > + "Probe Rust EDU driver sample (PCI ID: {}, 0x{:x}).\n", > + vendor, > + pdev.device_id() > + ); > + > + pdev.enable_device()?; > + pdev.set_master(); > + > + let mask = DmaMask::new::<28>(); > + > + // SAFETY: There are no concurrent calls to DMA allocation and mapping primitives. > + unsafe { pdev.dma_set_mask_and_coherent(mask)? }; > + > + let ca: Coherent<u64> = Coherent::zeroed(pdev.as_ref(), GFP_KERNEL)?; > + > + let irq = pdev > + .alloc_irq_vectors(1, 1, IrqTypes::default().with(pci::IrqType::Msi)) > + .inspect_err(|e| dev_err!(pdev, "alloc_irq_vectors failed: {:?}\n", e))?; > + > + // State shared with the IRQ handler (the BAR and the completion the > + // handler signals) lives in an `Arc<EduDriverData>`. `EduDriverData` > + // itself implements `irq::Handler`, and the registration takes an > + // `Arc<T>` via the `impl Handler for Arc<T>` blanket impl. This keeps > + // the handler's state out of `EduDriver` and avoids a self-reference. > + let data = Arc::pin_init( > + try_pin_init!(EduDriverData { > + bar <- pdev.iomap_region_sized(0, c"rust_driver_edu"), > + irq_test_completion <- Completion::new(), > + irq_dma_completion <- Completion::new(), > + dma: ca, > + }), > + GFP_KERNEL, > + )?; > + > + let req = irq::Registration::new( > + (*irq.start()).try_into()?, > + Flags::TRIGGER_NONE, > + c"rust_edu_irq", > + Ok(data.clone()), > + ); > + > + // Ordering matters: the handler is registered (`irq_handler <- req`) > + // *before* the `_:` block runs the self-tests, one of which raises an > + // interrupt and waits for the handler. Raising before the handler is > + // registered would hang (the completion is never signalled). > + Ok(try_pin_init!(Self { > + irq_handler <- req, > + // Side-effect block: run the staged self-tests against the mapped > + // BAR now that the handler is live. A failure here aborts probe. > + _: { > + let bar = data.bar.access(pdev.as_ref())?; > + EduDriver::init(pdev, bar, &data)?; > + dev_info!( > + pdev, > + "rust_driver_edu successfully initialized\n", > + ); > + }, > + data, > + pdev: pdev.into() > + })) > + }) > + } > +} > + > +impl irq::Handler for EduDriverData { > + fn handle(&self, pdev: &kernel::device::Device<Bound>) -> irq::IrqReturn { > + dev_dbg!(pdev, "irq handler called\n"); > + // `access()` only fails on device mismatch, so this branch is > + // structurally unreachable here, but it must be handled. > + let Ok(bar) = self.bar.access(pdev.as_ref()) else { > + dev_err!(pdev, "cannot access bar register inside irq handler\n"); > + return irq::IrqReturn::None; > + }; > + let status: u32 = bar.read(regs::IRQ_STATUS).into(); > + > + // DMA_IRQ > + if status & DMA_IRQ != 0 { > + dev_dbg!(pdev, "handling dma completion in irq\n"); > + bar.write(regs::IRQ_ACK, DMA_IRQ.into()); > + self.irq_dma_completion.complete(); > + } > + > + // TEST_IRQ > + let magic = status & !DMA_IRQ; > + if magic == IRQ_MAGIC_VALUE { > + dev_dbg!(pdev, "handling test completion in irq\n"); > + bar.write(regs::IRQ_ACK, magic.into()); > + self.irq_test_completion.complete(); > + } > + > + irq::IrqReturn::Handled > + } > +} > + > +#[pinned_drop] > +impl PinnedDrop for EduDriver { > + fn drop(self: Pin<&mut Self>) { > + dev_dbg!(self.pdev, "Remove Rust EDU driver sample.\n"); > + } > +} > + > +kernel::pci_device_table!( > + PCI_TABLE, > + MODULE_PCI_TABLE, > + <EduDriver as pci::Driver>::IdInfo, > + [( > + pci::DeviceId::from_id(pci::Vendor::from_raw(QEMU_VENDOR_ID), QEMU_EDU_DEVICE_ID), > + () > + )] > +); > + > +kernel::module_pci_driver! { > + type: EduDriver, > + name: "rust_driver_edu", > + authors: ["Maurice Hieronymus"], > + description: "Rust EDU driver", > + license: "GPL v2", > +} > > -- Regards ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-06-15 10:12 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-06-14 15:59 [PATCH 0/4] rust: samples: add an EDU PCI driver sample (MMIO + IRQ + DMA) Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 1/4] rust: pci: make Vendor::from_raw() public Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot 2026-06-14 16:47 ` Gary Guo 2026-06-14 15:59 ` [PATCH 2/4] rust: pci: add managed Device::enable_device() Maurice Hieronymus 2026-06-14 16:11 ` sashiko-bot 2026-06-14 19:06 ` Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 3/4] rust: completion: add complete() Maurice Hieronymus 2026-06-14 16:04 ` sashiko-bot 2026-06-14 17:38 ` Gary Guo 2026-06-14 19:07 ` Maurice Hieronymus 2026-06-14 15:59 ` [PATCH 4/4] rust: samples: add EDU PCI driver sample Maurice Hieronymus 2026-06-14 16:16 ` sashiko-bot 2026-06-15 10:12 ` Ewan Chorynski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox