Re: [PATCH v9 05/31] gpu: nova-core: set DMA mask width based on GPU architecture

[John Hubbard]

On 3/30/26 7:32 AM, Alexandre Courbot wrote:
> On Thu Mar 26, 2026 at 10:38 AM JST, John Hubbard wrote:
...
>>  kernel::pci_device_table!(
>> @@ -84,18 +77,15 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> impl PinInit<Self, E
>>              pdev.enable_device_mem()?;
>>              pdev.set_master();
>>  
>> -            // SAFETY: No concurrent DMA allocations or mappings can be made because
>> -            // the device is still being probed and therefore isn't being used by
>> -            // other threads of execution.
>> -            unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<GPU_DMA_BITS>())? };
>> -
>>              let bar = Arc::pin_init(
>>                  pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0"),
>>                  GFP_KERNEL,
>>              )?;
>> +            let spec = Spec::new(pdev.as_ref(), bar.access(pdev.as_ref())?)?;
>> +            dev_info!(pdev, "NVIDIA ({})\n", spec);
> 
> Now that we have moved to DMA mask into `Gpu::new`, what is the point of
> creating `spec` here? Its sole purpose is to be passed to `Gpu::new`,
> and that doesn't change by the end of the series.

Agreed. For v10, I've moved Spec creation back inside Gpu::new() where
it was before.

The original reason for pulling it into probe() was that the DMA mask
lived there (per your v6 feedback about the safety argument). Now that
the DMA mask moved into Gpu::new() with a better safety justification,
there is no reason for Spec to remain in probe().

...
>>  impl Gpu {
>>      pub(crate) fn new<'a>(
>> -        pdev: &'a pci::Device<device::Bound>,
>> +        pdev: &'a pci::Device<device::Core>,
>>          devres_bar: Arc<Devres<Bar0>>,
>>          bar: &'a Bar0,
>> +        spec: Spec,
>>      ) -> impl PinInit<Self, Error> + 'a {
>> -        try_pin_init!(Self {
>> -            spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
>> -                dev_info!(pdev,"NVIDIA ({})\n", spec);
>> -            })?,
>> +        let dma_mask = spec.chipset().arch().dma_mask();
>>  
>> +        try_pin_init!(Self {
>>              // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
>>              _: {
>> +                // SAFETY: `Gpu` owns all DMA allocations for this device, and we are
>> +                // still constructing it, so no concurrent DMA allocations can exist.
>> +                unsafe { pdev.dma_set_mask_and_coherent(dma_mask)? };
>> +
>>                  gfw::wait_gfw_boot_completion(bar)
>>                      .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
>>              },
>>  
>> -            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
>> +            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset())?,
>>  
>>              gsp_falcon: Falcon::new(
>>                  pdev.as_ref(),
>> -                spec.chipset,
>> +                spec.chipset(),
>>              )
>>              .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>>  
>> -            sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?,
>> +            sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset())?,
> 
> Ok, so you've removed the local variable as per my v8 feedback - now you
> can also access `chipset` (the member, not the method - which should not
> be needed anyway as `spec` doesn't need to be created in `driver.rs`)
> directly, and remove some noise from the diff.
> 

Done. With Spec back inside Gpu::new(), the try_pin_init! macro gives
direct field access to spec.chipset, so the diff no longer touches those
lines at all.

thanks,
-- 
John Hubbard