Message-Id: <0AABF976-BE2C-4A6E-9EC9-AD4F63807E6D@collabora.com>
Date: Tue, 14 Jan 2025 15:54:24 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Lyude Paul <lyude@...hat.com>
Cc: dri-devel@...ts.freedesktop.org,
 rust-for-linux@...r.kernel.org,
 Asahi Lina <lina@...hilina.net>,
 Danilo Krummrich <dakr@...nel.org>,
 mcanal@...lia.com,
 airlied@...hat.com,
 zhiw@...dia.com,
 cjia@...dia.com,
 jhubbard@...dia.com,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Wedson Almeida Filho <wedsonaf@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <benno.lossin@...ton.me>,
 Andreas Hindborg <a.hindborg@...sung.com>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...hat.com>,
 Mika Westerberg <mika.westerberg@...ux.intel.com>,
 open list <linux-kernel@...r.kernel.org>
Subject: Re: [WIP RFC v2 01/35] WIP: rust/drm: Add fourcc bindings

Hi Lyude,

>>> 
>>> +impl FormatInfo {
>>> +    // SAFETY: `ptr` must point to a valid instance of a `bindings::drm_format_info`
>>> +    pub(super) unsafe fn from_raw<'a>(ptr: *const bindings::drm_format_info) -> &'a Self {
>> 
>> I think FormatInfoRef would be more appropriate, since you seem to be creating a reference type (IIUC)
>> for a type that can also be owned.
>> 
>> This would be more in line with the GEM [1] patch, for example.
>> 
>> In other words, using `Ref` here will allow for both an owned `FormatInfo` and a `FormatInfoRef<‘_>`.
>> 
>> I am not sure about the role of lifetime ‘a here. If you wanted to tie the lifetime of &Self to that of the pointer,
>> this does not do it, specially considering that pointers do not have lifetimes associated with them.
>> 
>>> +        // SAFETY: Our data layout is identical
>>> +        unsafe { &*ptr.cast() }
>> 
>> It’s hard to know what is going on with both the reborrow and the cast in the same statement.
>> 
>> I am assuming that cast() is transforming to *Self, and the reborrow to &Self.
>> 
>> To be honest, I dislike this approach. My suggestion here is to rework it to be similar to, e.g., what
>> Alice did here for `ShrinkControl` [2].
> 
> Interesting. I did understand this wouldn't be tying the reference to any
> lifetime more specific then "is alive for the duration of the function this
> was called in" - which in pretty much all the cases we would be using this
> function in would be good enough to ensure safety.
> 
> I guess though I'm curious what precisely is the point of having another type
> instead of a reference would be? It seems like if we were to add a function in
> the future for something that needed a reference to a `FormatInfo`, that
> having to cast from `FormatInfo` to `FormatInfoRef` would be a bit confusing
> when you now have both `&FormatInfo` and `FormatInfoRef`.

I’ve realized since then that there’s more code using the same pattern as you did,
so it appears that it’s found acceptance in the rest of the community. Thus,
I retract what I said earlier.

The `unsafe { &*ptr.cast() }` construct seems to be widely used too, so that is also
not a problem for me anymore

> 
>> 
>> +/// This struct is used to pass information from page reclaim to the shrinkers.
>> +///
>> +/// # Invariants
>> +///
>> +/// `ptr` has exclusive access to a valid `struct shrink_control`.
>> +pub struct ShrinkControl<'a> {
>> + ptr: NonNull<bindings::shrink_control>,
>> + _phantom: PhantomData<&'a bindings::shrink_control>,
>> +}
>> +
>> +impl<'a> ShrinkControl<'a> {
>> + /// Create a `ShrinkControl` from a raw pointer.
>> + ///
>> + /// # Safety
>> + ///
>> + /// The pointer should point at a valid `shrink_control` for the duration of 'a.
>> + pub unsafe fn from_raw(ptr: *mut bindings::shrink_control) -> Self {
>> + Self {
>> + // SAFETY: Caller promises that this pointer is valid.
>> + ptr: unsafe { NonNull::new_unchecked(ptr) },
>> + _phantom: PhantomData,
>> + }
>> + }
>> 
>> Notice the use of PhantomData in her patch.

Some people have complained about introducing arbitrary smart pointers like
I suggested, so let’s drop this idea.
 
>> 
>> By the way, Alice, I wonder if we can just use Opaque here?
> 
> FWIW: I think the reason I didn't use Opaque is because it didn't really seem
> necessary. AFAICT the lifetime of drm_format_info follows rust's data aliasing
> rules: it's only ever mutated before pointers to it are stored elsewhere, thus
> holding a plain reference to it should be perfectly safe.

Do use Opaque though, it’s repr(transparent) and will make your code more similar
to what we already have upstream.

— Daniel

Powered by blists - more mailing lists