Re: [PATCH v5 3/4] rust: clk: add devres-managed clks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The newly introduced Rust wrapper `devm_enable_optional_with_rate` exposes a broken error path in the underlying C framework (`devm_clk_get_optional_enabled_with_rate`) that incorrectly calls `clk_disable_unprepare` on clocks that haven't been prepared or enabled, causing unbalanced global count decrements and state corruption.
--

commit f2d6842b5aa3017f9a5274d7b455e65224dd1e79
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. The clock will automatically be
disabled, unprepared and freed when the device is unbound from the bus
without further intervention by the driver.

> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> index dd5fd656271e1..b9a44f83843a3 100644
> --- a/rust/kernel/clk.rs
> +++ b/rust/kernel/clk.rs

[ ... ]

> +    /// Same as [`devm_enable_optional`], but also sets the rate.
> +    #[inline]
> +    pub fn devm_enable_optional_with_rate(
> +        dev: &Device<Bound>,
> +        name: Option<&CStr>,
> +        rate: Hertz,
> +    ) -> Result {
> +        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 exposing this C function to Rust
drivers introduce them to a broken error path?

Looking at the underlying C implementation:

drivers/clk/clk-devres.c:devm_clk_get_optional_enabled_with_rate() {
	...
	ret = clk_set_rate(clk, rate);
	if (ret)
		goto out_put_clk;

	ret = clk_prepare_enable(clk);
	if (ret)
		goto out_put_clk;

	return clk;

out_put_clk:
	devm_clk_put(dev, clk);
	return ERR_PTR(ret);
}

If clk_set_rate() fails (for example, due to an unsupported rate), the
error path calls devm_clk_put(). Since the devres initialization previously
registered clk_disable_unprepare() as the exit function, won't devm_clk_put()
execute it even though the clock was never actually prepared or enabled by
this consumer?

For shared clocks, couldn't this unbalanced decrement of the enable and
prepare counts forcefully turn off the underlying hardware clock while other
consumers are actively using it?

> +        Ok(())
> +    }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-clk-type-state-v5-0-67c5f326a16c@collabora.com?part=3