Message-ID: <59728663-3baf-449a-b0af-931e9490db19@proton.me>
Date: Fri, 21 Feb 2025 10:19:34 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Miguel Ojeda <ojeda@...nel.org>, Anna-Maria Behnsen <anna-maria@...utronix.de>, Frederic Weisbecker <frederic@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, Lyude Paul <lyude@...hat.com>, Guangbo Cui <2407018371@...com>, Dirk Behme <dirk.behme@...il.com>, Daniel Almeida <daniel.almeida@...labora.com>, Tamir Duberstein <tamird@...il.com>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org, "Timothy G." <c6qchwbke@...ay.firefox.com>
Subject: Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`

On 21.02.25 10:17, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@...ton.me> 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?

---
Cheers,
Benno

Powered by blists - more mailing lists