rust: DebugFS Bindings

These types live forever and do not require cleanup, so they can serve as ForeignOwnable.

Tested-by Dirk Behme <[email protected]>

Signed-off-by Matthew Maurer <[email protected]>

---
 rust/kernel/types.rs | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

rust/kernel/types.rs +7 -0

index 22985b6f69820d6df8ff3aae0bf815fad36a9d92..0a2b15cd05f91c69ef9c678555b845a23c19b82c 100644
@@ -142,6 +142,64 @@ unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
     unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
 }
 
+// SAFETY: The `into_foreign` function derives its pointer from a reference, so it is correctly
+// aligned.
+unsafe impl<T: 'static> ForeignOwnable for &'static T {
+    type PointedTo = T;
+    type Borrowed<'a> = &'a T;
+    type BorrowedMut<'a> = &'a T;
+

Just fyi, depending on what hits mainline first, this patch series or

https://lore.kernel.org/rust-for-linux/[email protected]/

rust/kernel/debugfs.rs fn create(_name: &CStr, _parent: Option<&Dir>) -> Self { · line 112

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
      }

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

Quoted 11 lines · Danilo Krummrich

rust/kernel/debugfs.rs fn create(_name: &CStr, _parent: Option<&Dir>) -> Self { · line 112

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
      }

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.
2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

If you want a .data() function, I can add it in,

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the

data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

rust/kernel/debugfs.rs +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 4 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the

Quoted 8 lines

data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

Quoted 90 lines · Danilo Krummrich

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

(quoted diff) +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 4 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the

Quoted 8 lines

data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 6 lines

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

I'm agreeing here. As one of the primary users of this api is going to be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make an example driver to emulate that file with just a local structure, but the reference counting and access logic just didn't seem to work out properly. Odds are I'm doing something stupid though...

So a file callback IS going to want to have access to the data of type T that is exposed somehow.

Quoted 98 lines · Greg Kroah-Hartman

Quoted 90 lines · Danilo Krummrich

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

(quoted diff) +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 4 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the

Quoted 8 lines

data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 6 lines

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

Quoted 5 lines

I'm agreeing here. As one of the primary users of this api is going to be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make an example driver to emulate that file with just a local structure, but the reference counting and access logic just didn't seem to work out properly. Odds are I'm doing something stupid though...

I think it technically works, but it imposes semantics on drivers that we shouldn't do; see the example below.

Quoted 2 lines

So a file callback IS going to want to have access to the data of type T that is exposed somehow.

With the current API we would need this:

	struct GPU {
	   fw: Arc<Firmware>,
	   fw_file: debugfs::File,
	}

and then I would initialize it the following way:

	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
	let fw_file = dir.create_file("firmware", fw.clone());

	fw.do_something();

This is bad, because now my Firmware instance in GPU needs to be reference counted, even though it should *never* out-live the GPU instance. This is error prone.

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

Quoted 123 lines · Danilo Krummrich

Quoted 98 lines · Greg Kroah-Hartman

Quoted 90 lines · Danilo Krummrich

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

(quoted diff) +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

	let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 4 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the

Quoted 8 lines

data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 6 lines

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

Quoted 5 lines

I'm agreeing here. As one of the primary users of this api is going to be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make an example driver to emulate that file with just a local structure, but the reference counting and access logic just didn't seem to work out properly. Odds are I'm doing something stupid though...

I think it technically works, but it imposes semantics on drivers that we shouldn't do; see the example below.

Quoted 2 lines

So a file callback IS going to want to have access to the data of type T that is exposed somehow.

With the current API we would need this:

	struct GPU {
	   fw: Arc<Firmware>,
	   fw_file: debugfs::File,
	}

and then I would initialize it the following way:

	let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
	let fw_file = dir.create_file("firmware", fw.clone());

Quoted 5 lines

	fw.do_something();

This is bad, because now my Firmware instance in GPU needs to be reference counted, even though it should *never* out-live the GPU instance. This is error prone.

Agreed, AND you just created a new fw structure that you really didn't need, wasting memory.

Quoted 17 lines

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

Quoted 93 lines · Greg Kroah-Hartman

Quoted 92 lines · Danilo Krummrich

Quoted 91 lines · Greg Kroah-Hartman

Quoted 90 lines · Danilo Krummrich

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

(quoted diff) +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

        let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 12 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I'm agreeing here. As one of the primary users of this api is going to be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make an example driver to emulate that file with just a local structure, but the reference counting and access logic just didn't seem to work out properly. Odds are I'm doing something stupid though...

Quoted 16 lines

Quoted 16 lines

I think it technically works, but it imposes semantics on drivers that we shouldn't do; see the example below.

Quoted 2 lines

So a file callback IS going to want to have access to the data of type T that is exposed somehow.

With the current API we would need this:

    struct GPU {
       fw: Arc<Firmware>,
       fw_file: debugfs::File,
    }

and then I would initialize it the following way:

Quoted 8 lines

Quoted 8 lines

    let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
    let fw_file = dir.create_file("firmware", fw.clone());

    fw.do_something();

This is bad, because now my Firmware instance in GPU needs to be reference counted, even though it should *never* out-live the GPU instance. This is error prone.

Quoted 3 lines

Agreed, AND you just created a new fw structure that you really didn't need, wasting memory.

In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just creates a new reference count to the same object.

Quoted 23 lines

Quoted 17 lines

Instead this should just be:

    struct GPU {
       fw: debugfs::File<Firmware>,
    }

and then I would initialize it the following way:

    let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
    let file = dir.create_file("firmware", fw);

    // debugfs::File<Firmware> dereferences to Firmware
    file.do_something();

    // Access to fw is prevented by the compiler, since it has been moved
    // into file.

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

        impl Dir {
           pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
              pin_init!(Self {
                 data <- data,
                 ...
              })
           }
        }

This allows us to do things like:

        #[pin_data]
        struct Firmware {
           #[pin]
           minor: debugfs::File<u32>,
           #[pin]
           major: debugfs::File<u32>,
           #[pin]
           buffer: debugfs::File<[u8]>,

        }

        impl Firmware {
           pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
              pin_init!(Self {
                 minor <- dir.create_file("minor", 1),
                 major <- dir.create_file("major", 2),
                 buffer <- dir.create_file("buffer", buffer),
              })
           }
        }

        // This is the only allocation we need.
        let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

        struct GPU {
           fw: debugfs::File<Firmware>,
        }

        let file = dir.create_file("firmware", Firmware::new());
        let file = KBox::pin_init(file, GFP_KERNEL)?;

        // debugfs::File<Firmware> dereferences to Firmware
        file.do_something();

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Quoted 95 lines · Danilo Krummrich

Quoted 93 lines · Greg Kroah-Hartman

Quoted 92 lines · Danilo Krummrich

Quoted 91 lines · Greg Kroah-Hartman

Quoted 90 lines · Danilo Krummrich

Quoted 32 lines · Matthew Maurer

Quoted 31 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

