Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`
From: Benno Lossin
Date: Sat Feb 22 2025 - 04:34:58 EST
On 21.02.25 12:39, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>
>> On 21.02.25 10:17, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@xxxxxxxxx> writes:
>>>
>>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>>> +/// Operational mode of [`HrTimer`].
>>>>> +#[derive(Clone, Copy)]
>>>>> +pub enum HrTimerMode {
>>>>> + /// Timer expires at the given expiration time.
>>>>> + Absolute,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + Relative,
>>>>> + /// Timer does not move between CPU cores.
>>>>> + Pinned,
>>>>> + /// Timer handler is executed in soft irq context.
>>>>> + Soft,
>>>>> + /// Timer handler is executed in hard irq context.
>>>>> + Hard,
>>>>> + /// Timer expires at the given expiration time.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + AbsolutePinned,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + RelativePinned,
>>>>> + /// Timer expires at the given expiration time.
>>>>> + /// Timer handler is executed in soft irq context.
>>>>> + AbsoluteSoft,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + /// Timer handler is executed in soft irq context.
>>>>> + RelativeSoft,
>>>>> + /// Timer expires at the given expiration time.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + /// Timer handler is executed in soft irq context.
>>>>> + AbsolutePinnedSoft,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + /// Timer handler is executed in soft irq context.
>>>>> + RelativePinnedSoft,
>>>>> + /// Timer expires at the given expiration time.
>>>>> + /// Timer handler is executed in hard irq context.
>>>>> + AbsoluteHard,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + /// Timer handler is executed in hard irq context.
>>>>> + RelativeHard,
>>>>> + /// Timer expires at the given expiration time.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + /// Timer handler is executed in hard irq context.
>>>>> + AbsolutePinnedHard,
>>>>> + /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> + /// Timer does not move between CPU cores.
>>>>> + /// Timer handler is executed in hard irq context.
>>>>> + RelativePinnedHard,
>>>>> +}
>>>>
>>>> At some point we probably want to move this to bitfields, or do you
>>>> think it's better to keep it like this?
>>>
>>> Yes, eventually the would transition. The main difficulty is that not
>>> all flag combinations are legal, and the zero value is also a flag.
>>> There was some promising work being shared on Zulip for this [1], but I
>>> don't think it is completed yet. Added Timothy to CC.
>>>
>>> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation
>>
>> Ah yeah I remember that. And also the complication about certain
>> combinations not being allowed.
>>
>>>>> +
>>>>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>>>>> + fn from(value: HrTimerMode) -> Self {
>>>>> + use bindings::*;
>>>>> + match value {
>>>>> + HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>>>>> + HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>>>>> + HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>>>>> + HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>>>>> + HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>>>>> + HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>>>>> + HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>>>>> + HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>>>>> + HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>>>>> + HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>>>>> + HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>>>>> + HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>>>>> + HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>>>>> + HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>>>>> + HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>>>>> + }
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +impl From<HrTimerMode> for u64 {
>>>>> + fn from(value: HrTimerMode) -> Self {
>>>>> + Into::<bindings::hrtimer_mode>::into(value) as u64
>>>>> + }
>>>>> +}
>>>>
>>>> Hmm do drivers really need these impls? If not, then you could also just
>>>> have a private function that does the conversion and use it only in the
>>>> abstraction layer.
>>>
>>> Similar to the other impls you commented on, I can move them private. I
>>> would prefer using `From` rather than some other function.
>>
>> What's the reason for you preferring `From`? I don't think it's
>> important to forbid access from the drivers, but if it's unnecessary,
>> why would we give them access in the first place?
>
> TIL trait implementations cannot be hidden.
Yeah trait impls are always public.
> I like `From` because every single rust developer seeing a `From`
> implementation will immediately know what it does. It is more
> idiomatic in that way than having another conversion function. I think
> using existing traits with well defined semantics is preferable to a
> local function.
Well yes, but that only holds if other people need to use it.
> But since driver implementer do not need it, I'm undecided. If you want
> them converted to a private function I can do that, for all the ones you
> called out. But I am also OK with keeping as is. You decide.
I think it's better to keep these enums somewhat opaque.
---
Cheers,
Benno