| Message ID | 429b7539f787ad360cd28fd1db6dc3d6c1fe289d.1736248242.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On Tue, Jan 07, 2025 at 04:51:37PM +0530, Viresh Kumar wrote: > Add from_cpu() and property_present() helpers to the device bindings. That says what, but not why. Also those are two totally different things, I'm going to argue with you about one of them... > + /// 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? 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? > + } > + > /// Obtain the raw `struct device *`. > pub(crate) fn as_raw(&self) -> *mut bindings::device { > self.0.get() > @@ -180,6 +195,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: `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? thanks, greg k-h
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@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 _) } + } } // SAFETY: Instances of `Device` are always reference-counted.
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
On Wed, Jan 8, 2025 at 12:52 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > + /// 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, Rust auto-converts `*mut` pointers to `*const` when necessary. Note that this should really be `self.as_raw().cast_const()` if you're just casting mut to const without changing the pointee type. Alice
On Wed, Jan 08, 2025 at 12:55:38PM +0100, Alice Ryhl wrote: > On Wed, Jan 8, 2025 at 12:52 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > + /// 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, > > Rust auto-converts `*mut` pointers to `*const` when necessary. Ah, so it's magic :) thanks for the explaination. greg k-h
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@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 >
On 08-01-25, 12:52, Greg Kroah-Hartman wrote: > How about fixing the reference count of the cpu device? :) I would try my best to avoid getting into that trap :) > 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? I can do that, so it will be a standalone helper function in cpu.rs and not a method in an `impl` block.
On 08-01-25, 14:42, Danilo Krummrich wrote: > 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. Right, though the device pointer always points to valid memory as the struct cpu is never freed. Isn't that enough for a pointer passed over FFI ? All the from_cpu() method returns is a reference, which will be used for a short period only (yes it is about the possibility of something going wrong in that period only and we need to ensure it doesn't break in such corner cases). FWIW, the cpufreq framework is registered with CPU hotplug layer, and so whenever a CPU disappears, the cpufreq core will stop using its device pointer before the CPU is removed. So technically we shouldn't land in a situation where the CPU is unregistered and cpufreq core is still using the CPU's device pointer.
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..5bfbc4bdfadc 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -6,6 +6,9 @@ use crate::{ bindings, + error::Result, + prelude::ENODEV, + str::CString, types::{ARef, Opaque}, }; use core::{fmt, ptr}; @@ -59,6 +62,18 @@ pub unsafe fn get_device(ptr: *mut bindings::device) -> ARef<Self> { unsafe { Self::as_ref(ptr) }.into() } + /// Creates a new ref-counted instance of device of a CPU. + pub fn from_cpu(cpu: u32) -> Result<ARef<Self>> { + // SAFETY: It is safe to call `get_cpu_device()` for any CPU number. + 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) }) + } + /// Obtain the raw `struct device *`. pub(crate) fn as_raw(&self) -> *mut bindings::device { self.0.get() @@ -180,6 +195,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: `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 _) } + } } // SAFETY: Instances of `Device` are always reference-counted.
Add from_cpu() and property_present() helpers to the device bindings. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- rust/bindings/bindings_helper.h | 1 + rust/kernel/device.rs | 21 +++++++++++++++++++++ 2 files changed, 22 insertions(+)