[RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

[RFC PATCH 0/3] gpu: nova-core: add basic timer subdevice implementation

[Alexandre Courbot]

Hi everyone,

This short RFC is based on top of Danilo's initial driver stub series
[1] and has for goal to initiate discussions and hopefully some design
decisions using the simplest subdevice of the GPU (the timer) as an
example, before implementing more devices allowing the GPU
initialization sequence to progress (Falcon being the logical next step
so we can get the GSP rolling).

It is kept simple and short for that purpose, and to avoid bumping into
a wall with much more device code because my assumptions were incorrect.

This is my first time trying to write Rust kernel code, and some of my
questions below are probably due to me not understanding yet how to use
the core kernel interfaces. So before going further I thought it would
make sense to raise the most obvious questions that came to my mind
while writing this draft:

- Where and how to store subdevices. The timer device is currently a
  direct member of the GPU structure. It might work for GSP devices
  which are IIUC supposed to have at least a few fixed devices required
  to bring the GSP up ; but as a general rule this probably won't scale
  as not all subdevices are present on all GPU variants, or in the same
  numbers. So we will probably need to find an equivalent to the
  `subdev` linked list in Nouveau.

- BAR sharing between subdevices. Right now each subdevice gets access
  to the full BAR range. I am wondering whether we could not split it
  into the relevant slices for each-subdevice, and transfer ownership of
  each slice to the device that is supposed to use it. That way each
  register would have a single owner, which is arguably safer - but
  maybe not as flexible as we will need down the road?

- On a related note, since the BAR is behind a Devres its availability
  must first be secured before any hardware access using try_access().
  Doing this on a per-register or per-operation basis looks overkill, so
  all methods that access the BAR take a reference to it, allowing to
  call try_access() from the highest-level caller and thus reducing the
  number of times this needs to be performed. Doing so comes at the cost
  of an extra argument to most subdevice methods ; but also with the
  benefit that we don't need to put the BAR behind another Arc and share
  it across all subdevices. I don't know which design is better here,
  and input would be very welcome.

- We will probably need sometime like a `Subdevice` trait or something
  down the road, but I'll wait until we have more than one subdevice to
  think about it.

The first 2 patches are small additions to the core Rust modules, that
the following patches make use of and which might be useful for other
drivers as well. The last patch is the naive implementation of the timer
device. I don't expect it to stay this way at all, so please point out
all the deficiencies in this very early code! :)

[1] https://lore.kernel.org/nouveau/20250209173048.17398-1-dakr@kernel.org/

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
Alexandre Courbot (3):
      rust: add useful ops for u64
      rust: make ETIMEDOUT error available
      gpu: nova-core: add basic timer device

 drivers/gpu/nova-core/driver.rs    |  4 +-
 drivers/gpu/nova-core/gpu.rs       | 35 ++++++++++++++-
 drivers/gpu/nova-core/nova_core.rs |  1 +
 drivers/gpu/nova-core/regs.rs      | 43 ++++++++++++++++++
 drivers/gpu/nova-core/timer.rs     | 91 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/error.rs               |  1 +
 rust/kernel/lib.rs                 |  1 +
 rust/kernel/num.rs                 | 32 ++++++++++++++
 8 files changed, 206 insertions(+), 2 deletions(-)
---
base-commit: 6484e46f33eac8dd42aa36fa56b51d8daa5ae1c1
change-id: 20250216-nova_timer-c69430184f54

Best regards,
-- 
Alexandre Courbot <acourbot@nvidia.com>

[PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

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.

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 <acourbot@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) {
+        ((self >> u32::BITS) as u32, self as u32)
+    }
+}

-- 
2.48.1

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Sergio González Collado]

On Mon, 17 Feb 2025 at 15:07, Alexandre Courbot <acourbot@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.
>
> 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 <acourbot@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) {
> +        ((self >> u32::BITS) as u32, self as u32)
> +    }
> +}
>
> --
> 2.48.1
>
>
Looks good :)
Reviewed-by: Sergio González Collado <sergio.collado@gmail.com>

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Daniel Almeida]

Hi Alex, 

> On 17 Feb 2025, at 11:04, Alexandre Courbot <acourbot@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.


> 
> 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 <acourbot@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.

> +        ((self >> u32::BITS) as u32, self as u32)
> +    }
> +}
> 
> -- 
> 2.48.1
> 

