Hello Alistair, On Mon Jun 22, 2026 at 4:45 AM -03, Alistair Popple wrote: > On 2026-06-22 at 00:36 +1000, Nicolás Antinori <[email protected]> > wrote... >> Replace the unsafe `kernel::transmute::FromBytes` trait implementation >> for the `FalconUCodeDescV3`, `PcirStruct`, `BitHeader`, `NpdeStruct`, >> and `PmuLookupTableHeader` structs with the derivable >> `zerocopy::FromBytes` trait. >> >> This change eliminates the manual unsafe implementations in favor of a >> derivable trait. When this trait is derived, validity checks are >> performed at compile time to make sure that the type can safely >> implement `FromBytes`. > > Nice, this looks good although due to some code movement this doesn't compile > when applied to drm-rust-next[1]. Would you mind rebasing on top of that? The > changes required should be relatively straight forward, they are mostly the > result of some new users of transmute::FromBytes being added to vbios.rs.
I did my work on rust-next [1] because drm-rust-next does not have the zerocopy crate present yet [2]. linux-next contains both zerocopy [3] and the new users of transmute::FromBytes if I am not mistaken (BitToken, PciRomHeader, and PmuLookupTableEntry), so I can make the changes there. I got confused because the issue was originally in the rust-for-linux tree. Since zerocopy wasn't in drm-rust-next yet, I wrongly assumed the work should happen there and that drm-rust-next would eventually pull them in. I am fairly new to kernel development, I apologize for the mix-up. > > Also what is you plan here? I have also been looking at some of these > conversions, specifically those involving the generated bindings. Really > appreciate your contributions, so no problem if you want to continue with some > of the conversions, just want to make sure we aren't duplicating effort here. I would be happy to handle the remaining conversions! I can send a v2 patch that includes those as well. Thank you [1] https://github.com/Rust-for-Linux/linux/tree/rust-next/rust [2] https://gitlab.freedesktop.org/drm/rust/kernel/-/tree/drm-rust-next/rust [3] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/rust > > - Alistair > > [1] - https://gitlab.freedesktop.org/drm/rust/kernel/-/tree/drm-rust-next > >> Link: https://github.com/Rust-for-Linux/linux/issues/1241 >> Suggested-by: Miguel Ojeda <[email protected]> >> Signed-off-by: Nicolás Antinori <[email protected]> >> --- >> NOTE: Compile-tested only. I don't own a piece of hardware where I can >> test the nova driver. >> >> drivers/gpu/nova-core/firmware.rs | 6 +---- >> drivers/gpu/nova-core/vbios.rs | 38 ++++++++----------------------- >> 2 files changed, 11 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/nova-core/firmware.rs >> b/drivers/gpu/nova-core/firmware.rs >> index ad37994ac15a..0afd1d3fe5ad 100644 >> --- a/drivers/gpu/nova-core/firmware.rs >> +++ b/drivers/gpu/nova-core/firmware.rs >> @@ -86,7 +86,7 @@ pub(crate) struct FalconUCodeDescV2 { >> >> /// Structure used to describe some firmwares, notably FWSEC-FRTS. >> #[repr(C)] >> -#[derive(Debug, Clone)] >> +#[derive(Debug, Clone, FromBytes)] >> pub(crate) struct FalconUCodeDescV3 { >> /// Header defined by `NV_BIT_FALCON_UCODE_DESC_HEADER_VDESC*` in >> OpenRM. >> hdr: u32, >> @@ -117,10 +117,6 @@ pub(crate) struct FalconUCodeDescV3 { >> _reserved: u16, >> } >> >> -// SAFETY: all bit patterns are valid for this type, and it doesn't use >> -// interior mutability. >> -unsafe impl FromBytes for FalconUCodeDescV3 {} >> - >> /// Enum wrapping the different versions of Falcon microcode descriptors. >> /// >> /// This allows handling both V2 and V3 descriptor formats through a >> diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs >> index 8b7d17a24660..754812bcbdde 100644 >> --- a/drivers/gpu/nova-core/vbios.rs >> +++ b/drivers/gpu/nova-core/vbios.rs >> @@ -13,11 +13,8 @@ >> Alignment, // >> }, >> sync::aref::ARef, >> - transmute::FromBytes, >> }; >> >> -use zerocopy::FromBytes as _; >> - >> use crate::{ >> driver::Bar0, >> firmware::{ >> @@ -301,7 +298,7 @@ pub(crate) fn fwsec_image(&self) -> &FwSecBiosImage { >> } >> >> /// PCI Data Structure as defined in PCI Firmware Specification >> -#[derive(Debug, Clone)] >> +#[derive(Debug, Clone, FromBytes)] >> #[repr(C)] >> struct PcirStruct { >> /// PCI Data Structure signature ("PCIR" or "NPDS") >> @@ -330,12 +327,9 @@ struct PcirStruct { >> max_runtime_image_len: u16, >> } >> >> -// SAFETY: all bit patterns are valid for `PcirStruct`. >> -unsafe impl FromBytes for PcirStruct {} >> - >> impl PcirStruct { >> fn new(dev: &device::Device, data: &[u8]) -> Result<Self> { >> - let (pcir, _) = >> PcirStruct::from_bytes_copy_prefix(data).ok_or(EINVAL)?; >> + let (pcir, _) = PcirStruct::read_from_prefix(data).map_err(|_| >> EINVAL)?; >> >> // Signature should be "PCIR" (0x52494350) or "NPDS" (0x5344504e). >> if &pcir.signature != b"PCIR" && &pcir.signature != b"NPDS" { >> @@ -371,7 +365,7 @@ fn image_size_bytes(&self) -> usize { >> /// This is the head of the BIT table, that is used to locate the Falcon >> data. The BIT table (with >> /// its header) is in the [`PciAtBiosImage`] and the falcon data it is >> pointing to is in the >> /// [`FwSecBiosImage`]. >> -#[derive(Debug, Clone, Copy)] >> +#[derive(Debug, Clone, Copy, FromBytes)] >> #[repr(C)] >> struct BitHeader { >> /// 0h: BIT Header Identifier (BMP=0x7FFF/BIT=0xB8FF) >> @@ -390,12 +384,9 @@ struct BitHeader { >> checksum: u8, >> } >> >> -// SAFETY: all bit patterns are valid for `BitHeader`. >> -unsafe impl FromBytes for BitHeader {} >> - >> impl BitHeader { >> fn new(data: &[u8]) -> Result<Self> { >> - let (header, _) = >> BitHeader::from_bytes_copy_prefix(data).ok_or(EINVAL)?; >> + let (header, _) = BitHeader::read_from_prefix(data).map_err(|_| >> EINVAL)?; >> >> // Check header ID and signature >> if header.id != 0xB8FF || &header.signature != b"BIT\0" { >> @@ -533,7 +524,7 @@ fn new(dev: &device::Device, data: &[u8]) -> >> Result<Self> { >> /// PCI Data Structure. It contains some fields that are redundant with the >> PCI Data Structure, but >> /// are needed for traversing the BIOS images. It is expected to be present >> in all BIOS images >> /// except for NBSI images. >> -#[derive(Debug, Clone)] >> +#[derive(Debug, Clone, FromBytes)] >> #[repr(C)] >> struct NpdeStruct { >> /// 00h: Signature ("NPDE") >> @@ -548,12 +539,9 @@ struct NpdeStruct { >> last_image: u8, >> } >> >> -// SAFETY: all bit patterns are valid for `NpdeStruct`. >> -unsafe impl FromBytes for NpdeStruct {} >> - >> impl NpdeStruct { >> fn new(dev: &device::Device, data: &[u8]) -> Option<Self> { >> - let (npde, _) = NpdeStruct::from_bytes_copy_prefix(data)?; >> + let (npde, _) = NpdeStruct::read_from_prefix(data).ok()?; >> >> // Signature should be "NPDE" (0x4544504E). >> if &npde.signature != b"NPDE" { >> @@ -845,6 +833,7 @@ fn new(data: &[u8]) -> Result<Self> { >> } >> >> #[repr(C)] >> +#[derive(FromBytes)] >> struct PmuLookupTableHeader { >> version: u8, >> header_len: u8, >> @@ -852,9 +841,6 @@ struct PmuLookupTableHeader { >> entry_count: u8, >> } >> >> -// SAFETY: all bit patterns are valid for `PmuLookupTableHeader`. >> -unsafe impl FromBytes for PmuLookupTableHeader {} >> - >> /// The [`PmuLookupTableEntry`] structure is used to find the >> [`PmuLookupTableEntry`] for a given >> /// application ID. >> /// >> @@ -867,7 +853,7 @@ struct PmuLookupTable { >> >> impl PmuLookupTable { >> fn new(dev: &device::Device, data: &[u8]) -> Result<Self> { >> - let (header, _) = >> PmuLookupTableHeader::from_bytes_copy_prefix(data).ok_or(EINVAL)?; >> + let (header, _) = >> PmuLookupTableHeader::read_from_prefix(data).map_err(|_| EINVAL)?; >> >> let header_len = usize::from(header.header_len); >> let entry_len = usize::from(header.entry_len); >> @@ -1013,15 +999,11 @@ pub(crate) fn header(&self) -> >> Result<FalconUCodeDesc> { >> let data = self.base.data.get(falcon_ucode_offset..).ok_or(EINVAL)?; >> match ver { >> 2 => { >> - let v2 = FalconUCodeDescV2::read_from_prefix(data) >> - .map_err(|_| EINVAL)? >> - .0; >> + let (v2, _) = >> FalconUCodeDescV2::read_from_prefix(data).map_err(|_| EINVAL)?; >> Ok(FalconUCodeDesc::V2(v2)) >> } >> 3 => { >> - let v3 = FalconUCodeDescV3::from_bytes_copy_prefix(data) >> - .ok_or(EINVAL)? >> - .0; >> + let (v3, _) = >> FalconUCodeDescV3::read_from_prefix(data).map_err(|_| EINVAL)?; >> Ok(FalconUCodeDesc::V3(v3)) >> } >> _ => { >> -- >> 2.47.3 >>
