Re: [PATCH v3 5/5] rust: samples: add EDU PCI driver sample

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