Re: [PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue handling

["Danilo Krummrich" <dakr@kernel.org>]

On Mon Sep 29, 2025 at 3:06 AM CEST, Alistair Popple wrote:
> On 2025-09-26 at 12:20 +1000, Alexandre Courbot <acourbot@nvidia.com> wrote...
>> On Thu Sep 25, 2025 at 3:32 PM JST, Alistair Popple wrote:
>> <snip>
>> >> > +    #[expect(unused)]
>> >> > +    pub(crate) fn receive_msg_from_gsp<M: GspMessageFromGsp, R>(
>> >> > +        &mut self,
>> >> > +        timeout: Delta,
>> >> > +        init: impl FnOnce(&M, SBuffer<core::array::IntoIter<&[u8], 2>>) -> Result<R>,
>> >> > +    ) -> Result<R> {
>> >> > +        let (driver_area, msg_header, slice_1) = wait_on(timeout, || {
>> >> > +            let driver_area = self.gsp_mem.driver_read_area();
>> >> > +            // TODO: find an alternative to as_flattened()
>> >> > +            #[allow(clippy::incompatible_msrv)]
>> >> > +            let (msg_header_slice, slice_1) = driver_area
>> >> > +                .0
>> >> > +                .as_flattened()
>> >> > +                .split_at(size_of::<GspMsgElement>());
>> >> > +
>> >> > +            // Can't fail because msg_slice will always be
>> >> > +            // size_of::<GspMsgElement>() bytes long by the above split.
>> >> > +            let msg_header = GspMsgElement::from_bytes(msg_header_slice).unwrap();
>> >> 
>> >> Any reason we're not just using unwrap_unchecked() here then?
>> >
>> > Because whilst my assertions about the code are currently correct if it ever
>> > changes I figured it would be better to explicitly panic than end up with
>> > undefined behaviour. Is there some other advantage to using unwrap_unchecked()?
>> > I can't imagine there'd be much of a performance difference.
>> 
>> Here I think we should just use the `?` operator. The function already
>> returns a `Result` so it would fit.
>
> Actually note quite true - this is in a closure that must return `Option<_>`
> so returning `Result` doesn't fit. However it still fits because I just noticed
> `::from_bytes()` returns an `Option` so `?` will still work.

More in general, as by now, unwrap() panics the kernel, which is an absolute
last resort, that should only be considered if there's really no other option.
Please also see [1] and [2].

[1] https://docs.kernel.org/process/coding-style.html#do-not-crash-the-kernel
[2] https://github.com/Rust-for-Linux/linux/issues/1191