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 >
