Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`
From: Andreas Hindborg <a.hindborg@kernel.org>
Date: 2025-02-21 12:17:36
"Benno Lossin" [off-list ref] writes:
On 21.02.25 10:17, Andreas Hindborg wrote:quoted
"Benno Lossin" [off-list ref] writes:quoted
On 18.02.25 14:27, Andreas Hindborg wrote:quoted
+/// 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.20situationAh yeah I remember that. And also the complication about certain combinations not being allowed.quoted
quoted
quoted
+ +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. 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. 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. Best regards, Andreas Hindborg