Re: [PATCH v2 9/9] gpu: nova-core: gsp: add CeGetFaultMethodBufferSize RM control command

[Eliot Courtney]

On Fri Mar 20, 2026 at 10:27 PM JST, Danilo Krummrich wrote:
> On Wed Mar 18, 2026 at 8:14 AM CET, Eliot Courtney wrote:
>> +/// Sends the `CeGetFaultMethodBufferSize` RM control command and waits for its reply.
>> +///
>> +/// Returns the CE fault method buffer size in bytes.
>> +#[expect(dead_code)]
>> +pub(crate) fn get_ce_fault_method_buffer_size(
>> +    cmdq: &Cmdq,
>> +    bar: &Bar0,
>> +    client: Handle<Client>,
>> +    subdevice: Handle<Subdevice>,
>> +) -> Result<u32> {
>> +    // Stack-allocate the request; CeGetFaultMethodBufferSizeParams is small (4 bytes).
>> +    let req = [0u8; size_of::<CeGetFaultMethodBufferSizeParams>()];
>> +
>> +    let cmd = RmControl::new(
>> +        client,
>> +        subdevice,
>> +        RmControlMsgFunction::CeGetFaultMethodBufferSize,
>> +        &req,
>> +    );
>> +    let reply = send_rm_control(cmdq, bar, cmd)?;
>> +
>> +    let params = CeGetFaultMethodBufferSizeParams::from_bytes(&reply).ok_or(EINVAL)?;
>> +
>> +    Ok(params.size())
>> +}
>
> This is similar to send_rm_control(), I think RmControl should be generic over
> the control message type, so you could just write something along the lines of:
>
> 	RmControl::<CeGetFaultMethodBufferSize>::new(...).send();
>
> Admittedly, CeGetFaultMethodBufferSize is a bit long, but that's GSP naming and
> get_ce_fault_method_buffer_size() isn't shorter anyway.
>
> (Btw. please try to avoid building functions named get_*() or *_get(), though
> I'm aware that it is part of the struct name in this case. Yet another reason
> not to solve this with a helper function. :)
>
> Alternatively, you could also consider a builder pattern approach.

I agree with this, and after thinking about it I also think we should
get rid of the other helper type methods like `get_gsp_info`. I was
trying to make this consistent with the existing way it's done (the
helper methods) but it does look increasingly bad.