— Daniel
> 

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

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 <acourbot@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 <acourbot@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.

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Timur Tabi]

On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote:
> > 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.

I'm new to Rust, so let me see if I get this right.

struct SplitU64 {
  high: u32,
  low: u32
}

So if you want to extract the upper 32 bits of a u64, you have to do this:

let split = some_u64.into_u32s();
let some_u32 = split.high;

as opposed to your original design:

let (some_u32, _) = some_u64.into_u32s();

Personally, I prefer the latter.  The other advantage is that into_u32s and
from_u32s are reciprocal:

assert_eq!(u64::from_u32s(u64::into_u32s(some_u64)), some_u64);

(or something like that)

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote:
> On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote:
>> > 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.
>
> I'm new to Rust, so let me see if I get this right.
>
> struct SplitU64 {
>   high: u32,
>   low: u32
> }
>
> So if you want to extract the upper 32 bits of a u64, you have to do this:
>
> let split = some_u64.into_u32s();
> let some_u32 = split.high;

More likely this would be something like:

  let SplitU64 { high: some_u32, .. } = some_u64;

Which is still a bit verbose, but a single-liner.

Actually. How about adding methods to this trait that return either
component?

  let some_u32 = some_u64.high_half();
  let another_u32 = some_u64.low_half();

These should be used most of the times, and using destructuring/tuple
would only be useful for a few select cases.

>
> as opposed to your original design:
>
> let (some_u32, _) = some_u64.into_u32s();
>
> Personally, I prefer the latter.  The other advantage is that into_u32s and
> from_u32s are reciprocal:
>
> assert_eq!(u64::from_u32s(u64::into_u32s(some_u64)), some_u64);
>
> (or something like that)

Yeah, having symmetry is definitely nice. OTOH there are no safeguards
against mixing up the order and the high and low components, so a
compromise will have to be made one way or the other. But if we also add
the methods I proposed above, that question should matter less.

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[John Hubbard]

On 2/18/25 5:21 PM, Alexandre Courbot wrote:
> On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote:
>> On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote:
...
> More likely this would be something like:
> 
>    let SplitU64 { high: some_u32, .. } = some_u64;
> 
> Which is still a bit verbose, but a single-liner.
> 
> Actually. How about adding methods to this trait that return either
> component?
> 
>    let some_u32 = some_u64.high_half();
>    let another_u32 = some_u64.low_half();
> 
> These should be used most of the times, and using destructuring/tuple
> would only be useful for a few select cases.

I think I like this approach best so far, because that is actually how
drivers tend to use these values: one or the other 32 bits at a time.
Registers are often grouped into 32-bit named registers, and driver code
wants to refer to them one at a time (before breaking some of them down
into smaller named fields)>

The .high_half() and .low_half() approach matches that very closely.
And it's simpler to read than the SplitU64 API, without losing anything
we need, right?

  
thanks,
-- 
John Hubbard

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

On Wed Feb 19, 2025 at 12:24 PM JST, John Hubbard wrote:
> On 2/18/25 5:21 PM, Alexandre Courbot wrote:
>> On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote:
>>> On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote:
> ...
>> More likely this would be something like:
>> 
>>    let SplitU64 { high: some_u32, .. } = some_u64;
>> 
>> Which is still a bit verbose, but a single-liner.
>> 
>> Actually. How about adding methods to this trait that return either
>> component?
>> 
>>    let some_u32 = some_u64.high_half();
>>    let another_u32 = some_u64.low_half();
>> 
>> These should be used most of the times, and using destructuring/tuple
>> would only be useful for a few select cases.
>
> I think I like this approach best so far, because that is actually how
> drivers tend to use these values: one or the other 32 bits at a time.
> Registers are often grouped into 32-bit named registers, and driver code
> wants to refer to them one at a time (before breaking some of them down
> into smaller named fields)>
>
> The .high_half() and .low_half() approach matches that very closely.
> And it's simpler to read than the SplitU64 API, without losing anything
> we need, right?

Yes, that looks like the optimal way to do this actually. It also
doesn't introduce any overhead as the destructuring was doing both
high_half() and low_half() in sequence, so in some cases it might
even be more efficient.

I'd just like to find a better naming. high() and low() might be enough?
Or are there other suggestions?

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[John Hubbard]

