Skip to content

Make miscdevice a direct field so that we can use offset_of. #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Make miscdevice a direct field so that we can use offset_of. #160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 18 additions & 22 deletions rust/kernel/miscdev.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ use core::pin::Pin;

/// A registration of a miscellaneous device.
pub struct Registration {
mdev: Option<bindings::miscdevice>,
registered: bool,
mdev: bindings::miscdevice,
_pin: PhantomPinned,
}

Expand All @@ -25,7 +26,8 @@ impl Registration {
/// It is allowed to move.
pub fn new() -> Self {
Self {
mdev: None,
registered: false,
mdev: bindings::miscdevice::default(),
_pin: PhantomPinned,
}
}
Expand All @@ -44,31 +46,29 @@ impl Registration {

/// Registers a miscellaneous device with the rest of the kernel.
///
/// It must be pinned because the memory block that represents the
/// registration is self-referential. If a minor is not given, the kernel
/// allocates a new one if possible.
/// It must be pinned because the memory block that represents the registration is
/// self-referential. If a minor is not given, the kernel allocates a new one if possible.
Comment on lines -47 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an unrelated change (and makes the comment longer than 80).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we not using 100 as the limit now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.rustfmt.toml is still the default, so it is still 80 for comments and 100 for code, but I am OK with changing it.

Don't worry about it too much, I plan to take a look into this and cleanup everything in one go (and enforce whatever we decide) -- I mentioned it here only because it looked like an spurious change (and makes it harder to look at the diff).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether the decision is to use 80 or 100, I think we should use it both for code and comments. It just looks silly to have code up to 100 and comments squashed at 80 on the left.

Anyway, do you really need me to re-spin this without changes to comments?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it is a bit weird, but it is the default. I guess the rationale is that longer comments are harder to read, but I will take a look around and find out what is usually done in most projects.

do you really need me to re-spin this without changes to comments?

If it is not trivial to do (e.g. because you will need to rebase other PRs too), then ignore it. However, in general, please try to not put unrelated changes in a commit (I am saying this for the future when we actually follow kernel's customs :-).

pub fn register<T: FileOperations>(
self: Pin<&mut Self>,
name: CStr<'static>,
minor: Option<i32>,
) -> KernelResult {
// SAFETY: We must ensure that we never move out of `this`.
let this = unsafe { self.get_unchecked_mut() };
if this.mdev.is_some() {
if this.registered {
// Already registered.
return Err(Error::EINVAL);
}

this.mdev = Some(bindings::miscdevice::default());
let dev = this.mdev.as_mut().unwrap();
dev.fops = FileOperationsVtable::<T>::build();
dev.name = name.as_ptr() as *const c_types::c_char;
dev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32);
let ret = unsafe { bindings::misc_register(dev) };
this.mdev.fops = FileOperationsVtable::<T>::build();
this.mdev.name = name.as_ptr() as *const c_types::c_char;
this.mdev.minor = minor.unwrap_or(bindings::MISC_DYNAMIC_MINOR as i32);

let ret = unsafe { bindings::misc_register(&mut this.mdev) };
if ret < 0 {
this.mdev = None;
return Err(Error::from_kernel_errno(ret));
}
this.registered = true;
Ok(())
}
}
Expand All @@ -79,19 +79,15 @@ impl Default for Registration {
}
}

// SAFETY: The only method is `register()`, which requires a (pinned) mutable
// `Registration`, so it is safe to pass `&Registration` to multiple threads
// because it offers no interior mutability.
// SAFETY: The only method is `register()`, which requires a (pinned) mutable `Registration`, so it
// is safe to pass `&Registration` to multiple threads because it offers no interior mutability.
Comment on lines -82 to +83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

unsafe impl Sync for Registration {}

impl Drop for Registration {
/// Removes the registration from the kernel if it has completed
/// successfully before.
/// Removes the registration from the kernel if it has completed successfully before.
Comment on lines -88 to +87
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

fn drop(&mut self) {
if let Some(ref mut dev) = self.mdev {
unsafe {
bindings::misc_deregister(dev);
}
if self.registered {
unsafe { bindings::misc_deregister(&mut self.mdev) }
}
}
}