Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Unbounded wait_for_completion() during probe can hang the system. - [High] 64-bit MMIO accesses will cause compilation failures on 32-bit architectures. - [Medium] Unconditional Handled return in IRQ handler defeats spurious interrupt detection. --
--- Patch [5]: [PATCH v3 5/5] rust: samples: add EDU PCI driver sample --- commit c231e5461a247a3fe7e624e02b7930ee66f864b3 Author: Maurice Hieronymus <[email protected]> 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/Kconfig b/samples/rust/Kconfig > index 0cae695acd842..115f71ed63283 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -107,6 +107,17 @@ config SAMPLE_RUST_I2C_CLIENT > > If unsure, say N. > > +config SAMPLE_RUST_DRIVER_EDU > + tristate "EDU Driver" > + depends on PCI [Severity: High] Does this missing 64BIT dependency cause build failures on 32-bit architectures? The driver defines 64-bit MMIO registers (like DMA_SRC(u64)), but the IoCapable<u64> trait is only conditionally implemented for Mmio on 64-bit architectures. Building on a 32-bit target will fail compilation because the 64-bit write method on pci::Bar will be unavailable. > + help > + This option builds the Rust EDU driver sample. [ ... ] > diff --git a/samples/rust/rust_driver_edu.rs b/samples/rust/rust_driver_edu.rs > new file mode 100644 > index 0000000000000..52f6c4cf3b08d > --- /dev/null > +++ b/samples/rust/rust_driver_edu.rs > @@ -0,0 +1,421 @@ [ ... ] > + fn test_irq(pdev: &pci::Device<Bound>, handler: &IrqHandler<'_>) -> > Result { > + dev_dbg!(pdev, "raising irq\n"); > + > + handler.bar.write(regs::IRQ_RAISE, IRQ_MAGIC_VALUE.into()); > + > + handler.irq_test_completion.wait_for_completion(); [Severity: High] Can this unbounded wait hang the system? Since test_irq() is called synchronously from the driver probe path, a dropped hardware interrupt or faulty device might block the thread indefinitely. This could prevent the kernel from booting or loading other drivers. Would a timed wait like wait_for_completion_timeout() avoid this risk? > + > + dev_info!(pdev, "irq test successful\n"); > + Ok(()) > + } > + > + fn test_dma(pdev: &pci::Device<Bound>, handler: &IrqHandler<'_>) -> > Result { > + dev_dbg!(pdev, "testing dma\n"); [ ... ] > + handler.bar.write( > + regs::DMA_COMMAND, > + regs::DMA_COMMAND::zeroed() > + .with_start_transfer(true) > + .with_direction(false) > + .with_raise_irq(true), > + ); > + > + handler.irq_dma_completion.wait_for_completion(); [Severity: High] Similarly, could this unbounded wait block the probe sequence indefinitely if the DMA completion interrupt is never received? > + > + // Destroy previous value to test roundtrip [ ... ] > +impl irq::Handler for IrqHandler<'_> { > + fn handle(&self) -> irq::IrqReturn { > + dev_dbg!(self.pdev, "irq handler called\n"); > + let status: u32 = self.bar.read(regs::IRQ_STATUS).into(); > + > + // DMA_IRQ > + if status & DMA_IRQ != 0 { > + dev_dbg!(self.pdev, "handling dma completion in irq\n"); > + self.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!(self.pdev, "handling test completion in irq\n"); > + self.bar.write(regs::IRQ_ACK, magic.into()); > + self.irq_test_completion.complete(); > + } > + > + irq::IrqReturn::Handled [Severity: Medium] Does this unconditional return of Handled defeat the kernel's spurious interrupt detection? If the status matches neither DMA_IRQ nor IRQ_MAGIC_VALUE, the handler processes no bits but still returns irq::IrqReturn::Handled. When a spurious interrupt occurs, the kernel will believe this device handled it and won't increment the spurious interrupt counter. This could prevent the IRQ subsystem from disabling a faulty IRQ line, potentially causing an interrupt storm. Should this return irq::IrqReturn::None when no expected bits are set? > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
