Re: [PATCH v4 2/2] rust: hrtimer: Make HrTimer repr(transparent)
FUJITA Tomonori <[email protected]>
| Newsgroups | org.kernel.vger.rust-for-linux |
|---|---|
| Message-ID | <[email protected]> |
On Mon, 10 Aug 2026 15:28:06 +0200 Andreas Hindborg <[email protected]> wrote: > FUJITA Tomonori <[email protected]> writes: > >> On Mon, 10 Aug 2026 13:07:20 +0200 >> Andreas Hindborg <[email protected]> wrote: >> >>> "FUJITA Tomonori" <[email protected]> writes: >>> >>>> From: FUJITA Tomonori <[email protected]> >>>> >>>> HrTimerCallbackContext acquires a &HrTimer<T> from a >>>> NonNull<HrTimer<T>> while a &mut HrTimer<T> can exist at the same >>>> time. This is sound only because HrTimer's sole field is >>>> Opaque<bindings::hrtimer>, which puts every byte behind an UnsafeCell. >>>> Adding a field to HrTimer that is not Opaque would make acquiring that >>>> shared reference unsound. >>>> >>>> Make HrTimer repr(transparent), which prevents multiple fields, so that >>>> such a refactor fails to compile instead of silently introducing >>>> unsoundness. This does not guarantee the remaining field stays behind >>>> Opaque, but it rules out the likely way of getting there. >>>> >>>> repr(transparent) cannot be combined with repr(C), so drop the latter. >>>> >>>> Suggested-by: Miguel Ojeda <[email protected]> >>>> Signed-off-by: FUJITA Tomonori <[email protected]> >>>> --- >>>> rust/kernel/time/hrtimer.rs | 6 +++++- >>>> rust/kernel/time/hrtimer/arc.rs | 2 +- >>>> rust/kernel/time/hrtimer/pin.rs | 2 +- >>>> rust/kernel/time/hrtimer/pin_mut.rs | 2 +- >>>> rust/kernel/time/hrtimer/tbox.rs | 2 +- >>>> 5 files changed, 9 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs >>>> index 1db84cd4cbe8..04f47af3541b 100644 >>>> --- a/rust/kernel/time/hrtimer.rs >>>> +++ b/rust/kernel/time/hrtimer.rs >>>> @@ -418,8 +418,12 @@ >>>> /// # Invariants >>>> /// >>>> /// * `self.timer` is initialized by `bindings::hrtimer_setup`. >>>> +// `repr(transparent)` is not merely about layout. `HrTimerCallbackContext` acquires a >>>> +// `&HrTimer<T>` while a `&mut HrTimer<T>` may exist, which is sound only because every byte of >>>> +// this type sits inside `Opaque`. Being transparent rejects a second field at compile time, >>>> +// but it does not enforce that the remaining field stays `Opaque`. >>> >>> Missing bullet. With that fixed: >>> >>> Reviewed-by: Andreas Hindborg <[email protected]> >> >> This is a plain `//` comment, not documentation. Did you mean that you >> want it documented as one of the `# Invariants` bullets instead? > > Ah, thanks for clarifying, I did not see that. No I guess it is fine. > Maybe add a newline? Documentation/rust/coding-guidelines.rst gives an example where a comment follows the documentation with no blank line in between. The existing code follows that too, so I think we should stay consistent with the documented convention here.