Joel Fernandes
2025-Oct-27 19:09 UTC
[PATCH v2 5/7] gpu: nova-core: add extra integer conversion functions and traits
Hello Alex, On 10/27/2025 8:54 AM, Alexandre Courbot wrote:> The core library's `From` implementations do not cover conversions > that are not portable or future-proof. For instance, even though it is > safe today, `From<usize>` is not implemented for `u64` because of the > possibility to support larger-than-64bit architectures in the future. > > However, the kernel supports a narrower set of architectures, and in the > case of Nova we only support 64-bit. This makes it helpful and desirable > to provide more infallible conversions, lest we need to rely on the `as` > keyword and carry the risk of silently losing data. > > Thus, introduce a new module `num` that provides safe const functions > performing more conversions allowed by the build target, as well as > `FromAs` and `IntoAs` traits that are just extensions of `From` and > `Into` to conversions that are known to be lossless.Why not just implement `From` and `Into` for the types missing it then, with adequate comments about why such conversions are Ok for the kernel, instead of introducing a new trait? This is exactly what `From`/`Into` is for right?> Suggested-by: Danilo Krummrich <dakr at kernel.org> > Link: https://lore.kernel.org/rust-for-linux/DDK4KADWJHMG.1FUPL3SDR26XF at kernel.org/ > Acked-by: Danilo Krummrich <dakr at kernel.org> > Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> > --- > drivers/gpu/nova-core/nova_core.rs | 1 + > drivers/gpu/nova-core/num.rs | 158 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 159 insertions(+) > > diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs > index e130166c1086..9180ec9c27ef 100644 > --- a/drivers/gpu/nova-core/nova_core.rs > +++ b/drivers/gpu/nova-core/nova_core.rs > @@ -13,6 +13,7 @@ > mod gfw; > mod gpu; > mod gsp; > +mod num; > mod regs; > mod vbios; > > diff --git a/drivers/gpu/nova-core/num.rs b/drivers/gpu/nova-core/num.rs > new file mode 100644 > index 000000000000..adb5a92f0d51 > --- /dev/null > +++ b/drivers/gpu/nova-core/num.rs > @@ -0,0 +1,158 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Numerical helpers functions and traits. > +//! > +//! This is essentially a staging module for code to mature until it can be moved to the `kernel` > +//! crate. > + > +use kernel::{build_error, macros::paste}; > + > +macro_rules! impl_lossless_as { > + ($from:ty as { $($into:ty),* }) => { > + $( > + paste! { > + #[doc = ::core::concat!( > + "Losslessly converts a [`", > + ::core::stringify!($from), > + "`] into a [`", > + ::core::stringify!($into), > + "`].")] > + /// > + /// This conversion is allowed as it is always lossless. Prefer this over the `as` > + /// keyword to ensure no lossy conversions are performed. > + /// > + /// This is for use from a `const` context. For non `const` use, prefer the [`FromAs`] > + /// and [`IntoAs`] traits. > + /// > + /// # Examples > + /// > + /// ``` > + /// use crate::num; > + /// > + #[doc = ::core::concat!( > + "assert_eq!(num::", > + ::core::stringify!($from), > + "_as_", > + ::core::stringify!($into), > + "(1", > + ::core::stringify!($from), > + "), 1", > + ::core::stringify!($into), > + ");")] > + /// ``` > + #[allow(unused)] > + pub(crate) const fn [<$from _as_ $into>](value: $from) -> $into { > + kernel::static_assert!(size_of::<$into>() >= size_of::<$from>()); > + > + value as $into > + } > + } > + )* > + }; > +} > + > +impl_lossless_as!(u8 as { u16, u32, u64, usize }); > +impl_lossless_as!(u16 as { u32, u64, usize }); > +impl_lossless_as!(u32 as { u64, usize } );I prefer consistency with the notation in num.rs in the rust standard library: impl_from!(u16 => usize) But I am Ok with your notation as well, which is concise.> +// `u64` and `usize` have the same size on 64-bit platforms. > +#[cfg(CONFIG_64BIT)] > +impl_lossless_as!(u64 as { usize } ); > + > +// A `usize` fits into a `u64` on 32 and 64-bit platforms. > +#[cfg(any(CONFIG_32BIT, CONFIG_64BIT))] > +impl_lossless_as!(usize as { u64 }); > + > +// A `usize` fits into a `u32` on 32-bit platforms. > +#[cfg(CONFIG_32BIT)] > +impl_lossless_as!(usize as { u32 }); > + > +/// Extension trait providing guaranteed lossless conversion to `Self` from `T`. > +/// > +/// The standard library's `From` implementations do not cover conversions that are not portable or > +/// future-proof. For instance, even though it is safe today, `From<usize>` is not implemented for > +/// [`u64`] because of the possibility to support larger-than-64bit architectures in the future. > +/// > +/// The workaround is to either deal with the error handling of [`TryFrom`] for an operation that > +/// technically cannot fail, or to use the `as` keyword, which can silently strip data if the > +/// destination type is smaller than the source. > +/// > +/// Both options are hardly acceptable for the kernel. It is also a much more architecture > +/// dependent environment, supporting only 32 and 64 bit architectures, with some modules > +/// explicitly depending on a specific bus width that could greatly benefit from infallible > +/// conversion operations. > +/// > +/// Thus this extension trait that provides, for the architecture the kernel is built for, safe > +/// conversion between types for which such conversion is lossless. > +/// > +/// In other words, this trait is implemented if, for the current build target and with `t: T`, the > +/// `t as Self` operation is completely lossless. > +/// > +/// Prefer this over the `as` keyword to ensure no lossy conversions are performed. > +/// > +/// If you need to perform a conversion in `const` context, use [`u64_as_usize`], > +/// [`u32_as_usize`], [`usize_as_u64`], etc. > +/// > +/// # Examples > +/// > +/// ``` > +/// use crate::num::FromAs; > +/// > +/// assert_eq!(usize::from_as(0xf00u32), 0xf00u32 as usize);This `from_as()` syntax will be very confusing for users IMO, honestly we should just keep it as `from()`, otherwise there is confusion and ambiguity around whether someone should use `::from()` or `::from_as()`. Please let us just keep all infallible conversions to use `from()`/`into()` and all infallible ones to use `try_from()`/`try_into()`. No need to additional `_as()` prefix, as it creates confusion. In the end of the day, the fact the conversion is lossless is not relevant, all conversions that don't use the `as` keyword should be expected to be lossless right? thanks, - Joel
Joel Fernandes
2025-Oct-27 19:11 UTC
[PATCH v2 5/7] gpu: nova-core: add extra integer conversion functions and traits
On 10/27/2025 3:09 PM, Joel Fernandes wrote: [...]>> +/// Extension trait providing guaranteed lossless conversion to `Self` from `T`. >> +/// >> +/// The standard library's `From` implementations do not cover conversions that are not portable or >> +/// future-proof. For instance, even though it is safe today, `From<usize>` is not implemented for >> +/// [`u64`] because of the possibility to support larger-than-64bit architectures in the future. >> +/// >> +/// The workaround is to either deal with the error handling of [`TryFrom`] for an operation that >> +/// technically cannot fail, or to use the `as` keyword, which can silently strip data if the >> +/// destination type is smaller than the source. >> +/// >> +/// Both options are hardly acceptable for the kernel. It is also a much more architecture >> +/// dependent environment, supporting only 32 and 64 bit architectures, with some modules >> +/// explicitly depending on a specific bus width that could greatly benefit from infallible >> +/// conversion operations. >> +/// >> +/// Thus this extension trait that provides, for the architecture the kernel is built for, safe >> +/// conversion between types for which such conversion is lossless. >> +/// >> +/// In other words, this trait is implemented if, for the current build target and with `t: T`, the >> +/// `t as Self` operation is completely lossless. >> +/// >> +/// Prefer this over the `as` keyword to ensure no lossy conversions are performed. >> +/// >> +/// If you need to perform a conversion in `const` context, use [`u64_as_usize`], >> +/// [`u32_as_usize`], [`usize_as_u64`], etc. >> +/// >> +/// # Examples >> +/// >> +/// ``` >> +/// use crate::num::FromAs; >> +/// >> +/// assert_eq!(usize::from_as(0xf00u32), 0xf00u32 as usize); > > This `from_as()` syntax will be very confusing for users IMO, honestly we should > just keep it as `from()`, otherwise there is confusion and ambiguity around > whether someone should use `::from()` or `::from_as()`. Please let us just keep > all infallible conversions to use `from()`/`into()` and all infallible ones tos/infallible ones/fallible ones/. Sorry, thanks, - Joel
Danilo Krummrich
2025-Oct-27 19:23 UTC
[PATCH v2 5/7] gpu: nova-core: add extra integer conversion functions and traits
On 10/27/25 8:09 PM, Joel Fernandes wrote:> Why not just implement `From` and `Into` for the types missing it then, with > adequate comments about why such conversions are Ok for the kernel, instead of > introducing a new trait? This is exactly what `From`/`Into` is for right?https://doc.rust-lang.org/reference/items/implementations.html#r-items.impl.trait.orphan-rule.intro
Alexandre Courbot
2025-Oct-28 07:23 UTC
[PATCH v2 5/7] gpu: nova-core: add extra integer conversion functions and traits
On Tue Oct 28, 2025 at 4:09 AM JST, Joel Fernandes wrote:> Hello Alex, > > On 10/27/2025 8:54 AM, Alexandre Courbot wrote: >> The core library's `From` implementations do not cover conversions >> that are not portable or future-proof. For instance, even though it is >> safe today, `From<usize>` is not implemented for `u64` because of the >> possibility to support larger-than-64bit architectures in the future. >> >> However, the kernel supports a narrower set of architectures, and in the >> case of Nova we only support 64-bit. This makes it helpful and desirable >> to provide more infallible conversions, lest we need to rely on the `as` >> keyword and carry the risk of silently losing data. >> >> Thus, introduce a new module `num` that provides safe const functions >> performing more conversions allowed by the build target, as well as >> `FromAs` and `IntoAs` traits that are just extensions of `From` and >> `Into` to conversions that are known to be lossless. > > Why not just implement `From` and `Into` for the types missing it then, with > adequate comments about why such conversions are Ok for the kernel, instead of > introducing a new trait? This is exactly what `From`/`Into` is for right?I wish we could. :) The link Danilo sent should clarify why this is not possible. <snip>>> +impl_lossless_as!(u8 as { u16, u32, u64, usize }); >> +impl_lossless_as!(u16 as { u32, u64, usize }); >> +impl_lossless_as!(u32 as { u64, usize } ); > > I prefer consistency with the notation in num.rs in the rust standard library: > impl_from!(u16 => usize) > But I am Ok with your notation as well, which is concise.You're right, something like `impl_as` is both shorter and more consistent.> >> +// `u64` and `usize` have the same size on 64-bit platforms. >> +#[cfg(CONFIG_64BIT)] >> +impl_lossless_as!(u64 as { usize } ); >> + >> +// A `usize` fits into a `u64` on 32 and 64-bit platforms. >> +#[cfg(any(CONFIG_32BIT, CONFIG_64BIT))] >> +impl_lossless_as!(usize as { u64 }); >> + >> +// A `usize` fits into a `u32` on 32-bit platforms. >> +#[cfg(CONFIG_32BIT)] >> +impl_lossless_as!(usize as { u32 }); >> + >> +/// Extension trait providing guaranteed lossless conversion to `Self` from `T`. >> +/// >> +/// The standard library's `From` implementations do not cover conversions that are not portable or >> +/// future-proof. For instance, even though it is safe today, `From<usize>` is not implemented for >> +/// [`u64`] because of the possibility to support larger-than-64bit architectures in the future. >> +/// >> +/// The workaround is to either deal with the error handling of [`TryFrom`] for an operation that >> +/// technically cannot fail, or to use the `as` keyword, which can silently strip data if the >> +/// destination type is smaller than the source. >> +/// >> +/// Both options are hardly acceptable for the kernel. It is also a much more architecture >> +/// dependent environment, supporting only 32 and 64 bit architectures, with some modules >> +/// explicitly depending on a specific bus width that could greatly benefit from infallible >> +/// conversion operations. >> +/// >> +/// Thus this extension trait that provides, for the architecture the kernel is built for, safe >> +/// conversion between types for which such conversion is lossless. >> +/// >> +/// In other words, this trait is implemented if, for the current build target and with `t: T`, the >> +/// `t as Self` operation is completely lossless. >> +/// >> +/// Prefer this over the `as` keyword to ensure no lossy conversions are performed. >> +/// >> +/// If you need to perform a conversion in `const` context, use [`u64_as_usize`], >> +/// [`u32_as_usize`], [`usize_as_u64`], etc. >> +/// >> +/// # Examples >> +/// >> +/// ``` >> +/// use crate::num::FromAs; >> +/// >> +/// assert_eq!(usize::from_as(0xf00u32), 0xf00u32 as usize); > > This `from_as()` syntax will be very confusing for users IMO, honestly we should > just keep it as `from()`, otherwise there is confusion and ambiguity around > whether someone should use `::from()` or `::from_as()`. Please let us just keep > all infallible conversions to use `from()`/`into()` and all infallible ones to > use `try_from()`/`try_into()`. No need to additional `_as()` prefix, as it > creates confusion. In the end of the day, the fact the conversion is lossless is > not relevant, all conversions that don't use the `as` keyword should be expected > to be lossless right?I wanted to use `from`/`into` initially but this unfortunately clashes with the `From` and `Into` traits and forces callers to disambiguate which trait we want to use with a turbofish, which goes against the intent of keeping the syntax short. I'm happy to consider better names though.