Re: [PATCH] rust: math: Add KernelMathExt trait with a mul_div helper

[Benno Lossin]

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@xxxxxxxxxx/
> [2] - https://lore.kernel.org/all/CANiq72kVvLogBSVKz0eRg6V4LDB1z7b-6y1WPLSQfXXLW7X3cw@xxxxxxxxxxxxxx/
> [3] - https://lore.kernel.org/all/20250524-rust-next-pwm-working-fan-for-sending-v1-2-bdd2d5094ff7@xxxxxxxxxxx/

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

>
> Signed-off-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxx>
> +
> +//! 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,