| Message ID | 854f7b8c9cbcc7f38fe5ed548290f41224478b40.1736766672.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Rust bindings for cpufreq and OPP core + sample driver | expand |
On Mon, Jan 13, 2025 at 04:52:58PM +0530, Viresh Kumar wrote: > This implements cpu::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/bindings/bindings_helper.h | 1 + > rust/kernel/cpu.rs | 26 ++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 3 files changed, 28 insertions(+) > create mode 100644 rust/kernel/cpu.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index e9fdceb568b8..d63e7f6d10ea 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/cpu.h> > #include <linux/cred.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs > new file mode 100644 > index 000000000000..422e874627d2 > --- /dev/null > +++ b/rust/kernel/cpu.rs > @@ -0,0 +1,26 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Generic CPU definitions. > +//! > +//! C header: [`include/linux/cpu.h`](srctree/include/linux/cpu.h) > + > +use crate::{ > + bindings, > + device::Device, > + error::Result, > + prelude::ENODEV, > +}; > + > +/// Creates a new instance of CPU's device. > +pub fn from_cpu(cpu: u32) -> Result<&'static Device> { > + // 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. I thought it was pointed out that it could be freed when a cpu was hot-unplugged? Or is that a different device in the cpu code? We seem to have 2 of them and it's not obvious which is which :( thanks, greg k-h
On 14-01-25, 19:44, Greg KH wrote: > > +pub fn from_cpu(cpu: u32) -> Result<&'static Device> { > > + // 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. > > I thought it was pointed out that it could be freed when a cpu was > hot-unplugged? Or is that a different device in the cpu code? We seem > to have 2 of them and it's not obvious which is which :( I did reply [1] to that earlier. The CPU can get unregistered but the memory for the device is never freed (it is part of struct cpu). Some calls on the CPU device may fail later on (if called for an unregisted dev), but should never crash the kernel.
On Wed, Jan 15, 2025 at 12:50:50PM +0530, Viresh Kumar wrote: > On 14-01-25, 19:44, Greg KH wrote: > > > +pub fn from_cpu(cpu: u32) -> Result<&'static Device> { > > > + // 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. > > > > I thought it was pointed out that it could be freed when a cpu was > > hot-unplugged? Or is that a different device in the cpu code? We seem > > to have 2 of them and it's not obvious which is which :( > > I did reply [1] to that earlier. The CPU can get unregistered but the > memory for the device is never freed (it is part of struct cpu). Some > calls on the CPU device may fail later on (if called for an unregisted > dev), but should never crash the kernel. Ah, but that's not really something that SAFETY should override, right? Yes, you know your implementation of this will stop using the pointer in the hotplug callback before it goes away but that's not documented here. And having the device "fail" afterward isn't really ok either as you are relying on the driver core to always check for this and I'm not so sure that it always does on all codepaths. But, I'm ok with this for now, as you are just copying the bad C model at the moment, but it really feels like a huge foot-gun waiting to go off. Any way to put some more documentation here as in "use this at your own risk!"? thanks, greg k-h
On 15-01-25, 08:54, Greg KH wrote: > Ah, but that's not really something that SAFETY should override, right? > > Yes, you know your implementation of this will stop using the pointer in > the hotplug callback before it goes away but that's not documented here. > And having the device "fail" afterward isn't really ok either as you are > relying on the driver core to always check for this and I'm not so sure > that it always does on all codepaths. > > But, I'm ok with this for now, as you are just copying the bad C model > at the moment, but it really feels like a huge foot-gun waiting to go > off. Any way to put some more documentation here as in "use this at > your own risk!"? What about marking it unsafe ? That would require callers to document why it is safe to call this. And yes add more documentation here too.
On Wed, Jan 15, 2025 at 01:28:59PM +0530, Viresh Kumar wrote: > On 15-01-25, 08:54, Greg KH wrote: > > Ah, but that's not really something that SAFETY should override, right? > > > > Yes, you know your implementation of this will stop using the pointer in > > the hotplug callback before it goes away but that's not documented here. > > And having the device "fail" afterward isn't really ok either as you are > > relying on the driver core to always check for this and I'm not so sure > > that it always does on all codepaths. > > > > But, I'm ok with this for now, as you are just copying the bad C model > > at the moment, but it really feels like a huge foot-gun waiting to go > > off. Any way to put some more documentation here as in "use this at > > your own risk!"? > > What about marking it unsafe ? That would require callers to document > why it is safe to call this. And yes add more documentation here too. Sure, that's fine with me.
On Wed, Jan 15, 2025 at 8:54 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Wed, Jan 15, 2025 at 12:50:50PM +0530, Viresh Kumar wrote: > > On 14-01-25, 19:44, Greg KH wrote: > > > > +pub fn from_cpu(cpu: u32) -> Result<&'static Device> { > > > > + // 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. > > > > > > I thought it was pointed out that it could be freed when a cpu was > > > hot-unplugged? Or is that a different device in the cpu code? We seem > > > to have 2 of them and it's not obvious which is which :( > > > > I did reply [1] to that earlier. The CPU can get unregistered but the > > memory for the device is never freed (it is part of struct cpu). Some > > calls on the CPU device may fail later on (if called for an unregisted > > dev), but should never crash the kernel. > > Ah, but that's not really something that SAFETY should override, right? > > Yes, you know your implementation of this will stop using the pointer in > the hotplug callback before it goes away but that's not documented here. > And having the device "fail" afterward isn't really ok either as you are > relying on the driver core to always check for this and I'm not so sure > that it always does on all codepaths. > > But, I'm ok with this for now, as you are just copying the bad C model > at the moment, but it really feels like a huge foot-gun waiting to go > off. Any way to put some more documentation here as in "use this at > your own risk!"? On the C side, how do you normally prevent uses of the device after it became invalid? Alice
On 15-01-25, 09:10, Alice Ryhl wrote: > On the C side, how do you normally prevent uses of the device after it > became invalid? IIUC, the per-cpu variable (cpu_sys_devices) that stores the pointers to CPU devices is cleared and later calls to get_cpu_device() returns NULL. The hot-unplug notifiers are fired for existing users (which have registered for the notifier, like cpufreq), wherein they can remove per-cpu sysfs files for example. But otherwise there is no way in the C code to disallow users of the CPU device pointer, if it is already fetched before the CPU is removed. The device pointer's memory doesn't get free here though and it works.
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index e9fdceb568b8..d63e7f6d10ea 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/cpu.h> #include <linux/cred.h> #include <linux/errname.h> #include <linux/ethtool.h> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs new file mode 100644 index 000000000000..422e874627d2 --- /dev/null +++ b/rust/kernel/cpu.rs @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generic CPU definitions. +//! +//! C header: [`include/linux/cpu.h`](srctree/include/linux/cpu.h) + +use crate::{ + bindings, + device::Device, + error::Result, + prelude::ENODEV, +}; + +/// Creates a new instance of CPU's device. +pub fn from_cpu(cpu: u32) -> Result<&'static Device> { + // 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 { Device::as_ref(ptr) }) +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 2d5c3d7d2e21..e9106b29c359 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -39,6 +39,7 @@ pub mod block; #[doc(hidden)] pub mod build_assert; +pub mod cpu; pub mod cred; pub mod device; pub mod device_id;
This implements cpu::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/bindings/bindings_helper.h | 1 + rust/kernel/cpu.rs | 26 ++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 3 files changed, 28 insertions(+) create mode 100644 rust/kernel/cpu.rs