Quoted 11 lines · Danilo Krummrich

Quoted 8 lines · Matthew Maurer

(quoted diff) +7 -0

+    fn create_file<D: ForeignOwnable>(&self, _name: &CStr, data: D) -> File
+    where
+        for<'a> D::Borrowed<'a>: Display,
+    {
+        File {
+            _foreign: ForeignHolder::new(data),
+        }
       }

Quoted 5 lines

Quoted 2 lines

What's the motivation for the ForeignHolder abstraction? Why not just make it File<D> and store data directly?

1. A File<D> can't be held in collection data structures as easily unless all your files contain the *same* backing type.

That sounds reasonable.

Quoted 4 lines

2. None of the APIs or potential APIs for File care about which type it's wrapping, nor are they supposed to. If nothing you can do with a File is different depending on the backing type, making it polymorphic is just needlessly confusing.

What if I want to access file.data() and do something with the data? Then I'd necessarily need to put my data in an Arc and reference count it to still be able to access it.

Quoted 8 lines

Quoted 2 lines

That doesn't seem like a reasonable requirement to be able to access data exposed via debugfs.

pub fn data(&self) -> D would go against my understanding of Greg's request for DebugFS files to not really support anything other than delete. I was even considering making D not be retained in the disabled debugfs case, but left it in for now for so that the lifecycles wouldn't change.

Well, that's because the C side does not have anything else. But the C side has no type system that deals with ownership:

In C you just stuff a pointer of your private data into debugfs_create_file() without any implication of ownership. debugfs has a pointer, the driver has a pointer. The question of the ownership semantics is not answered by the API, but by the implementation of the driver.

The Rust API is different, and it's even implied by the name of the trait you expect the data to implement: ForeignOwnable.

The File *owns* the data, either entirely or a reference count of the data.

If the *only* way to access the data the File now owns is by making it reference counted, it:

1) Is additional overhead imposed on users.

2) It has implications on the ownership design of your driver. Once something is reference counted, you loose the guarantee the something can't out-live some event.

I don't want that people have to stuff their data structures into Arc (i.e. reference count them), even though that's not necessary. It makes it easy to make mistakes. Things like:

        let foo = bar.clone();

can easily be missed in reviews, whereas some contributor falsely changing a KBox to an Arc is much harder to miss.

Quoted 1 line

If you want a .data() function, I can add it in,

I think it could even be an implementation of Deref.

Quoted 12 lines

but I don't think
it'll improve flexibility in most cases. If you want to do something with the data and it's not in an Arc / behind a handle of some kind, you'll need something providing threadsafe interior mutability in the data structure. If that's a lock, then I have a hard time believing that Arc<Mutex<T>>(or if it's a global, a &'static Mutex<T>, which is why I added that in the stack) is so much more expensive than Box<Mutex<T>> that it's worth a more complex API. If it's an atomic, e.g. Arc<AtomicU8>, then I can see the benefit to having Box<AtomicU8> over that, but it still seems so slim that I think the simpler "File is just a handle to how long the file stays alive, it doesn't let you do anything else" API makes sense.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I don't really see what is complicated about File<T> -- it's a File and it owns data of type T that is exposed via debugfs. Seems pretty straight forward to me.

Maybe the performance cost is not a huge argument here, but maintainability in terms of clarity about ownership and lifetime of an object as explained above clearly is.

The part that is not straightforward is that the primary purpose of T may not be "to be exposed via DebugFS", and leaves us in the unusual world where DebugFS (or at least our bindings to it) are load-bearing in builds that have DebugFS disabled. It can work, but it definitely seems confusing and is privileging DebugFS holding a reference to a data structure over other things holding a reference to it.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

I'm agreeing here. As one of the primary users of this api is going to be a "soc info" module, like drivers/soc/qcom/socinfo.c, I tried to make an example driver to emulate that file with just a local structure, but the reference counting and access logic just didn't seem to work out properly. Odds are I'm doing something stupid though...

I have a version of this that works with my initial driver, and keep intending to revise it to work on top of the new one, but I keep getting requests for API changes before I get around to reworking it ;)

The way I intended my ForeignOwnable variant of this to work was that I would have a *single* struct that contained all the scanned data. For the socinfo module, we know how big this could be, so we could put this as a static in the module, in which case we'd have &'static MyInfo. If we didn't want to do that or didn't know, we'd produce an Arc<MyInfo>. Then, for each file, we'd create a file that passed in either my_arc_info.clone() or my_info if we had the static reference, with the functions that do the printing distinguishing the behavior of each file.

Doing the file-owns-the-specific-field version *could* work, but means that the debugfs tree code would be interleaved with the calls to parsing code, or that we'd need two copies of the struct, an initial one with all the parsed data, and a follow-up one that has has them all wrapped in File.

Quoted 16 lines

Quoted 16 lines

Quoted 16 lines

I think it technically works, but it imposes semantics on drivers that we shouldn't do; see the example below.

Quoted 2 lines

So a file callback IS going to want to have access to the data of type T that is exposed somehow.

With the current API we would need this:

    struct GPU {
       fw: Arc<Firmware>,
       fw_file: debugfs::File,
    }

and then I would initialize it the following way:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

    let fw = Arc::new(Firmware::new(), GFP_KERNEL)?;
    let fw_file = dir.create_file("firmware", fw.clone());

    fw.do_something();

This is bad, because now my Firmware instance in GPU needs to be reference counted, even though it should *never* out-live the GPU instance. This is error prone.

If Firmware outliving the GPU instance causes a bug, not just a resource leak, I'd argue that you should be passing a narrower handle to your debugfs file. If it just causes a resource leak, then I'd argue that if you leak a resource (the debugfs file itself), then leaking a resource is not unexpected.

For the narrower handle example, consider e.g.

```
struct Firmware {
  info: Arc<FirmwareInfo>,
  my_handle: Handle // Some handle whose drop is load bearing
}
// ...
let fw: Firmware = build_fw()?;
let fw_file = fw.info.clone();
fw.do_something();
```

Quoted 27 lines

Quoted 3 lines

Agreed, AND you just created a new fw structure that you really didn't need, wasting memory.

In case you refer to fw.clone(), since fw is an Arc<Firmware>, clone() just creates a new reference count to the same object.

Quoted 20 lines

Quoted 19 lines

Instead this should just be:

    struct GPU {
       fw: debugfs::File<Firmware>,
    }

and then I would initialize it the following way:

    let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
    let file = dir.create_file("firmware", fw);

    // debugfs::File<Firmware> dereferences to Firmware
    file.do_something();

    // Access to fw is prevented by the compiler, since it has been moved
    // into file.

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

Quoted 3 lines

Quoted 3 lines

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

This seems odd to me because with this model the DebugFS file itself becomes deeply involved with the ownership of the devices or other data.

There are a few issues I think this creates:
1. It discourages file-per-field, because you can no longer have the DebugFS file uniquely own something, which you're now trying to encourage. This seems likely to result in people having debugfs files that render out structured data, which I think we generally don't want?

