Re: [PATCH v2 1/3] nova-core: Simplify `transmute` and `transmute_mut` in fwsec.rs

["Alexandre Courbot" <acourbot@nvidia.com>]

On Fri Oct 24, 2025 at 5:51 AM JST, Daniel del Castillo wrote:
> This patch solves one of the existing mentions of COHA, a task
> in the Nova task list about improving the `CoherentAllocation` API.
> It uses the new `from_bytes` method from the `FromBytes` trait as
> well as the `as_slice` and `as_slice_mut` methods from
> `CoherentAllocation`.
>
> Signed-off-by: Daniel del Castillo <delcastillodelarosadaniel@gmail.com>
>
> ---
>
> I confirmed by talking to Alexandre Courbot, that the reading/writing
> methods in `CoherentAllocation` can never be safe, so
> this patch doesn't actually change `CoherentAllocation`, but rather
> tries to solve one of the existing references to [COHA].
>
> V1 -> V2: Split previous patch into two. One per reference to COHA.
>           Improved comments. Let me know if they are okay now.
>           Use of `{...}` syntax for the `if let`
>
>  drivers/gpu/nova-core/firmware/fwsec.rs | 129 +++++++++++-------------
>  1 file changed, 60 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
> index 8edbb5c0572c..507ef3868565 100644
> --- a/drivers/gpu/nova-core/firmware/fwsec.rs
> +++ b/drivers/gpu/nova-core/firmware/fwsec.rs
> @@ -11,12 +11,12 @@
>  //! - The ucode signature, so the GSP falcon can run FWSEC in HS mode.
>  
>  use core::marker::PhantomData;
> -use core::mem::{align_of, size_of};
> +use core::mem::size_of;
>  use core::ops::Deref;
>  
>  use kernel::device::{self, Device};
>  use kernel::prelude::*;
> -use kernel::transmute::FromBytes;
> +use kernel::transmute::{AsBytes, FromBytes};
>  
>  use crate::dma::DmaObject;
>  use crate::driver::Bar0;
> @@ -35,7 +35,7 @@ struct FalconAppifHdrV1 {
>      entry_size: u8,
>      entry_count: u8,
>  }
> -// SAFETY: any byte sequence is valid for this struct.
> +// SAFETY: Any byte sequence is valid for this struct.
>  unsafe impl FromBytes for FalconAppifHdrV1 {}
>  
>  #[repr(C, packed)]
> @@ -44,7 +44,7 @@ struct FalconAppifV1 {
>      id: u32,
>      dmem_base: u32,
>  }
> -// SAFETY: any byte sequence is valid for this struct.
> +// SAFETY: Any byte sequence is valid for this struct.
>  unsafe impl FromBytes for FalconAppifV1 {}
>  
>  #[derive(Debug)]
> @@ -68,8 +68,10 @@ struct FalconAppifDmemmapperV3 {
>      ucode_cmd_mask1: u32,
>      multi_tgt_tbl: u32,
>  }
> -// SAFETY: any byte sequence is valid for this struct.
> +// SAFETY: Any byte sequence is valid for this struct.

I appreciate the capitalization, but these changes are a bit
distracting. :) If you absolutely want to do this, let it be its own
patch so the current one stays focused on what it actually does.

>  unsafe impl FromBytes for FalconAppifDmemmapperV3 {}
> +// SAFETY: This struct doesn't contain unitialized bytes and doesn't have interior mutability.

Typo: s/unitialized/uninitialized (and in other comments as well).

Otherwise this looks ok - it doesn't apply cleanly on drm-rust-next
though, could you rebase for the next version?

Thanks for the cleanup!