Re: [PATCH v3 2/2] rust: lockdep: Use Pin for all LockClassKey usages

[Mitchell Levy <levymitchell0@gmail.com>]

On Fri, Feb 07, 2025 at 12:22:19AM +0000, Benno Lossin wrote:
> On 06.02.25 22:27, Boqun Feng wrote:
> > On Wed, Feb 05, 2025 at 08:30:50PM +0000, Benno Lossin wrote:
> >> On 05.02.25 20:59, Mitchell Levy wrote:
> >>> diff --git a/rust/kernel/sync/lock.rs b/rust/kernel/sync/lock.rs
> >>> index 41dcddac69e2..119e5f569bdb 100644
> >>> --- a/rust/kernel/sync/lock.rs
> >>> +++ b/rust/kernel/sync/lock.rs
> >>> @@ -7,12 +7,9 @@
> >>>
> >>>  use super::LockClassKey;
> >>>  use crate::{
> >>> -    init::PinInit,
> >>> -    pin_init,
> >>> -    str::CStr,
> >>> -    types::{NotThreadSafe, Opaque, ScopeGuard},
> >>> +    init::PinInit, pin_init, str::CStr, types::NotThreadSafe, types::Opaque, types::ScopeGuard,
> >>>  };
> >>> -use core::{cell::UnsafeCell, marker::PhantomPinned};
> >>> +use core::{cell::UnsafeCell, marker::PhantomPinned, pin::Pin};
> >>>  use macros::pin_data;
> >>>
> >>>  pub mod mutex;
> >>> @@ -121,7 +118,7 @@ unsafe impl<T: ?Sized + Send, B: Backend> Sync for Lock<T, B> {}
> >>>
> >>>  impl<T, B: Backend> Lock<T, B> {
> >>>      /// Constructs a new lock initialiser.
> >>> -    pub fn new(t: T, name: &'static CStr, key: &'static LockClassKey) -> impl PinInit<Self> {
> >>> +    pub fn new(t: T, name: &'static CStr, key: Pin<&'static LockClassKey>) -> impl PinInit<Self> {
> >>
> >> Static references do not need `Pin`, since `Pin::static_ref` [1] exists,
> >> so you can just as well not add the `Pin` here and the other places
> >> where you have `Pin<&'static T>`.
> >>
> > 
> > You're right about `Pin` not needed for 'static. However, the
> > `Pin<&'static LockClassKey>` signature is the intermediate state, and
> > eventually we will need to support initializing a lock (and others) with
> > a dynamically allocated `LockClassKey`, and when that is available, the
> > type of `key` will become `Pin<&'a LockClassKey>`, so `Pin` is needed.
> > 
> > So I would like to keep this patch as it is. Works for you?
> 
> Makes sense and fine by me. It would be nice if it was also mentioned in
> the commit message. Thanks!

This makes sense and is a good point. Will resend with a fixed commit
message.

Thanks,
Mitchell

> ---
> Cheers,
> Benno
> 
> > 
> > Regards,
> > Boqun
> > 
> >> The reasoning is that since the data lives for `'static` at that
> >> location, it will never move (since it is borrowed for `'static` after
> >> all).
> >>
> >> [1]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.static_ref
> >>
> >> ---
> >> Cheers,
> >> Benno
> >>
> >>>          pin_init!(Self {
> >>>              data: UnsafeCell::new(t),
> >>>              _pin: PhantomPinned,
> >>> diff --git a/rust/kernel/sync/lock/global.rs b/rust/kernel/sync/lock/global.rs
> >>> index 480ee724e3cc..d65f94b5caf2 100644
> >>> --- a/rust/kernel/sync/lock/global.rs
> >>> +++ b/rust/kernel/sync/lock/global.rs
> >>> @@ -13,6 +13,7 @@
> >>>  use core::{
> >>>      cell::UnsafeCell,
> >>>      marker::{PhantomData, PhantomPinned},
> >>> +    pin::Pin,
> >>>  };
> >>>
> >>>  /// Trait implemented for marker types for global locks.
> >>
> >>
>