Re: [PATCH v2 4/6] gpu: nova-core: gsp: cmdq: move message trimming into its own function

[Alexandre Courbot]

On Fri Nov 6, 2026 at 6:08 PM JST, Eliot Courtney wrote:
> On Fri Nov 6, 2026 at 4:06 PM JST, Alexandre Courbot wrote:
>> Trimming the two driver-owned slices of the command queue to the
>> advertised length of a message is going to be used by both RPC and GMC
>> messages, so factor that part out into a private function.
>>
>> The parsing itself can be improved by leveraging `split_at_checked` and
>> `get`, so use them.
>>
>> No functional change intended.
>>
>> Suggested-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
>> Link: https://lore.kernel.org/all/DMN62G2CDG3N.ETT4GRFXWKKU@xxxxxxxxxx/
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs     | 21 +++++++++++++++++++++
>>  drivers/gpu/nova-core/gsp/cmdq/rpc.rs | 20 +-------------------
>>  2 files changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 9dd11ac8952b..f3151aa8ea26 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -719,3 +719,24 @@ fn drain(&mut self) -> Result {
>>          }
>>      }
>>  }
>> +
>> +/// Helper function that trims the concatenation of `slice_1` and `slice_2` to `length`.
>> +///
>> +/// Returns the truncated `(slice_1, slice_2)` pair, or `EIO` if `length > slice_1.len() +
>> +/// slice_2.len()`.
>> +fn trim_message_slice<'a>(
>> +    (slice_1, slice_2): (&'a [u8], &'a [u8]),
>> +    length: usize,
>> +) -> Result<(&'a [u8], &'a [u8])> {
>> +    // See if `slice_1` covers the whole message, take it entirely if it doesn't.
>> +    let slice_1 = slice_1
>> +        .split_at_checked(length)
>> +        .map(|split| split.0)
>> +        .unwrap_or(slice_1);
>> +    // `slice_2` is the remainder of the message not covered by `slice_1`.
>> +    let slice_2 = slice_2
>> +        .get(..length.saturating_sub(slice_1.len()))
>> +        .ok_or(EIO)?;
>> +
>> +    Ok((slice_1, slice_2))
>> +}
>
> I think .split_at_checked(length).map(|x| x.0) is the same as
> .get(..length), so you can prolly just write:
>
> ```
> let slice_1 = slice_1.get(..length).unwrap_or(slice_1);

Oh yeah, I was just too fixated on using `split_at_checked` and didn't
notice using `get` directly was simpler, thanks!

> let slice_2 = slice_2.get(..length - slice_1.len()).ok_or(EIO)?;
> ```
>
> Up to you if you want to use a checked_sub or saturating_sub but it's
> locally clear that it won't underflow imo.

Yes, this is indeed super trivial. It's actually one of the potential-panic-site
dilemmas where I have trouble deciding what to do.

In the end for this one I've decided to go for your suggestion of using
`-` with a `PANIC:` comment, as the panic site is eventually optimized
away, and the generated assembly is the most efficient.