| Message ID | 20240618234025.15036-1-dakr@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Device / Driver and PCI Rust abstractions | expand |
On 19-06-24, 01:39, Danilo Krummrich wrote: > - move base device ID abstractions to a separate source file (Greg) > - remove `DeviceRemoval` trait in favor of using a `Devres` callback to > unregister drivers > - remove `device::Data`, we don't need this abstraction anymore now that we > `Devres` to revoke resources and registrations Hi Danilo, I am working on writing bindings for CPUFreq drivers [1] and was looking to rebase over staging/rust-device, and I am not sure how to proceed after device::Data is dropped now. What I was doing at probe() was something like this: fn probe(dev: &mut platform::Device, id_info: Option<&Self::IdInfo>) -> Result<Self::Data> { let data = Arc::<DeviceData>::from(kernel::new_device_data!( cpufreq::Registration::new(), (), "CPUFreqDT::Registration" )?); ... // Need a mutable object to be passed to register() here. data.registrations() .ok_or(ENXIO)? .as_pinned_mut() .register(c_str!("cpufreq-dt"), ...)?; Ok(data) } The register() function of cpufreq core needs a mutable pointer to `self` and it worked earlier as Data used a RevocableMutex. But with Devres, we don't have a Mutex anymore and devres.try_access() doesn't give a mutable object. I am a bit confused on how to get this going. I looked at how PCI bus is implemented in the staging/dev but I couldn't find an end driver using this work. Maybe I am making an mistake and missing the obvious. Thanks.
On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote: > On 19-06-24, 01:39, Danilo Krummrich wrote: > > - move base device ID abstractions to a separate source file (Greg) > > - remove `DeviceRemoval` trait in favor of using a `Devres` callback to > > unregister drivers > > - remove `device::Data`, we don't need this abstraction anymore now that we > > `Devres` to revoke resources and registrations > > Hi Danilo, > > I am working on writing bindings for CPUFreq drivers [1] and was > looking to rebase over staging/rust-device, and I am not sure how to > proceed after device::Data is dropped now. As it should be dropped :) A struct device does not have a "data" pointer, it has specific other pointers to hold data in, but they better be accessed by their proper name if you want rust code to be reviewable by anyone. Also, you shouldn't be accessing that field directly anyway, that's what the existing dev_set_drvdata/dev_get_drvdata() calls are for. Just use them please. thanks, greg k-h
On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote: > On 19-06-24, 01:39, Danilo Krummrich wrote: > > - move base device ID abstractions to a separate source file (Greg) > > - remove `DeviceRemoval` trait in favor of using a `Devres` callback to > > unregister drivers > > - remove `device::Data`, we don't need this abstraction anymore now that we > > `Devres` to revoke resources and registrations > > Hi Danilo, > > I am working on writing bindings for CPUFreq drivers [1] and was > looking to rebase over staging/rust-device, and I am not sure how to > proceed after device::Data is dropped now. > > What I was doing at probe() was something like this: > > fn probe(dev: &mut platform::Device, id_info: Option<&Self::IdInfo>) -> Result<Self::Data> { > let data = Arc::<DeviceData>::from(kernel::new_device_data!( > cpufreq::Registration::new(), > (), > "CPUFreqDT::Registration" > )?); > > ... > > // Need a mutable object to be passed to register() here. > data.registrations() > .ok_or(ENXIO)? > .as_pinned_mut() > .register(c_str!("cpufreq-dt"), ...)?; > > Ok(data) > } > > The register() function of cpufreq core needs a mutable pointer to > `self` and it worked earlier as Data used a RevocableMutex. But with > Devres, we don't have a Mutex anymore and devres.try_access() doesn't > give a mutable object. If you want to split `cpufreq::Registration` in `new()` and `register()`, you probably want to pass the registration object to `Devres` in `register()` instead. However, I wouldn't recommend splitting it up (unless you absolutely have to), it's way cleaner (and probably less racy) if things are registered once the registration is created. > > I am a bit confused on how to get this going. I looked at how PCI bus > is implemented in the staging/dev but I couldn't find an end driver > using this work. The PCI abstraction did not need to change for that, since it uses the generalized `driver::Registration`, which is handled by the `Module` structure instead. However, staging/dev also contains the `drm::drv::Registration` type [1], which in principle does the same thing as `cpufreq::Registration` just for a DRM device. If you're looking for an example driver making use of this, please have a look at Nova [1]. [1] https://github.com/Rust-for-Linux/linux/blob/staging/dev/rust/kernel/drm/drv.rs [2] https://gitlab.freedesktop.org/drm/nova/-/blob/nova-next/drivers/gpu/drm/nova/driver.rs > > Maybe I am making an mistake and missing the obvious. > > Thanks. > > -- > viresh > > [1] https://lore.kernel.org/all/cover.1717750631.git.viresh.kumar@linaro.org/ >
On Wed, Jun 19, 2024 at 02:17:58PM +0200, Greg KH wrote: > On Wed, Jun 19, 2024 at 05:34:07PM +0530, Viresh Kumar wrote: > > On 19-06-24, 01:39, Danilo Krummrich wrote: > > > - move base device ID abstractions to a separate source file (Greg) > > > - remove `DeviceRemoval` trait in favor of using a `Devres` callback to > > > unregister drivers > > > - remove `device::Data`, we don't need this abstraction anymore now that we > > > `Devres` to revoke resources and registrations > > > > Hi Danilo, > > > > I am working on writing bindings for CPUFreq drivers [1] and was > > looking to rebase over staging/rust-device, and I am not sure how to > > proceed after device::Data is dropped now. > > As it should be dropped :) > > A struct device does not have a "data" pointer, it has specific other > pointers to hold data in, but they better be accessed by their proper > name if you want rust code to be reviewable by anyone. > > Also, you shouldn't be accessing that field directly anyway, that's what > the existing dev_set_drvdata/dev_get_drvdata() calls are for. Just use > them please. Sorry that this was confusing. `device::Data` was a generic type for drivers to store their private data in. It was meant to be handled by subsystems to store it in their particular driver structure. Is most cases of course this eventually ends up in a call to dev_set_drvdata() (e.g. through pci_set_drvdata()). The reason we had this in the first place was that `device::Data` was also used to store resources and registrations. With my rework to `Devres` this isn't the case anymore, and hence `device::Data` does not add any value anymore and was removed. > > thanks, > > greg k-h >
On 19-06-24, 14:36, Danilo Krummrich wrote: > If you want to split `cpufreq::Registration` in `new()` and `register()`, you > probably want to pass the registration object to `Devres` in `register()` > instead. > > However, I wouldn't recommend splitting it up (unless you absolutely have to), > it's way cleaner (and probably less racy) if things are registered once the > registration is created. > The PCI abstraction did not need to change for that, since it uses the > generalized `driver::Registration`, which is handled by the `Module` structure > instead. > > However, staging/dev also contains the `drm::drv::Registration` type [1], which > in principle does the same thing as `cpufreq::Registration` just for a DRM > device. > > If you're looking for an example driver making use of this, please have a look > at Nova [1]. Thanks for the pointers Danilo. There is more to it now and I still don't know what's the best way forward. :( Devres will probably work well with the frameworks that provide a bus, where a device and driver are matched and probe/remove are called. Obviously Devres needs a struct device, whose probing/removal can allocate/free resources. The CPUFreq framework is a bit different. There is no bus, device or driver there. The device for the framework is the CPU device, but we don't (can't) really bind a struct driver to it. There are more layers in the kernel which use the CPU devices directly, like cpuidle, etc. And so the CPU device isn't really private to the cpufreq/cpuidle frameworks. Most of the cpufreq drivers register with the cpufreq core from their module_init() function, and unregister from module_exit(). There is no probe/remove() callbacks available. Some drivers though have a platform device/driver model implemented over an imaginary platform device, a hack implemented to get them working because of special requirements (one of them is to allow defer probing to work). The driver I am implementing, cpufreq-dt, also has one such platform device which is created at runtime. But there will be others without a platform device. The point is that the Rust cpufreq core can't do the Devres stuff itself and it can't expect a struct device to be made available to it by the driver. Some cpufreq drivers will have a platform device, some won't. One way to make this whole work is to reintroduce the Data part, just for cpufreq core, but I really don't want to do it. What else can be done ?
On Thu, Jun 20, 2024 at 03:35:56PM +0530, Viresh Kumar wrote: > On 19-06-24, 14:36, Danilo Krummrich wrote: > > If you want to split `cpufreq::Registration` in `new()` and `register()`, you > > probably want to pass the registration object to `Devres` in `register()` > > instead. > > > > However, I wouldn't recommend splitting it up (unless you absolutely have to), > > it's way cleaner (and probably less racy) if things are registered once the > > registration is created. > > > The PCI abstraction did not need to change for that, since it uses the > > generalized `driver::Registration`, which is handled by the `Module` structure > > instead. > > > > However, staging/dev also contains the `drm::drv::Registration` type [1], which > > in principle does the same thing as `cpufreq::Registration` just for a DRM > > device. > > > > If you're looking for an example driver making use of this, please have a look > > at Nova [1]. > > Thanks for the pointers Danilo. > > There is more to it now and I still don't know what's the best way > forward. :( > > Devres will probably work well with the frameworks that provide a bus, > where a device and driver are matched and probe/remove are called. > Obviously Devres needs a struct device, whose probing/removal can > allocate/free resources. Indeed, but please note that this was the case before as well. When we had `device::Data` with a `Revokable<T>` for Registrations this revokable was revoked through the `DeviceRemoval` trait when the driver was unbound from the device. > > The CPUFreq framework is a bit different. There is no bus, device or > driver there. The device for the framework is the CPU device, but we > don't (can't) really bind a struct driver to it. There are more layers > in the kernel which use the CPU devices directly, like cpuidle, etc. > And so the CPU device isn't really private to the cpufreq/cpuidle > frameworks. If there is no bus, device or driver, then those abstractions aren't for your use case. Those are abstractions around the device / driver core. > > Most of the cpufreq drivers register with the cpufreq core from their > module_init() function, and unregister from module_exit(). There is no > probe/remove() callbacks available. Some drivers though have a > platform device/driver model implemented over an imaginary platform > device, a hack implemented to get them working because of special > requirements (one of them is to allow defer probing to work). The > driver I am implementing, cpufreq-dt, also has one such platform > device which is created at runtime. But there will be others without a > platform device. > > The point is that the Rust cpufreq core can't do the Devres stuff > itself and it can't expect a struct device to be made available to it > by the driver. Some cpufreq drivers will have a platform device, some > won't. That seems to be purely a design question for cpufreq drivers then. What prevents you from always creating a corresponding platform device? If you really want some drivers to bypass the device / driver model (not sure if that's a good idea though), you need separate abstractions for that. > > One way to make this whole work is to reintroduce the Data part, just > for cpufreq core, but I really don't want to do it. That doesn't help you either. As mentioned above, `device::Data` was supposed to receive a callback (`DeviceRemoval`) from the underlying driver (platform_driver in your case) on device detach to revoke the registration. By using `Devres` instead, nothing changes semantically, but it makes the resulting code cleaner. > What else can be done ? Think about what you want the lifetime of your cpufreq registration to be. Currently, it seems you want to do both, bind it to probe() / remove(), in case the driver is implemented as platform_driver, and to module_init() / module_exit(), in case the device / driver model is bypassed. > > -- > viresh >