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

[sashiko-bot@kernel.org]

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