Re: [PATCH v2 00/10] Device / Driver and PCI Rust abstractions

[Danilo Krummrich <dakr@redhat.com>]

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
>