rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
       [not found] <CGME20250609215523eucas1p22f6d7d84b403badfb77af7df973b97a9@eucas1p2.samsung.com>
@ 2025-06-09 21:53 ` Michal Wilczynski
  2025-06-09 22:21   ` Benno Lossin
                     ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Michal Wilczynski @ 2025-06-09 21:53 UTC (permalink / raw)
  To: Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Marek Szyprowski
  Cc: linux-kernel, rust-for-linux, Michal Wilczynski

The PWM subsystem and other kernel modules often need to perform a
64 by 64-bit multiplication followed by a 64-bit division. Performing
this naively in Rust risks overflow on the intermediate multiplication.
The kernel provides the C helper 'mul_u64_u64_div_u64' for this exact
purpose.

Introduce a safe Rust wrapper for this function to make it available to
Rust drivers.

Following feedback from the mailing list [1], this functionality is
provided via a 'KernelMathExt' extension trait. This allows for
idiomatic, method style calls (e.g. val.mul_div()) and provides a
scalable pattern for adding helpers for other integer types in the
future.

The safe wrapper is named 'mul_div' and not 'mul_u64_u64_div_u64' [2]
because its behavior differs from the underlying C function. The C
helper traps on a division by zero, whereas this safe wrapper returns
`None`, thus exhibiting different and safer behavior.

This is required for the Rust PWM TH1520 driver [3].

[1] - https://lore.kernel.org/all/DAFQ19RBBSQL.3OGUXOQ0PA9YH@kernel.org/
[2] - https://lore.kernel.org/all/CANiq72kVvLogBSVKz0eRg6V4LDB1z7b-6y1WPLSQfXXLW7X3cw@mail.gmail.com/
[3] - https://lore.kernel.org/all/20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@samsung.com/

Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
---
 rust/kernel/lib.rs  |  1 +
 rust/kernel/math.rs | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 6b4774b2b1c37f4da1866e993be6230bc6715841..d652c92633b82525f37e5cd8a040d268e0c191d1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -85,6 +85,7 @@
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
 pub mod list;
+pub mod math;
 pub mod miscdevice;
 pub mod mm;
 #[cfg(CONFIG_NET)]
diff --git a/rust/kernel/math.rs b/rust/kernel/math.rs
new file mode 100644
index 0000000000000000000000000000000000000000..b89e23f9266117dcf96561fbf13b1c66a4851b48
--- /dev/null
+++ b/rust/kernel/math.rs
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2025 Samsung Electronics Co., Ltd.
+// Author: Michal Wilczynski <m.wilczynski@samsung.com>
+
+//! Safe wrappers for kernel math helpers.
+//!
+//! This module provides safe, idiomatic Rust wrappers for C functions, whose
+//! FFI bindings are auto-generated in the `bindings` crate.
+
+use crate::bindings;
+
+/// An extension trait that provides access to kernel math helpers on primitive integer types.
+pub trait KernelMathExt: Sized {
+    /// Multiplies self by `multiplier and divides by divisor.
+    ///
+    /// This wrapper around the kernel's `mul_u64_u64_div_u64` C helper ensures that no
+    /// overflow occurs during the intermediate multiplication.
+    ///
+    /// # Returns
+    /// * Some(result) if the division is successful.
+    /// * None if the divisor is zero.
+    fn mul_div(self, multiplier: Self, divisor: Self) -> Option<Self>;
+}
+
+impl KernelMathExt for u64 {
+    fn mul_div(self, multiplier: u64, divisor: u64) -> Option<u64> {
+        if divisor == 0 {
+            return None;
+        }
+        // SAFETY: The C function `mul_u64_u64_div_u64` is safe to call because the divisor
+        // is guaranteed to be non-zero. The FFI bindings use `u64`, matching our types.
+        Some(unsafe { bindings::mul_u64_u64_div_u64(self, multiplier, divisor) })
+    }
+}

