Re: [PATCH v11 5/7] samples: rust: Add debugfs sample driver

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Sep 8, 2025 at 3:08 PM CEST, Greg Kroah-Hartman wrote:
> On Thu, Sep 04, 2025 at 09:13:56PM +0000, Matthew Maurer wrote:
>> Adds a new sample driver that demonstrates the debugfs APIs.
>> 
>> The driver creates a directory in debugfs and populates it with a few
>> files:
>> - A read-only file that displays a fwnode property.
>> - A read-write file that exposes an atomic counter.
>> - A read-write file that exposes a custom struct.
>> 
>> This sample serves as a basic example of how to use the `debugfs::Dir`
>> and `debugfs::File` APIs to create and manage debugfs entries.
>> 
>> Signed-off-by: Matthew Maurer <mmaurer@google.com>
>> ---
>>  MAINTAINERS                  |   1 +
>>  samples/rust/Kconfig         |  11 ++++
>>  samples/rust/Makefile        |   1 +
>>  samples/rust/rust_debugfs.rs | 151 +++++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 164 insertions(+)
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 8f2dbf71ca3f8f97e4d7619375279ed11d1261b2..5b6db584a33dd7ee39de3fdd0085d2bd7b7bef0e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7481,6 +7481,7 @@ F:	rust/kernel/devres.rs
>>  F:	rust/kernel/driver.rs
>>  F:	rust/kernel/faux.rs
>>  F:	rust/kernel/platform.rs
>> +F:	samples/rust/rust_debugfs.rs
>>  F:	samples/rust/rust_driver_platform.rs
>>  F:	samples/rust/rust_driver_faux.rs
>>  
>> diff --git a/samples/rust/Kconfig b/samples/rust/Kconfig
>> index 7f7371a004ee0a8f67dca99c836596709a70c4fa..01101db41ae31b08a86d048cdd27da8ef9bb23a2 100644
>> --- a/samples/rust/Kconfig
>> +++ b/samples/rust/Kconfig
>> @@ -62,6 +62,17 @@ config SAMPLE_RUST_DMA
>>  
>>  	  If unsure, say N.
>>  
>> +config SAMPLE_RUST_DEBUGFS
>> +	tristate "DebugFS Test Module"
>> +	depends on DEBUG_FS
>> +	help
>> +	  This option builds the Rust DebugFS Test module sample.
>> +
>> +	  To compile this as a module, choose M here:
>> +	  the module will be called rust_debugfs.
>> +
>> +	  If unsure, say N.
>> +
>>  config SAMPLE_RUST_DRIVER_PCI
>>  	tristate "PCI Driver"
>>  	depends on PCI
>> diff --git a/samples/rust/Makefile b/samples/rust/Makefile
>> index bd2faad63b4f3befe7d1ed5139fe25c7a8b6d7f6..61276222a99f8cc6d7f84c26d0533b30815ebadd 100644
>> --- a/samples/rust/Makefile
>> +++ b/samples/rust/Makefile
>> @@ -4,6 +4,7 @@ ccflags-y += -I$(src)				# needed for trace events
>>  obj-$(CONFIG_SAMPLE_RUST_MINIMAL)		+= rust_minimal.o
>>  obj-$(CONFIG_SAMPLE_RUST_MISC_DEVICE)		+= rust_misc_device.o
>>  obj-$(CONFIG_SAMPLE_RUST_PRINT)			+= rust_print.o
>> +obj-$(CONFIG_SAMPLE_RUST_DEBUGFS)		+= rust_debugfs.o
>>  obj-$(CONFIG_SAMPLE_RUST_DMA)			+= rust_dma.o
>>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PCI)		+= rust_driver_pci.o
>>  obj-$(CONFIG_SAMPLE_RUST_DRIVER_PLATFORM)	+= rust_driver_platform.o
>> diff --git a/samples/rust/rust_debugfs.rs b/samples/rust/rust_debugfs.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..8d394f06b37e69ea1c30a3b0d8444c80562cc5ab
>> --- /dev/null
>> +++ b/samples/rust/rust_debugfs.rs
>> @@ -0,0 +1,151 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +// Copyright (C) 2025 Google LLC.
>> +
>> +//! Sample DebugFS exporting platform driver
>> +//!
>> +//! To successfully probe this driver with ACPI, use an ssdt that looks like
>> +//!
>> +//! ```dsl
>> +//! DefinitionBlock ("", "SSDT", 2, "TEST", "VIRTACPI", 0x00000001)
>> +//! {
>> +//!    Scope (\_SB)
>> +//!    {
>> +//!        Device (T432)
>> +//!        {
>> +//!            Name (_HID, "LNUXDEBF")  // ACPI hardware ID to match
>> +//!            Name (_UID, 1)
>> +//!            Name (_STA, 0x0F)        // Device present, enabled
>> +//!            Name (_DSD, Package () { // Sample attribute
>> +//!                ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>> +//!                Package() {
>> +//!                    Package(2) {"compatible", "sample-debugfs"}
>> +//!                }
>> +//!            })
>> +//!            Name (_CRS, ResourceTemplate ()
>> +//!            {
>> +//!                Memory32Fixed (ReadWrite, 0xFED00000, 0x1000)
>> +//!            })
>> +//!        }
>> +//!    }
>> +//! }
>> +//! ```
>> +
>> +use core::str::FromStr;
>> +use core::sync::atomic::AtomicUsize;
>> +use core::sync::atomic::Ordering;
>> +use kernel::c_str;
>> +use kernel::debugfs::{Dir, File};
>> +use kernel::new_mutex;
>> +use kernel::prelude::*;
>> +use kernel::sync::Mutex;
>> +
>> +use kernel::{acpi, device::Core, of, platform, str::CString, types::ARef};
>> +
>> +kernel::module_platform_driver! {
>> +    type: RustDebugFs,
>> +    name: "rust_debugfs",
>> +    authors: ["Matthew Maurer"],
>> +    description: "Rust DebugFS usage sample",
>> +    license: "GPL",
>> +}
>> +
>> +#[pin_data]
>> +struct RustDebugFs {
>> +    pdev: ARef<platform::Device>,
>> +    // As we only hold these for drop effect (to remove the directory/files) we have a leading
>> +    // underscore to indicate to the compiler that we don't expect to use this field directly.
>> +    _debugfs: Dir,
>> +    #[pin]
>> +    _compatible: File<CString>,
>> +    #[pin]
>> +    counter: File<AtomicUsize>,
>> +    #[pin]
>> +    inner: File<Mutex<Inner>>,
>
> Why are you saving the file pointers here at all?  They shouldn't be
> needed to keep around, all that should be needed is the Dir, right?  If
> not, this needs to be revisited...

It's not really the file pointers, but the actual data, i.e. a CString,
AtomicUsize and a Mutex<Inner>. That they're wrapped by the File<T> type, only
means that they're exported via debugfs (if enabled, otherwise they are not).

>> +impl platform::Driver for RustDebugFs {
>> +    type IdInfo = ();
>> +    const OF_ID_TABLE: Option<of::IdTable<Self::IdInfo>> = None;
>> +    const ACPI_ID_TABLE: Option<acpi::IdTable<Self::IdInfo>> = Some(&ACPI_TABLE);
>> +
>> +    fn probe(
>> +        pdev: &platform::Device<Core>,
>> +        _info: Option<&Self::IdInfo>,
>> +    ) -> Result<Pin<KBox<Self>>> {
>> +        let result = KBox::try_pin_init(RustDebugFs::new(pdev), GFP_KERNEL)?;
>> +        // We can still mutate fields through the files which are atomic or mutexed:
>> +        result.counter.store(91, Ordering::Relaxed);
>
> Here?  This feels odd, the file is the wrapper for the variable you want
> the file to show?  That feels backwards.  Usually the variable is always
> present, and then you can create a debugfs file to read/modify it.  But
> if debugfs is NOT enabled, you can still use the variable, it's just the
> file logic that is gone, right?

Please see [1]. :)

[1] https://lore.kernel.org/lkml/DCNG8UF8XFT2.12S9I7MBNV5PX@kernel.org/

> So what would this file look like without debugfs being enabled?  Would
> the atomic variable still be here?  I think it wouldn't, which is not
> ok...

Yes, that atomic would still be there, the File<AtomicUsize> behaves exactly
like a standalone AtomicUsize (except that it exports it in debugfs), and in
case of debugfs disabled you would access the atomic the exact same way, just
that no actual debugfs file exporting the AtomicUsize is created in the
background -- so fully transparent.