2. If I have a module and want to add a new DebugFS file to it, the change will edit the core structures of the drivers rather than just adding an auxiliary field. This is partially remedied if we make File<D> implement Deref<Target=D>.

It will still either incur
churn or imprecision around commonly derived traits like equality and debug. If I have #[derive(PartialEq)] on my struct including DeviceData, and then I wrap that into a File<DeviceData>, we'll

get stuck with a quandry:
a. Should files x == y if the underlying private data is equal?
This has the benefit of preserving the behavior of the derive, but means that distinct files may compare equal if they have the same contents.
b. Should files x == y only be true if the underlying *file* is
the same? This seems logical for *file* equality, but will break equality on the field or structures including it.

This will also be an incompatible change for anyone already using an
explicit .deref() or an explicit impl Deref<Target = T> method to work with the field, because while deref coercion will automatically chain, impl Deref<Target = T> *cannot* automatically chain.

3. If you create an object specifically for debugging, then it will *have* to be created and kept around even if DebugFS is disabled, because we can't tell whether the user is relying on its existence. (My current implementation keeps it around in that case as well, but that's not guaranteed or required - we could cause it to just immediately drop data that someone tried to attach.) This means that rather than relying on the API to determine whether the information is needed, the modules will need to check the config if they want to get rid of it.

Quoted 21 lines

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

        impl Dir {
           pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
              pin_init!(Self {
                 data <- data,
                 ...
              })
           }
        }

This allows us to do things like:

Quoted 8 lines

        #[pin_data]
        struct Firmware {
           #[pin]
           minor: debugfs::File<u32>,
           #[pin]
           major: debugfs::File<u32>,
           #[pin]
           buffer: debugfs::File<[u8]>,

Nit: This can't work, because a &[u8] is double-width, and we only get to send one pointer into the file. We'd need to add a requirement T: Sized if we really wanted to have File embed the data.

Quoted 16 lines

        }

        impl Firmware {
           pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
              pin_init!(Self {
                 minor <- dir.create_file("minor", 1),
                 major <- dir.create_file("major", 2),
                 buffer <- dir.create_file("buffer", buffer),
              })
           }
        }

        // This is the only allocation we need.
        let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

Quoted 7 lines

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

If it's easy to use the model you described earlier where the sole owner of the data is the debugfs file, then this is mostly fine, because to use the old version, you'd already have it live in a ForeignOwnable, which most likely means its own allocation. However, if you're already forced into an environment where the objects you're embedding into a file are reference counted, you now have an extra allocation required for every File. Because the File has to be able to respond to data(), we also won't have these Files truncate to zero size in the no debugfs scenario.

This means that if you want a File to render an object that is reference counted, and they're dynamically created and destroyed, you will now be required to incur an allocation per file, even when DebugFS is disabled.

Quoted 13 lines

        struct GPU {
           fw: debugfs::File<Firmware>,
        }

        let file = dir.create_file("firmware", Firmware::new());
        let file = KBox::pin_init(file, GFP_KERNEL)?;

        // debugfs::File<Firmware> dereferences to Firmware
        file.do_something();

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Quoted 34 lines · Matthew Maurer

Quoted 13 lines · Danilo Krummrich

        impl Firmware {
           pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
              pin_init!(Self {
                 minor <- dir.create_file("minor", 1),
                 major <- dir.create_file("major", 2),
                 buffer <- dir.create_file("buffer", buffer),
              })
           }
        }

        // This is the only allocation we need.
        let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

Quoted 7 lines

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

Quoted 57 lines · Danilo Krummrich

Quoted 34 lines · Matthew Maurer

Quoted 13 lines · Danilo Krummrich

         impl Firmware {
            pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
               pin_init!(Self {
                  minor <- dir.create_file("minor", 1),
                  major <- dir.create_file("major", 2),
                  buffer <- dir.create_file("buffer", buffer),
               })
            }
         }

         // This is the only allocation we need.
         let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

Quoted 7 lines

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

Quoted 5 lines

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

     struct Segment<T> {
         elements: [T; 16]
     }

or make the segments variable-sized and grow them accordingly.

Quoted 78 lines · Benno Lossin

Quoted 57 lines · Danilo Krummrich

Quoted 34 lines · Matthew Maurer

Quoted 13 lines · Danilo Krummrich

         impl Firmware {
            pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
               pin_init!(Self {
                  minor <- dir.create_file("minor", 1),
                  major <- dir.create_file("major", 2),
                  buffer <- dir.create_file("buffer", buffer),
               })
            }
         }

         // This is the only allocation we need.
         let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

Quoted 7 lines

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

Quoted 5 lines

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

Quoted 5 lines

     struct Segment<T> {
         elements: [T; 16]
     }

or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Quoted 61 lines · Danilo Krummrich

Quoted 60 lines · Benno Lossin

Quoted 35 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

Quoted 5 lines

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

     struct Segment<T> {
         elements: [T; 16]
     }

Quoted 3 lines

Quoted 1 line

or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Haha I didn't know this already existed in the kernel :) Yeah then we should make XArray work for this use-case.

Quoted 66 lines · Benno Lossin

Quoted 64 lines · Danilo Krummrich

Quoted 60 lines · Benno Lossin

Quoted 35 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

	struct Client {
	   id: File<ClientId>,
	   data: File<ClientData>,
	   ...
	}

Quoted 5 lines

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

     struct Segment<T> {
         elements: [T; 16]
     }

Quoted 1 line

or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Quoted 2 lines

Haha I didn't know this already existed in the kernel :) Yeah then we should make XArray work for this use-case.

