[PATCH V2 0/2] rust: Add basic clock bindings

[PATCH V2 0/2] rust: Add basic clock bindings

[Viresh Kumar]

Hello,

This adds initial bindings for the clk APIs. These provide the minimal
functionality needed for common use cases, making them straightforward
to introduce in the first iteration.

These will be used by Rust based cpufreq / OPP layers to begin with.

For now I have added them under the maintainership umbrella of the common clk
framework, please let me know if I should do it differently.

This was posted earlier as part of the series implementing cpufreq/OPP Rust
bindings [1] (since its V6 version). In order to make sure this gets properly
reviewed and I don't accidentally miss relevant reviewers, I am posting it
separately now.

If possible, I would like to get these merged via the PM tree along with
cpufreq/OPP bindings, but its okay otherwise too.

--
Viresh

[1] https://lore.kernel.org/all/cover.1738832118.git.viresh.kumar@linaro.org/

V1->V2:
- Post this as an independent series.
- Include more APIs, apart from clk_get() and clk_put().

Viresh Kumar (2):
  rust: Add clk helpers
  rust: Add basic bindings for clk APIs

 MAINTAINERS                     |   2 +
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/clk.c              |  57 +++++++++++++++++
 rust/helpers/helpers.c          |   1 +
 rust/kernel/clk.rs              | 104 ++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 6 files changed, 166 insertions(+)
 create mode 100644 rust/helpers/clk.c
 create mode 100644 rust/kernel/clk.rs

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 1/2] rust: Add clk helpers

[Viresh Kumar]

In order to prepare for adding Rust abstractions for the clock APIs,
this patch adds clock helpers required by the Rust implementation.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS                     |  1 +
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/clk.c              | 57 +++++++++++++++++++++++++++++++++
 rust/helpers/helpers.c          |  1 +
 4 files changed, 60 insertions(+)
 create mode 100644 rust/helpers/clk.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 25c86f47353d..726110d3c988 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5778,6 +5778,7 @@ F:	include/dt-bindings/clock/
 F:	include/linux/clk-pr*
 F:	include/linux/clk/
 F:	include/linux/of_clk.h
+F:	rust/helpers/clk.c
 X:	drivers/clk/clkdev.c
 
 COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..4e4e16c3b479 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/clk.h>
 #include <linux/cred.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
diff --git a/rust/helpers/clk.c b/rust/helpers/clk.c
new file mode 100644
index 000000000000..3c63b50ad6fb
--- /dev/null
+++ b/rust/helpers/clk.c
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/clk.h>
+
+#ifndef CONFIG_HAVE_CLK
+struct clk *rust_helper_clk_get(struct device *dev, const char *id)
+{
+	return clk_get(dev, id);
+}
+
+void rust_helper_clk_put(struct clk *clk)
+{
+	clk_put(clk);
+}
+
+int rust_helper_clk_enable(struct clk *clk)
+{
+	return clk_enable(clk);
+}
+
+void rust_helper_clk_disable(struct clk *clk)
+{
+	clk_disable(clk);
+}
+
+unsigned long rust_helper_clk_get_rate(struct clk *clk)
+{
+	return clk_get_rate(clk);
+}
+
+int rust_helper_clk_set_rate(struct clk *clk, unsigned long rate)
+{
+	return clk_set_rate(clk, rate);
+}
+#endif
+
+#ifndef CONFIG_HAVE_CLK_PREPARE
+int rust_helper_clk_prepare(struct clk *clk)
+{
+	return clk_prepare(clk);
+}
+
+void rust_helper_clk_unprepare(struct clk *clk)
+{
+	clk_unprepare(clk);
+}
+#endif
+
+int rust_helper_clk_prepare_enable(struct clk *clk)
+{
+	return clk_prepare_enable(clk);
+}
+
+void rust_helper_clk_disable_unprepare(struct clk *clk)
+{
+	clk_disable_unprepare(clk);
+}
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..4700ee7aaf85 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -11,6 +11,7 @@
 #include "bug.c"
 #include "build_assert.c"
 #include "build_bug.c"
+#include "clk.c"
 #include "cred.c"
 #include "device.c"
 #include "err.c"
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 1/2] rust: Add clk helpers

[Daniel Almeida]

Hi Viresh,

