| Message ID | 2025021026-exert-accent-b4c6@gregkh (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 78418f300d3999f1cf8a9ac71065bf2eca61f4dd |
| Headers | show |
| Series | Driver core: Add faux bus devices | expand |
On 10.02.25 13:30, Greg Kroah-Hartman wrote: > From: Lyude Paul <lyude@redhat.com> > > This introduces a module for working with faux devices in rust, along with > adding sample code to show how the API is used. Unlike other types of > devices, we don't provide any hooks for device probe/removal - since these > are optional for the faux API and are unnecessary in rust. For me it would be useful to know why this is the case. I looked at the dummy regulator driver and it still has a `probe` function. Presumably, because it has to wait until other resources are usable and that is the case once `probe` gets called. But doesn't the same hold for Rust? Or are Rust modules loaded later than C modules? Because if I were to rewrite the regulator driver in Rust (assuming we had the abstractions), the `probe` code would be put into the `Module::init` function, right? Or am I missing something? Thanks! Benno
On Mon, Feb 10, 2025 at 04:32:15PM +0000, Benno Lossin wrote: > On 10.02.25 13:30, Greg Kroah-Hartman wrote: > > From: Lyude Paul <lyude@redhat.com> > > > > This introduces a module for working with faux devices in rust, along with > > adding sample code to show how the API is used. Unlike other types of > > devices, we don't provide any hooks for device probe/removal - since these > > are optional for the faux API and are unnecessary in rust. > > For me it would be useful to know why this is the case. I looked at the > dummy regulator driver and it still has a `probe` function. Presumably, > because it has to wait until other resources are usable and that is the > case once `probe` gets called. But doesn't the same hold for Rust? Or > are Rust modules loaded later than C modules? Because if I were to > rewrite the regulator driver in Rust (assuming we had the abstractions), > the `probe` code would be put into the `Module::init` function, right? > Or am I missing something? AFAICS, the only reason for the dummy regulator driver to have probe() is because it calls devm_regulator_register() (which given that it neither ever removes the driver nor the device is meaningless, but let's ignore that). Actually, not even that would be a blocker, since the same would be valid as long as you ensure to call devm_*() after faux_device_create() and before faux_device_remove(). But obviously, it's way cleaner to enforce this through having the scope of the probe() callback. In Rust we only need devres for real device resources, which a faux device can never have. Given the example above, in Rust we wouldn't have anything like devm_regulator_register(), but a module structure with a regulator::Registration member, such that the registration is automatically removed when the module is dropped. I cannot predict if we ever gonna need probe() for the faux bus in Rust. Maybe at some point we will, and then we can add the corresponding abstraction. But for now, I don't see what we would need it for.
On Mon, 2025-02-10 at 13:30 +0100, Greg Kroah-Hartman wrote: > +/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. > +/// > +/// [`struct faux_device`]: srctree/include/linux/device/faux.h > +#[repr(transparent)] > +pub struct Registration(NonNull<bindings::faux_device>); One small tidbit I noticed this morning that slipped my mind - I had put #[repr(transparent)] here because when I was initially figuring out the bindings before patch submission I had `Registration` embed faux_device, which ended up not really making any sense and getting replaced with the NonNull. So we should be able to drop the #[repr(transparent)] here > +
On Mon, Feb 10, 2025 at 01:30:26PM +0100, Greg Kroah-Hartman wrote: > From: Lyude Paul <lyude@redhat.com> > > This introduces a module for working with faux devices in rust, along with > adding sample code to show how the API is used. Unlike other types of > devices, we don't provide any hooks for device probe/removal - since these > are optional for the faux API and are unnecessary in rust. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Cc: Maíra Canal <mairacanal@riseup.net> > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Acked-by: Danilo Krummrich <dakr@kernel.org>
On Mon, Feb 10, 2025 at 04:32:15PM +0000, Benno Lossin wrote: > On 10.02.25 13:30, Greg Kroah-Hartman wrote: > > From: Lyude Paul <lyude@redhat.com> > > > > This introduces a module for working with faux devices in rust, along with > > adding sample code to show how the API is used. Unlike other types of > > devices, we don't provide any hooks for device probe/removal - since these > > are optional for the faux API and are unnecessary in rust. > > For me it would be useful to know why this is the case. I looked at the > dummy regulator driver and it still has a `probe` function. Presumably, > because it has to wait until other resources are usable and that is the > case once `probe` gets called. But doesn't the same hold for Rust? Or > are Rust modules loaded later than C modules? Because if I were to > rewrite the regulator driver in Rust (assuming we had the abstractions), > the `probe` code would be put into the `Module::init` function, right? > Or am I missing something? I left the probe() in the dummy driver to keep the same pattern of the existing code, but there is no real requirement that it has that at all. I could have rewritten the dummy driver to not need it, and maybe, after time, we realize that no one needs the probe/remove function and then we can remove it then. thanks, greg k-h
On Mon, 10 Feb 2025 13:30:26 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > From: Lyude Paul <lyude@redhat.com> > > This introduces a module for working with faux devices in rust, along with > adding sample code to show how the API is used. Unlike other types of > devices, we don't provide any hooks for device probe/removal - since these > are optional for the faux API and are unnecessary in rust. > > Signed-off-by: Lyude Paul <lyude@redhat.com> > Cc: Maíra Canal <mairacanal@riseup.net> > Cc: Danilo Krummrich <dakr@kernel.org> > Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > v4: - new patch added to this series > - api tweaked due to parent pointer added to faux_device create > function. > v3: - no change > v2: - renamed vdev variable to fdev thanks to Mark > MAINTAINERS | 2 + > rust/bindings/bindings_helper.h | 1 + > rust/kernel/faux.rs | 67 ++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > samples/rust/Kconfig | 10 +++++ > samples/rust/Makefile | 1 + > samples/rust/rust_driver_faux.rs | 29 ++++++++++++++ > 7 files changed, 111 insertions(+) > create mode 100644 rust/kernel/faux.rs > create mode 100644 samples/rust/rust_driver_faux.rs > > diff --git a/MAINTAINERS b/MAINTAINERS > index 25c86f47353d..19ea159b2309 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7116,8 +7116,10 @@ F: rust/kernel/device.rs > F: rust/kernel/device_id.rs > F: rust/kernel/devres.rs > F: rust/kernel/driver.rs > +F: rust/kernel/faux.rs > F: rust/kernel/platform.rs > F: samples/rust/rust_driver_platform.rs > +F: samples/rust/rust_driver_faux.rs > > DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) > M: Nishanth Menon <nm@ti.com> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index 55354e4dec14..f46cf3bb7069 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -11,6 +11,7 @@ > #include <linux/blk_types.h> > #include <linux/blkdev.h> > #include <linux/cred.h> > +#include <linux/device/faux.h> > #include <linux/errname.h> > #include <linux/ethtool.h> > #include <linux/file.h> > diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs > new file mode 100644 > index 000000000000..5acc0c02d451 > --- /dev/null > +++ b/rust/kernel/faux.rs > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +//! Abstractions for the faux bus. > +//! > +//! This module provides bindings for working with faux devices in kernel modules. > +//! > +//! C header: [`include/linux/device/faux.h`] > + > +use crate::{bindings, device, error::code::*, prelude::*}; > +use core::ptr::{addr_of_mut, null, null_mut, NonNull}; > + > +/// The registration of a faux device. > +/// > +/// This type represents the registration of a [`struct faux_device`]. When an instance of this type > +/// is dropped, its respective faux device will be unregistered from the system. > +/// > +/// # Invariants > +/// > +/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. > +/// > +/// [`struct faux_device`]: srctree/include/linux/device/faux.h > +#[repr(transparent)] > +pub struct Registration(NonNull<bindings::faux_device>); > + > +impl Registration { > + /// Create and register a new faux device with the given name. > + pub fn new(name: &CStr) -> Result<Self> { > + // SAFETY: > + // - `name` is copied by this function into its own storage I think the comment is missing about the newly added `parent` argument. Perhaps we can add support in Rust already? Something simple as like pub fn new(name: &CStr, parent: Option<&device::Device>) -> Result<Self> { let dev = unsafe { bindings::faux_device_create(name.as_char_ptr(), parent.map_or(null_mut(), device::Device::as_raw), null()) }; ... } should work (although I haven't tested it). Best, Gary > + // - `faux_ops` is safe to leave NULL according to the C API > + let dev = unsafe { bindings::faux_device_create(name.as_char_ptr(), null_mut(), null()) }; > + > + // The above function will return either a valid device, or NULL on failure > + // INVARIANT: The device will remain registered until faux_device_destroy() is called, which > + // happens in our Drop implementation. > + Ok(Self(NonNull::new(dev).ok_or(ENODEV)?)) > + } > + > + fn as_raw(&self) -> *mut bindings::faux_device { > + self.0.as_ptr() > + } > +} > + > +impl AsRef<device::Device> for Registration { > + fn as_ref(&self) -> &device::Device { > + // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be > + // a valid initialized `device`. > + unsafe { device::Device::as_ref(addr_of_mut!((*self.as_raw()).dev)) } > + } > +} > + > +impl Drop for Registration { > + fn drop(&mut self) { > + // SAFETY: `self.0` is a valid registered faux_device via our type invariants. > + unsafe { bindings::faux_device_destroy(self.as_raw()) } > + } > +} > + > +// SAFETY: The faux device API is thread-safe as guaranteed by the device core, as long as > +// faux_device_destroy() is guaranteed to only be called once - which is guaranteed by our type not > +// having Copy/Clone. > +unsafe impl Send for Registration {} > + > +// SAFETY: The faux device API is thread-safe as guaranteed by the device core, as long as > +// faux_device_destroy() is guaranteed to only be called once - which is guaranteed by our type not > +// having Copy/Clone. > +unsafe impl Sync for Registration {} > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 496ed32b0911..398242f92a96 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -46,6 +46,7 @@ > pub mod devres; > pub mod driver; > pub mod error; > +pub mod faux; > #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] > pub mod firmware; > pub mod fs; > diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig > index 918dbead2c0b..3b6eae84b297 100644 > --- a/samples/rust/Kconfig > +++ b/samples/rust/Kconfig > @@ -61,6 +61,16 @@ config SAMPLE_RUST_DRIVER_PLATFORM > > If unsure, say N. > > +config SAMPLE_RUST_DRIVER_FAUX > + tristate "Faux Driver" > + help > + This option builds the Rust Faux driver sample. > + > + To compile this as a module, choose M here: > + the module will be called rust_driver_faux. > + > + If unsure, say N. > + > config SAMPLE_RUST_HOSTPROGS > bool "Host programs" > help > diff --git a/samples/rust/Makefile b/samples/rust/Makefile > index 5a8ab0df0567..0dbc6d90f1ef 100644 > --- a/samples/rust/Makefile > +++ b/samples/rust/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE) += rust_misc_device.o > obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o > obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o > +obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o > > rust_print-y := rust_print_main.o rust_print_events.o > > diff --git a/samples/rust/rust_driver_faux.rs b/samples/rust/rust_driver_faux.rs > new file mode 100644 > index 000000000000..048c6cb98b29 > --- /dev/null > +++ b/samples/rust/rust_driver_faux.rs > @@ -0,0 +1,29 @@ > +// SPDX-License-Identifier: GPL-2.0-only > + > +//! Rust faux device sample. > + > +use kernel::{c_str, faux, prelude::*, Module}; > + > +module! { > + type: SampleModule, > + name: "rust_faux_driver", > + author: "Lyude Paul", > + description: "Rust faux device sample", > + license: "GPL", > +} > + > +struct SampleModule { > + _reg: faux::Registration, > +} > + > +impl Module for SampleModule { > + fn init(_module: &'static ThisModule) -> Result<Self> { > + pr_info!("Initialising Rust Faux Device Sample\n"); > + > + let reg = faux::Registration::new(c_str!("rust-faux-sample-device"))?; > + > + dev_info!(reg.as_ref(), "Hello from faux device!\n"); > + > + Ok(Self { _reg: reg }) > + } > +}
diff --git a/MAINTAINERS b/MAINTAINERS index 25c86f47353d..19ea159b2309 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7116,8 +7116,10 @@ F: rust/kernel/device.rs F: rust/kernel/device_id.rs F: rust/kernel/devres.rs F: rust/kernel/driver.rs +F: rust/kernel/faux.rs F: rust/kernel/platform.rs F: samples/rust/rust_driver_platform.rs +F: samples/rust/rust_driver_faux.rs DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS) M: Nishanth Menon <nm@ti.com> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index 55354e4dec14..f46cf3bb7069 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -11,6 +11,7 @@ #include <linux/blk_types.h> #include <linux/blkdev.h> #include <linux/cred.h> +#include <linux/device/faux.h> #include <linux/errname.h> #include <linux/ethtool.h> #include <linux/file.h> diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs new file mode 100644 index 000000000000..5acc0c02d451 --- /dev/null +++ b/rust/kernel/faux.rs @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0-only + +//! Abstractions for the faux bus. +//! +//! This module provides bindings for working with faux devices in kernel modules. +//! +//! C header: [`include/linux/device/faux.h`] + +use crate::{bindings, device, error::code::*, prelude::*}; +use core::ptr::{addr_of_mut, null, null_mut, NonNull}; + +/// The registration of a faux device. +/// +/// This type represents the registration of a [`struct faux_device`]. When an instance of this type +/// is dropped, its respective faux device will be unregistered from the system. +/// +/// # Invariants +/// +/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`]. +/// +/// [`struct faux_device`]: srctree/include/linux/device/faux.h +#[repr(transparent)] +pub struct Registration(NonNull<bindings::faux_device>); + +impl Registration { + /// Create and register a new faux device with the given name. + pub fn new(name: &CStr) -> Result<Self> { + // SAFETY: + // - `name` is copied by this function into its own storage + // - `faux_ops` is safe to leave NULL according to the C API + let dev = unsafe { bindings::faux_device_create(name.as_char_ptr(), null_mut(), null()) }; + + // The above function will return either a valid device, or NULL on failure + // INVARIANT: The device will remain registered until faux_device_destroy() is called, which + // happens in our Drop implementation. + Ok(Self(NonNull::new(dev).ok_or(ENODEV)?)) + } + + fn as_raw(&self) -> *mut bindings::faux_device { + self.0.as_ptr() + } +} + +impl AsRef<device::Device> for Registration { + fn as_ref(&self) -> &device::Device { + // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be + // a valid initialized `device`. + unsafe { device::Device::as_ref(addr_of_mut!((*self.as_raw()).dev)) } + } +} + +impl Drop for Registration { + fn drop(&mut self) { + // SAFETY: `self.0` is a valid registered faux_device via our type invariants. + unsafe { bindings::faux_device_destroy(self.as_raw()) } + } +} + +// SAFETY: The faux device API is thread-safe as guaranteed by the device core, as long as +// faux_device_destroy() is guaranteed to only be called once - which is guaranteed by our type not +// having Copy/Clone. +unsafe impl Send for Registration {} + +// SAFETY: The faux device API is thread-safe as guaranteed by the device core, as long as +// faux_device_destroy() is guaranteed to only be called once - which is guaranteed by our type not +// having Copy/Clone. +unsafe impl Sync for Registration {} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 496ed32b0911..398242f92a96 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -46,6 +46,7 @@ pub mod devres; pub mod driver; pub mod error; +pub mod faux; #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] pub mod firmware; pub mod fs; diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig index 918dbead2c0b..3b6eae84b297 100644 --- a/samples/rust/Kconfig +++ b/samples/rust/Kconfig @@ -61,6 +61,16 @@ config SAMPLE_RUST_DRIVER_PLATFORM If unsure, say N. +config SAMPLE_RUST_DRIVER_FAUX + tristate "Faux Driver" + help + This option builds the Rust Faux driver sample. + + To compile this as a module, choose M here: + the module will be called rust_driver_faux. + + If unsure, say N. + config SAMPLE_RUST_HOSTPROGS bool "Host programs" help diff --git a/samples/rust/Makefile b/samples/rust/Makefile index 5a8ab0df0567..0dbc6d90f1ef 100644 --- a/samples/rust/Makefile +++ b/samples/rust/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE) += rust_misc_device.o obj-$(CONFIG_SAMPLE_RUST_PRINT) += rust_print.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI) += rust_driver_pci.o obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM) += rust_driver_platform.o +obj-$(CONFIG_SAMPLE_RUST_DRIVER_FAUX) += rust_driver_faux.o rust_print-y := rust_print_main.o rust_print_events.o diff --git a/samples/rust/rust_driver_faux.rs b/samples/rust/rust_driver_faux.rs new file mode 100644 index 000000000000..048c6cb98b29 --- /dev/null +++ b/samples/rust/rust_driver_faux.rs @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0-only + +//! Rust faux device sample. + +use kernel::{c_str, faux, prelude::*, Module}; + +module! { + type: SampleModule, + name: "rust_faux_driver", + author: "Lyude Paul", + description: "Rust faux device sample", + license: "GPL", +} + +struct SampleModule { + _reg: faux::Registration, +} + +impl Module for SampleModule { + fn init(_module: &'static ThisModule) -> Result<Self> { + pr_info!("Initialising Rust Faux Device Sample\n"); + + let reg = faux::Registration::new(c_str!("rust-faux-sample-device"))?; + + dev_info!(reg.as_ref(), "Hello from faux device!\n"); + + Ok(Self { _reg: reg }) + } +}