Re: [PATCH 03/17] rust: pci: expose the allocated interrupt type
"Danilo Krummrich" <[email protected]>
| Newsgroups | dev.linux.lists.nova-gpu,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <[email protected]> |
On Sun Aug 9, 2026 at 11:42 PM CEST, John Hubbard wrote: > Yes, please. Then my patches 2 and 3 collapse into a single patch that > adds count(), irq_type() and an index-to-IrqVector accessor. Or, they go > away entirely if you end up putting those on the type yourself. I sent out a patch series [1] for this, applied your nova-core patches on top of it and resolved the conflicts with the diff below (also pushed a branch in [2]). Note that I optimized for a clean diff and not for optimal code. I think there are more improvements we can make; I will comment on the corresponding patches of this series. [1] https://lore.kernel.org/all/[email protected]/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=nova/irq diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index 3fbf117a99ee..21d7df0744ed 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -20,7 +20,10 @@ use crate::{ gpu::Gpu, - irq::gsp::GspIrq, // + irq::{ + gsp::GspIrq, + SubtreeVectors, // + }, }; /// Counter for generating unique auxiliary device IDs. @@ -39,6 +42,12 @@ pub(crate) struct NovaCore<'bound> { bar: pci::Bar<'bound, BAR0_SIZE>, #[allow(clippy::type_complexity)] _reg: auxiliary::Registration<'bound, ForLt!(())>, + /// Self-referential borrow of `vectors`, so this does not have to be repeated in the + /// constructor. Will go away with self-referential pin-init. + vectors_ref: &'bound SubtreeVectors<'bound>, + /// PCI interrupt vector allocation. Dropped last (struct field drop order). + #[pin] + vectors: SubtreeVectors<'bound>, } pub(crate) struct NovaCoreDriver; @@ -87,38 +96,34 @@ fn probe<'bound>( pdev.enable_device_mem()?; pdev.set_master(); - // A PCI device has one interrupt vector allocation, so it is made here for every - // subtree nova-core services, and each handler takes the vector for its own subtree. - let vectors = crate::irq::alloc_vectors(pdev, crate::irq::gsp::GSP_SUBTREE)?; - let gsp_vector = vectors.vector_for(crate::irq::gsp::GSP_SUBTREE)?; - let irq_type = vectors.irq_type(); - Ok(try_pin_init!(NovaCore { + vectors: crate::irq::alloc_vectors(pdev, crate::irq::gsp::GSP_SUBTREE)?, + // SAFETY: `vectors` is initialized above, lives at a pinned stable address, and + // is dropped after all fields that use `vectors_ref` (struct field drop order). + vectors_ref: unsafe { &*core::ptr::from_ref(vectors.as_ref().get_ref()) }, bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?, // TODO: Use `&bar` self-referential pin-init syntax once available. // // SAFETY: `bar` is initialized before this expression is evaluated // (`try_pin_init!()` initializes fields in the order they appear here), lives at a // pinned stable address, and is dropped after `gpu` (struct field drop order). - gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }, vectors), + gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }, vectors_ref), // Quiesce the interrupt tree before registering the handler below. _: { // SAFETY: as for the `bar` borrow above. let bar = unsafe { &*core::ptr::from_ref(bar) }; - crate::irq::gsp::quiesce(bar, gpu.chipset(), irq_type); + crate::irq::gsp::quiesce(bar, gpu.chipset(), vectors_ref.irq_type()); }, // Register the permanent GSP SWGEN0 handler before enabling the interrupt. // - // SAFETY: `bar` is initialized before this expression is evaluated, lives at a - // pinned stable address, and is dropped after `_gsp_irq` (declared first, so - // dropped first), so the handler's borrow stays valid for its whole lifetime. - // `_gsp_irq` is stored in `NovaCore`, whose `Drop` runs `free_irq`, so the - // registration is never leaked. + // SAFETY: `bar` and `vectors` are initialized and pinned (see above). `_gsp_irq` + // is declared before `vectors` in the struct, so it is dropped first, ensuring + // `free_irq` runs before the vectors are freed. The registration is stored in + // `NovaCore` and never leaked. _gsp_irq <- unsafe { GspIrq::new( pdev, - gsp_vector, - irq_type, + vectors_ref, &*core::ptr::from_ref(bar), gpu.cmdq(), gpu.chipset(), @@ -129,7 +134,7 @@ fn probe<'bound>( _: { // SAFETY: as for the `bar` borrow above. let bar = unsafe { &*core::ptr::from_ref(bar) }; - crate::irq::gsp::enable(bar, gpu.chipset(), irq_type); + crate::irq::gsp::enable(bar, gpu.chipset(), vectors_ref.irq_type()); gpu.cmdq().drain()?; }, _reg: auxiliary::Registration::new( diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index 9700eff6db86..11a66a597298 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -339,7 +339,7 @@ pub(crate) fn cmdq(&self) -> Arc<Cmdq> { pub(crate) fn new( pdev: &'gpu pci::Device<device::Core<'_>>, bar: Bar0<'gpu>, - vectors: SubtreeVectors<'gpu>, + vectors: &'gpu SubtreeVectors<'gpu>, ) -> impl PinInit<Self, Error> + 'gpu { let dev = pdev.as_ref(); diff --git a/drivers/gpu/nova-core/irq.rs b/drivers/gpu/nova-core/irq.rs index ddf322f2e623..6f2ab85ccdc9 100644 --- a/drivers/gpu/nova-core/irq.rs +++ b/drivers/gpu/nova-core/irq.rs @@ -16,6 +16,7 @@ use kernel::{ device::Bound, + irq, pci::{ self, IrqType, @@ -28,9 +29,8 @@ /// /// MSI-X raises a separate table entry per subtree, so subtree `N` arrives on entry `N`. MSI has a /// single message that every subtree raises, so all of them arrive on the one allocated entry. -#[derive(Clone, Copy)] pub(crate) struct SubtreeVectors<'a> { - vectors: pci::IrqAllocation<'a>, + vectors: pci::IrqVectorRegistration<'a>, /// `TOP` bit of every subtree nova-core services. serviced: u32, } @@ -41,18 +41,21 @@ pub(crate) fn irq_type(&self) -> IrqType { self.vectors.irq_type() } - /// Returns the vector that delivers `subtree`, a single `TOP` bit of the form - /// `interrupt_tree::vector_subtree_mask` returns. + /// Returns an [`irq::IrqRequest`] for the vector that delivers `subtree`. /// /// # Errors /// /// `EINVAL` if `subtree` names anything other than a single subtree nova-core services. - pub(crate) fn vector_for(&self, subtree: u32) -> Result<pci::IrqVector<'a>> { + pub(crate) fn request_for( + &self, + subtree: u32, + ) -> Result<irq::IrqRequest<'_, &'_ pci::IrqVectorRegistration<'_>>> { if subtree.count_ones() != 1 || subtree & self.serviced == 0 { return Err(EINVAL); } - self.vectors.vector(entry_index(self.irq_type(), subtree)) + self.vectors + .request(entry_index(self.irq_type(), subtree) as usize) } } diff --git a/drivers/gpu/nova-core/irq/doorbell_test.rs b/drivers/gpu/nova-core/irq/doorbell_test.rs index bfdfee732892..2c123d0cdc52 100644 --- a/drivers/gpu/nova-core/irq/doorbell_test.rs +++ b/drivers/gpu/nova-core/irq/doorbell_test.rs @@ -144,7 +144,13 @@ struct SelftestGuard<'a, 'r> { bar: Bar0<'a>, tree: Tree, doorbell: LeafIndex, - reg: Option<Pin<KBox<irq::Registration<'r, DoorbellTestHandler<'a>>>>>, + reg: Option< + Pin< + KBox< + irq::Registration<'r, DoorbellTestHandler<'a>, &'r pci::IrqVectorRegistration<'r>>, + >, + >, + >, } impl<'a, 'r> SelftestGuard<'a, 'r> { @@ -195,11 +201,11 @@ pub(crate) fn run_selftest<'a>( pdev: &'a pci::Device<Bound>, bar: Bar0<'a>, chipset: Chipset, - vectors: SubtreeVectors<'_>, + vectors: &'a SubtreeVectors<'a>, ) -> Result { // The interrupt type decides how the handler rearms delivery, so the tree takes it from // probe's allocation. - let vector = vectors.vector_for(DOORBELL_SUBTREE)?; + let request = vectors.request_for(DOORBELL_SUBTREE)?; let irq_type = vectors.irq_type(); let tree = Tree::new(chipset, irq_type, DOORBELL_SUBTREE); let doorbell = LeafIndex::new::<DOORBELL_LEAF>(); @@ -252,7 +258,14 @@ pub(crate) fn run_selftest<'a>( // SAFETY: the registration is owned by `guard` below and dropped before this function // returns, so its `Drop` (which calls `free_irq()`) always runs and the registration is // never leaked or `mem::forget`-ed. - unsafe { pdev.request_irq(vector, irq::Flags::TRIGGER_NONE, c"nova-core", handler_init) }, + unsafe { + irq::Registration::new( + request, + irq::Flags::TRIGGER_NONE, + c"nova-core", + handler_init, + ) + }, GFP_KERNEL, )?; diff --git a/drivers/gpu/nova-core/irq/gsp.rs b/drivers/gpu/nova-core/irq/gsp.rs index ecd716b92d4e..558f944c4c0d 100644 --- a/drivers/gpu/nova-core/irq/gsp.rs +++ b/drivers/gpu/nova-core/irq/gsp.rs @@ -202,22 +202,21 @@ fn handle_threaded(&self) -> irq::IrqReturn { #[pin_data(PinnedDrop)] pub(crate) struct GspIrq<'a> { #[pin] - reg: irq::ThreadedRegistration<'a, GspInterrupt<'a>>, + reg: irq::ThreadedRegistration<'a, GspInterrupt<'a>, &'a pci::IrqVectorRegistration<'a>>, /// Borrowed BAR0 and the interrupt tree, used by the teardown to disable the GSP source. bar: Bar0<'a>, tree: Tree, } impl<'a> GspIrq<'a> { - /// Registers the GSP SWGEN0 threaded handler on `vector`. + /// Registers the GSP SWGEN0 threaded handler for the GSP subtree in `vectors`. /// /// # Safety /// /// The caller must not leak the returned value: its [`Drop`] runs `free_irq`. pub(crate) unsafe fn new( pdev: &'a pci::Device<device::Bound>, - vector: pci::IrqVector<'a>, - irq_type: pci::IrqType, + vectors: &'a super::SubtreeVectors<'a>, bar: Bar0<'a>, cmdq: Arc<Cmdq>, chipset: Chipset, @@ -227,15 +226,15 @@ pub(crate) unsafe fn new( // SAFETY: the caller guarantees the returned `GspIrq` is not leaked, so this // registration's `Drop` (`free_irq`) always runs. reg <- unsafe { - pdev.request_threaded_irq( - vector, + irq::ThreadedRegistration::new( + vectors.request_for(GSP_SUBTREE)?, irq::Flags::TRIGGER_NONE, c"nova-core", - GspInterrupt::new(bar, cmdq, chipset, irq_type, dev), + GspInterrupt::new(bar, cmdq, chipset, vectors.irq_type(), dev), ) }, bar, - tree: Tree::new(chipset, irq_type, GSP_SUBTREE), + tree: Tree::new(chipset, vectors.irq_type(), GSP_SUBTREE), }) } }