Message-Id: <DAKIBXCX5OZ1.2ACJKVK1K4TER@kernel.org>
Date: Thu, 12 Jun 2025 13:10:48 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Michal Wilczynski" <m.wilczynski@...sung.com>, "Miguel Ojeda"
 <ojeda@...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>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Danilo Krummrich" <dakr@...nel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>,
 Uwe Kleine-König <ukleinek@...nel.org>
Cc: <linux-kernel@...r.kernel.org>, <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div
 helper

On Thu Jun 12, 2025 at 1:00 PM CEST, Michal Wilczynski wrote:
> On 6/10/25 00:21, Benno Lossin wrote:
>> On Mon Jun 9, 2025 at 11:53 PM CEST, Michal Wilczynski wrote:
>>> +    /// * None if the divisor is zero.
>>> +    fn mul_div(self, multiplier: Self, divisor: Self) -> Option<Self>;
>>> +}
>>> +
>>> +impl KernelMathExt for u64 {
>> 
>> Can you also implement this for the other types that have a
>> `mul_..._div` function in the kernel?
>
> +Uwe
>
> Hi Benno,
>
> Thank you for the review and feedback on the patch.
>
> Regarding your suggestion to implement the trait for other types, I've
> taken a closer look at the existing kernel helpers (e.g., in
> lib/math/div64.c). My finding is that while some signed division helpers
> exist, there don't appear to be standard, exported mul_sX_sX_div_sX
> functions that are direct equivalents of the u64 version. This makes the
> generic trait idea less broadly applicable than I initially hoped.
>
> Furthermore, a more significant issue has come to my attention regarding
> the u64 C function itself. The x86-specific static inline implementation
> uses assembly that triggers a #DE (Divide Error) exception if the final
> quotient overflows the 64-bit result. This would lead to a kernel panic.
>
> /*
>  * Will generate an #DE when the result doesn't fit u64, could fix with an
>  * __ex_table[] entry when it becomes an issue.
>  */
> static inline u64 mul_u64_u64_div_u64(...)
>
> Given that a direct FFI binding would expose this potentially panicking
> behavior to safe Rust, I am now reconsidering if binding this function
> is the right approach.
>
> Perhaps this should be handled in pure Rust. For my specific case with
> the PWM driver, it's likely that a simple checked_mul would be
> sufficient. In practice, many drivers use this function for calculations
> like the following, where one of the multiplicands is not a full 64-bit
> value:
> wf->duty_offset_ns = DIV64_U64_ROUND_UP((u64)wfhw->duty_offset_cnt * NSEC_PER_SEC,
>                                         ddata->clk_rate_hz);
>
> In this common pattern, the intermediate multiplication often does not
> risk overflowing a u64. Using checked_mul would be completely safe and
> avoid the FFI complexity and the overflow risk entirely.
>
> Given these points, do you still think pursuing the general-purpose
> KernelMathExt trait via an FFI wrapper is the desired direction? Or
> would you prefer that I pivot to a simpler, safer, pure-Rust approach
> using checked_mul directly within the PWM driver where it is needed?

My understanding was that your use-case required the multiplication &
division operation to work even if `a * b` would overflow and only the
division by `c` would bring it back into u64 range. If you don't need
that, then I would just use `checked_mul` as it is much simpler.

We can always reintroduce a `KernelMathExt` trait later when the need
arises.

---
Cheers,
Benno

Powered by blists - more mailing lists