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

["Benno Lossin" <lossin@kernel.org>]

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? (also,
`i` is never incremented in the `continue` case, so it will act like a
`break`?)

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