Re: [PATCH v4 2/2] rust: hrtimer: Make HrTimer repr(transparent)

FUJITA Tomonori <[email protected]> writes:

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

Ok then.

Best regards,
Andreas Hindborg