Skip to content
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.

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

rust/kernel: chrdev: remove unnecessary pinning #304

Closed

Conversation

TheSven73
Copy link
Collaborator

chrdev's Registration and RegistrationInner do not contain any
self-referential or borrowed-with-kernel structures. Therefore they
do not require pinning.

Remove all pinning. This simplifies the code greatly, and eliminates
an unsafe block, and a fallible Box allocation.

As part of this commit, also update chrdev client drivers.

Signed-off-by: Sven Van Asbroeck [email protected]

`chrdev`'s `Registration` and `RegistrationInner` do not contain any
self-referential or borrowed-with-kernel structures. Therefore they
do not require pinning.

Remove all pinning. This simplifies the code greatly, and eliminates
an unsafe block, and a fallible `Box` allocation.

As part of this commit, also update `chrdev` client drivers.

Signed-off-by: Sven Van Asbroeck <[email protected]>
@ksquirrel
Copy link
Member

Review of aae24e980c83:

  • ✔️ Commit aae24e9: Looks fine!

@nbdd0121
Copy link
Member

I think we should keep the pinning, and use cdev_init instead of cdev_alloc. This removes an indirection and an allocation.

@TheSven73
Copy link
Collaborator Author

I'm in the process of learning about pinning. Can folks confirm that the pinning is indeed redundant here? I'm not 100% sure myself.

If so, then pinning can be removed. Next, the question arises whether it should be removed. That's a different question altogether :)

@nbdd0121
Copy link
Member

Pinning essentially means that a stable address is wanted and the object cannot be freely moved. So miscdev requires pinning because it contains a bindings::miscdevice. chrdev does not currently requires pinning, because it only contains pointers to bindings::cdev. So yes, it can be removed.

But I think we probably want to remove this indirection, and let RegistrationInner contains bindings::cdev instead. If we do that, then pinning would be required. So I think we probably shouldn't remove pinning for now.

@TheSven73
Copy link
Collaborator Author

Very simple to do - just revert #207

@nbdd0121
Copy link
Member

Ah, sorry, I am not aware of the PR.

We could make RegistrationInner itself a kobject and the parent of cdev it contains though? I am just pointing out reasons that we might want it pinned, not that we should or should not make the change.

@TheSven73
Copy link
Collaborator Author

@ojeda How can I assign a "Prio: low" label to this PR?

@ojeda
Copy link
Member

ojeda commented Jun 10, 2021

Let me give you Triage permissions :)

@TheSven73
Copy link
Collaborator Author

No consensus that this is worthwhile.

@TheSven73 TheSven73 closed this Jul 2, 2021
@TheSven73 TheSven73 deleted the rust-for-linux-remove-pin branch July 8, 2021 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants