Message-ID: <Z36A0g9g6qkRZSjh@pollux>
Date: Wed, 8 Jan 2025 14:42:42 +0100
From: Danilo Krummrich <dakr@...hat.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Viresh Kumar <viresh.kumar@...aro.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
	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>,
	Danilo Krummrich <dakr@...nel.org>, linux-pm@...r.kernel.org,
	Vincent Guittot <vincent.guittot@...aro.org>,
	Stephen Boyd <sboyd@...nel.org>, Nishanth Menon <nm@...com>,
	rust-for-linux@...r.kernel.org,
	Manos Pitsidianakis <manos.pitsidianakis@...aro.org>,
	Erik Schilling <erik.schilling@...aro.org>,
	Alex Bennée <alex.bennee@...aro.org>,
	Joakim Bech <joakim.bech@...aro.org>, Rob Herring <robh@...nel.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH V6 04/15] rust: device: Add few helpers

On Wed, Jan 08, 2025 at 12:52:54PM +0100, Greg Kroah-Hartman wrote:
> 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?  :)

I think that's really what is needed, otherwise it'll never work with the
guarantees the Rust `Device` abstraction provides.

The patch below is still not valid I think. It assumes that a CPU device never
becomes invalid, but that isn't true.

There's a hotplug path [1] where the device is unregistered.

[1] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/base/cpu.c#L94

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

Powered by blists - more mailing lists