Re: [PATCH v2 07/15] gpu: nova-core: add an interrupt delivery self-test

[Alexandre Courbot]

On Sat Aug 29, 2026 at 10:33 AM JST, John Hubbard wrote:
> A GPU interrupt can be lost in the MSI or MSI-X allocation, in the GIN
> tree's enable bits, or in the rearm. Every one of those failures looks
> the same to the driver: no interrupt arrives, and nothing in the symptom
> says which one broke.
>
> Add an optional probe-time self-test that injects the CPU doorbell
> through the GIN software trigger. One injection would pass even with a
> broken rearm, because the first message-signaled interrupt arrives
> whether the driver rearms or not. The test injects twice, and waits for
> the first handler to rearm before it injects again.
>
> Run it before GSP boot on a quiesced tree, and fail probe unless exactly
> two deliveries arrive, each delivery finds only the doorbell pending,
> and the leaf ends clear. Under MSI-X the injected subtree has its own
> table entry, so the delivery exercises that entry too.
>
> Assisted-by: Cursor:claude-opus-5
> Reviewed-by: Will Pierce <wpierce@xxxxxxxxxx>
> Co-developed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
> Signed-off-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/Kconfig               |  15 +
>  drivers/gpu/nova-core/gpu.rs                |   8 +
>  drivers/gpu/nova-core/irq.rs                |   2 +
>  drivers/gpu/nova-core/irq/doorbell_test.rs  | 294 ++++++++++++++++++++
>  drivers/gpu/nova-core/irq/interrupt_tree.rs |  73 +++--

The whole edit of `interrupt_tree.rs` changes or removes code that has
been added in the previous patch, and looks like churn that can be
squashed into patch 6, or am I missing something? I've tried squashing
it and things build just fine, so unless there is a good reason not to,
let's squash. It also makes this patch cleaner as it really only adds
the test.

<...>
> diff --git a/drivers/gpu/nova-core/irq.rs b/drivers/gpu/nova-core/irq.rs
> index c6bf1dbacabe..37dea5abf833 100644
> --- a/drivers/gpu/nova-core/irq.rs
> +++ b/drivers/gpu/nova-core/irq.rs
> @@ -8,6 +8,8 @@
>  //!
>  //! See `Documentation/gpu/nova/core/interrupts.rst`.
>  
> +#[cfg(CONFIG_NOVA_CORE_IRQ_SELFTEST)]
> +pub(crate) mod doorbell_test;
>  mod hal;
>  mod interrupt_tree;
>  mod regs;
> diff --git a/drivers/gpu/nova-core/irq/doorbell_test.rs b/drivers/gpu/nova-core/irq/doorbell_test.rs
> new file mode 100644
> index 000000000000..3fd8b26e135e
> --- /dev/null
> +++ b/drivers/gpu/nova-core/irq/doorbell_test.rs
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +//! Interrupt delivery self-test, driven through the CPU doorbell vector.
> +//!
> +//! Exercises the whole PCI interrupt path (GPU to PCIe to CPU to handler) with no GSP dependency:
> +//! it injects a known vector through the GIN software trigger and confirms the handler runs. Two
> +//! interrupts are triggered one at a time, which also covers the rearm that every delivery after
> +//! the first depends on. Gated behind `CONFIG_NOVA_CORE_IRQ_SELFTEST` and run before GSP boot, so
> +//! it never observes or clears GSP interrupt state.
> +//!
> +//! See `Documentation/gpu/nova/core/interrupts.rst`.
> +
> +use core::pin::Pin;
> +
> +use kernel::{
> +    device::Bound,
> +    irq,
> +    pci,
> +    prelude::*,
> +    sync::{
> +        atomic::{
> +            Atomic,
> +            Relaxed, //
> +        },
> +        Completion, //
> +    },
> +    time, //
> +};
> +
> +use super::interrupt_tree::{
> +    GinVector,
> +    LeafEnableGuard,
> +    LeafMask,
> +    Subtree,
> +    TopEnableGuard,
> +    Tree, //
> +};

