Re: [PATCH v2 2/3] device: rust: expand documentation for Device

["Danilo Krummrich" <dakr@kernel.org>]

On Thu Jul 24, 2025 at 9:03 AM CEST, Alice Ryhl wrote:
> On Tue, Jul 22, 2025 at 05:00:00PM +0200, Danilo Krummrich wrote:
>> The documentation for the generic Device type is outdated and deserves
>> much more detail.
>> 
>> Hence, expand the documentation and cover topics such as device types,
>> device contexts, as well as information on how to use the generic device
>> infrastructure to implement bus and class specific device types.
>> 
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> A few nits below, but in general looks good.
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
>> -/// This structure represents the Rust abstraction for a C `struct device`. This implementation
>> -/// abstracts the usage of an already existing C `struct device` within Rust code that we get
>> -/// passed from the C side.
>> +/// This structure represents the Rust abstraction for a C `struct device`. A [`Device`] can either
>> +/// exist as temporary reference (see also [`Device::from_raw`]), which is only valid within a
>> +/// certain scope or as [`ARef<Device>`], owning a dedicated reference count.
>
> Doesn't there need to be a comma between "scope" and "or"?
>
> It's possible that I'm confusing the danish and english comma rules, but
> I got confused when reading this.

No, I think you're right.

>> +/// # Implementing Class Devices
>> +///
>> +/// Class device implementations require less infrastructure and depend slightly more on the
>> +/// specific subsystem.
>> +///
>> +/// An example implementation for a class device could look like this.
>> +///
>> +/// ```ignore
>> +/// #[repr(C)]
>> +/// #[pin_data]
>> +/// pub struct Device<T: class::Driver> {
>> +///     dev: Opaque<bindings::class_device_type>,
>> +///     #[pin]
>> +///     data: T::Data,
>
> Should the `dev` field not also be pinned?

I think we should just remove any pin stuff from the example, that's an
implementation detail.

--

In case you're curious, the reason it's there is because that's how drm::Device
is defined. However, drm::Device doesn't need the pin stuff either, but I forgot
to remove it. See drm::Device::new():

    /// Create a new `drm::Device` for a `drm::Driver`.
    pub fn new(dev: &device::Device, data: impl PinInit<T::Data, Error>) -> Result<ARef<Self>> {
        // SAFETY:
        // - `VTABLE`, as a `const` is pinned to the read-only section of the compilation,
        // - `dev` is valid by its type invarants,
        let raw_drm: *mut Self = unsafe {
            bindings::__drm_dev_alloc(
                dev.as_raw(),
                &Self::VTABLE,
                mem::size_of::<Self>(),
                mem::offset_of!(Self, dev),
            )
        }
        .cast();
        let raw_drm = NonNull::new(from_err_ptr(raw_drm)?).ok_or(ENOMEM)?;

        // SAFETY: `raw_drm` is a valid pointer to `Self`.
        let raw_data = unsafe { ptr::addr_of_mut!((*raw_drm.as_ptr()).data) };

        // SAFETY:
        // - `raw_data` is a valid pointer to uninitialized memory.
        // - `raw_data` will not move until it is dropped.
        unsafe { data.__pinned_init(raw_data) }.inspect_err(|_| {
            // SAFETY: `__drm_dev_alloc()` was successful, hence `raw_drm` must be valid and the
            // refcount must be non-zero.
            unsafe { bindings::drm_dev_put(ptr::addr_of_mut!((*raw_drm.as_ptr()).dev).cast()) };
        })?;

        // SAFETY: The reference count is one, and now we take ownership of that reference as a
        // `drm::Device`.
        Ok(unsafe { ARef::from_raw(raw_drm) })
    }

While we use data.__pinned_init(), I don't think the drm::Device needs any of
the pin macros.

- Danilo