On 3/23/26 5:45 AM, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 7:53 AM JST, John Hubbard wrote:
>> Move Spec creation and the dev_info log from the driver's probe() into
>> Gpu::new(), so that GPU-specific identification lives in the Gpu
>> constructor.
>
> That's not what this patch does - Spec is already created in `Gpu::new()`.
Fixed in v8, along with all of the other comments below.
thanks,
--
John Hubbard
>
>>
>> Restructure Gpu::new() to use pin_init_scope wrapping try_pin_init!,
>> which allows running fallible setup code (Spec::new) before the
>> pin-initializer. Add Spec::chipset() accessor for use by later patches.
>
> What is missing is why we are doing this. It looks completely unneeded
> even when looking at the following patches.
>
>>
>> The DMA mask setup stays in probe() where the safety argument for
>> dma_set_mask_and_coherent is straightforward.
>
> Knowing the history of the series I understand why this sentence is
> here, but as of this revision it is not relevant.
>
>>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Gary Guo <gary@garyguo.net>
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> ---
>> drivers/gpu/nova-core/gpu.rs | 49 +++++++++++++++++++++---------------
>> 1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 3b4ccc3d18b9..8f317d213908 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -102,7 +102,7 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
>> });
>>
>> impl Chipset {
>> - pub(crate) const fn arch(self) -> Architecture {
>> + pub(crate) const fn arch(&self) -> Architecture {
>
> Why are we taking `self` by reference now? `Chipset` implements `Copy`
> so this should not be needed.
>
>> match self {
>> Self::TU102 | Self::TU104 | Self::TU106 | Self::TU117 | Self::TU116 => {
>> Architecture::Turing
>> @@ -241,6 +241,10 @@ fn new(dev: &device::Device, bar: &Bar0) -> Result<Spec> {
>> dev_err!(dev, "Unsupported chipset: {}\n", boot42);
>> })
>> }
>> +
>> + pub(crate) fn chipset(&self) -> Chipset {
>> + self.chipset
>> + }
>
> Short doccomment please, even if it is obvious by the method name what
> it does.
>
>> }
>>
>> impl TryFrom<regs::NV_PMC_BOOT_42> for Spec {
>> @@ -289,32 +293,37 @@ pub(crate) fn new<'a>(
>> devres_bar: Arc<Devres<Bar0>>,
>> bar: &'a Bar0,
>> ) -> impl PinInit<Self, Error> + 'a {
>> - try_pin_init!(Self {
>> - spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
>> - dev_info!(pdev,"NVIDIA ({})\n", spec);
>> - })?,
>> + pin_init::pin_init_scope(move || {
>> + let spec = Spec::new(pdev.as_ref(), bar)?;
>> + dev_info!(pdev, "NVIDIA ({})\n", spec);
>> +
>> + let chipset = spec.chipset();
>>
>> - // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
>> - _: {
>> - gfw::wait_gfw_boot_completion(bar)
>> - .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
>> - },
>> + Ok(try_pin_init!(Self {
>> + // We must wait for GFW_BOOT completion before doing any significant setup
>> + // on the GPU.
>> + _: {
>> + 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, chipset)?,
>>
>> - gsp_falcon: Falcon::new(
>> - pdev.as_ref(),
>> - spec.chipset,
>> - )
>> - .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>> + gsp_falcon: Falcon::new(
>> + pdev.as_ref(),
>> + chipset,
>> + )
>> + .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>>
>> - sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?,
>> + sec2_falcon: Falcon::new(pdev.as_ref(), chipset)?,
>>
>> - gsp <- Gsp::new(pdev),
>> + gsp <- Gsp::new(pdev),
>>
>> - _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
>> + _: { gsp.boot(pdev, bar, chipset, gsp_falcon, sec2_falcon)? },
>>
>> - bar: devres_bar,
>> + bar: devres_bar,
>> + spec,
>> + }))
>
> This diff is basically a no-op? The commit message should help me make
> sense of it but since it is incorrect, I can only guess - and I suspect
> you are doing this dance because `Revision` and `Spec` do not implement
> `Copy`.
>
> `Spec` is 4 bytes, both it and `Revision` are prime candidates for a
> `#[derive(Clone, Copy)]` (and `fmt::Debug` while we are at it).
>
> I expect that just doing that will simplify the following patches as well.