[wedsonaf]

No description provided.

Can you describe the design decisions you went for here in a bit more detail? (e.g. exposing Pin in the public API)

Can you describe the design decisions you went for here in a bit more detail? (e.g. exposing Pin in the public API)

Yes, absolutely.

When misc_register succeeds, the miscdevice that was passed in is added to an intrusive linked list. This means that it is not safe to move the miscdevice we used when calling misc_register, otherwise we will create dangling pointers (in the list managed by the kernel).

I can see at least two options:

1. Always wrap a misc device registration in a Box (i.e., it is implicitly pinned).
2. Explicitly indicate that a registration must be pinned.

Option 1 has the drawbacks that it requires an extra allocation per registration, and the user may not be aware that they cannot move it.
Option 2 has the drawback that it has a more complicated API.

I chose option 2 because it makes it clear to the user that they are not supposed to move it. Also, when we create devices, we usually create additional state beyond the registration. Option 2 allows for zero-cost abstraction in this case, i.e., we can allocate a single struct containing the registration plus any additional fields we need. Option 1 would require us to allocate the registration separately and store a pointer to it in the struct holding additional state for the device.

Additionally, option 2 does not preclude option 1: we can always have an API that wraps a registration in a box and returns that if the caller does not need zero-cost.

One aspect of this implementation that I'm not satisfied with is the presence of the "registered" field. At the moment I have it because I need to ensure that we don't try to unregister a registration that wasn't ever registered or whose registration attempt failed. I tried using MaybeUninit (which would prevent drop from being called on a registration that isn't completed yet), but I couldn't find a good combination of MaybUninit and Pin that gave me the behaviour that I needed.

If you all have suggestions for improving this, I'm all ears.

Thanks!

Is there a reason not to have Registration::new() do the pin? is there ever a use case for a non-pinned value?

We cannot pin in new because we do not have a location that we know can be pinned.

We could create one, which is option 2 I mention in the other message, but it entails an extra allocation (i.e., makes the abstraction not zero-cost).

Another option is to pass an uninitialised pinned Registration, which is then initialised in new(). But I couldn't find a clean way to do this when the registration is embedded in another struct because the documentation says that we cannot initialise individual fields of structs using MaybeUninit.

Sorry, I'm clearly thinking slowly, but the implementation right now already involves a heap alloc, is there a way to use this API that doesn't?

Sure, we can allocate the registration on the stack, pin it, register, use the device, sleep/wait, then return (the implicit drop would unregister before the memory is freed). The pin_mut macro allows us to do this without any unsafe code.

But this isn't the most common usage. The most common usage would be something like this:

struct DeviceContext {
     x: SomeType,
     y: SomeType,
     z: SomeType,
     reg: miscdev::Registration,
}

This needs a single allocation. The user would allocate a single block of memory for all fields in the struct then pin the whole thing. To call Registration::register, one would use pinned project (there are crates to do this without any unsafe code, for example this).

If we were to allocate inside Registration::new, the example DeviceContext above would require two allocations instead.

Ok, I'm following now. Can we do Registration::new_boxed() and Registration::new() then, to let folks who don't mind the extra alloc have a convenient API.

Yes, I'll add new_boxed.

I actually added new_pinned -- since the constructor will have a pinned memory location, I also call register from the it so the returned Registration is ready in one shot on success.

Sorry, took me a few read throughts, but now I get everything that's going on here and I've got an actual review.

Thanks for being patient with me!

Ok, I'm following now. Can we do Registration::new_boxed() and Registration::new() then, to let folks who don't mind the extra alloc have a convenient API.

A few trivial comments from a very quick read.

@alex @ojeda Thanks for all the comments! I believe I've addressed them. I'd appreciate any additional feedback when you have the chance.

@alex @ojeda Thanks for all the comments! I believe I've addressed them. I'd appreciate any additional feedback when you have the chance.

Thanks to you for joining the effort!