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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <4AD8A8F3-EA7E-4FBE-9F0D-58CF7BB09ED5@collabora.com>
Date: Fri, 21 Feb 2025 11:29:21 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Viresh Kumar <viresh.kumar@...aro.org>,
 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 <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Russell King <linux@...linux.org.uk>,
 linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH V2 2/2] rust: Add basic bindings for clk APIs



> On 21 Feb 2025, at 10:56, Danilo Krummrich <dakr@...nel.org> wrote:
> 
> 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 am a bit confused here. If NULL is valid, then why should we have to specifically
consider that in the functions? No functions so far explicitly dereferences that value,
they only pass it to the clk framework.

Or are you referring to the safety comments only? In which case I do agree (sorry for
the oversight by the way)

> 
> 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?

MaybeNull<T> sounds nice.

> 
>> +
>> +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.
>