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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Jan 08, 2025 at 04:32:42PM +0530, Viresh Kumar wrote:
> 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.

How about fixing the reference count of the cpu device?  :)

But seriously, this is NOT a generic 'struct device' thing, it is a 'cpu
device' thing.  So putting this function in device.rs is probably not
the proper place for it at all, sorry.  Why not put it in the cpu.rs
file instead?

> The new helper will be used by Rust based cpufreq drivers.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  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@linaro.org>
> ---
>  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 _) }

I hate to ask, but how was this compiling if the const wasn't there
before?  There's no type-checking happening here?  If not, how are we
ever going to notice when function parameters change?  If there is type
checking, how did this ever build without the const?

confused,

greg k-h