Re: [PATCH v2 1/2] rust: introduce abstractions for fwctl

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

On Wed Jan 28, 2026 at 1:04 AM CET, Jason Gunthorpe wrote:
> On Tue, Jan 27, 2026 at 09:07:37PM +0100, Danilo Krummrich wrote:
>> On Tue Jan 27, 2026 at 8:57 PM CET, Zhi Wang wrote:
>> > The fwctl_alloc_device() helper allocates a raw struct fwctl_device
>> > without private driver data here. The Rust driver object should be
>> > already allocated and initialized separately before reaching this
>> > point.
>> >
>> > We rely on the standard dev->parent chain to access the rust driver
>> > object from the fwctl callbacks.
>> 
>> (I will go for a thorough review soon, but for now a quick drive-by comment.)
>> 
>> IIUC, you are saying that the user is supposed to use the private data of the
>> parent device in fwctl callbacks. Let's not make this a design choice please.
>> Instead, allow the user pass in separate private data for the fwctl device as
>> well.
>> 
>> This serves the purpose of clear ownership and lifetime of the data. E.g. the
>> fwctl device does not necessarily exist as long as the parent device is bound.
>> 
>> It is a good thing if driver authors are forced to take a decision about which
>> object owns the data and what's the scope of the data.

I think we were talking about different things. :)

Zhi mentioned the private data of the parent device being used in fwctl
callbacks, but we should use private data stored in fwctl_device::dev instead.

> I'm not sure what the model is in rust, but the expection here is for
> the driver to have a window between alloc and register where the
> driver can fill in any information into the core structures that is
> needed before calling registration.

Yes, in Rust you can't have an instance of a structure without initializing it
(unless you put it in a MaybeUninit container, etc.).

For dynamic allocations Rust solves this with initializers. For instance, where
C does:

	struct foo *foo = kzalloc(sizeof(*foo), GFP_KERNEL);

	/* Maybe broken if foo is used here already. */

	foo_initialize(foo, ...);

you'd have something like this in Rust:

	struct Foo {
	    data: Mutex<Data>,
	    index: u32,
	}

	impl Foo {
	    fn new(index: u32, args: Args) -> impl PinInit<Self, Error> {
	        try_pin_init!(Self {
	            data <- new_mutex!(args.to_data()?),
	            index.
	        })
	    }
	}

The returned object is not an instance of struct Foo yet, it is an initializer,
i.e. impl PinInit<Foo, Error>; no memory has been allocated yet. Note that this
initializer can also fail if executed, since args.to_data() can fail.

Let's allocate the memory now:

	let foo = KBox::pin_init(Foo::new(...), GFP_KERNEL)?;

This allocates the memory (i.e. KBox::pin_init() calls kmalloc() eventually) and
executes the initializer. (The call fails if either the memory allocation fails,
or the initializer fails.) This way there is no way for the caller to use foo
before it has been initialized.

(What about the "pin" thing? Let's assume Data contains something
self-referential, in this case pin-init does prevent the user from moving data
out of Foo, which would be a bug since it is a self-referential structure.
Actually, the mutex must not be moved either.)

Back to this abstraction. :)

In this case Registration::new() returns an initializer, but also allocates the
C struct fwctl_device within the initializer.

AFAICS, _fwctl_alloc_device() already initializes the structure properly, so it
seems there is nothing to be done. But let's assume there is. In this case it
wouldn't help much if we would create a separate fwctl::Device structure before,
as this one also has to be properly initialized once created. So the constructor
of fwctl::Device has to take the corresponding arguments.

Though, sometimes there are cases where we have to defer some initialization.
This is where we usually use separate types or type states. Let's assume
something in the device only ever gets initialized after registration for some
reason. In this case you could have a fwctl::Device<Unregistred> and a
fwctl::Device<Registered> and correspondingly treat the inner data as partially
uninitialized (which requires unsafe code).

Either way, I think it would be cleaner if fwctl::Device has a constructor

	impl Device<T> {
	    fn new(
	        parent: &Device<Bound>,
	        data: impl PinInit<T, Error>
	    ) -> Result<ARef<Self>>;
	}

where T is the driver's private data for the struct fwctl_device.

And Registration::new() can take a &fwctl::Device<T> and the parent
&Device<Bound> of course. This would also be in line with what we do in other
class device abstractions.