Re: [PATCH v4 2/3] rust: clk: add devres-managed clks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Missing `#[inline]` annotations on small abstraction functions.

Pre-existing issues:
- [High] Error path in `devm_clk_get_optional_enabled_with_rate` triggers unbalanced `clk_disable_unprepare`.
--

commit 6968c407fbf832032a4c2355e55f45330a6b1049
Author: Daniel Almeida <daniel.almeida@collabora.com>

rust: clk: add devres-managed clks

The clk API allows fine-grained control, but some drivers might be
more interested in a "set and forget" API. Expand the current API to
support this by automatically disabling, unpreparing and freeing the
clock when the device is unbound from the bus.

> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index a62e4c7e252e3..692ee88ca772a 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs
> @@ -95,6 +95,49 @@ impl Sealed for super::Prepared {}
>          impl Sealed for super::Enabled {}
>      }
>  
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound device.
> +    ///
> +    /// [`devres`]: crate::devres::Devres
> +    pub fn devm_enable(dev: &Device<Bound>, name: Option<&CStr>) -> Result {

[Severity: Low]
Should this small forwarding function be annotated with #[inline]?
According to the Rust subsystem guidelines, functions in abstractions
that are small or simply forward to a binding call require this annotation.

> +        let name = name.map_or(ptr::null(), |n| n.as_char_ptr());
> +
> +        // SAFETY: It is safe to call [`devm_clk_get_enabled`] with a valid
> +        // device pointer.
> +        from_err_ptr(unsafe { bindings::devm_clk_get_enabled(dev.as_raw(), name) })?;
> +        Ok(())
> +    }
> +
> +    /// Obtains and enables a [`devres`]-managed [`Clk`] for a bound 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<Bound>, name: Option<&CStr>) -> Result {

[Severity: Low]
Does this function also need an #[inline] annotation since it just
forwards to a C binding?

> +        let name = name.map_or(ptr::null(), |n| n.as_char_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<Bound>,
> +        name: Option<&CStr>,
> +        rate: Hertz,
> +    ) -> Result {

[Severity: Low]
Should this small wrapper function be annotated with #[inline] to comply
with the Rust subsystem guidelines for abstractions?

> +        let name = name.map_or(ptr::null(), |n| n.as_char_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())

[Severity: High]
This is a pre-existing issue, but does the underlying C function
devm_clk_get_optional_enabled_with_rate() have an unbalanced
clk_disable_unprepare() on its error path?

Looking at drivers/clk/clk-devres.c:devm_clk_get_optional_enabled_with_rate(),
it registers a devres entry with clk_disable_unprepare() as the cleanup
action.

If clk_set_rate() or clk_prepare_enable() fails, the error path executes
devm_clk_put(), which releases the devres entry and immediately invokes the
cleanup action:

drivers/clk/clk-devres.c:devm_clk_get_optional_enabled_with_rate() {
    ...
out_put_clk:
    devm_clk_put(dev, clk);
    return ERR_PTR(ret);
}

Because the clock was never successfully prepared or enabled, won't this
underflow the clock's enable/prepare counts? For a shared clock, could this
prematurely disable the clock for other devices?

> +        })?;
> +        Ok(())
> +    }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618-clk-type-state-v4-0-8be082786080@collabora.com?part=2