Ah wait, I meant for SegmentedVec<T> to store multiple T in the same allocation, so it would only grow sometimes and amortize the allocations just like Vec. It seems to me that XArray only stores pointers, so you have to box everything (which we're trying to avoid IIUC).

Quoted 67 lines · Benno Lossin

Quoted 66 lines · Benno Lossin

Quoted 64 lines · Danilo Krummrich

Quoted 60 lines · Benno Lossin

Quoted 33 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

 struct Client {
    id: File<ClientId>,
    data: File<ClientData>,
    ...

Quoted 7 lines

}

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

     struct Segment<T> {
         elements: [T; 16]
     }

Quoted 1 line

or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Quoted 7 lines

Quoted 2 lines

Haha I didn't know this already existed in the kernel :) Yeah then we should make XArray work for this use-case.

Ah wait, I meant for SegmentedVec<T> to store multiple T in the same allocation, so it would only grow sometimes and amortize the allocations just like Vec. It seems to me that XArray only stores pointers, so you have to box everything (which we're trying to avoid IIUC).

Yes, that sounds good. And if the potential number of Client instances can get pretty large Vec isn't a good choice anyways. If we really get a ton of clients, they should be allocated with a kmem_cache and stored in an XArray, maple tree, etc.

Quoted 76 lines · Danilo Krummrich

Quoted 67 lines · Benno Lossin

Quoted 66 lines · Benno Lossin

Quoted 64 lines · Danilo Krummrich

Quoted 60 lines · Benno Lossin

Quoted 33 lines · Danilo Krummrich

Quoted 12 lines · Matthew Maurer

If we implement *only* pinned files, we run into an additional problem
- you can't easily extend a pinned vector. This means that you cannot have dynamically created devices unless you're willing to put every new File into its own Box, because you aren't allowed to move any of the previously allocated Files for a resize.

Where previously you would have had

```
debug_files: Vec<File>
```

Quoted 5 lines

you would now have

```
debug_files: Vec<PinBox<File<T>>>
```

Stuffing single File instances into a Vec seems like the wrong thing to do.

Instead you may have instances of some data structure that is created dynamically in your driver that you want to expose through debugfs.

Let's say you have (userspace) clients that can be registered arbitrarily, then you want a Vec<Client>, which contains the client instances. In order to provide information about the Client in debugfs you then have the client embed things as discussed above.

 struct Client {
    id: File<ClientId>,
    data: File<ClientData>,
    ...

Quoted 7 lines

}

I think that makes much more sense than keeping a Vec<Arc<Client>> *and* a Vec<File> separately. Also, note that with the above, your Client instances don't need to be reference counted anymore.

I think this addresses the concerns below.

You still have the issue that Client now needs to be pinned and the vector can't be resized. But if you know that it's bounded, then we could just make Pin<Vec<T>> work as expected (not relocating the underlying allocation by not exposing push, only push_within_capacity).

We also could have a SegmentedVec<T> that doesn't move elements. Essentially it is

     enum SegmentedVec<T> {
         Cons(Segment<T>, KBox<SegmentedVec<T>>)
         Nul,
     }

     struct Segment<T> {
         elements: [T; 16]
     }

Quoted 1 line

or make the segments variable-sized and grow them accordingly.

That sounds a lot like the perfect application for XArray. :)

Quoted 7 lines

Quoted 2 lines

Haha I didn't know this already existed in the kernel :) Yeah then we should make XArray work for this use-case.

Ah wait, I meant for SegmentedVec<T> to store multiple T in the same allocation, so it would only grow sometimes and amortize the allocations just like Vec. It seems to me that XArray only stores pointers, so you have to box everything (which we're trying to avoid IIUC).

Quoted 4 lines

Yes, that sounds good. And if the potential number of Client instances can get pretty large Vec isn't a good choice anyways. If we really get a ton of clients, they should be allocated with a kmem_cache and stored in an XArray, maple tree, etc.

OK. I've outlined why I disagree, but it sounds like consensus here is that:
1. You want a File<T> that implements Deref to expose a &T.
2. To enable T to take on values which are not ForeignOwnable, you want me to make create_file return a PinInit<File<T>> (We will also grow a T: Sized bound here.)

You are aware that:
1. A File can no longer be placed in a normal Rust collection, you will either need to box it or use a special collection kind.

2. Adding or removing DebugFS support for some data in a driver may cause more noise (code edits non-local to the debug logic) than expected.

The one thing I still don't see a consensus on:

What do you want me to do about standard traits for File? If we're intending it to be heavily used throughout structs, we'll effectively break #[derive] if I don't add support. For example, if we had

```
#[derive(Debug, PartialEq)]
struct FooAttrs {
  name: String,
  size: usize,
}
// In my current patch world someone adds `File`, it's by putting
`FooAttrs` into an `Arc` and passing that into `create_file`, no
modifications to the structure are made.
```
before, we have an obvious semantics for this. If someone adds `File`
with the new API, we get
```
#[derive(Debug, PartialEq)]
#[pin_data]
struct FooAttrs {
  #[pin]
  name: File<String>,
  #[pin]
  size: File<usize>,
}
```

This means that for the #[derive] to keep working, File needs to implement these traits. Do we want it to:
A. Purely delegate, so making this sort of change keeps the same behavior of derived traits
B. Do what makes sense for a File, so e.g. Debug might print 'File {
data: "name_of_foo" }', and PartialEq might test for equality of the dentry if not an error

I'm guessing you want A, but I've been through so many API reworks at this point that I wanted to ask in case there's disagreement here.

I think we can add an additional API like this:

	impl Dir {
	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
	      pin_init!(Self {
	         data <- data,
	         ...
	      })
	   }
	}

This allows us to do things like:

	#[pin_data]
	struct Firmware {
	   #[pin]
	   minor: debugfs::File<u32>,
	   #[pin]
	   major: debugfs::File<u32>,
	   #[pin]
	   buffer: debugfs::File<[u8]>,
	}

	impl Firmware {
	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
	      pin_init!(Self {
	         minor <- dir.create_file("minor", 1),
	         major <- dir.create_file("major", 2),
	         buffer <- dir.create_file("buffer", buffer),
	      })
	   }
	}

	// This is the only allocation we need.
	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

	let file = dir.create_file("firmware", Firmware::new());
	let file = KBox::pin_init(file, GFP_KERNEL)?;

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Danilo Krummrich

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

Quoted 6 lines

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

	impl Dir {
	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
	      pin_init!(Self {
	         data <- data,
	         ...
	      })
	   }
	}

This allows us to do things like:

	#[pin_data]
	struct Firmware {
	   #[pin]
	   minor: debugfs::File<u32>,
	   #[pin]
	   major: debugfs::File<u32>,
	   #[pin]
	   buffer: debugfs::File<[u8]>,
	}

	impl Firmware {
	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
	      pin_init!(Self {
	         minor <- dir.create_file("minor", 1),
	         major <- dir.create_file("major", 2),
	         buffer <- dir.create_file("buffer", buffer),
	      })
	   }
	}

	// This is the only allocation we need.
	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

	let file = dir.create_file("firmware", Firmware::new());
	let file = KBox::pin_init(file, GFP_KERNEL)?;

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Quoted 87 lines · Danilo Krummrich

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Danilo Krummrich

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

Quoted 6 lines

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

	impl Dir {
	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
	      pin_init!(Self {
	         data <- data,
	         ...
	      })
	   }
	}

This allows us to do things like:

	#[pin_data]
	struct Firmware {
	   #[pin]
	   minor: debugfs::File<u32>,
	   #[pin]
	   major: debugfs::File<u32>,
	   #[pin]
	   buffer: debugfs::File<[u8]>,
	}

	impl Firmware {
	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
	      pin_init!(Self {
	         minor <- dir.create_file("minor", 1),
	         major <- dir.create_file("major", 2),
	         buffer <- dir.create_file("buffer", buffer),
	      })
	   }
	}

	// This is the only allocation we need.
	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

	let file = dir.create_file("firmware", Firmware::new());
	let file = KBox::pin_init(file, GFP_KERNEL)?;

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

Quoted 2 lines

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Does this actually work in practice for anything except immutable data? I mean, let's take Rust Binder as an example and lets say that I want to expose a directory for each Process object with some of the fields exposed. Let's just simplify Rust Binder a bit and only include some of the fields:

#[pin_data]