On 2/19/25 4:51 AM, Alexandre Courbot wrote:
> Yes, that looks like the optimal way to do this actually. It also
> doesn't introduce any overhead as the destructuring was doing both
> high_half() and low_half() in sequence, so in some cases it might
> even be more efficient.
> 
> I'd just like to find a better naming. high() and low() might be enough?
> Or are there other suggestions?
> 

Maybe use "32" instead of "half":

     .high_32()  / .low_32()
     .upper_32() / .lower_32()


thanks,
-- 
John Hubbard

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Dave Airlie]

On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 2/19/25 4:51 AM, Alexandre Courbot wrote:
> > Yes, that looks like the optimal way to do this actually. It also
> > doesn't introduce any overhead as the destructuring was doing both
> > high_half() and low_half() in sequence, so in some cases it might
> > even be more efficient.
> >
> > I'd just like to find a better naming. high() and low() might be enough?
> > Or are there other suggestions?
> >
>
> Maybe use "32" instead of "half":
>
>      .high_32()  / .low_32()
>      .upper_32() / .lower_32()
>

The C code currently does upper_32_bits and lower_32_bits, do we want
to align or diverge here?

Dave.

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Daniel Almeida]

> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote:
> 
> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote:
>> 
>> On 2/19/25 4:51 AM, Alexandre Courbot wrote:
>>> Yes, that looks like the optimal way to do this actually. It also
>>> doesn't introduce any overhead as the destructuring was doing both
>>> high_half() and low_half() in sequence, so in some cases it might
>>> even be more efficient.
>>> 
>>> I'd just like to find a better naming. high() and low() might be enough?
>>> Or are there other suggestions?
>>> 
>> 
>> Maybe use "32" instead of "half":
>> 
>>     .high_32()  / .low_32()
>>     .upper_32() / .lower_32()
>> 
> 
> The C code currently does upper_32_bits and lower_32_bits, do we want
> to align or diverge here?
> 
> Dave.


My humble suggestion here is to use the same nomenclature. `upper_32_bits` and
`lower_32_bits` immediately and succinctly informs the reader of what is going on.

— Daniel

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[John Hubbard]

On 2/19/25 3:13 PM, Daniel Almeida wrote:
>> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote:
>> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote:
>>> On 2/19/25 4:51 AM, Alexandre Courbot wrote:
>>>> Yes, that looks like the optimal way to do this actually. It also
>>>> doesn't introduce any overhead as the destructuring was doing both
>>>> high_half() and low_half() in sequence, so in some cases it might
>>>> even be more efficient.
>>>>
>>>> I'd just like to find a better naming. high() and low() might be enough?
>>>> Or are there other suggestions?
>>>>
>>>
>>> Maybe use "32" instead of "half":
>>>
>>>      .high_32()  / .low_32()
>>>      .upper_32() / .lower_32()
>>>
>>
>> The C code currently does upper_32_bits and lower_32_bits, do we want
>> to align or diverge here?

This sounds like a trick question, so I'm going to go with..."align". haha :)

>>
>> Dave.
> 
> 
> My humble suggestion here is to use the same nomenclature. `upper_32_bits` and
> `lower_32_bits` immediately and succinctly informs the reader of what is going on.
> 

Yes. I missed the pre-existing naming in C, but since we have it and it's
well-named as well, definitely this is the way to go.

thanks,
-- 
John Hubbard

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

On Thu Feb 20, 2025 at 9:14 AM JST, John Hubbard wrote:
> On 2/19/25 3:13 PM, Daniel Almeida wrote:
>>> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote:
>>> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote:
>>>> On 2/19/25 4:51 AM, Alexandre Courbot wrote:
>>>>> Yes, that looks like the optimal way to do this actually. It also
>>>>> doesn't introduce any overhead as the destructuring was doing both
>>>>> high_half() and low_half() in sequence, so in some cases it might
>>>>> even be more efficient.
>>>>>
>>>>> I'd just like to find a better naming. high() and low() might be enough?
>>>>> Or are there other suggestions?
>>>>>
>>>>
>>>> Maybe use "32" instead of "half":
>>>>
>>>>      .high_32()  / .low_32()
>>>>      .upper_32() / .lower_32()
>>>>
>>>
>>> The C code currently does upper_32_bits and lower_32_bits, do we want
>>> to align or diverge here?
>
> This sounds like a trick question, so I'm going to go with..."align". haha :)
>
>>>
>>> Dave.
>> 
>> 
>> My humble suggestion here is to use the same nomenclature. `upper_32_bits` and
>> `lower_32_bits` immediately and succinctly informs the reader of what is going on.
>> 
>
> Yes. I missed the pre-existing naming in C, but since we have it and it's
> well-named as well, definitely this is the way to go.

