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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DD53RWODAWPS.3MBKE59UU905Q@kernel.org>
Date: Mon, 29 Sep 2025 09:24:19 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alistair Popple" <apopple@...dia.com>
Cc: "Alexandre Courbot" <acourbot@...dia.com>, "Lyude Paul"
 <lyude@...hat.com>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, "Miguel Ojeda" <ojeda@...nel.org>, "Alex
 Gaynor" <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary
 Guo" <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v2 05/10] gpu: nova-core: gsp: Add GSP command queue
 handling

On Mon Sep 29, 2025 at 3:06 AM CEST, Alistair Popple wrote:
> On 2025-09-26 at 12:20 +1000, Alexandre Courbot <acourbot@...dia.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