Nouveau - Dec 2025 - [PATCH v2 12/13] gpu: nova-core: add PIO support for loading firmware images

On 12/2/25 3:48 PM, Timur Tabi wrote:
> On Tue, 2025-12-02 at 15:40 -0800, John Hubbard wrote:
>> In fact, I just finished looking through my Hopper/Blackwell PIO code,
which
>> also needs 4-byte alignment, and concluded that returning -EINVAL for
misaligned
>> data seems to be the appropriate way to handle things.
> 
> I've added this for v3:
> 
>         // Rejecting misaligned images here allows us to avoid checking
>         // inside the loops.
>         if img.len() % 4 != 0 {
>             return Err(EINVAL);
>         }
Looks good.
> 
> And I manually create the &[u8; 4] now:
> 
>             for word in block.chunks_exact(4) {
>                 let w = [word[0], word[1], word[2], word[3]];
Yes, this is probably the best way. Although...
>                 regs::NV_PFALCON_FALCON_IMEMD::default()
>                     .set_data(u32::from_le_bytes(w))
>                     .write(bar, &E::ID, port);
> 
> word[3] will always exist because of chunks_exact(4).
> 
Interesting, I was just looking at this, and the 4-byte manual
construction bothered me a little ("why must I do this?"), so I'm
currently wondering if "// PANIC..." plus an "infallible"
.unwrap()
is reasonable, for example:

impl Falcon<Fsp> {
...
    pub(crate) fn write_emem(&self, bar: &Bar0, offset: u32, data:
&[u8]) -> Result {
        if offset % 4 != 0 || data.len() % 4 != 0 {
            return Err(EINVAL);
        }
...
        for chunk in data.chunks_exact(4) {
            // PANIC: `chunks_exact(4)` guarantees each chunk is exactly 4
bytes.
            let word = u32::from_le_bytes(chunk.try_into().unwrap());
            regs::NV_PFALCON_FALCON_EMEM_DATA::default()
                .set_data(word)
                .write(bar, &Fsp::ID);
        }

...but actually, I think your way is better, because you don't have
just justify an .unwrap().

What do you think?

I figured you'd enjoy this, coming as it does just one email after I
wrote "never .unwrap()". haha :)

thanks,
-- 
John Hubbard

On Tue, 2025-12-02 at 16:35 -0800, John Hubbard wrote:
> ??????? for chunk in data.chunks_exact(4) {
> ??????????? // PANIC: `chunks_exact(4)` guarantees each chunk is exactly 4
bytes.
> ??????????? let word = u32::from_le_bytes(chunk.try_into().unwrap());
> ??????????? regs::NV_PFALCON_FALCON_EMEM_DATA::default()
> ??????????????? .set_data(word)
> ??????????????? .write(bar, &Fsp::ID);
> ??????? }
> 
> ...but actually, I think your way is better, because you don't have
> just justify an .unwrap().
> 
> What do you think?
I agree.  Avoiding unwrap(), even with a comment, is always a good idea.
> I figured you'd enjoy this, coming as it does just one email after I
> wrote "never .unwrap()". haha :)
I think your code is effectively identical to mine, except that I don't need
the PANIC comment.  I
suspect that in both cases, the compiler cannot tell that each chunk is always 4
bytes and
try_into() will never panic.  In my case, word[3] always exists and will never
panic either.  So I'm
guess that the compiler will still emit code to check for panic.  I don't
know.