Re: [PATCH v8 12/14] rust: hrtimer: add `HrTimerMode`

[Andreas Hindborg <a.hindborg@kernel.org>]

"Benno Lossin" <benno.lossin@proton.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