Re: [PATCH V6 04/15] rust: device: Add few helpers

[Viresh Kumar]

On 07-01-25, 12:56, Greg Kroah-Hartman wrote:
> On Tue, Jan 07, 2025 at 04:51:37PM +0530, Viresh Kumar wrote:
> > +    /// Creates a new ref-counted instance of device of a CPU.
> > +    pub fn from_cpu(cpu: u32) -> Result<ARef<Self>> {
> 
> Why is this a reference counted device, yet the C structure is NOT
> properly reference counted at all?

Ahh, I completely missed that it is not reference counted at all.

> Are you _sure_ this is going to work properly?
> 
> And really, we should fix up the C side to properly reference count all
> of this.  Just read the comment in cpu_device_release() for a hint at
> what needs to be done here.
> 
> > +        // SAFETY: It is safe to call `get_cpu_device()` for any CPU number.
> 
> For any number at all, no need to say "CPU" here, right?
> 
> > +        let ptr = unsafe { bindings::get_cpu_device(cpu) };
> > +        if ptr.is_null() {
> > +            return Err(ENODEV);
> > +        }
> > +
> > +        // SAFETY: By the safety requirements, ptr is valid.
> > +        Ok(unsafe { Device::get_device(ptr) })
> 
> So why is this device reference counted?  I get it that it should be,
> but how does that play with the "real" device here?

How about this:

Subject: [PATCH] rust: device: Add from_cpu()

This implements Device::from_cpu(), which returns a reference to
`Device` for a CPU. The C struct is created at initialization time for
CPUs and is never freed and so `ARef` isn't returned from this function.

The new helper will be used by Rust based cpufreq drivers.

Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
---
 rust/kernel/device.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 66ba0782551a..007f9ffab08b 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,6 +6,8 @@
 
 use crate::{
     bindings,
+    error::Result,
+    prelude::ENODEV,
     str::CString,
     types::{ARef, Opaque},
 };
@@ -60,6 +62,20 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> {
         unsafe { Self::as_ref(ptr) }.into()
     }
 
+    /// Creates a new instance of CPU's device.
+    pub fn from_cpu(cpu: u32) -> Result<&'static Self> {
+        // SAFETY: The pointer returned by `get_cpu_device()`, if not `NULL`, is a valid pointer to
+        // a `struct device` and is never freed by the C code.
+        let ptr = unsafe { bindings::get_cpu_device(cpu) };
+        if ptr.is_null() {
+            return Err(ENODEV);
+        }
+
+        // SAFETY: The pointer returned by `get_cpu_device()`, if not `NULL`, is a valid pointer to
+        // a `struct device` and is never freed by the C code.
+        Ok(unsafe { Self::as_ref(ptr) })
+    }
+
     /// Obtain the raw `struct device *`.
     pub(crate) fn as_raw(&self) -> *mut bindings::device {
         self.0.get()

-------------------------8<-------------------------

> > +    /// Checks if property is present or not.
> > +    pub fn property_present(&self, name: &CString) -> bool {
> > +        // SAFETY: `name` is null-terminated. `self.as_raw` is valid because `self` is valid.
> > +        unsafe { bindings::device_property_present(self.as_raw(), name.as_ptr() as *const _) }
> 
> is "self.as_raw()" a constant pointer too?

Subject: [PATCH] rust: device: Add property_present()

This implements Device::property_present(), which calls C APIs
device_property_present() helper.

The new helper will be used by Rust based cpufreq drivers.

Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
---
 rust/bindings/bindings_helper.h | 1 +
 rust/kernel/device.rs           | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 43f5c381aab0..70e4b7b0f638 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -31,6 +31,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/platform_device.h>
 #include <linux/poll.h>
+#include <linux/property.h>
 #include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/security.h>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index d5e6a19ff6b7..66ba0782551a 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -6,6 +6,7 @@
 
 use crate::{
     bindings,
+    str::CString,
     types::{ARef, Opaque},
 };
 use core::{fmt, ptr};
@@ -180,6 +181,12 @@ unsafe fn printk(&self, klevel: &[u8], msg: fmt::Arguments<'_>) {
             )
         };
     }
+
+    /// Checks if property is present or not.
+    pub fn property_present(&self, name: &CString) -> bool {
+        // SAFETY: By the invariant of `CString`, `name` is null-terminated.
+        unsafe { bindings::device_property_present(self.as_raw() as *const _, name.as_ptr() as *const _) }
+    }
 }
 
 // SAFETY: Instances of `Device` are always reference-counted.

-- 
viresh