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 <[email protected]> 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/[email protected]?part=2