> On 21 Feb 2025, at 03:33, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> In order to prepare for adding Rust abstractions for the clock APIs,
> this patch adds clock helpers required by the Rust implementation.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> MAINTAINERS                     |  1 +
> rust/bindings/bindings_helper.h |  1 +
> rust/helpers/clk.c              | 57 +++++++++++++++++++++++++++++++++
> rust/helpers/helpers.c          |  1 +
> 4 files changed, 60 insertions(+)
> create mode 100644 rust/helpers/clk.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 25c86f47353d..726110d3c988 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5778,6 +5778,7 @@ F: include/dt-bindings/clock/
> F: include/linux/clk-pr*
> F: include/linux/clk/
> F: include/linux/of_clk.h
> +F: rust/helpers/clk.c
> X: drivers/clk/clkdev.c
> 
> COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 55354e4dec14..4e4e16c3b479 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
> #include <linux/blk-mq.h>
> #include <linux/blk_types.h>
> #include <linux/blkdev.h>
> +#include <linux/clk.h>
> #include <linux/cred.h>
> #include <linux/errname.h>
> #include <linux/ethtool.h>
> diff --git a/rust/helpers/clk.c b/rust/helpers/clk.c
> new file mode 100644
> index 000000000000..3c63b50ad6fb
> --- /dev/null
> +++ b/rust/helpers/clk.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/clk.h>
> +
> +#ifndef CONFIG_HAVE_CLK

This is a bit confusing. Can you add a comment explaining how
we get inlined stubs if these configs are not set, thus explaining why
we need to define them in helpers.c?

This will let everyone know why we have #ifndef here when the logical thing
would be #ifdef. It will also make it clear why the rust code is not gated
by these configs.

> +struct clk *rust_helper_clk_get(struct device *dev, const char *id)
> +{
> + return clk_get(dev, id);
> +}
> +
> +void rust_helper_clk_put(struct clk *clk)
> +{
> + clk_put(clk);
> +}
> +
> +int rust_helper_clk_enable(struct clk *clk)
> +{
> + return clk_enable(clk);
> +}
> +
> +void rust_helper_clk_disable(struct clk *clk)
> +{
> + clk_disable(clk);
> +}
> +
> +unsigned long rust_helper_clk_get_rate(struct clk *clk)
> +{
> + return clk_get_rate(clk);
> +}
> +
> +int rust_helper_clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + return clk_set_rate(clk, rate);
> +}
> +#endif
> +
> +#ifndef CONFIG_HAVE_CLK_PREPARE

Same here.

> +int rust_helper_clk_prepare(struct clk *clk)
> +{
> + return clk_prepare(clk);
> +}
> +
> +void rust_helper_clk_unprepare(struct clk *clk)
> +{
> + clk_unprepare(clk);
> +}
> +#endif
> +
> +int rust_helper_clk_prepare_enable(struct clk *clk)
> +{
> + return clk_prepare_enable(clk);
> +}
> +
> +void rust_helper_clk_disable_unprepare(struct clk *clk)
> +{
> + clk_disable_unprepare(clk);
> +}
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 0640b7e115be..4700ee7aaf85 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -11,6 +11,7 @@
> #include "bug.c"
> #include "build_assert.c"
> #include "build_bug.c"
> +#include "clk.c"
> #include "cred.c"
> #include "device.c"
> #include "err.c"
> -- 
> 2.31.1.272.g89b43f80a514
> 
> 

With this change,

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

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

[Viresh Kumar]

Add initial bindings for the clk APIs. These provide the minimal
functionality needed for common use cases, making them straightforward
to introduce in the first iteration.

These will be used by Rust based cpufreq / OPP layers to begin with.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 MAINTAINERS        |   1 +
 rust/kernel/clk.rs | 104 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 3 files changed, 106 insertions(+)
 create mode 100644 rust/kernel/clk.rs

diff --git a/MAINTAINERS b/MAINTAINERS
index 726110d3c988..96e2574f41c0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5779,6 +5779,7 @@ F:	include/linux/clk-pr*
 F:	include/linux/clk/
 F:	include/linux/of_clk.h
 F:	rust/helpers/clk.c
+F:	rust/kernel/clk.rs
 X:	drivers/clk/clkdev.c
 
 COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