struct Process {
    task: ARef<Task>,
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: RBTree<i32, Arc<Thread>>,
    nodes: RBTree<u64, DArc<Node>>,
    requested_thread_count: u32,
    max_threads: u32,
    started_thread_count: u32,
}

Rust Binder already does expose some debugging data through a file system, though it doesn't do so using debugfs. It exposes a lot of data, but among them are the pid, the number of threads and nodes, as well as the values of requested_thread_count, started_thread_count, and max_threads.

Now, we run into problem number one: pinning is not supported inside mutexes. But let's say we solved that and we could do this:

#[pin_data]
struct Process {
task: File<ARef<Task>>, // prints the pid
#[pin]
inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
requested_thread_count: File<u32>,
max_threads: File<u32>,
started_thread_count: File<u32>,
}

However, this still doesn't work! Debugfs may get triggered at any time and need to read these fields, and there's no way for it to take the spinlock with the above design - it doesn't know where the spinlock is. For the integers I guess we could make them atomic to allow reading them in parallel with mutation, but that option is not available for the red/black trees.

What is the intended solution in this case? If the argument is that this is a rare case, then keep in mind that this is a real-world example of debugging information that we actually expose today in a real driver. With Matt's current approach, it's relatively easy - just store a bunch of File<Arc<Process>> instances somewhere and define each one to take the mutex and print the relevant value.

Quoted 143 lines · Alice Ryhl

Quoted 87 lines · Danilo Krummrich

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Danilo Krummrich

Instead this should just be:

struct GPU {
fw: debugfs::File<Firmware>,
}

and then I would initialize it the following way:

let fw = KBox::new(Firmware::new(), GFP_KERNEL)?; let file = dir.create_file("firmware", fw);

// debugfs::File<Firmware> dereferences to Firmware file.do_something();

// Access to fw is prevented by the compiler, since it has been moved // into file.

Quoted 6 lines

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

     impl Dir {
        pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
           pin_init!(Self {
              data <- data,
              ...
           })
        }
     }

This allows us to do things like:

     #[pin_data]
     struct Firmware {
        #[pin]
        minor: debugfs::File<u32>,
        #[pin]
        major: debugfs::File<u32>,
        #[pin]
        buffer: debugfs::File<[u8]>,
     }

     impl Firmware {
        pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
           pin_init!(Self {
              minor <- dir.create_file("minor", 1),
              major <- dir.create_file("major", 2),
              buffer <- dir.create_file("buffer", buffer),
           })
        }
     }

     // This is the only allocation we need.
     let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

     struct GPU {
        fw: debugfs::File<Firmware>,
     }

     let file = dir.create_file("firmware", Firmware::new());
     let file = KBox::pin_init(file, GFP_KERNEL)?;

     // debugfs::File<Firmware> dereferences to Firmware
     file.do_something();

Quoted 2 lines

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Does this actually work in practice for anything except immutable data? I mean, let's take Rust Binder as an example and lets say that I want to expose a directory for each Process object with some of the fields exposed. Let's just simplify Rust Binder a bit and only include some of the fields:

#[pin_data]

struct Process {
    task: ARef<Task>,
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: RBTree<i32, Arc<Thread>>,
    nodes: RBTree<u64, DArc<Node>>,
    requested_thread_count: u32,
    max_threads: u32,
    started_thread_count: u32,
}

Rust Binder already does expose some debugging data through a file system, though it doesn't do so using debugfs. It exposes a lot of data, but among them are the pid, the number of threads and nodes, as well as the values of requested_thread_count, started_thread_count, and max_threads.

Now, we run into problem number one: pinning is not supported inside mutexes. But let's say we solved that and we could do this:

#[pin_data]
struct Process {
task: File<ARef<Task>>, // prints the pid
#[pin]
inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
requested_thread_count: File<u32>,
max_threads: File<u32>,
started_thread_count: File<u32>,
}

However, this still doesn't work! Debugfs may get triggered at any time and need to read these fields, and there's no way for it to take the spinlock with the above design - it doesn't know where the spinlock is. For the integers I guess we could make them atomic to allow reading them in parallel with mutation, but that option is not available for the red/black trees.

Quoted 6 lines

What is the intended solution in this case? If the argument is that this is a rare case, then keep in mind that this is a real-world example of debugging information that we actually expose today in a real driver. With Matt's current approach, it's relatively easy - just store a bunch of File<Arc<Process>> instances somewhere and define each one to take the mutex and print the relevant value.

How would your example look like with the current approach? IIUC, it also wouldn't work, because the debugfs data can't be mutated?

Quoted 152 lines · Benno Lossin

Quoted 149 lines · Alice Ryhl

Quoted 87 lines · Danilo Krummrich

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Danilo Krummrich

Instead this should just be:

struct GPU {
fw: debugfs::File<Firmware>,
}

and then I would initialize it the following way:

let fw = KBox::new(Firmware::new(), GFP_KERNEL)?; let file = dir.create_file("firmware", fw);

// debugfs::File<Firmware> dereferences to Firmware file.do_something();

// Access to fw is prevented by the compiler, since it has been moved // into file.

Quoted 6 lines

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

     impl Dir {
        pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
           pin_init!(Self {
              data <- data,
              ...
           })
        }
     }

This allows us to do things like:

     #[pin_data]
     struct Firmware {
        #[pin]
        minor: debugfs::File<u32>,
        #[pin]
        major: debugfs::File<u32>,
        #[pin]
        buffer: debugfs::File<[u8]>,
     }

     impl Firmware {
        pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
           pin_init!(Self {
              minor <- dir.create_file("minor", 1),
              major <- dir.create_file("major", 2),
              buffer <- dir.create_file("buffer", buffer),
           })
        }
     }

     // This is the only allocation we need.
     let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

     struct GPU {
        fw: debugfs::File<Firmware>,
     }

     let file = dir.create_file("firmware", Firmware::new());
     let file = KBox::pin_init(file, GFP_KERNEL)?;

     // debugfs::File<Firmware> dereferences to Firmware
     file.do_something();

Quoted 2 lines

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Does this actually work in practice for anything except immutable data? I mean, let's take Rust Binder as an example and lets say that I want to expose a directory for each Process object with some of the fields exposed. Let's just simplify Rust Binder a bit and only include some of the fields:

#[pin_data]

struct Process {
    task: ARef<Task>,
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: RBTree<i32, Arc<Thread>>,
    nodes: RBTree<u64, DArc<Node>>,
    requested_thread_count: u32,
    max_threads: u32,
    started_thread_count: u32,
}

Rust Binder already does expose some debugging data through a file system, though it doesn't do so using debugfs. It exposes a lot of data, but among them are the pid, the number of threads and nodes, as well as the values of requested_thread_count, started_thread_count, and max_threads.

Now, we run into problem number one: pinning is not supported inside mutexes. But let's say we solved that and we could do this:

#[pin_data]
struct Process {
task: File<ARef<Task>>, // prints the pid
#[pin]
inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
requested_thread_count: File<u32>,
max_threads: File<u32>,
started_thread_count: File<u32>,
}

