Message-ID: <7a6852e9-54f7-4fc1-b052-f673f9eddd97@gmail.com>
Date: Wed, 21 May 2025 16:41:42 +0200
From: Christian Schrefl <chrisi.schrefl@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>, Arnd Bergmann <arnd@...db.de>,
 Lee Jones <lee@...nel.org>, Daniel Almeida <daniel.almeida@...labora.com>,
 Gerald Wisböck <gerald.wisboeck@...ther.ink>,
 rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/2] rust: miscdevice: add additional data to
 MiscDeviceRegistration

Hi Danilo

On 21.05.25 4:12 PM, Danilo Krummrich wrote:
> On Wed, May 21, 2025 at 01:55:36PM +0200, Greg Kroah-Hartman wrote:
>> On Sat, May 17, 2025 at 03:42:29PM +0200, Danilo Krummrich wrote:
>>> On Sat, May 17, 2025 at 01:33:49PM +0200, Christian Schrefl wrote:
>>>> +pub struct MiscDeviceRegistration<T: MiscDevice> {
>>>>      #[pin]
>>>>      inner: Opaque<bindings::miscdevice>,
>>>> +    #[pin]
>>>> +    data: UnsafePinned<T::RegistrationData>,
>>>>      _t: PhantomData<T>,
>>>>  }
>>>
>>> I recommend not to store data within a Registration type itself.
>>>
>>> I know that this is designed with the focus on using misc device directly from
>>> the module scope; and in this context it works great.
>>>
>>> However, it becomes quite suboptimal when used from a driver scope. For
>>> instance, if the misc device is registered within a platform driver's probe()
>>> function.
>>>
>>> I know this probably isn't supported yet. At least, I assume it isn't supported
>>> "officially", given that the abstraction does not provide an option to set a
>>> parent device. Yet I think we should consider it.
>>
>> It's going to be a requirement to properly set the parent device, and
>> as you point out, this really should be in some sort of scope, not just
>> a module.
>>
>> But, we have two types of users of a misc device, one like this is
>> written, for a module-scope, and one for the "normal" device scope.  The
>> device scope is going to be tricker as it can, and will, get
>> disconnected from the device separately from the misc device lifespan,
>> so when that logic is added, it's going to be tricky as you point out.
>>
>> So I'll take this now, but in the future this is going to have to be
>> cleaned up and modified.
> 
> I'm about to sketch up something based on this patch that works properly for
> both cases, i.e. module-scope and driver-scope.

Let me know if you have any questions for me (if you want also privately 
or on Zulip).

I currently don't have the time to work on this myself. I'm not that
familiar with the C side and the Devres abstraction and would need to look
into these before getting much work done here.

> I think it would also be good for the misc device abstraction to demonstrate
> how to properly make class device abstractions (such as misc device, DRM device,
> input device etc.) go along with bus devices in the context of a driver.
> 
> misc device isn't *the* perfect example, given that it doesn't have the typical
> create and register split and another complication is that we also have to deal
> with the module-scope case, but it's still a very good candidate given that it
> is very simple compared to other class devices.

Cheers
Christian

Powered by blists - more mailing lists