Re: [PATCH v4 1/2] rust: add basic Pin<Vec<T, A>> abstractions

[Markus Probst <markus.probst@posteo.de>]

On Mon, 2025-10-13 at 10:03 +0200, Benno Lossin wrote:
> On Mon Oct 13, 2025 at 12:11 AM CEST, Markus Probst wrote:
> > 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).
> 
> You can just make the length of the slice be the desired length?
That would work, but creates another allocation on the heap (Vec<I>)
that could have been avoided. I don't think it would make `Pin<Vec<T,
A>>` obsolete.

Or would you rather say, such allocations don't matter?

> (also,
> `i` is never incremented in the `continue` case, so it will act like
> a
> `break`?)
You just found a bug in v1.

Thanks
- Markus Probst

[1] https://docs.rs/arrayvec/latest/arrayvec/struct.ArrayString.html

> 
> One option that we have would be storing the initializers in a vec:
> 
>     fn probe(
>         pdev: &I2cClient<kernel::device::Core>,
>         _id_info: Option<&Self::IdInfo>,
>     ) -> Result<Pin<KBox<Self>>> {
>         let idev = pdev.as_ref();
>     
>         let Some(fwnode) = idev.fwnode() else {
>             return Err(EINVAL);
>         };
>     
>         let client: ARef<I2cClient> = pdev.into();
>     
>         client
>             .write_byte_data(1, 0)
>             .inspect_err(|err| dev_err!(idev, "unable to remove led
> mask: {err:?}\n"))?;
>     
>         let mut led_init = KVec::new();
>     
>         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);
>     
>                 led_init.push(LedClassDev::new(
>                     Some(idev),
>                     None,
>                     LedInitData::new().fwnode(&child),
>                     Atmega1608Led {
>                         addr,
>                         id,
>                         client,
>                         mode_lock,
>                     },
>                 ))?;
>                 i += 1;
>             }
>         }
>         let leds = Vec::pin_init_slice(led_init, GFP_KERNEL)?;
>         Ok(KBox::new(Self { client, leds }, GFP_KERNEL)?.into())
>     }
> 
> And `Vec::pin_init_slice` would have the following signature:
> 
>     fn pin_init_slice<T, I, E>(this: Vec<I>, flags: alloc::Flags) ->
> Result<Pin<Box<[T]>>>
>     where
>         I: PinInit<T, E>,
>         Error: From<E>;
> 
> ---
> Cheers,
> Benno
> 
> > 
> > > 
> > > 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