However, this still doesn't work! Debugfs may get triggered at any time and need to read these fields, and there's no way for it to take the spinlock with the above design - it doesn't know where the spinlock is. For the integers I guess we could make them atomic to allow reading them in parallel with mutation, but that option is not available for the red/black trees.

What is the intended solution in this case? If the argument is that this is a rare case, then keep in mind that this is a real-world example of debugging information that we actually expose today in a real driver. With Matt's current approach, it's relatively easy - just store a bunch of File<Arc<Process>> instances somewhere and define each one to take the mutex and print the relevant value.

Quoted 2 lines

How would your example look like with the current approach? IIUC, it also wouldn't work, because the debugfs data can't be mutated?

I would store a bunch of File<Arc<Process>> instances somewhere. Each one has a closure that takes the spinlock and prints the appropriate value.

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly. We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

So how about a platform driver that exposes values read from a platform device (i.e. a soc info driver), that also includes a local-to-the-device data structure that can be locked and modified? That should cover all the use cases that I can think of at the moment.

Quoted 4 lines · Greg Kroah-Hartman

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly. We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

Well, that's possible with both approaches. Data behind a lock becomes mutable once you grabbed the lock. That's the same in both cases.

The difference is that with the pin-init approach I propose you can't have what Alice sketched up. And I think it's even desirable that you can't do it.

Let's me explain the difference on a simplified example, based on Alice' example.

