All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Alexandre Courbot" <acourbot@nvidia.com>
To: "Christian" <christiansantoslima21@gmail.com>,
	"Miguel Ojeda" <ojeda@kernel.org>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <benno.lossin@proton.me>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Danilo Krummrich" <dakr@kernel.org>,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org,
	~lkcamp/patches@lists.sr.ht, richard120310@gmail.com
Subject: Re: [PATCH v9] rust: transmute: Add methods for FromBytes trait
Date: Thu, 21 Aug 2025 20:13:20 +0900	[thread overview]
Message-ID: <DC828042PKDV.16NDVKIGBNTJH@nvidia.com> (raw)
In-Reply-To: <CABm2a9cyvsXd1OS+tvDoDDEfMgBOJeetBVQU0x3m5E1vV1CApw@mail.gmail.com>

Hi Christian,

On Tue Aug 19, 2025 at 4:04 AM JST, Christian wrote:
> Hi, Alexandre.
>
> On Mon, Aug 18, 2025 at 8:28 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Wed Aug 13, 2025 at 3:00 AM JST, Christian wrote:
>> > Hi, Alexandre.
>> >
>> >> I mentioned it on v8 [1] and v7 [2], but the tests that break due to
>> >> this change need to be updated by this patch as well. This includes:
>> >>
>> >> * The doctests in `rust/kernel/dma.rs`,
>> >> * The `samples/rust/rust_dma.rs` sample,
>> >> * The example for `FromBytes` introduced by this patch which uses `?` without
>> >> defining a function.
>> >
>> > Sorry for my inattention, I'll fix this in the next version.
>>
>> Ah, as it turns out you might not need to.
>>
>> We discussed this patch a bit during the DRM sync, and the consensus was
>> that it would probably be better to keep things a bit simpler for the
>> first version. The `FromBytesSized` trait in particular was not very
>> popular; a better long-term way to provide implementations for
>> `FromBytes` would be to use a derive macro, but that's out of scope for
>> now.
>>
>> Instead, we agreed that the following would make a good first version:
>>
>> - Make the `FromBytes` trait depend on `Sized`,
>> - Provide default implementations for `from_bytes` and `from_bytes_mut`
>>   directly in the `FromBytes` trait,
>> - No implementation for slices for now,
>> - Consequently, no user code will break due to the addition of the
>>   methods, which is a big plus.
>>
>> The simpler version that would result from this covers all the immediate
>> use-cases and would be easier to merge, which will give us some time to
>> think about how to handle the non-sized use cases (probably via a derive
>> macro).
>>
>> Do you think you could write the next version along these lines?
>
> Yes, no problem.

Thanks - I also noticed a few other things while building with rustc
1.78 and clippy enabled:

- The `is_aligned` method for pointer types has only been stabilized
  with 1.79, meaning we need to explicitly enable the feature in
  `rust/kernel/lib.rs`:

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c0..c859a8984bae1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -18,6 +18,7 @@
 //
 // Stable since Rust 1.79.0.
 #![feature(inline_const)]
+#![feature(pointer_is_aligned)]
 //
 // Stable since Rust 1.81.0.
 #![feature(lint_reasons)]

(adding Miguel as a heads-up, and in case enabling a feature is not as easy as
I think :))

- Clippy was no happy with our use of `foo` in the doctest. This is easy to
fix:

diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
index 9bb707aabdc9a..9e4413fe9c32a 100644
--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -14,9 +14,9 @@
 /// ```
 /// use kernel::transmute::FromBytes;
 ///
-/// let foo = [1, 2, 3, 4];
+/// let raw = [1, 2, 3, 4];
 ///
-/// let result = u32::from_bytes(&foo).unwrap();
+/// let result = u32::from_bytes(&raw).unwrap();
 ///
 /// #[cfg(target_endian = "little")]
 /// assert_eq!(*result, 0x4030201);

- The patch adding `size_of*` to the prelude is not available yet, so for
safety we should probably import them:

diff --git a/rust/kernel/transmute.rs b/rust/kernel/transmute.rs
index 9e4413fe9c32a..3320e9e95057f 100644
--- a/rust/kernel/transmute.rs
+++ b/rust/kernel/transmute.rs
@@ -2,6 +2,8 @@

 //! Traits for transmuting types.

+use core::mem::{size_of, size_of_val};
+
 /// Types for which any bit pattern is valid.
 ///
 /// Not all types are valid for all values. For example, a `bool` must be either zero or one, so

With that, and the simplification discussed above, everything should be ok.

Do you think you can send the next iteration soonish? We have a consequent
patch series on nova-core that I would like to send which depends on this, and
since it is also very handy to have generally speaking it would be nice to have
it secured for the next merge window.

  reply	other threads:[~2025-08-21 11:13 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-11 21:38 [PATCH v9] rust: transmute: Add methods for FromBytes trait Christian S. Lima
2025-08-12 14:24 ` Alexandre Courbot
2025-08-12 18:00   ` Christian
2025-08-18 11:28     ` Alexandre Courbot
2025-08-18 12:20       ` Alice Ryhl
2025-08-18 19:04       ` Christian
2025-08-21 11:13         ` Alexandre Courbot [this message]
2025-08-21 13:50           ` Miguel Ojeda
2025-08-22  3:09             ` Alexandre Courbot
2025-08-22  8:07               ` Miguel Ojeda
2025-08-22 13:49                 ` Alexandre Courbot
2025-08-22 20:08                   ` Miguel Ojeda
2025-08-13  1:06 ` kernel test robot
2025-08-14  8:06 ` Benno Lossin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=DC828042PKDV.16NDVKIGBNTJH@nvidia.com \
    --to=acourbot@nvidia.com \
    --cc=a.hindborg@kernel.org \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=benno.lossin@proton.me \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=christiansantoslima21@gmail.com \
    --cc=dakr@kernel.org \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ojeda@kernel.org \
    --cc=richard120310@gmail.com \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=tmgross@umich.edu \
    --cc=~lkcamp/patches@lists.sr.ht \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.