Message-ID: <2a31fcd045582382987c8c1da7c4b7d58a1dff61.camel@posteo.de>
Date: Sun, 12 Oct 2025 22:11:46 +0000
From: Markus Probst <markus.probst@...teo.de>
To: Benno Lossin <lossin@...nel.org>, Danilo Krummrich <dakr@...nel.org>, 
 Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Lee
 Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>
Cc: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>, Vlastimil Babka	
 <vbabka@...e.cz>, "Liam R. Howlett" <Liam.Howlett@...cle.com>, Uladzislau
 Rezki	 <urezki@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo
 <gary@...yguo.net>, 	bjorn3_gh@...tonmail.com, Andreas Hindborg
 <a.hindborg@...nel.org>, Alice Ryhl	 <aliceryhl@...gle.com>, Trevor Gross
 <tmgross@...ch.edu>, 	rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org, 	linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 1/2] rust: add basic Pin<Vec<T, A>> abstractions

On Sun, 2025-10-12 at 23:31 +0200, Benno Lossin wrote:
> On Sun Oct 12, 2025 at 6:57 PM CEST, Markus Probst wrote:
> > From what I can tell, there is no way to get a `Pin<&mut Vec<T,
> > A>>`
> > from a `&mut Pin<Vec<T, A>>`. We can only get `Pin<&mut [T]>` which
> > is
> > not usable in our case.
> 
> Hmm yeah that's true.
> 
> > If there is way, without the extension trait or an extra struct, I
> > would be happy to implement it.
> 
> So I tried to look for the usage site of this and I found this usage
> in
> your v1:
> 
>     +        let mut leds = KPinnedVec::with_capacity(
>     +            Atmega1608LedAddress::VALUES.len() *
> Atmega1608LedId::VALUES.len(),
>     +            GFP_KERNEL,
>     +        )?;
>     +
>     +        let mut i = 0;
>     +        for addr in Atmega1608LedAddress::VALUES {
>     +            let mode_lock = Arc::pin_init(new_mutex!(()),
> GFP_KERNEL)?;
>     +
>     +            for id in Atmega1608LedId::VALUES {
>     +                let Some(child) =
>     +                   
> fwnode.get_child_by_name(&CString::try_from_fmt(fmt!("led@{i}"))?)
>     +                else {
>     +                    continue;
>     +                };
>     +
>     +                let client = ARef::clone(&client);
>     +                let mode_lock = Arc::clone(&mode_lock);
>     +
>     +                leds.push_pin_init(LedClassDev::new(
>     +                    Some(idev),
>     +                    None,
>     +                    LedInitData::new().fwnode(&child),
>     +                    Atmega1608Led {
>     +                        addr,
>     +                        id,
>     +                        client,
>     +
>     +                        mode_lock,
>     +                    },
>     +                ))?;
>     +                i += 1;
>     +            }
>     +        }
>     +        Ok(KBox::new(Self { client, leds }, GFP_KERNEL)?.into())
> 
> And I think using `Vec` for this is just wrong. `Vec` is a data
> structure that supports growing and shrinking the allocation. But you
> just need a fixed size buffer that holds all your data. Do you think
> that `Pin<Box<[LedClassDev]>>` would suffice if it had proper support
> from pin-init?
As you can see in v1, the number of leds (or vec entries) depends on
the fwnode (see the continue statement there). I don't think that
counts as fixed size. `Pin<KBox<[Option<LedClassDev>]>>` could
potentially be used instead of `Pin<KVec<LedClassDev>>` in my scenario,
but that would require an extra byte of allocation for the max leds of
24 each and the code would look more ugly. At the point I use Option in
the slice, its basically an unoptimized Vec (instead of storing the
length, it stores if an item in the buffer is present or not).

> 
> Also, please don't top-post [1] and take a look at your mail client
> configuration, it puts lots of extra `> ` at the end which looks
> pretty
> strange [2].
Yes, I did notice that. It is not present when writing a reply, but
after it got sent for some reason (most replies, not all). It is GNOME
Evolution in its default settings basically. My distro ships a 4 months
outdated version (3.56.2), which shouldn't be too old, but I will
investiage.

Thanks
- Markus Probst
> 
> [1]:
> https://docs.kernel.org/process/submitting-patches.html#use-trimmed-interleaved-replies-in-email-discussions
> [2]:
> https://lore.kernel.org/all/e550b0862e9ea87e50688d1ec8f623638d170a3a.camel@posteo.de
> 
> ---
> Cheers,
> Benno
> 
> > Thanks
> > - Markus Probst
> > 
> > On Sun, 2025-10-12 at 18:26 +0200, Benno Lossin wrote:
> > > On Sun Oct 12, 2025 at 4:52 PM CEST, Markus Probst wrote:
> > > > @@ -109,6 +111,21 @@ pub struct Vec<T, A: Allocator> {
> > > >      _p: PhantomData<A>,
> > > >  }
> > > >  
> > > > +/// Extension for Pin<Vec<T, A>>
> > > > +pub trait PinnedVecExt<T> {
> > > 
> > > Why is this an extension trait? Couldn't we directly implement
> > > this
> > > on
> > > `Vec<T>` with `self: Pin<&mut Self>`?
> > > 
> > > ---
> > > Cheers,
> > > Benno
> > > 
> > > > +    /// Pin-initializes P and appends it to the back of the
> > > > [`Vec`] instance without reallocating.
> > > > +    fn push_pin_init<E: From<PushError<P>>, P: PinInit<T,
> > > > E>>(&mut
> > > > self, init: P) -> Result<(), E>;
> > > > +
> > > > +    /// Shortens the vector, setting the length to `len` and
> > > > drops
> > > > the removed values.
> > > > +    /// If `len` is greater than or equal to the current
> > > > length,
> > > > this does nothing.
> > > > +    ///
> > > > +    /// This has no effect on the capacity and will not
> > > > allocate.
> > > > +    fn truncate(&mut self, len: usize);
> > > > +
> > > > +    /// Removes the last element from a vector and drops it
> > > > returning true, or false if it is empty.
> > > > +    fn pop(&mut self) -> bool;
> > > > +}
> > > > +
> > > >  /// Type alias for [`Vec`] with a [`Kmalloc`] allocator.
> > > >  ///
> > > >  /// # Examples

Powered by blists - more mailing lists