Re: [PATCH 0/8] Device::drvdata() and driver/driver interaction (auxiliary)

Alice Ryhl
2025-10-29T13:03:21Z

On Tue, Oct 21, 2025 at 12:34:22AM +0200, Danilo Krummrich wrote:
> tl;dr:
>
> Implement a safe Device<Bound>::drvdata() accessor (used for driver to
> driver interactions) based on the auxiliary bus.
>
> This provides a way to derive a driver's device private data when
> serving as a parent in a driver hierarchy, such as a driver utilizing
> the auxiliary bus.
>
> Please have a look at patch 8 ("samples: rust: auxiliary: illustrate
> driver interaction") to see how it turns out.
>
> --
>
> Full cover letter:
>
> In C dev_get_drvdata() has specific requirements under which it is valid
> to access the returned pointer. That is, drivers have to ensure that
>
> (1) for the duration the returned pointer is accessed the driver is
> bound and remains to be bound to the corresponding device,
>
> (2) the returned void * is treated according to the driver's private
> data type, i.e. according to what has been passed to
> dev_set_drvdata().
>
> In Rust, (1) can be ensured by simply requiring the Bound device
> context, i.e. provide the drvdata() method for Device<Bound> only.
>
> For (2) we would usually make the device type generic over the driver
> type, e.g. Device<T: Driver>, where <T as Driver>::Data is the type of
> the driver's private data.
>
> However, a device does not have a driver type known at compile time and
> may be bound to multiple drivers throughout its lifetime.
>
> Hence, in order to be able to provide a safe accessor for the driver's
> device private data, we have to do the type check on runtime.
>
> This is achieved by letting a driver assert the expected type, which is
> then compared to a type hash stored in struct device_private when
> dev_set_drvdata() is called [2].
>
> Example:
>
> // `dev` is a `&Device<Bound>`.
> let data = dev.drvdata::<SampleDriver>()?;
>
> There are two aspects to note:
>
> (1) Technically, the same check could be achieved by comparing the
> struct device_driver pointer of struct device with the struct
> device_driver pointer of the driver struct (e.g. struct
> pci_driver).
>
> However, this would - in addition the pointer comparison - require
> to tie back the private driver data type to the struct
> device_driver pointer of the driver struct to prove correctness.
>
> Besides that, accessing the driver struct (stored in the module
> structure) isn't trivial and would result into horrible code and
> API ergonomics.
>
> (2) Having a direct accessor to the driver's private data is not
> commonly required (at least in Rust): Bus callback methods already
> provide access to the driver's device private data through a &self
> argument, while other driver entry points such as IRQs,
> workqueues, timers, IOCTLs, etc. have their own private data with
> separate ownership and lifetime.
>
> In other words, a driver's device private data is only relevant
> for driver model contexts (such a file private is only relevant
> for file contexts).
>
> Having that said, the motivation for accessing the driver's device
> private data with Device<Bound>::drvdata() are interactions between
> drivers. For instance, when an auxiliary driver calls back into its
> parent, the parent has to be capable to derive its private data from the
> corresponding device (i.e. the parent of the auxiliary device).
>
> Therefore this patch series also contains the corresponding patches for
> the auxiliary bus abstraction, i.e. guarantee that the auxiliary
> device's parent is guaranteed to be bound when the auxiliary device
> itself is guaranteed to be bound, plus the corresponding
> Device<Bound>::parent() method.
>
> Finally, illustrate how things turn out by updating the auxiliary sample
> driver.
>
> Similarly, the same thing can be done for PCI virtual function drivers
> calling back into the corresponding physical function driver or MFD.
>
> The former (PCI PF/VF interaction) will be addressed by a separate patch
> series. Both, auxiliary and PCI PF/VF is required by the Nova project.
>
> A branch containing the series can be found in [1].
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drvdata
> [2] Type hash (TypeId) stored in struct device_private:
>
> The Rust type stored in struct device_private could be replaced
> by a dedicated (and transparent) private pointer (e.g.
> struct device_private::rust).
>
> While I'm not overly concerned about the extra allocation (not a
> hot path at all), I still wanted to try to store it directly in
> struct device_private, see how it turns out and gather opinions.
>
> Additionally, I don't expect any additional Rust specific
> private data to be required. But even if, changing things up to
> use a separate transparent allocation in the future is trivial.
>
> Danilo Krummrich (8):
> rust: device: narrow the generic of drvdata_obtain()
> rust: device: introduce Device::drvdata()
> rust: auxiliary: consider auxiliary devices always have a parent
> rust: auxiliary: unregister on parent device unbind
> rust: auxiliary: move parent() to impl Device
> rust: auxiliary: implement parent() for Device<Bound>
> samples: rust: auxiliary: misc cleanup of ParentDriver::connect()
> samples: rust: auxiliary: illustrate driver interaction
>
> drivers/base/base.h | 16 ++++
> drivers/gpu/drm/nova/file.rs | 2 +-
> drivers/gpu/nova-core/driver.rs | 8 +-
> rust/bindings/bindings_helper.h | 6 ++
> rust/kernel/auxiliary.rs | 108 ++++++++++++++++----------
> rust/kernel/device.rs | 83 ++++++++++++++++++--
> rust/kernel/pci.rs | 2 +-
> rust/kernel/platform.rs | 2 +-
> rust/kernel/usb.rs | 4 +-
> samples/rust/rust_driver_auxiliary.rs | 44 +++++++----
> 10 files changed, 207 insertions(+), 68 deletions(-)

It looks like there are some patches that add code that doesn't pass
rustfmt, which are then fixed in follow-up commits. You might want to do
a pass of rustfmt after each commit.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>