.../userspace-api/ioctl/ioctl-number.rst | 1 + MAINTAINERS | 1 + samples/rust/Kconfig | 10 ++ samples/rust/Makefile | 1 + samples/rust/rust_misc_device.rs | 132 ++++++++++++++++++ 5 files changed, 145 insertions(+) create mode 100644 samples/rust/rust_misc_device.rs
It has been suggested that the driver should use dev_info() instead of
pr_info() however there is currently no scaffolding to successfully pull
a 'struct device' out from driver data post register(). This is being
worked on and we will convert this over in due course.
Lee Jones (4):
Documentation: ioctl-number: Carve out some identifiers for use by
sample drivers
samples: rust: Provide example using the new Rust MiscDevice
abstraction
sample: rust_misc_device: Demonstrate additional get/set value
functionality
MAINTAINERS: Add Rust Misc Sample to MISC entry
.../userspace-api/ioctl/ioctl-number.rst | 1 +
MAINTAINERS | 1 +
samples/rust/Kconfig | 10 ++
samples/rust/Makefile | 1 +
samples/rust/rust_misc_device.rs | 132 ++++++++++++++++++
5 files changed, 145 insertions(+)
create mode 100644 samples/rust/rust_misc_device.rs
--
2.47.0.338.g60cca15819-goog
On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > It has been suggested that the driver should use dev_info() instead of > pr_info() however there is currently no scaffolding to successfully pull > a 'struct device' out from driver data post register(). This is being > worked on and we will convert this over in due course. I think you're going too fast with this series. Please address the comments you receive before sending out new versions. Also, please document the changes you have made from one version to the next, otherwise it's gonna be very hard to review this. Thanks, Danilo > > Lee Jones (4): > Documentation: ioctl-number: Carve out some identifiers for use by > sample drivers > samples: rust: Provide example using the new Rust MiscDevice > abstraction > sample: rust_misc_device: Demonstrate additional get/set value > functionality > MAINTAINERS: Add Rust Misc Sample to MISC entry > > .../userspace-api/ioctl/ioctl-number.rst | 1 + > MAINTAINERS | 1 + > samples/rust/Kconfig | 10 ++ > samples/rust/Makefile | 1 + > samples/rust/rust_misc_device.rs | 132 ++++++++++++++++++ > 5 files changed, 145 insertions(+) > create mode 100644 samples/rust/rust_misc_device.rs > > -- > 2.47.0.338.g60cca15819-goog > >
On Fri, 06 Dec 2024, Danilo Krummrich wrote: > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > > It has been suggested that the driver should use dev_info() instead of > > pr_info() however there is currently no scaffolding to successfully pull > > a 'struct device' out from driver data post register(). This is being > > worked on and we will convert this over in due course. > > I think you're going too fast with this series. > > Please address the comments you receive before sending out new versions. > > Also, please document the changes you have made from one version to the next, > otherwise it's gonna be very hard to review this. I can add a change log. What comments were missed? -- Lee Jones [李琼斯]
On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote: > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > > > It has been suggested that the driver should use dev_info() instead of > > > pr_info() however there is currently no scaffolding to successfully pull > > > a 'struct device' out from driver data post register(). This is being > > > worked on and we will convert this over in due course. > > > > I think you're going too fast with this series. > > > > Please address the comments you receive before sending out new versions. > > > > Also, please document the changes you have made from one version to the next, > > otherwise it's gonna be very hard to review this. > > I can add a change log. > > What comments were missed? I think MiscDevice should ideally use the generic `Registration` type from [1]. I see that you use `InPlaceModule` now, which is fine. But since this is just a sample, we could probably afford to wait until the generic type lands. Also, there was a comment about how we can make use of the `dev_*` macros. I really think we should fix those before we land a sample driver. It's gonna be hard to explain people later on that they shouldn't do what the example does... [1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/ > > -- > Lee Jones [李琼斯]
On Fri, 06 Dec 2024, Danilo Krummrich wrote: > On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote: > > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > > > > It has been suggested that the driver should use dev_info() instead of > > > > pr_info() however there is currently no scaffolding to successfully pull > > > > a 'struct device' out from driver data post register(). This is being > > > > worked on and we will convert this over in due course. > > > > > > I think you're going too fast with this series. > > > > > > Please address the comments you receive before sending out new versions. > > > > > > Also, please document the changes you have made from one version to the next, > > > otherwise it's gonna be very hard to review this. > > > > I can add a change log. > > > > What comments were missed? > > I think MiscDevice should ideally use the generic `Registration` type from [1]. How can an in-tree driver use out-of-tree functionality? > I see that you use `InPlaceModule` now, which is fine. But since this is just a > sample, we could probably afford to wait until the generic type lands. > > Also, there was a comment about how we can make use of the `dev_*` macros. > > I really think we should fix those before we land a sample driver. It's gonna > be hard to explain people later on that they shouldn't do what the example > does... We're authoring the sample based on what is available at the moment. There will always be something better / more convenient coming down the pipe. We don't usually put off contributors pending acceptance of out-of-tree functionality, sample or otherwise. As I've already mentioned, I'd be _more than_ happy to keep improving this over time as new and improved helpers / infra. arrives. > [1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/ -- Lee Jones [李琼斯]
On Fri, Dec 06, 2024 at 01:14:45PM +0000, Lee Jones wrote: > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote: > > > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > > > > > It has been suggested that the driver should use dev_info() instead of > > > > > pr_info() however there is currently no scaffolding to successfully pull > > > > > a 'struct device' out from driver data post register(). This is being > > > > > worked on and we will convert this over in due course. > > > > > > > > I think you're going too fast with this series. > > > > > > > > Please address the comments you receive before sending out new versions. > > > > > > > > Also, please document the changes you have made from one version to the next, > > > > otherwise it's gonna be very hard to review this. > > > > > > I can add a change log. > > > > > > What comments were missed? > > > > I think MiscDevice should ideally use the generic `Registration` type from [1]. > > How can an in-tree driver use out-of-tree functionality? AFAICT, this sample module is in the exact same stage as the generic device / driver infrastructure. Both are on the mailing list in discussion for inclusion into the kernel. Labeling one as in-tree and the other one as out-of-tree is clearly misleading. I'm just saying it would be good to align this. If the sample driver is time critical, I have no problem if you go ahead with the current `MiscDeviceRegistration` and `InPlaceModule`, but again, why not align it from the get-go? > > > I see that you use `InPlaceModule` now, which is fine. But since this is just a > > sample, we could probably afford to wait until the generic type lands. > > > > Also, there was a comment about how we can make use of the `dev_*` macros. > > > > I really think we should fix those before we land a sample driver. It's gonna > > be hard to explain people later on that they shouldn't do what the example > > does... > > We're authoring the sample based on what is available at the moment. Well, for this I have to disagree, not being able to use the `dev_*` macros is simply meaning that the abstraction is incomplete (in this aspect). I don't see the need to land a sample driver that tells the user to do the wrong thing, i.e. use the `pr_*` macros. As Alice mentioned, you can get the miscdevice pointer from the file private data in open() and then make it accessible in the other fops hooks. If we go for this solution it will change the callbacks of `MiscDevice` and maybe even some other architectural aspects. This needs to be addressed first. > > There will always be something better / more convenient coming down the > pipe. We don't usually put off contributors pending acceptance of > out-of-tree functionality, sample or otherwise. No one asks for this here. But if the example reveals shortcomings, we shouldn't promote them as example. > > As I've already mentioned, I'd be _more than_ happy to keep improving > this over time as new and improved helpers / infra. arrives. > > > [1] https://lore.kernel.org/rust-for-linux/20241205141533.111830-3-dakr@kernel.org/ > > -- > Lee Jones [李琼斯]
On Fri, 06 Dec 2024, Danilo Krummrich wrote: > On Fri, Dec 06, 2024 at 01:14:45PM +0000, Lee Jones wrote: > > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > > On Fri, Dec 06, 2024 at 12:54:30PM +0000, Lee Jones wrote: > > > > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > > > > > > On Fri, Dec 06, 2024 at 12:42:11PM +0000, Lee Jones wrote: > > > > > > It has been suggested that the driver should use dev_info() instead of > > > > > > pr_info() however there is currently no scaffolding to successfully pull > > > > > > a 'struct device' out from driver data post register(). This is being > > > > > > worked on and we will convert this over in due course. > > > > > > > > > > I think you're going too fast with this series. > > > > > > > > > > Please address the comments you receive before sending out new versions. > > > > > > > > > > Also, please document the changes you have made from one version to the next, > > > > > otherwise it's gonna be very hard to review this. > > > > > > > > I can add a change log. > > > > > > > > What comments were missed? > > > > > > I think MiscDevice should ideally use the generic `Registration` type from [1]. > > > > How can an in-tree driver use out-of-tree functionality? > > AFAICT, this sample module is in the exact same stage as the generic device / driver > infrastructure. > > Both are on the mailing list in discussion for inclusion into the kernel. > Labeling one as in-tree and the other one as out-of-tree is clearly misleading. If I was saying that, I'd agree with you. I was asking how MiscDevice (in-tree) could use Registration (out-of-free). > I'm just saying it would be good to align this. If the sample driver is time > critical, I have no problem if you go ahead with the current > `MiscDeviceRegistration` and `InPlaceModule`, but again, why not align it from > the get-go? Because it's not available yet. :) > > > I see that you use `InPlaceModule` now, which is fine. But since this is just a > > > sample, we could probably afford to wait until the generic type lands. > > > > > > Also, there was a comment about how we can make use of the `dev_*` macros. > > > > > > I really think we should fix those before we land a sample driver. It's gonna > > > be hard to explain people later on that they shouldn't do what the example > > > does... > > > > We're authoring the sample based on what is available at the moment. > > Well, for this I have to disagree, not being able to use the `dev_*` macros is > simply meaning that the abstraction is incomplete (in this aspect). > > I don't see the need to land a sample driver that tells the user to do the wrong > thing, i.e. use the `pr_*` macros. > > As Alice mentioned, you can get the miscdevice pointer from the file private > data in open() and then make it accessible in the other fops hooks. If we go for > this solution it will change the callbacks of `MiscDevice` and maybe even some > other architectural aspects. > > This needs to be addressed first. The issue about ever growing dependencies _can_ be that authors have other priorities and are slow to turn things around, which may end up with nothing being accepted and contributors getting frustrated. However, taking into consideration how swift Alice is with these things, I'd be happy to wait for this part if people are insistent. > > There will always be something better / more convenient coming down the > > pipe. We don't usually put off contributors pending acceptance of > > out-of-tree functionality, sample or otherwise. > > No one asks for this here. But if the example reveals shortcomings, we shouldn't > promote them as example. IMHO it's reasonable for the sample to represent the current status of the frameworks in use. As advancements / adaptions are introduced we can use them to continually improve the example. -- Lee Jones [李琼斯]
On Fri, Dec 06, 2024 at 04:49:18PM +0000, Lee Jones wrote: > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > Also, there was a comment about how we can make use of the `dev_*` macros. > > > > > > > > I really think we should fix those before we land a sample driver. It's gonna > > > > be hard to explain people later on that they shouldn't do what the example > > > > does... > > > > > > We're authoring the sample based on what is available at the moment. > > > > Well, for this I have to disagree, not being able to use the `dev_*` macros is > > simply meaning that the abstraction is incomplete (in this aspect). > > > > I don't see the need to land a sample driver that tells the user to do the wrong > > thing, i.e. use the `pr_*` macros. > > > > As Alice mentioned, you can get the miscdevice pointer from the file private > > data in open() and then make it accessible in the other fops hooks. If we go for > > this solution it will change the callbacks of `MiscDevice` and maybe even some > > other architectural aspects. > > > > This needs to be addressed first. > > The issue about ever growing dependencies _can_ be that authors have > other priorities and are slow to turn things around, which may end up > with nothing being accepted and contributors getting frustrated. I would share your argumentation if 1) we'd talk about a real driver, where people are actually waiting for, 2) it'd be about a new feature, performance improvement, etc. What we have here is different: You wrote a sample implementation for a new and just landed abstraction that reveals a shortcoming. (Which is great, because it means the sample already served an important purpose.) IMHO, the consequence should not be to merge the sample as is anyways, because another purpose of the sample implementation is to tell people "look, this is exactly how it should look like, please do it the same way". Instead, we should fix the shortcoming, adjust the sample implementation and merge it then. Just to make it clear, for a real driver I think it would be reasonable to just go ahead, but for a sample that should educate, we should fix things first. > > However, taking into consideration how swift Alice is with these things, > I'd be happy to wait for this part if people are insistent. > > > > There will always be something better / more convenient coming down the > > > pipe. We don't usually put off contributors pending acceptance of > > > out-of-tree functionality, sample or otherwise. > > > > No one asks for this here. But if the example reveals shortcomings, we shouldn't > > promote them as example. > > IMHO it's reasonable for the sample to represent the current status of > the frameworks in use. As advancements / adaptions are introduced we > can use them to continually improve the example. > > -- > Lee Jones [李琼斯]
On Fri, 06 Dec 2024, Danilo Krummrich wrote: > On Fri, Dec 06, 2024 at 04:49:18PM +0000, Lee Jones wrote: > > On Fri, 06 Dec 2024, Danilo Krummrich wrote: > > > > > Also, there was a comment about how we can make use of the `dev_*` macros. > > > > > > > > > > I really think we should fix those before we land a sample driver. It's gonna > > > > > be hard to explain people later on that they shouldn't do what the example > > > > > does... > > > > > > > > We're authoring the sample based on what is available at the moment. > > > > > > Well, for this I have to disagree, not being able to use the `dev_*` macros is > > > simply meaning that the abstraction is incomplete (in this aspect). > > > > > > I don't see the need to land a sample driver that tells the user to do the wrong > > > thing, i.e. use the `pr_*` macros. > > > > > > As Alice mentioned, you can get the miscdevice pointer from the file private > > > data in open() and then make it accessible in the other fops hooks. If we go for > > > this solution it will change the callbacks of `MiscDevice` and maybe even some > > > other architectural aspects. > > > > > > This needs to be addressed first. > > > > The issue about ever growing dependencies _can_ be that authors have > > other priorities and are slow to turn things around, which may end up > > with nothing being accepted and contributors getting frustrated. > > I would share your argumentation if > > 1) we'd talk about a real driver, where people are actually waiting for, > 2) it'd be about a new feature, performance improvement, etc. > > What we have here is different: > > You wrote a sample implementation for a new and just landed abstraction that > reveals a shortcoming. (Which is great, because it means the sample already > served an important purpose.) > > IMHO, the consequence should not be to merge the sample as is anyways, because > another purpose of the sample implementation is to tell people "look, this is > exactly how it should look like, please do it the same way". > > Instead, we should fix the shortcoming, adjust the sample implementation and > merge it then. > > Just to make it clear, for a real driver I think it would be reasonable to just > go ahead, but for a sample that should educate, we should fix things first. Provided that we stay within certain tolerances, I don't see any of what you've said as particularly unreasonable. I'll have an out-of-band chat with Alice on Monday with a view to conjuring up a game plan. -- Lee Jones [李琼斯]
© 2016 - 2025 Red Hat, Inc.