Re: [PATCH V2 2/2] rust: Add basic bindings for clk APIs

From: Danilo Krummrich
Date: Fri Feb 21 2025 - 08:57:05 EST


On Fri, Feb 21, 2025 at 12:03:39PM +0530, Viresh Kumar wrote:
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);

I remember that Stephen explained that NULL is valid value for struct clk. As a
consequence, all functions implemented for `Clk` have to consider this.

I wonder if it could make sense to have a transparent wrapper type
`MaybeNull<T>` (analogous to `NonNull<T>`) to make this fact more obvious for
cases like this?

> +
> +impl Clk {
> + /// Creates `Clk` instance for a device and a connection id.
> + pub fn new(dev: &Device, name: Option<&CStr>) -> Result<Self> {
> + let con_id = if let Some(name) = name {
> + name.as_ptr() as *const _
> + } else {
> + ptr::null()
> + };
> +
> + // SAFETY: It is safe to call `clk_get()`, on a device pointer earlier received from the C
> + // code.
> + Ok(Self(from_err_ptr(unsafe {
> + bindings::clk_get(dev.as_raw(), con_id)
> + })?))
> + }
> +
> + /// Obtain the raw `struct clk *`.
> + pub fn as_raw(&self) -> *mut bindings::clk {
> + self.0
> + }
> +
> + /// Clock enable.
> + pub fn enable(&self) -> Result<()> {
> + // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> + // use it now.

This is not true.

1. There is no type invariant documented for `Clk`.
2. The pointer contained in an instance of `Clk` may be NULL, hence `self` does
not necessarily own a reference.

The same applies for all other functions in this implementation.