Message-Id: <DCYM4TPGMFF5.3J6H7VPADC0W0@kernel.org>
Date: Sun, 21 Sep 2025 18:18:52 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "Michael Turquette" <mturquette@...libre.com>, "Stephen Boyd"
 <sboyd@...nel.org>, "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>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "Rafael J. Wysocki"
 <rafael@...nel.org>, "Viresh Kumar" <viresh.kumar@...aro.org>,
 <linux-clk@...r.kernel.org>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] rust: clk: use the type-state pattern

On Wed Sep 10, 2025 at 7:28 PM CEST, Daniel Almeida wrote:
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a device.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable(dev: &Device, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
> +        // device pointer.

It's not, since this calls into devres it is only safe with a pointer to a bound
device, i.e. you need to require &Device<Bound>.

You also need to justify the CStr pointer in terms of being NULL and its
lifetime.

> +        from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }
> +
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a device.
> +    ///
> +    /// This does not print any error messages if the clock is not found.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable_optional(dev: &Device, name: Option<&CStr>) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_optional_enabled`] with a
> +        // valid device pointer.
> +        from_err_ptr(unsafe { bindings::devm_clk_get_optional_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }
> +
> +    /// Same as [`devm_enable_optional`], but also sets the rate.
> +    pub fn devm_enable_optional_with_rate(
> +        dev: &Device,
> +        name: Option<&CStr>,
> +        rate: Hertz,
> +    ) -> Result {
> +        let name = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +        // SAFETY: It is safe to call
> +        // [`devm_clk_get_optional_enabled_with_rate`] with a valid device
> +        // pointer.
> +        from_err_ptr(unsafe {
> +            bindings::devm_clk_get_optional_enabled_with_rate(dev.as_raw(), name, rate.as_hz())
> +        })?;
> +        Ok(())
> +    }

I think those should be added in a separate patch.

> +    impl Clk<Unprepared> {
>          /// Gets [`Clk`] corresponding to a [`Device`] and a connection id.
>          ///
>          /// Equivalent to the kernel's [`clk_get`] API.
>          ///
>          /// [`clk_get`]: https://docs.kernel.org/core-api/kernel-api.html#c.clk_get
> -        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> +        #[inline]
> +        pub fn get(dev: &Device, name: Option<&CStr>) -> Result<Clk<Unprepared>> {

Not related to your change, but I'm not sure we should allow drivers to mess
with clocks when they can't prove that they're still bound to the corresponding
device.

It's not introducing any safety issues or unsoundness, but it's not the correct
thing to do semantically.

>              let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
>  
>              // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.
> -            //
> +            let inner = from_err_ptr(unsafe { bindings::clk_get(dev.as_raw(), con_id) })?;
> +
>              // INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.
> -            Ok(Self(from_err_ptr(unsafe {
> -                bindings::clk_get(dev.as_raw(), con_id)
> -            })?))
> +            Ok(Self {
> +                inner,
> +                _phantom: PhantomData,
> +            })
>          }
>  
> -        /// Obtain the raw [`struct clk`] pointer.
> +        /// Behaves the same as [`Self::get`], except when there is no clock
> +        /// producer. In this case, instead of returning [`ENOENT`], it returns
> +        /// a dummy [`Clk`].
>          #[inline]
> -        pub fn as_raw(&self) -> *mut bindings::clk {
> -            self.0
> +        pub fn get_optional(dev: &Device, name: Option<&CStr>) -> Result<Clk<Unprepared>> {
> +            let con_id = name.map_or(ptr::null(), |n| n.as_ptr());
> +
> +            // SAFETY: It is safe to call [`clk_get`] for a valid device pointer.

What about con_id?

> +            let inner = from_err_ptr(unsafe { bindings::clk_get_optional(dev.as_raw(), con_id) })?;
> +
> +            // INVARIANT: The reference-count is decremented when [`Clk`] goes out of scope.

I know you're consistent with other places, but this seems a odd. This doesn't
correspond to: "A [`Clk`] instance holds either a pointer to a valid [`struct
clk`] created by the C portion of the kernel or a NULL pointer."

> +            Ok(Self {
> +                inner,
> +                _phantom: PhantomData,
> +            })
>          }

<snip>

> +            // SAFETY: By the type invariants, self.as_raw() is a valid argument for

Missing backticks (also in a few other places).

> +            // [`clk_put`].
> +            unsafe { bindings::clk_put(self.as_raw()) };
>          }
>      }

Powered by blists - more mailing lists