Re: [PATCH v3 10/16] rust: io: register: make register have a typed base
"Alexandre Courbot" <[email protected]>
| Newsgroups | dev.linux.lists.nova-gpu,dev.linux.lists.driver-core,org.freedesktop.lists.dri-devel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-pci,org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
On Thu Aug 27, 2026 at 11:32 PM JST, Gary Guo wrote: > On Thu Aug 27, 2026 at 2:42 PM BST, Alexandre Courbot wrote: >> On Wed Aug 19, 2026 at 8:09 PM JST, Gary Guo wrote: >>> Previously `register!` defined registers can be used on any untyped I/O >>> regions. With all users specifying their desired register type now, >>> propagate the specified type and restrict I/O access only when type >>> matches. >>> >>> Also, add an `io_project!` example which is enabled by this change. >>> >>> Signed-off-by: Gary Guo <[email protected]> >>> --- >>> rust/kernel/io.rs | 13 +++++++++++ >>> rust/kernel/io/register.rs | 55 ++++++++++++++++++++++++++++++++-------------- >>> rust/macros/io/register.rs | 34 ++++++++++++++-------------- >>> 3 files changed, 68 insertions(+), 34 deletions(-) >>> >>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs >>> index 84dd876b3407..4542187d6b91 100644 >>> --- a/rust/kernel/io.rs >>> +++ b/rust/kernel/io.rs >>> @@ -1692,21 +1692,34 @@ pub fn project_loc<U, L>(self, location: L) -> <T::Backend as IoBackend>::View<' >>> /// The syntax is of form `io_project!(io, proj)` where `io` is an expression to a type that >>> /// implements [`Io`] and `proj` is a [projection specification](kernel::ptr::project!). >>> /// >>> +/// `io_project!` can also project to subview of registers defined with [`register!`] macro. >> >> nit: "to a subview". >> >> <...> >>> +/// Helper function for register alias implementation. >>> +/// >>> +/// This is used to enforce base matching. Only called during const eval. >>> +#[doc(hidden)] >>> +#[inline(always)] >>> +pub const fn alias_offset<Base: ?Sized, Alias: Register<Base = Base>>() -> usize { >>> + Alias::OFFSET >>> +} >>> + >>> +/// Helper function for register element alias implementation. >>> +/// >>> +/// This is used to enforce base matching and provide bounds checking. Only called during const >>> +/// eval. >>> +#[doc(hidden)] >>> +#[inline(always)] >>> +pub const fn element_alias_offset<Base: ?Sized, Alias: RegisterArray<Base = Base>>( >>> + idx: usize, >>> +) -> usize { >>> + build_assert!(idx < Alias::SIZE); >>> + Alias::OFFSET + idx * Alias::STRIDE >>> +} >> >> You could convert the `build_assert!` (always good to eschew) into a >> `const_assert!` if you turn `idx` into a generic parameter: >> >> pub const fn element_alias_offset< >> Base: ?Sized, >> Alias: RegisterArray<Base = Base>, >> const IDX: usize, >> >() -> usize { >> crate::const_assert!(IDX < Alias::SIZE); >> Alias::OFFSET + IDX * Alias::STRIDE >> } >> >> You get a better error message, the const parameter is used in a >> very controlled environment well within the expressive power of const >> generics, and that function is not public interface anyway. >> >> If you don't like it (recent discussions make me think you might not >> :)), then let's remove the `build_assert!` and keep the `static_assert` >> emitted by the macro until this patch; it's not as elegant as checking >> the condition into the same block of code that uses it, but again we are >> in a controlled environment and static_assert > const_assert > >> build_assert so this may actually be my preferred solution. You can >> document the invariant in `element_alias_offset`. > > The error message should be identical? This function is only called during const > evaluation, so even if I replace this build_assert to assert it'll be fine and > never actually cause linker error or runtime panic. Indeed, I missed that. Even in this case, it's still better to use something the compiler enforces can only run at const eval. `build_assert` is something that in an ideal world shouldn't need to exist, so let's avoid proliferating its use if we can avoid it. > > I wonder if I should add > > assert_in_const_eval() > > or maybe > > #[const_eval_only] > fn ... > > which expands to > > #[inline(always)] > fn const ... { > assert_in_const_eval() > } > > so that it's clear to reader that this is a const-eval-only function. If that doesn't delay the series, sure.