Re: [PATCH 2/8] rust: device: add drvdata accessors

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sun Jun 22, 2025 at 4:43 AM JST, Danilo Krummrich wrote:
<snip>
> +impl Device<Internal> {
> +    /// Store a pointer to the bound driver's private data.
> +    pub fn set_drvdata(&self, data: impl ForeignOwnable) {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        unsafe { bindings::dev_set_drvdata(self.as_raw(), data.into_foreign().cast()) }
> +    }
> +
> +    /// Take ownership of the private data stored in this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called once after a preceding call to [`Device::set_drvdata`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_obtain<T: ForeignOwnable>(&self) -> T {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::from_foreign(ptr.cast()) }
> +    }
> +
> +    /// Borrow the driver's private data bound to this [`Device`].
> +    ///
> +    /// # Safety
> +    ///
> +    /// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
> +    ///   [`Device::drvdata_obtain`].
> +    /// - The type `T` must match the type of the `ForeignOwnable` previously stored by
> +    ///   [`Device::set_drvdata`].
> +    pub unsafe fn drvdata_borrow<T: ForeignOwnable>(&self) -> T::Borrowed<'_> {
> +        // SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
> +        let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
> +
> +        // SAFETY: By the safety requirements of this function, `ptr` comes from a previous call to
> +        // `into_foreign()`.
> +        unsafe { T::borrow(ptr.cast()) }
> +    }
> +}

This is a comment triggered by an intuition, so please ignore it if it
doesn't make any sense (it probably doesn't :)).

I have a hunch that we could make more of the methods above safe by
either introducing a typestate to `Internal`, or (which comes down to
the same) using two separate device contexts, one used until
`set_drvdata` is called, and one after, the latter being able to provide
a safe implementation of `drvdata_borrow` (since we know that
`set_drvdata` must have been called).

Since buses must do an unsafe cast to `Device<Internal>` anyway, why not
encode the driver's data type and whether the driver data has been set
or not in that cast as well. E.g, instead of having:

    let pdev = unsafe { &*pdev.cast::<Device<Internal>>() };
    ...
    let foo = unsafe { pdev.as_ref().drvdata_borrow::<Pin<KBox<T>>>() };

You would do:

    let pdev = unsafe { &*pdev.cast::<Device<InternalSet<Pin<KBox<T>>>>>() };
    ...
    // The type of the driver data is already known from `pdev`'s type,
    // so this can be safe.
    let foo = pdev.as_ref().drvdata_borrow();

I don't see any use of `drvdata_borrow` in this patchset, so I cannot
really assess the benefit of making it safe, but for your consideration.
^_^;

And if we can only move `set_drvdata` somewhere else (not sure where
though), then we could assume that `Internal` always has its driver data
set and deal with a single context.

I don't think the design of `Device` allows us to work with anything
else then references to it, so it is unlikely that we can make
`set_drvdata` and `drvdata_obtain` morph its type, which is unfortunate
as it would have allowed us to make these methods safe as well.