supernit: missing empty line.

<...>
> +/// Runs the interrupt delivery self-test.
> +///
> +/// Quiesces the interrupt tree, registers a temporary handler, and injects the doorbell vector
> +/// through the GIN software trigger twice, one delivery at a time. This validates the PCI
> +/// interrupt path from GIN to the ISR without GSP firmware, including the rearm without which only
> +/// the first interrupt would arrive. The handler, its IRQ registration, and all tree state are
> +/// torn down before this returns.
> +///
> +/// # Errors
> +///
> +/// `EIO` if the doorbell is already pending before the test, if the delivery count is not two, if
> +/// the doorbell bit is still set once the source is stopped, or if either delivery found a pending
> +/// bit other than the doorbell. `ETIMEDOUT` if either delivery does not arrive within the timeout.
> +pub(crate) fn run_selftest<'a>(
> +    pdev: &'a pci::Device<Bound>,
> +    bar: Bar0<'a>,
> +    chipset: Chipset,
> +) -> Result {
> +    // The allocated interrupt type decides how the handler rearms delivery, so the vectors are
> +    // allocated before the tree is built.
> +    let vectors = super::alloc_vectors(pdev, DOORBELL_SUBTREE.into())?;
> +    let request = vectors.request_for(DOORBELL_SUBTREE)?;
> +    let irq_type = vectors.irq_type();
> +    let tree = Tree::new(bar, chipset, irq_type, DOORBELL_SUBTREE.into());
> +    let doorbell = DOORBELL_VECTOR.leaf_index();
> +    let doorbell_mask = DOORBELL_VECTOR.leaf_mask();
> +
> +    // Under MSI-X the subtree index is also the table entry the delivery arrives on, so a pass
> +    // shows that the per-subtree routing works. Under MSI every subtree shares one entry.
> +    dev_info!(
> +        pdev.as_ref(),
> +        "interrupt self-test: starting on vector {}, subtree {}, with {:?}\n",
> +        DOORBELL_VECTOR.into_raw(),
> +        DOORBELL_SUBTREE.index(),
> +        irq_type,
> +    );
> +
> +    // No delivery may reach the CPU before a handler is registered. `drain` enables the top level
> +    // as the last step of its cycle, so disable it again afterward.
> +    tree.disable_leaf(doorbell, doorbell_mask);
> +    tree.drain();
> +    tree.disable_top();

How about making `drain` *not* call `reenable_top()` in the end, and
making it the responsibility of the caller to reenable the tree if they
need so? This would remove this unneeded flip-flop which looks like a
tiny race condition.

Also, shouldn't we call `disable_all_leaves()` to make sure no
potentially spurious vector remains enabled? Otherwise the mask test
would fail, IIUC.

(bonus point: `disable_all_leaves` gets a user and doesn't need to be
marked `dead_code` anymore.)

<...>
> @@ -251,6 +275,10 @@ fn subtree_leaves(index: u32) -> impl Iterator<Item = LeafIndex> {
>  }
>  
>  /// The GIN CPU interrupt tree for a single PCIe function.
> +///
> +/// Copying one is copying a borrowed BAR pointer and three small values, which an interrupt
> +/// handler needs so that it owns a tree of its own.
> +#[derive(Clone, Copy)]

Mmm I'm not very comfortable with having several copies of `Tree`
concurrently accessing the registers. I've managed to remove that derive
directive, and thankfully the solution is simple: after you pass the
`Tree` to the `DoorbellTestHandler`, just access it through
`reg.handler().tree` to build the `SelfTestResources`:

    let resources = SelftestResources {
        _leaf_guard: reg
            .handler()
            .tree
            .enable_leaf_guarded(doorbell, doorbell_mask),
        _top_guard: reg.handler().tree.enable_top_guarded(),
        reg,
    };

(notice how `reg` is now initialized last)

Then right after you have `handler` and can access it through
`handler.tree`.

That way there is only one copy of `Tree` and we avoid potential future
footguns.