new file mode 100644
index 000000000000..c212cd3167e1
--- /dev/null
+++ b/rust/kernel/clk.rs
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Clock abstractions.
+//!
+//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
+
+use crate::{
+    bindings,
+    device::Device,
+    error::{from_err_ptr, to_result, Result},
+    prelude::*,
+};
+
+use core::ptr;
+
+/// A simple implementation of `struct clk` from the C code.
+#[repr(transparent)]
+pub struct Clk(*mut bindings::clk);
+
+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.
+        to_result(unsafe { bindings::clk_enable(self.0) })
+    }
+
+    /// Clock disable.
+    pub fn disable(&self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        unsafe { bindings::clk_disable(self.0) };
+    }
+
+    /// Clock prepare.
+    pub fn prepare(&self) -> Result<()> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        to_result(unsafe { bindings::clk_prepare(self.0) })
+    }
+
+    /// Clock unprepare.
+    pub fn unprepare(&self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        unsafe { bindings::clk_unprepare(self.0) };
+    }
+
+    /// Clock prepare enable.
+    pub fn prepare_enable(&self) -> Result<()> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        to_result(unsafe { bindings::clk_prepare_enable(self.0) })
+    }
+
+    /// Clock disable unprepare.
+    pub fn disable_unprepare(&self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        unsafe { bindings::clk_disable_unprepare(self.0) };
+    }
+
+    /// Clock get rate.
+    pub fn rate(&self) -> usize {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        unsafe { bindings::clk_get_rate(self.0) }
+    }
+
+    /// Clock set rate.
+    pub fn set_rate(&self, rate: usize) -> Result<()> {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // use it now.
+        to_result(unsafe { bindings::clk_set_rate(self.0, rate) })
+    }
+}
+
+impl Drop for Clk {
+    fn drop(&mut self) {
+        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
+        // relinquish it now.
+        unsafe { bindings::clk_put(self.0) };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..324b86f127a0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod clk;
 pub mod cred;
 pub mod device;
 pub mod device_id;
-- 
2.31.1.272.g89b43f80a514

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

[Daniel Almeida]

Hi Viresh, thank you for working on this.


> On 21 Feb 2025, at 03:33, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> Add initial bindings for the clk APIs. These provide the minimal
> functionality needed for common use cases, making them straightforward
> to introduce in the first iteration.
> 
> These will be used by Rust based cpufreq / OPP layers to begin with.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> MAINTAINERS        |   1 +
> rust/kernel/clk.rs | 104 +++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs |   1 +
> 3 files changed, 106 insertions(+)
> create mode 100644 rust/kernel/clk.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 726110d3c988..96e2574f41c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5779,6 +5779,7 @@ F: include/linux/clk-pr*
> F: include/linux/clk/
> F: include/linux/of_clk.h
> F: rust/helpers/clk.c
> +F: rust/kernel/clk.rs
> X: drivers/clk/clkdev.c
> 
> COMMON INTERNET FILE SYSTEM CLIENT (CIFS and SMB3)
> diff --git a/rust/kernel/clk.rs b/rust/kernel/clk.rs
> new file mode 100644
> index 000000000000..c212cd3167e1
> --- /dev/null
> +++ b/rust/kernel/clk.rs
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Clock abstractions.
> +//!
> +//! C header: [`include/linux/clk.h`](srctree/include/linux/clk.h)
> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    prelude::*,
> +};
> +
> +use core::ptr;
> +
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);

