On Sat Jul 5, 2025 at 5:06 PM CEST, Danilo Krummrich wrote:
> On Sat, Jul 05, 2025 at 01:15:06PM +0200, Benno Lossin wrote:
>> On Sat Jun 21, 2025 at 9:43 PM CEST, Danilo Krummrich wrote:
>> > +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()`.
>>
>> Well, you're also relying on `dev_get_drvdata` to return the same
>> pointer that was given to `dev_set_drvdata`.
>>
>> Otherwise the safety docs look fine.
>
> Great! What do you think about:
>
> diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
> index 146eba147d2f..b01cb8e8dab3 100644
> --- a/rust/kernel/device.rs
> +++ b/rust/kernel/device.rs
> @@ -80,8 +80,11 @@ 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()`.
> + // SAFETY:
> + // - By the safety requirements of this function, `ptr` comes from a previous call to
> + // `into_foreign()`.
> + // - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
> + // in `into_foreign()`.
Looks good, though I haven't done a full review, but you can have my:
Acked-by: Benno Lossin <lossin@xxxxxxxxxx>
---
Cheers,
Benno
> unsafe { T::from_foreign(ptr.cast()) }
> }
