Re: [PATCH v3 1/2] rust: miscdevice: add additional data to MiscDeviceRegistration

[Christian Schrefl <chrisi.schrefl@gmail.com>]

Hi Greg,

On 21.05.25 2:04 PM, Greg Kroah-Hartman 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.
> 
> Nope, can't take it, it breaks the build from my tree:
> 
> error[E0432]: unresolved import `crate::types::UnsafePinned`
>   --> rust/kernel/miscdevice.rs:20:37
>    |
> 20 |     types::{ForeignOwnable, Opaque, UnsafePinned},
>    |                                     ^^^^^^^^^^^^ no `UnsafePinned` in `types`
> 
> error[E0432]: unresolved import `pin_init::Wrapper`
>   --> rust/kernel/miscdevice.rs:23:5
>    |
> 23 | use pin_init::Wrapper;
>    |     ^^^^^^^^^^^^^^^^^ no `Wrapper` in the root
> 
> error: aborting due to 2 previous errors
> 
> :(

This requires my `UnsafePinned` [0] patches (& with that pin-init-next) like I wrote
in the cover letter.

[0]: https://lore.kernel.org/rust-for-linux/20250511-rust_unsafe_pinned-v4-0-a86c32e47e3d@gmail.com/

Cheers
Christian