Agreed, I wasn't aware of the C equivalents either, but since they exist
we should definitely use the same naming scheme.

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Danilo Krummrich]

On Fri, Feb 21, 2025 at 08:35:54PM +0900, Alexandre Courbot wrote:
> On Thu Feb 20, 2025 at 9:14 AM JST, John Hubbard wrote:
> > On 2/19/25 3:13 PM, Daniel Almeida wrote:
> >>> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote:
> >>> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote:
> >>>> On 2/19/25 4:51 AM, Alexandre Courbot wrote:
> >>>>> Yes, that looks like the optimal way to do this actually. It also
> >>>>> doesn't introduce any overhead as the destructuring was doing both
> >>>>> high_half() and low_half() in sequence, so in some cases it might
> >>>>> even be more efficient.
> >>>>>
> >>>>> I'd just like to find a better naming. high() and low() might be enough?
> >>>>> Or are there other suggestions?
> >>>>>
> >>>>
> >>>> Maybe use "32" instead of "half":
> >>>>
> >>>>      .high_32()  / .low_32()
> >>>>      .upper_32() / .lower_32()
> >>>>
> >>>
> >>> The C code currently does upper_32_bits and lower_32_bits, do we want
> >>> to align or diverge here?
> >
> > This sounds like a trick question, so I'm going to go with..."align". haha :)
> >
> >>>
> >>> Dave.
> >> 
> >> 
> >> My humble suggestion here is to use the same nomenclature. `upper_32_bits` and
> >> `lower_32_bits` immediately and succinctly informs the reader of what is going on.
> >> 
> >
> > Yes. I missed the pre-existing naming in C, but since we have it and it's
> > well-named as well, definitely this is the way to go.
> 
> Agreed, I wasn't aware of the C equivalents either, but since they exist
> we should definitely use the same naming scheme.

IIUC, we're still talking about extending the u64 primitive type.

Hence, I think there is no necessity to do align with the corresponding C
nameing scheme. I think this would only be the case if we'd write an abstraction
for the C API.

In this case though we extend an existing Rust type, so we should do something
that aligns with the corresponding Rust type.

In this specific case I think it goes hand in hand though.

- Danilo

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Sergio González Collado]

> Actually. How about adding methods to this trait that return either
> component?
>
>   let some_u32 = some_u64.high_half();
>   let another_u32 = some_u64.low_half();
>
> These should be used most of the times, and using destructuring/tuple
> would only be useful for a few select cases.
>

Indeed very nice!

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Dirk Behme]

On 17/02/2025 15:04, Alexandre Courbot 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.
> 
> 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 <acourbot@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) {
> +        ((self >> u32::BITS) as u32, self as u32)
> +    }
> +}
Just as a question: Would it make sense to make this more generic?

For example

u64 -> u32, u32 / u32, u32 -> u64 (as done here)
u32 -> u16, u16 / u16, u16 -> u32
u16 -> u8, u8 / u8, u8 -> u16

Additionally, I wonder if this might be combined with the Integer trait
[1]? But the usize and signed ones might not make sense here...

Dirk

[1] E.g.

https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Alexandre Courbot]

On Tue Feb 18, 2025 at 7:07 PM JST, Dirk Behme wrote:
> On 17/02/2025 15:04, Alexandre Courbot 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.
>> 
>> 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 <acourbot@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) {
>> +        ((self >> u32::BITS) as u32, self as u32)
>> +    }
>> +}
> Just as a question: Would it make sense to make this more generic?
>
> For example
>
> u64 -> u32, u32 / u32, u32 -> u64 (as done here)
> u32 -> u16, u16 / u16, u16 -> u32
> u16 -> u8, u8 / u8, u8 -> u16
>
> Additionally, I wonder if this might be combined with the Integer trait
> [1]? But the usize and signed ones might not make sense here...
>
> Dirk
>
> [1] E.g.
>
> https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0

