Message-Id: <DG09ZUY530SW.K62KDIT4LIXN@kernel.org>
Date: Wed, 28 Jan 2026 15:01:25 +0100
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Jason Gunthorpe" <jgg@...dia.com>
Cc: "Zhi Wang" <zhiw@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <aliceryhl@...gle.com>, <bhelgaas@...gle.com>, <kwilczynski@...nel.org>,
 <ojeda@...nel.org>, <alex.gaynor@...il.com>, <boqun.feng@...il.com>,
 <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>,
 <a.hindborg@...nel.org>, <tmgross@...ch.edu>, <markus.probst@...teo.de>,
 <helgaas@...nel.org>, <cjia@...dia.com>, <smitra@...dia.com>,
 <ankita@...dia.com>, <aniketa@...dia.com>, <kwankhede@...dia.com>,
 <targupta@...dia.com>, <acourbot@...dia.com>, <joelagnelf@...dia.com>,
 <jhubbard@...dia.com>, <zhiwang@...nel.org>, <daniel.almeida@...labora.com>
Subject: Re: [PATCH v2 1/2] rust: introduce abstractions for fwctlg

On Wed Jan 28, 2026 at 2:20 PM CET, Jason Gunthorpe wrote:
> On Wed, Jan 28, 2026 at 02:21:38AM +0100, Danilo Krummrich wrote:
>> 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. :)
>
> Well, I've always been talking about this :)

Fair enough. :)

>> In this case Registration::new() returns an initializer, but also allocates the
>> C struct fwctl_device within the initializer.
>
> In a normal C implementation this would allocate both the core and
> driver struct using one memory and a container_of() relationship.

That's what happens here as well, see below.

>> AFAICS, _fwctl_alloc_device() already initializes the structure properly, so it
>> seems there is nothing to be done. 
>
> It does the first part of a three step sequence
>
> 1) Allocate memory and initialize core code
> 2) Steup driver related data
> 3) "register" to make the device live and begin concurrent access
>
> I don't think 1 and 3 can be in the same function. The driver must have
> the opportunity to do its #2 step in this sequence.

Yes, the driver has this opportunity, again see below.

>> 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.
>
> If it goes like this is there some way rust can retain the
> container_of layout and avoid a second memory allocation?

There is no second memory allocation. In the implementation of
fwctl::Device::new() above we call _fwctl_alloc_device() with a size (and
layout) such that this allocation is suitable to initialize the second argument
(i.e. data: impl PinInit<T, Error>) within this allocation.

This is the equivalent to the subclassing pattern as you would implement it in C
with container_of(). See also the drm::Device implementation [1], which does the
exact same thing.

When it comes to the Registration object (which returns an initializer as well),
this would go into the initializer (initializer can be nested) of the driver's
device private data of the parent device, i.e. no separate allocation, it just
lives in the driver's device private data of the parent device.

Alternatively, we can also just not return a Registration object at all and
leave it up to devres to entirely, i.e. equivalent to a
devm_fwctl_register_device() function. The Registration object is only needed if
we intend to unregister the fwctl device before the parent device unbind.

[1] https://rust.docs.kernel.org/src/kernel/drm/device.rs.html#98

Powered by blists - more mailing lists