> +
> +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.
> +        to_result(unsafe { bindings::clk_enable(self.0) })
> +    }
> +
> +    /// Clock disable.
> +    pub fn disable(&self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        unsafe { bindings::clk_disable(self.0) };
> +    }
> +
> +    /// Clock prepare.
> +    pub fn prepare(&self) -> Result<()> {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        to_result(unsafe { bindings::clk_prepare(self.0) })
> +    }
> +
> +    /// Clock unprepare.
> +    pub fn unprepare(&self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        unsafe { bindings::clk_unprepare(self.0) };
> +    }
> +
> +    /// Clock prepare enable.
> +    pub fn prepare_enable(&self) -> Result<()> {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        to_result(unsafe { bindings::clk_prepare_enable(self.0) })
> +    }
> +
> +    /// Clock disable unprepare.
> +    pub fn disable_unprepare(&self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        unsafe { bindings::clk_disable_unprepare(self.0) };
> +    }
> +
> +    /// Clock get rate.
> +    pub fn rate(&self) -> usize {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        unsafe { bindings::clk_get_rate(self.0) }
> +    }
> +
> +    /// Clock set rate.
> +    pub fn set_rate(&self, rate: usize) -> Result<()> {

It might be worth it to add a type here to make it clear what in what unit is `rate` expressed as.

Something like:

pub struct Hertz(pub u32);

pub fn set_rate(&self, rate: Hertz);

Assuming `rate` is in Hertz.


> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // use it now.
> +        to_result(unsafe { bindings::clk_set_rate(self.0, rate) })
> +    }
> +}
> +
> +impl Drop for Clk {
> +    fn drop(&mut self) {
> +        // SAFETY: By the type invariants, we know that `self` owns a reference, so it is safe to
> +        // relinquish it now.
> +        unsafe { bindings::clk_put(self.0) };
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..324b86f127a0 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,7 @@
> pub mod block;
> #[doc(hidden)]
> pub mod build_assert;
> +pub mod clk;
> pub mod cred;
> pub mod device;
> pub mod device_id;
> -- 
> 2.31.1.272.g89b43f80a514
> 
> 


This is working fine on Tyr, so:

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>

With the minor change above:

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

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

[Danilo Krummrich]

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.

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

[Daniel Almeida]

> On 21 Feb 2025, at 10:56, Danilo Krummrich <dakr@kernel.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.
> 

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

[Danilo Krummrich]

On Fri, Feb 21, 2025 at 11:29:21AM -0300, Daniel Almeida wrote:
> 
> 
> > On 21 Feb 2025, at 10:56, Danilo Krummrich <dakr@kernel.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.

This was badly phrased, the current implementation does not need to consider it
indeed. What I meant is that we have to consider it potentially. Especially,
when adding new functionality later on. For instance, when accessing fields of
struct clk directly. Maybe this only becomes relevant once we write a clk driver
itself in Rust, but still.

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

Yeah, it's probably the correct thing to do, to make things obvious.

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

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1609 bytes --]

Hi,

On Fri, Feb 21, 2025 at 03:47:57PM +0100, Danilo Krummrich wrote:
> On Fri, Feb 21, 2025 at 11:29:21AM -0300, Daniel Almeida wrote:
> > > On 21 Feb 2025, at 10:56, Danilo Krummrich <dakr@kernel.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.
> 
> This was badly phrased, the current implementation does not need to consider it
> indeed. What I meant is that we have to consider it potentially. Especially,
> when adding new functionality later on. For instance, when accessing fields of
> struct clk directly. 

Just a drive-by comment - the current implementation will never have
a NULL clock anyways. That is only returned by the clk_get functions
with the _optional() suffix. You are right now only using clk_get(),
which will instead returns ERR_PTR(-ENOENT).

> Maybe this only becomes relevant once we write a clk driver itself
> in Rust, but still.

For a clock provider implementation you will mainly use 'struct
clk_hw', which is different from 'struct clk'.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rob Herring]

On Fri, Feb 21, 2025 at 04:28:18PM +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Feb 21, 2025 at 03:47:57PM +0100, Danilo Krummrich wrote:
> > On Fri, Feb 21, 2025 at 11:29:21AM -0300, Daniel Almeida wrote:
> > > > On 21 Feb 2025, at 10:56, Danilo Krummrich <dakr@kernel.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.
> > 
> > This was badly phrased, the current implementation does not need to consider it
> > indeed. What I meant is that we have to consider it potentially. Especially,
> > when adding new functionality later on. For instance, when accessing fields of
> > struct clk directly. 
> 
> Just a drive-by comment - the current implementation will never have
> a NULL clock anyways. That is only returned by the clk_get functions
> with the _optional() suffix. You are right now only using clk_get(),
> which will instead returns ERR_PTR(-ENOENT).

It would be nice to handle the optional case from the start. Otherwise, 
driver writers handle optional or not optional themselves. The not 
optional case is typically some form of error message duplicated in 
every driver.

Every foo_get() needs foo_get_optional(), so let's figure out the rust 
way to handle this once for everyone.

Rob

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

[Viresh Kumar]

On 21-02-25, 15:59, Rob Herring wrote:
> It would be nice to handle the optional case from the start. Otherwise, 
> driver writers handle optional or not optional themselves. The not 
> optional case is typically some form of error message duplicated in 
> every driver.
> 
> Every foo_get() needs foo_get_optional(), so let's figure out the rust 
> way to handle this once for everyone.

Are we talking about adding another field here (like below code) or
something else ?

impl Clk {
        pub fn get(dev: &Device, name: Option<&CStr>, optional: bool) -> Result<Self> {
                ...

                let clk = if optional {
                        bindings::clk_get(dev.as_raw(), con_id)
                else {
                        bindings::clk_get_optional(dev.as_raw(), con_id)
                };

                Ok(Self(from_err_ptr(clk)?))
        }
}

-- 
viresh

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

[Rob Herring]

On Mon, Feb 24, 2025 at 4:00 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 21-02-25, 15:59, Rob Herring wrote:
> > It would be nice to handle the optional case from the start. Otherwise,
> > driver writers handle optional or not optional themselves. The not
> > optional case is typically some form of error message duplicated in
> > every driver.
> >
> > Every foo_get() needs foo_get_optional(), so let's figure out the rust
> > way to handle this once for everyone.
>
> Are we talking about adding another field here (like below code) or
> something else ?

Either way, but generally I think 2 functions are preferred over 1
function and flags.

The harder part here is in C we just return NULL and all subsequent
functions (e.g. clk_enable()) just return with no error for a NULL
struct clk. For rust, I think we'd need a dummy Clk returned and then
handle comparing the passed in reference to the dummy Clk in the rust
bindings.

>
> impl Clk {
>         pub fn get(dev: &Device, name: Option<&CStr>, optional: bool) -> Result<Self> {
>                 ...
>
>                 let clk = if optional {
>                         bindings::clk_get(dev.as_raw(), con_id)
>                 else {
>                         bindings::clk_get_optional(dev.as_raw(), con_id)
>                 };
>
>                 Ok(Self(from_err_ptr(clk)?))
>         }
> }
>
> --
> viresh

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

[Viresh Kumar]

On 05-03-25, 14:09, Rob Herring wrote:
> Either way, but generally I think 2 functions are preferred over 1
> function and flags.
> 
> The harder part here is in C we just return NULL and all subsequent
> functions (e.g. clk_enable()) just return with no error for a NULL
> struct clk. For rust, I think we'd need a dummy Clk returned and then
> handle comparing the passed in reference to the dummy Clk in the rust
> bindings.

I have implemented it differently in V3:

https://lore.kernel.org/all/023e3061cc164087b9079a9f6cb7e9fbf286794e.1740995194.git.viresh.kumar@linaro.org/

So even for a NULL value returned from clk_get_optional(), Rust users still get
OptionalClk (Deref as Clk) and they keep using it as if a valid Clk is returned
and will keep calling all clk APIs (which will return early for NULL clks).

-- 
viresh

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

[Viresh Kumar]

On 21-02-25, 15:47, Danilo Krummrich wrote:
> This was badly phrased, the current implementation does not need to consider it
> indeed. What I meant is that we have to consider it potentially. Especially,
> when adding new functionality later on. For instance, when accessing fields of
> struct clk directly. Maybe this only becomes relevant once we write a clk driver
> itself in Rust, but still.

I don't think we will _ever_ access fields of the struct clk directly.
For the most common use, common clk API, the struct clk is defined in
drivers/clk/clk.c.

> > MaybeNull<T> sounds nice.
> 
> Yeah, it's probably the correct thing to do, to make things obvious.

Still need this ? Any example code that I can refer to implement it or
if someone can help with implementing it ?

-- 
viresh

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

[Daniel Almeida]

> +
> +use crate::{
> +    bindings,
> +    device::Device,
> +    error::{from_err_ptr, to_result, Result},
> +    prelude::*,
> +};
> +
> +use core::ptr;
> +
> +/// A simple implementation of `struct clk` from the C code.
> +#[repr(transparent)]
> +pub struct Clk(*mut bindings::clk);
> +
> +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()
> +        };

Viresh, one thing I forgot to comment. Maybe it’s better if we keep the
`get` nomenclature here instead of `new`.

This is to make clear that nothing is getting spawned. A reference to a system
resource is (potentially) being returned instead.

This would also mean refactoring the docs for this function.

— Daniel