I agree something more generic would be nice. One drawback I see though
is that it would have to use more generic (and lengthy) method names -
i.e. `from_components(u32, u32)` instead of `from_u32s`.

I quickly tried to write a completely generic trait where the methods
are auto-implemented from constants and associated types, but got stuck
by the impossibility to use `as` in that context without a macro.

Regardless, I was looking for an already existing trait/module to
leverage instead of introducing a whole new one, maybe the one you
linked is what I was looking for?

Re: [PATCH RFC 1/3] rust: add useful ops for u64

[Dirk Behme]

On 18/02/2025 14:07, Alexandre Courbot wrote:
> On Tue Feb 18, 2025 at 7:07 PM JST, Dirk Behme wrote:
>> On 17/02/2025 15:04, Alexandre Courbot 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.
>>>
>>> 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 <acourbot@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) {
>>> +        ((self >> u32::BITS) as u32, self as u32)
>>> +    }
>>> +}
>> Just as a question: Would it make sense to make this more generic?
>>
>> For example
>>
>> u64 -> u32, u32 / u32, u32 -> u64 (as done here)
>> u32 -> u16, u16 / u16, u16 -> u32
>> u16 -> u8, u8 / u8, u8 -> u16
>>
>> Additionally, I wonder if this might be combined with the Integer trait
>> [1]? But the usize and signed ones might not make sense here...
>>
>> Dirk
>>
>> [1] E.g.
>>
>> https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0
> 
> I agree something more generic would be nice. One drawback I see though
> is that it would have to use more generic (and lengthy) method names -
> i.e. `from_components(u32, u32)` instead of `from_u32s`.
> 
> I quickly tried to write a completely generic trait where the methods
> are auto-implemented from constants and associated types, but got stuck
> by the impossibility to use `as` in that context without a macro.


Being inspired by the Integer trait example [1] above, just as an idea,
I wonder if anything like

impl_split_merge! {
    (u64, u32),
    (u32, u16),
    (u16, u8),
}

would be implementable?


> Regardless, I was looking for an already existing trait/module to
> leverage instead of introducing a whole new one, maybe the one you
> linked is what I was looking for?
Cheers,

Dirk

[PATCH RFC 2/3] rust: make ETIMEDOUT error available

[Alexandre Courbot]

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/error.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index f6ecf09cb65f4ebe9b88da68b3830ae79aa4f182..8858eb13b3df674b54572d2a371b8ec1303492dd 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -64,6 +64,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(ETIMEDOUT, "Connection timed out.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");

-- 
2.48.1

Re: [PATCH RFC 2/3] rust: make ETIMEDOUT error available

[Daniel Almeida]

Hi Alex,

> On 17 Feb 2025, at 11:04, Alexandre Courbot <acourbot@nvidia.com> wrote:
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> rust/kernel/error.rs | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f4ebe9b88da68b3830ae79aa4f182..8858eb13b3df674b54572d2a371b8ec1303492dd 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
>     declare_err!(EPIPE, "Broken pipe.");
>     declare_err!(EDOM, "Math argument out of domain of func.");
>     declare_err!(ERANGE, "Math result not representable.");
> +    declare_err!(ETIMEDOUT, "Connection timed out.");
>     declare_err!(ERESTARTSYS, "Restart the system call.");
>     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
>     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> 
> -- 
> 2.48.1
> 
> 

FYI this is a conflict with https://lore.kernel.org/rust-for-linux/20250207132623.168854-8-fujita.tomonori@gmail.com/

[PATCH RFC 3/3] gpu: nova-core: add basic timer device

[Alexandre Courbot]

Add a basic timer device and exercise it during device probing. This
first draft is probably very questionable.

One point in particular which should IMHO receive attention: the generic
wait_on() method aims at providing similar functionality to Nouveau's
nvkm_[num]sec() macros. Since this method will be heavily used with
different conditions to test, I'd like to avoid monomorphizing it
entirely with each instance ; that's something that is achieved in
nvkm_xsec() using functions that the macros invoke.

I have tried achieving the same result in Rust using closures (kept
as-is in the current code), but they seem to be monomorphized as well.
Calling extra functions could work better, but looks also less elegant
to me, so I am really open to suggestions here.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpu/nova-core/driver.rs    |  4 +