---
base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
change-id: 20250609-math-rust-v1-d3989515e32e

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
  2025-06-09 21:53 ` [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper Michal Wilczynski
@ 2025-06-09 22:21   ` Benno Lossin
  2025-06-12 11:00     ` Michal Wilczynski
  2025-06-10 13:09   ` kernel test robot
  2025-06-13 13:32   ` Alexandre Courbot
  2 siblings, 1 reply; 6+ messages in thread
From: Benno Lossin @ 2025-06-09 22:21 UTC (permalink / raw)
  To: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Marek Szyprowski
  Cc: linux-kernel, rust-for-linux

On Mon Jun 9, 2025 at 11:53 PM CEST, Michal Wilczynski wrote:
> The PWM subsystem and other kernel modules often need to perform a
> 64 by 64-bit multiplication followed by a 64-bit division. Performing
> this naively in Rust risks overflow on the intermediate multiplication.
> The kernel provides the C helper 'mul_u64_u64_div_u64' for this exact
> purpose.
>
> Introduce a safe Rust wrapper for this function to make it available to
> Rust drivers.
>
> Following feedback from the mailing list [1], this functionality is

You could turn this into a `Suggested-by`.

> provided via a 'KernelMathExt' extension trait. This allows for
> idiomatic, method style calls (e.g. val.mul_div()) and provides a
> scalable pattern for adding helpers for other integer types in the
> future.
>
> The safe wrapper is named 'mul_div' and not 'mul_u64_u64_div_u64' [2]
> because its behavior differs from the underlying C function. The C
> helper traps on a division by zero, whereas this safe wrapper returns
> `None`, thus exhibiting different and safer behavior.
>
> This is required for the Rust PWM TH1520 driver [3].
>
> [1] - https://lore.kernel.org/all/DAFQ19RBBSQL.3OGUXOQ0PA9YH@kernel.org/
> [2] - https://lore.kernel.org/all/CANiq72kVvLogBSVKz0eRg6V4LDB1z7b-6y1WPLSQfXXLW7X3cw@mail.gmail.com/
> [3] - https://lore.kernel.org/all/20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@samsung.com/

Please use the `Link: https://... [.]` format.

>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  rust/kernel/lib.rs  |  1 +
>  rust/kernel/math.rs | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c37f4da1866e993be6230bc6715841..d652c92633b82525f37e5cd8a040d268e0c191d1 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -85,6 +85,7 @@
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
>  pub mod list;
> +pub mod math;
>  pub mod miscdevice;
>  pub mod mm;
>  #[cfg(CONFIG_NET)]
> diff --git a/rust/kernel/math.rs b/rust/kernel/math.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b89e23f9266117dcf96561fbf13b1c66a4851b48
> --- /dev/null
> +++ b/rust/kernel/math.rs
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Safe wrappers for kernel math helpers.

I wouldn't stress the fact that these are safe, they better be!

