Hi Daniel!
On Tue Feb 18, 2025 at 6:10 AM JST, Daniel Almeida wrote:
> Hi Alex,
>
>> On 17 Feb 2025, at 11:04, Alexandre Courbot <acour...@nvidia.com> wrote:
>>
>> It is common to build a u64 from its high and low parts obtained from
>> two 32-bit registers. Conversely, it is also common to split a u64 into
>> two u32s to write them into registers. Add an extension trait for u64
>> that implement these methods in a new `num` module.
>
> Thank you for working on that. I find myself doing this manually extremely
> often indeed.
Are you aware of existing upstream code that could benefit from this?
This would allow me to split that patch out of this series.
>
>
>>
>> It is expected that this trait will be extended with other useful
>> operations, and similar extension traits implemented for other types.
>>
>> Signed-off-by: Alexandre Courbot <acour...@nvidia.com>
>> ---
>> rust/kernel/lib.rs | 1 +
>> rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++
>> 2 files changed, 33 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index
>> 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77
>> 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -59,6 +59,7 @@
>> pub mod miscdevice;
>> #[cfg(CONFIG_NET)]
>> pub mod net;
>> +pub mod num;
>> pub mod of;
>> pub mod page;
>> #[cfg(CONFIG_PCI)]
>> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b
>> --- /dev/null
>> +++ b/rust/kernel/num.rs
>> @@ -0,0 +1,32 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Numerical and binary utilities for primitive types.
>> +
>> +/// Useful operations for `u64`.
>> +pub trait U64Ext {
>> + /// Build a `u64` by combining its `high` and `low` parts.
>> + ///
>> + /// ```
>> + /// use kernel::num::U64Ext;
>> + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef),
>> 0x01234567_89abcdef);
>> + /// ```
>> + fn from_u32s(high: u32, low: u32) -> Self;
>> +
>> + /// Returns the `(high, low)` u32s that constitute `self`.
>> + ///
>> + /// ```
>> + /// use kernel::num::U64Ext;
>> + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567,
>> 0x89abcdef));
>> + /// ```
>> + fn into_u32s(self) -> (u32, u32);
>> +}
>> +
>> +impl U64Ext for u64 {
>> + fn from_u32s(high: u32, low: u32) -> Self {
>> + ((high as u64) << u32::BITS) | low as u64
>> + }
>> +
>> + fn into_u32s(self) -> (u32, u32) {
>
> I wonder if a struct would make more sense here.
>
> Just recently I had to debug an issue where I forgot the
> right order for code I had just written. Something like:
>
> let (pgcount, pgsize) = foo(); where the function actually
> returned (pgsize, pgcount).
>
> A proper struct with `high` and `low` might be more verbose, but
> it rules out this issue.
Mmm indeed, so we would have client code looking like:
let SplitU64 { high, low } = some_u64.into_u32();
instead of
let (high, low) = some_u64.into_u32();
which is correct, and
let (low, high) = some_u64.into_u32();
which is incorrect, but is likely to not be caught.
Since the point of these methods is to avoid potential errors in what
otherwise appears to be trivial code, I agree it would be better to
avoid introducing a new trap because the elements of the returned tuple
are not clearly named. Not sure which is more idiomatic here.
