Hi Alexandre, On 11/1/25 12:47, Alexandre Courbot wrote: > 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 <[email protected]> >> >> --- >> >> 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). > I will move the capitalization to another patch and fix the typo.
> Otherwise this looks ok - it doesn't apply cleanly on drm-rust-next > though, could you rebase for the next version? About this, I was basing myself on nova-next [1]. I will rebase on top of drm-rust-next for the next version. Link: https://gitlab.freedesktop.org/drm/nova [1] Link: https://gitlab.freedesktop.org/drm/rust/kernel/-/tree/drm-rust-next [2] > > Thanks for the cleanup! Thanks to you for the reviews and the patience!