> +//!
> +//! This module provides safe, idiomatic Rust wrappers for C functions, whose
> +//! FFI bindings are auto-generated in the `bindings` crate.
> +
> +use crate::bindings;
> +
> +/// An extension trait that provides access to kernel math helpers on primitive integer types.
> +pub trait KernelMathExt: Sized {

We should add this trait to the prelude.

> +    /// Multiplies self by `multiplier and divides by divisor.
> +    ///
> +    /// This wrapper around the kernel's `mul_u64_u64_div_u64` C helper ensures that no

The trait shouldn't make a reference to `u64`.

> +    /// overflow occurs during the intermediate multiplication.
> +    ///
> +    /// # Returns
> +    /// * Some(result) if the division is successful.

Use backtics (`) for code in documentation and comments.

> +    /// * 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?

> +    fn mul_div(self, multiplier: u64, divisor: u64) -> Option<u64> {
> +        if divisor == 0 {
> +            return None;
> +        }
> +        // SAFETY: The C function `mul_u64_u64_div_u64` is safe to call because the divisor
> +        // is guaranteed to be non-zero. The FFI bindings use `u64`, matching our types.

Let's not talk about "safe to call", but rather say it like this:

    // SAFETY: `mul_u64_u64_div_u64` requires the divisor to be non-zero
    // which is checked above. It has no other requirements.

---
Cheers,
Benno

> +        Some(unsafe { bindings::mul_u64_u64_div_u64(self, multiplier, divisor) })
> +    }
> +}
>
> ---
> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
> change-id: 20250609-math-rust-v1-d3989515e32e
>
> Best regards,


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
  2025-06-09 21:53 ` [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper Michal Wilczynski
  2025-06-09 22:21   ` Benno Lossin
@ 2025-06-10 13:09   ` kernel test robot
  2025-06-13 13:32   ` Alexandre Courbot
  2 siblings, 0 replies; 6+ messages in thread
From: kernel test robot @ 2025-06-10 13:09 UTC (permalink / raw)
  To: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich, Marek Szyprowski
  Cc: oe-kbuild-all, linux-kernel, rust-for-linux, Michal Wilczynski

Hi Michal,

kernel test robot noticed the following build errors:

[auto build test ERROR on 19272b37aa4f83ca52bdf9c16d5d81bdd1354494]

url:    https://github.com/intel-lab-lkp/linux/commits/Michal-Wilczynski/rust-math-Add-KernelMathExt-trait-with-a-mul_div-helper/20250610-055722
base:   19272b37aa4f83ca52bdf9c16d5d81bdd1354494
patch link:    https://lore.kernel.org/r/20250609-math-rust-v1-v1-1-285fac00031f%40samsung.com
patch subject: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250610/202506102043.Tvsj0sXM-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
rustc: rustc 1.78.0 (9b00956e5 2024-04-29)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250610/202506102043.Tvsj0sXM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506102043.Tvsj0sXM-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error[E0425]: cannot find function `mul_u64_u64_div_u64` in crate `bindings`
   --> rust/kernel/math.rs:32:33
   |
   32 |         Some(unsafe { bindings::mul_u64_u64_div_u64(self, multiplier, divisor) })
   |                                 ^^^^^^^^^^^^^^^^^^^ not found in `bindings`

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
  2025-06-09 22:21   ` Benno Lossin
@ 2025-06-12 11:00     ` Michal Wilczynski
  2025-06-12 11:10       ` Benno Lossin
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Wilczynski @ 2025-06-12 11:00 UTC (permalink / raw)
  To: Benno Lossin, Miguel Ojeda, Alex Gaynor, Boqun Feng, Gary Guo,
	Björn Roy Baron, Andreas Hindborg, Alice Ryhl, Trevor Gross,
	Danilo Krummrich, Marek Szyprowski, Uwe Kleine-König
  Cc: linux-kernel, rust-for-linux



On 6/10/25 00:21, Benno Lossin wrote:
> On Mon Jun 9, 2025 at 11:53 PM CEST, Michal Wilczynski wrote:
>> The PWM subsystem and other kernel modules often need to perform a
>> 64 by 64-bit multiplication followed by a 64-bit division. Performing
>> this naively in Rust risks overflow on the intermediate multiplication.
>> The kernel provides the C helper 'mul_u64_u64_div_u64' for this exact
>> purpose.
>>
>> Introduce a safe Rust wrapper for this function to make it available to
>> Rust drivers.
>>
>> Following feedback from the mailing list [1], this functionality is
> 
> You could turn this into a `Suggested-by`.
> 
>> provided via a 'KernelMathExt' extension trait. This allows for
>> idiomatic, method style calls (e.g. val.mul_div()) and provides a
>> scalable pattern for adding helpers for other integer types in the
>> future.
>>
>> The safe wrapper is named 'mul_div' and not 'mul_u64_u64_div_u64' [2]
>> because its behavior differs from the underlying C function. The C
>> helper traps on a division by zero, whereas this safe wrapper returns
>> `None`, thus exhibiting different and safer behavior.
>>
>> This is required for the Rust PWM TH1520 driver [3].
>>
>> [1] - https://lore.kernel.org/all/DAFQ19RBBSQL.3OGUXOQ0PA9YH@kernel.org/
>> [2] - https://lore.kernel.org/all/CANiq72kVvLogBSVKz0eRg6V4LDB1z7b-6y1WPLSQfXXLW7X3cw@mail.gmail.com/
>> [3] - https://lore.kernel.org/all/20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@samsung.com/
> 
> Please use the `Link: https://... [.]` format.
> 
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  rust/kernel/lib.rs  |  1 +
>>  rust/kernel/math.rs | 34 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 35 insertions(+)
>>
>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
>> index 6b4774b2b1c37f4da1866e993be6230bc6715841..d652c92633b82525f37e5cd8a040d268e0c191d1 100644
>> --- a/rust/kernel/lib.rs
>> +++ b/rust/kernel/lib.rs
>> @@ -85,6 +85,7 @@
>>  #[cfg(CONFIG_KUNIT)]
>>  pub mod kunit;
>>  pub mod list;
>> +pub mod math;
>>  pub mod miscdevice;
>>  pub mod mm;
>>  #[cfg(CONFIG_NET)]
>> diff --git a/rust/kernel/math.rs b/rust/kernel/math.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..b89e23f9266117dcf96561fbf13b1c66a4851b48
>> --- /dev/null
>> +++ b/rust/kernel/math.rs
>> @@ -0,0 +1,34 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
>> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
>> +
>> +//! Safe wrappers for kernel math helpers.
> 
> I wouldn't stress the fact that these are safe, they better be!
> 
>> +//!
>> +//! This module provides safe, idiomatic Rust wrappers for C functions, whose
>> +//! FFI bindings are auto-generated in the `bindings` crate.
>> +
>> +use crate::bindings;
>> +
>> +/// An extension trait that provides access to kernel math helpers on primitive integer types.
>> +pub trait KernelMathExt: Sized {
> 
> We should add this trait to the prelude.
> 
>> +    /// Multiplies self by `multiplier and divides by divisor.
>> +    ///
>> +    /// This wrapper around the kernel's `mul_u64_u64_div_u64` C helper ensures that no
> 
> The trait shouldn't make a reference to `u64`.
> 
>> +    /// overflow occurs during the intermediate multiplication.
>> +    ///
>> +    /// # Returns
>> +    /// * Some(result) if the division is successful.
> 
> Use backtics (`) for code in documentation and comments.
> 
>> +    /// * 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?

> 
>> +    fn mul_div(self, multiplier: u64, divisor: u64) -> Option<u64> {
>> +        if divisor == 0 {
>> +            return None;
>> +        }
>> +        // SAFETY: The C function `mul_u64_u64_div_u64` is safe to call because the divisor
>> +        // is guaranteed to be non-zero. The FFI bindings use `u64`, matching our types.
> 
> Let's not talk about "safe to call", but rather say it like this:
> 
>     // SAFETY: `mul_u64_u64_div_u64` requires the divisor to be non-zero
>     // which is checked above. It has no other requirements.
> 
> ---
> Cheers,
> Benno
> 
>> +        Some(unsafe { bindings::mul_u64_u64_div_u64(self, multiplier, divisor) })
>> +    }
>> +}
>>
>> ---
>> base-commit: 19272b37aa4f83ca52bdf9c16d5d81bdd1354494
>> change-id: 20250609-math-rust-v1-d3989515e32e
>>
>> Best regards,
> 
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
  2025-06-12 11:00     ` Michal Wilczynski
@ 2025-06-12 11:10       ` Benno Lossin
  0 siblings, 0 replies; 6+ messages in thread
From: Benno Lossin @ 2025-06-12 11:10 UTC (permalink / raw)
  To: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Andreas Hindborg, Alice Ryhl,
	Trevor Gross, Danilo Krummrich, Marek Szyprowski,
	Uwe Kleine-König
  Cc: linux-kernel, rust-for-linux

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper
  2025-06-09 21:53 ` [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper Michal Wilczynski
  2025-06-09 22:21   ` Benno Lossin
  2025-06-10 13:09   ` kernel test robot
@ 2025-06-13 13:32   ` Alexandre Courbot
  2 siblings, 0 replies; 6+ messages in thread
From: Alexandre Courbot @ 2025-06-13 13:32 UTC (permalink / raw)
  To: Michal Wilczynski, Miguel Ojeda, Alex Gaynor, Boqun Feng,
	Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
	Alice Ryhl, Trevor Gross, Danilo Krummrich, Marek Szyprowski
  Cc: linux-kernel, rust-for-linux

On Tue Jun 10, 2025 at 6:53 AM JST, Michal Wilczynski wrote:
> The PWM subsystem and other kernel modules often need to perform a
> 64 by 64-bit multiplication followed by a 64-bit division. Performing
> this naively in Rust risks overflow on the intermediate multiplication.
> The kernel provides the C helper 'mul_u64_u64_div_u64' for this exact
> purpose.
>
> Introduce a safe Rust wrapper for this function to make it available to
> Rust drivers.
>
> Following feedback from the mailing list [1], this functionality is
> provided via a 'KernelMathExt' extension trait. This allows for
> idiomatic, method style calls (e.g. val.mul_div()) and provides a
> scalable pattern for adding helpers for other integer types in the
> future.
>
> The safe wrapper is named 'mul_div' and not 'mul_u64_u64_div_u64' [2]
> because its behavior differs from the underlying C function. The C
> helper traps on a division by zero, whereas this safe wrapper returns
> `None`, thus exhibiting different and safer behavior.
>
> This is required for the Rust PWM TH1520 driver [3].
>
> [1] - https://lore.kernel.org/all/DAFQ19RBBSQL.3OGUXOQ0PA9YH@kernel.org/
> [2] - https://lore.kernel.org/all/CANiq72kVvLogBSVKz0eRg6V4LDB1z7b-6y1WPLSQfXXLW7X3cw@mail.gmail.com/
> [3] - https://lore.kernel.org/all/20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@samsung.com/
>
> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
> ---
>  rust/kernel/lib.rs  |  1 +
>  rust/kernel/math.rs | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
>
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 6b4774b2b1c37f4da1866e993be6230bc6715841..d652c92633b82525f37e5cd8a040d268e0c191d1 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -85,6 +85,7 @@
>  #[cfg(CONFIG_KUNIT)]
>  pub mod kunit;
>  pub mod list;
> +pub mod math;
>  pub mod miscdevice;
>  pub mod mm;
>  #[cfg(CONFIG_NET)]
> diff --git a/rust/kernel/math.rs b/rust/kernel/math.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..b89e23f9266117dcf96561fbf13b1c66a4851b48
> --- /dev/null
> +++ b/rust/kernel/math.rs
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2025 Samsung Electronics Co., Ltd.
> +// Author: Michal Wilczynski <m.wilczynski@samsung.com>
> +
> +//! Safe wrappers for kernel math helpers.
> +//!
> +//! This module provides safe, idiomatic Rust wrappers for C functions, whose
> +//! FFI bindings are auto-generated in the `bindings` crate.
> +
> +use crate::bindings;
> +
> +/// An extension trait that provides access to kernel math helpers on primitive integer types.
> +pub trait KernelMathExt: Sized {
> +    /// Multiplies self by `multiplier and divides by divisor.
> +    ///
> +    /// This wrapper around the kernel's `mul_u64_u64_div_u64` C helper ensures that no
> +    /// overflow occurs during the intermediate multiplication.
> +    ///
> +    /// # Returns
> +    /// * Some(result) if the division is successful.
> +    /// * None if the divisor is zero.
> +    fn mul_div(self, multiplier: Self, divisor: Self) -> Option<Self>;
> +}
> +
> +impl KernelMathExt for u64 {
> +    fn mul_div(self, multiplier: u64, divisor: u64) -> Option<u64> {
> +        if divisor == 0 {
> +            return None;
> +        }

Would it make sense to turn `divisor` into a `NonZero<Self>` and return
`u64`? This could remove the need for the caller to perform a check if
it can statically infer that the divisor is not zero.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2025-06-13 13:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <CGME20250609215523eucas1p22f6d7d84b403badfb77af7df973b97a9@eucas1p2.samsung.com>
2025-06-09 21:53 ` [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper Michal Wilczynski
2025-06-09 22:21   ` Benno Lossin
2025-06-12 11:00     ` Michal Wilczynski
2025-06-12 11:10       ` Benno Lossin
2025-06-10 13:09   ` kernel test robot
2025-06-13 13:32   ` Alexandre Courbot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).