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

Support per-device context for chrdev #604

Open
danobi opened this issue Jan 3, 2022 · 5 comments
Open

Support per-device context for chrdev #604

danobi opened this issue Jan 3, 2022 · 5 comments
Labels
• lib Related to the `rust/` library.

Comments

@danobi
Copy link

danobi commented Jan 3, 2022

Currently, miscdev::Registration::new and miscdev::Registration::new_pinned both support taking a context to be passed to the FileOpener::open trait method. We'll take it on faith it's useful (IMO it is).

chrdev does not have this support yet. I think it'd make sense to add a context param to chrdev::Registration::register (as opposed to new and new_pinned) b/c the same chrdev::Registration can be used to register multiple cdevs (as opposed to miscdev which can only register one).

@danobi
Copy link
Author

danobi commented Jan 3, 2022

I've taken a look at implementing this feature but it's currently made tricky b/c of #207. The linked PR (effectively) made chrdev::Cdev hold struct cdev * instead of struct cdev. This is a problem because we cannot upcast a pointer (struct cdev *) to chrdev::Cdev as you would be able to with the actual structure.

I've racked my brain for a bit but I can't think of any tricks from rust side to derive a context value without a container_of upcast.

I'm considering sending out a RFC to lkml about adding a void *private field to struct cdev b/c the struct is managed by a kobject and the use-after-free issue ought to be an issue for a lot of other drivers. Having an extra private field ought to solve our problem here.

@danobi
Copy link
Author

danobi commented Jan 3, 2022

I've sent out an RFC: https://lore.kernel.org/lkml/[email protected]/

@ojeda ojeda added prio: normal • lib Related to the `rust/` library. labels Jan 3, 2022
@wedsonaf
Copy link

wedsonaf commented Jan 4, 2022

Currently, miscdev::Registration::new and miscdev::Registration::new_pinned both support taking a context to be passed to the FileOpener::open trait method. We'll take it on faith it's useful (IMO it is).

Some evidence beyond faith of the usefulness of a context: the Binder code uses it (https://github.com/Rust-for-Linux/linux/blob/rust/drivers/android/process.rs#L810) to hold some state that is shared by all files that share a device. It isn't a global because we can have more than one device (each with several files), and globals should generally be avoided anyway.

@wedsonaf
Copy link

wedsonaf commented Jan 4, 2022

I've taken a look at implementing this feature but it's currently made tricky b/c of #207. The linked PR (effectively) made chrdev::Cdev hold struct cdev * instead of struct cdev. This is a problem because we cannot upcast a pointer (struct cdev *) to chrdev::Cdev as you would be able to with the actual structure.

What do you need this for?

The upstream maintainer of chrdev advised us not to expose this yet because he feels new character devices should just use miscdev.

@danobi
Copy link
Author

danobi commented Jan 5, 2022

I've taken a look at implementing this feature but it's currently made tricky b/c of #207. The linked PR (effectively) made chrdev::Cdev hold struct cdev * instead of struct cdev. This is a problem because we cannot upcast a pointer (struct cdev *) to chrdev::Cdev as you would be able to with the actual structure.

What do you need this for?

I'm rewriting some of the examples from LDD3 in rust as a fun exercise. Thought I could shake out a few bugs / missing features in rust-for-linux along the way too. LDD3 uses cdev so I thought I'd stick to the original recipe.

The upstream maintainer of chrdev advised us not to expose this yet because he feels new character devices should just use miscdev.

Oh, in that case I'll just use miscdev. Didn't know they were interchangeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
• lib Related to the `rust/` library.
Development

No branches or pull requests

3 participants