ForeignOwnable API
------------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: SpinLock<ProcessInner>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here we have to create an Arc<Process> (let's call it process) and create files from it.

	let file_threads = dir.create_file("threads", process);
	let file_max_threads = dir.create_file("max_threads", process);

In the file system callback of both of these, we now have an Arc<Process>, hence we can access:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

and in the other file:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

Pin-Init API
------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: File<SpinLock<ProcessInner>>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here Process does not need to be within an Arc and no separate file instances need to be kept around, that happens already within the constructor of Process:

	pin_init!(Process {
	   inner <- dir.create_file("process_inner", ...),
	   [...]
	})

The file itself has a reference to SpinLock<ProcessInner>, hence we can access:

	let guard = inner.lock();

	read_or_write(guard.threads)
	read_or_write(guard.max_threads)

The difference is that with the ForeignOwnable API it was possible to have separate files for threads and max_threads.

While with the pin-init one we either have to have a single file exposing ProcessInner (which is what I did above) or protect threads and max_threads with separate locks (of course max_threads could also just be an atomic).

(If you like I can sketch up this case as well.)

At a first glance this seems like an undesirable limitation, but I argue that this is a good thing.

The reason I think so is what I also explained in [1], but let me adjust it a bit for this reply:

threads and max_threads being protected by the same lock means that they are in a certain relationship to each other. Meaning that they only really make sense looking at them atomically.

So I argue it does not make sense to expose those values to userspace through separate files.

For instance:

	$ cat max_threads && cat threads
	$ 5
	$ 10

This way you may read 5 max_threads, but 10 actual threads, because things may have changed in between the two cat commands.

However, if instead, they are exposed through a single file, we get an atomic view of them, such that the semantic relationship between them is preserved.

For instance:

	$ cat process_info
	$ threads: 2
	$ max_threads: 10

So, what I'm trying to say is, I think it's a good thing if fields that are protected by the same lock can't be exposed through separate files, because it means that the fields only make sense in the context of each other.

Or saying it the other way around, if it makes sense to expose fields through separate files, it means they're unrelated to each other and hence should be protected with separate locks, rather than a common one.

IMHO it's even a good thing beyond the scope of debugfs, because it forces developers to really think about organizing structures properly, e.g. in a way that only fields that really belong behind a certain lock are placed behind this lock.

Quoted 4 lines

So how about a platform driver that exposes values read from a platform device (i.e. a soc info driver), that also includes a local-to-the-device data structure that can be locked and modified? That should cover all the use cases that I can think of at the moment.

Yes, I also really like to have that.

But, again, both approaches can do this. It's just that I really discourage the one that forces us to have an Arc instance on structures exposed through debugfs, since this messes with the driver's lifetime and ownership architecture in a bad way.

Quoted 110 lines · Danilo Krummrich

Quoted 4 lines · Greg Kroah-Hartman

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly. We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

Well, that's possible with both approaches. Data behind a lock becomes mutable once you grabbed the lock. That's the same in both cases.

The difference is that with the pin-init approach I propose you can't have what Alice sketched up. And I think it's even desirable that you can't do it.

Let's me explain the difference on a simplified example, based on Alice' example.

ForeignOwnable API
------------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: SpinLock<ProcessInner>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here we have to create an Arc<Process> (let's call it process) and create files from it.

	let file_threads = dir.create_file("threads", process);
	let file_max_threads = dir.create_file("max_threads", process);

In the file system callback of both of these, we now have an Arc<Process>, hence we can access:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

and in the other file:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

Pin-Init API
------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: File<SpinLock<ProcessInner>>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here Process does not need to be within an Arc and no separate file instances need to be kept around, that happens already within the constructor of Process:

	pin_init!(Process {
	   inner <- dir.create_file("process_inner", ...),
	   [...]
	})

The file itself has a reference to SpinLock<ProcessInner>, hence we can access:

	let guard = inner.lock();

	read_or_write(guard.threads)
	read_or_write(guard.max_threads)

The difference is that with the ForeignOwnable API it was possible to have separate files for threads and max_threads.

While with the pin-init one we either have to have a single file exposing ProcessInner (which is what I did above) or protect threads and max_threads with separate locks (of course max_threads could also just be an atomic).

(If you like I can sketch up this case as well.)

At a first glance this seems like an undesirable limitation, but I argue that this is a good thing.

The reason I think so is what I also explained in [1], but let me adjust it a bit for this reply:

threads and max_threads being protected by the same lock means that they are in a certain relationship to each other. Meaning that they only really make sense looking at them atomically.

So I argue it does not make sense to expose those values to userspace through separate files.

For instance:

	$ cat max_threads && cat threads
	$ 5
	$ 10

This way you may read 5 max_threads, but 10 actual threads, because things may have changed in between the two cat commands.

Quoted 8 lines

However, if instead, they are exposed through a single file, we get an atomic view of them, such that the semantic relationship between them is preserved.

For instance:

	$ cat process_info
	$ threads: 2
	$ max_threads: 10

I think you mean to write:

(quoted diff) +0 -0

 	$ cat process_info
 	threads: 2
 	max_threads: 10

right?

So, what I'm trying to say is, I think it's a good thing if fields that are protected by the same lock can't be exposed through separate files, because it means that the fields only make sense in the context of each other.

Quoted 3 lines · Danilo Krummrich

So, what I'm trying to say is, I think it's a good thing if fields that are protected by the same lock can't be exposed through separate files, because it means that the fields only make sense in the context of each other.

I think that even the pin-init API can have multiple files for different elements of the locked structure, you just need to nest File:

    #[pin_data]
    struct Process {
        task: ARef<Task>,
        #[pin]
        inner: File<File<SpinLock<ProcessInner>>>,
    }
    
    pub(crate) struct ProcessInner {
        threads: RBTree<i32, Arc<Thread>>,
        max_threads: u32,
    }

Now you can do:

    pin_init!(Process {
        inner <- dir.create_file(
            "threads",
            dir.create_file("max_threads", new_spinlock!(...)),
        ),
        // ...
    })

But I'd say this will at least raise eyebrows for the reviewers, which is good, since it catches the footgun.

Quoted 4 lines · Greg Kroah-Hartman

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly. We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

Well, that's possible with both approaches. Data behind a lock becomes mutable once you grabbed the lock. That's the same in both cases.

The difference is that with the pin-init approach I propose you can't have what Alice sketched up. And I think it's even desirable that you can't do it.

Let's me explain the difference on a simplified example, based on Alice' example.

ForeignOwnable API
------------------

	#[pin_data]
	struct Process {
	    task: ARef<Task>,
	    #[pin]
	    inner: SpinLock<ProcessInner>,
	}
	
	pub(crate) struct ProcessInner {
	    threads: RBTree<i32, Arc<Thread>>,
	    max_threads: u32,
	}

Here we have to create an Arc<Process> (let's call it process) and create files from it.

	let file_threads = dir.create_file("threads", process);
	let file_max_threads = dir.create_file("max_threads", process);

In the file system callback of both of these, we now have an Arc<Process>, hence we can access:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

and in the other file:

	let guard = process.inner.lock();

	read_or_write(guard.max_threads);

Where do you actually specify this callback? At the moment, the API asks for Display and thus it can only read values?

Quoted 7 lines · Alice Ryhl

Quoted 2 lines · Benno Lossin

How would your example look like with the current approach? IIUC, it also wouldn't work, because the debugfs data can't be mutated?

I would store a bunch of File<Arc<Process>> instances somewhere. Each one has a closure that takes the spinlock and prints the appropriate value.

Ok, I think we need to see some "real" examples here of the api in use before figuring it out further as I'm totally confused :)

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly.

We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

So how about a platform driver that exposes values read from a platform device (i.e. a soc info driver), that also includes a local-to-the-device data structure that can be locked and modified? That should cover all the use cases that I can think of at the moment.

thanks,

greg k-h

Quoted 8 lines · Greg Kroah-Hartman

Quoted 7 lines · Alice Ryhl

Quoted 2 lines · Benno Lossin

How would your example look like with the current approach? IIUC, it also wouldn't work, because the debugfs data can't be mutated?

I would store a bunch of File<Arc<Process>> instances somewhere. Each one has a closure that takes the spinlock and prints the appropriate value.

But you could also do that with the pin-init design?

Quoted 2 lines

Ok, I think we need to see some "real" examples here of the api in use before figuring it out further as I'm totally confused :)

Agreed :)

Quoted 2 lines

Yes, we need to be able to have a debugfs file callback handle a mutable structure in order to lock things correctly.

To me this seems orthogonal to storing the value in-place or in a ForeignOwnable.

Quoted 2 lines

We also need to have it be mutable so that it can MODIFY the value (everyone seems to forget that debugfs allows that...)

Well that changes things a lot IMO... How does the C side usually handle synchronization here? Does the driver decide that the structure exposed to debugfs is locked with eg a spinlock and then in the debugfs callback they just lock the same one?

Quoted 87 lines · Danilo Krummrich

Quoted 18 lines · Greg Kroah-Hartman

Quoted 17 lines · Danilo Krummrich

Instead this should just be:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

and then I would initialize it the following way:

	let fw = KBox::new(Firmware::new(), GFP_KERNEL)?;
	let file = dir.create_file("firmware", fw);

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

	// Access to fw is prevented by the compiler, since it has been moved
	// into file.

Quoted 6 lines

Quoted 2 lines

This is much better, since now I have the guarantee that my Firmare instance can't out-live the GPU instance.

That's better, yes, but how would multiple files for the same "structure" work here? Like a debugfs-file-per-field of a structure that we often have?

That is a very good question and I thought about this as well, because with only the current API this would require us to have more and more dynamic allocations if we want to have a more fine grained filesystem representations of structures.

The idea I have for this is to use pin-init, which we do in quite some other places as well.

I think we can add an additional API like this:

	impl Dir {
	   pub fn create_file<T>(&self, data: impl PinInit<T>) -> impl PinInit<Self> {
	      pin_init!(Self {
	         data <- data,
	         ...
	      })
	   }
	}

This allows us to do things like:

	#[pin_data]
	struct Firmware {
	   #[pin]
	   minor: debugfs::File<u32>,
	   #[pin]
	   major: debugfs::File<u32>,
	   #[pin]
	   buffer: debugfs::File<[u8]>,
	}

	impl Firmware {
	   pub fn new(&dir: debugfs::Dir, buffer: [u8]) -> impl PinInit<Self> {
	      pin_init!(Self {
	         minor <- dir.create_file("minor", 1),
	         major <- dir.create_file("major", 2),
	         buffer <- dir.create_file("buffer", buffer),
	      })
	   }
	}

	// This is the only allocation we need.
	let fw = KBox::pin_init(Firmware::new(...), GFP_KERNEL)?;

With this everything is now in a single allocation and since we're using pin-init, Dir::create_file() can safely store pointers of the corresponding data in debugfs_create_file(), since this structure is guaranteed to be pinned in memory.

Actually, we can also implement *only this*, since with this my previous example would just become this:

	struct GPU {
	   fw: debugfs::File<Firmware>,
	}

	let file = dir.create_file("firmware", Firmware::new());
	let file = KBox::pin_init(file, GFP_KERNEL)?;

	// debugfs::File<Firmware> dereferences to Firmware
	file.do_something();

Quoted 2 lines

Given that, I think we should change things to use pin-init right away for the debugfs::File API.

Does this actually work in practice for anything except immutable data? I mean, let's take Rust Binder as an example and lets say that I want to expose a directory for each Process object with some of the fields exposed. Let's just simplify Rust Binder a bit and only include some of the fields:

#[pin_data]

struct Process {
    task: ARef<Task>,
    #[pin]
    inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
    threads: RBTree<i32, Arc<Thread>>,
    nodes: RBTree<u64, DArc<Node>>,
    requested_thread_count: u32,
    max_threads: u32,
    started_thread_count: u32,
}

Rust Binder already does expose some debugging data through a file system, though it doesn't do so using debugfs. It exposes a lot of data, but among them are the pid, the number of threads and nodes, as well as the values of requested_thread_count, started_thread_count, and max_threads.

Now, we run into problem number one: pinning is not supported inside mutexes. But let's say we solved that and we could do this:

#[pin_data]
struct Process {
task: File<ARef<Task>>, // prints the pid
#[pin]
inner: SpinLock<ProcessInner>,
}

pub(crate) struct ProcessInner {
threads: File<RBTree<i32, Arc<Thread>>>, // prints the count
nodes: File<RBTree<u64, DArc<Node>>>, // prints the count
requested_thread_count: File<u32>,
max_threads: File<u32>,
started_thread_count: File<u32>,
}

However, this still doesn't work! Debugfs may get triggered at any time and need to read these fields, and there's no way for it to take the spinlock with the above design - it doesn't know where the spinlock is. For the integers I guess we could make them atomic to allow reading them in parallel with mutation, but that option is not available for the red/black trees.

What is the intended solution in this case? If the argument is that this is a rare case, then keep in mind that this is a real-world example of debugging information that we actually expose today in a real driver.

samples/rust/rust_debugfs.rs line 60

Quoted 5 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +5 -0

+        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
+        // how to print it
+        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
+            writeln!(f, "locked value: {:#010x}", *val.lock())
+        });

While cute, is this really going to be the way to describe all "custom" debugfs function callbacks? No other way to point to a function itself instead? Look at "fun" debugfs functions like qh_lines() in drivers/usb/host/ehci-dbg.c that is dumping tons of data out. Putting that inline here is going to be a bit ackward :)

So can you show an example of a "traditional" debugfs file output with multiple lines that is dealing with a dynamically allocated device that is associated with the module (not the static example you showed here), as that's going to be the real way this is used, not with static variables.

thanks,

greg k-h

Quoted 8 lines · Greg Kroah-Hartman

samples/rust/rust_debugfs.rs line 60

Quoted 5 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +5 -0

+        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
+        // how to print it
+        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
+            writeln!(f, "locked value: {:#010x}", *val.lock())
+        });

Quoted 5 lines

While cute, is this really going to be the way to describe all "custom" debugfs function callbacks? No other way to point to a function itself instead? Look at "fun" debugfs functions like qh_lines() in drivers/usb/host/ehci-dbg.c that is dumping tons of data out. Putting that inline here is going to be a bit ackward :)

Good news, function pointers are legal to pass in here as well already, I can add that usage to make it clear.

Quoted 6 lines

So can you show an example of a "traditional" debugfs file output with multiple lines that is dealing with a dynamically allocated device that is associated with the module (not the static example you showed here), as that's going to be the real way this is used, not with static variables.

Sure, do we want to:
* Finish creating the driver struct early in init, then call dynamic .create(&str) or .destroy(&str) .modify(&str) type things on it in init to show how it would work
* Actually wire up an input source to drive create/destroy/modify dynamically (e.g. I could implement a miscdevice) - if you want this one, do you have a preference on where I get my input signal from?

Quoted 26 lines · Matthew Maurer

Quoted 8 lines · Greg Kroah-Hartman

samples/rust/rust_debugfs.rs line 60

Quoted 5 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +5 -0

+        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
+        // how to print it
+        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
+            writeln!(f, "locked value: {:#010x}", *val.lock())
+        });

Quoted 5 lines

While cute, is this really going to be the way to describe all "custom" debugfs function callbacks? No other way to point to a function itself instead? Look at "fun" debugfs functions like qh_lines() in drivers/usb/host/ehci-dbg.c that is dumping tons of data out. Putting that inline here is going to be a bit ackward :)

Good news, function pointers are legal to pass in here as well already, I can add that usage to make it clear.

Quoted 6 lines

So can you show an example of a "traditional" debugfs file output with multiple lines that is dealing with a dynamically allocated device that is associated with the module (not the static example you showed here), as that's going to be the real way this is used, not with static variables.

Quoted 7 lines

Sure, do we want to:
* Finish creating the driver struct early in init, then call dynamic .create(&str) or .destroy(&str) .modify(&str) type things on it in init to show how it would work
* Actually wire up an input source to drive create/destroy/modify dynamically (e.g. I could implement a miscdevice) - if you want this one, do you have a preference on where I get my input signal from?

I think the idea was to show how it works in a real driver context, e.g. a platform driver, just like what samples/rust/rust_driver_platform.rs does. Not a miscdevice registered from a module, which is a rather rare use-case.

If you rebase on the latest driver-core-next, you can write a platform driver with an ACPI ID table, which can easily probed by passing -acpitable file=ssdt.aml to qemu, i.e. no need to mess with OF.

Quoted 36 lines · Danilo Krummrich

Quoted 26 lines · Matthew Maurer

Quoted 8 lines · Greg Kroah-Hartman

samples/rust/rust_debugfs.rs line 60

Quoted 5 lines · Matthew Maurer

samples/rust/rust_debugfs.rs new file +5 -0

+        // An `Arc<Mutex<usize>>` doesn't implement display, so let's give explicit instructions on
+        // how to print it
+        let file_2 = sub.fmt_file(c_str!("arc_backed"), my_arc.clone(), &|val, f| {
+            writeln!(f, "locked value: {:#010x}", *val.lock())
+        });

Quoted 5 lines

While cute, is this really going to be the way to describe all "custom" debugfs function callbacks? No other way to point to a function itself instead? Look at "fun" debugfs functions like qh_lines() in drivers/usb/host/ehci-dbg.c that is dumping tons of data out. Putting that inline here is going to be a bit ackward :)

Good news, function pointers are legal to pass in here as well already, I can add that usage to make it clear.

Quoted 6 lines

So can you show an example of a "traditional" debugfs file output with multiple lines that is dealing with a dynamically allocated device that is associated with the module (not the static example you showed here), as that's going to be the real way this is used, not with static variables.

Quoted 7 lines

Sure, do we want to:
* Finish creating the driver struct early in init, then call dynamic .create(&str) or .destroy(&str) .modify(&str) type things on it in init to show how it would work
* Actually wire up an input source to drive create/destroy/modify dynamically (e.g. I could implement a miscdevice) - if you want this one, do you have a preference on where I get my input signal from?

Quoted 7 lines

I think the idea was to show how it works in a real driver context, e.g. a platform driver, just like what samples/rust/rust_driver_platform.rs does. Not a miscdevice registered from a module, which is a rather rare use-case.

If you rebase on the latest driver-core-next, you can write a platform driver with an ACPI ID table, which can easily probed by passing -acpitable file=ssdt.aml to qemu, i.e. no need to mess with OF.

I'm confused as to how registering as a platform driver would result in an input source that would let me trigger the creation/destruction of DebugFS files. I need some kind of input stream to do that. Is there some input stream that's available to a platform driver that I'm missing, or are you suggesting that the input stream would effectively be the probe's id_info field? If I did that, wouldn't I still have a static arrangement of DebugFS files in my driver struct?

I could have misunderstood, but I don't think that's what Greg is asking for - I think he wants to see how at a data structure level, I can handle creating and destroying DebugFS files that correspond to some kind of object being created and destroyed, rather than just having a static list of slots in my driver struct for keeping them alive.

This series provides safe DebugFS bindings for Rust, with a sample module using them.

Signed-off-by Matthew Maurer <[email protected]>