[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87mseffyh7.fsf@kernel.org>
Date: Fri, 21 Feb 2025 10:17:40 +0100
From: Andreas Hindborg <a.hindborg@...nel.org>
To: "Benno Lossin" <benno.lossin@...ton.me>
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`
"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
>> +
>> +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.
Best regards,
Andreas Hindborg
Powered by blists - more mailing lists