Re: [PATCH v2 1/5] gpu: nova-core: use checked arithmetic in FWSEC firmware parsing

From: John Hubbard <jhubbard@nvidia.com>
To: Danilo Krummrich <dakr@kernel.org>,
	Joel Fernandes <joelagnelf@nvidia.com>
Cc: linux-kernel@vger.kernel.org, "Paul Walmsley" <pjw@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Alexandre Ghiti" <alex@ghiti.fr>,
	"Miguel Ojeda" <ojeda@kernel.org>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Gary Guo" <gary@garyguo.net>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Benno Lossin" <lossin@kernel.org>,
	"Andreas Hindborg" <a.hindborg@kernel.org>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Trevor Gross" <tmgross@umich.edu>,
	"Alexandre Courbot" <acourbot@nvidia.com>,
	"Alistair Popple" <apopple@nvidia.com>,
	"Timur Tabi" <ttabi@nvidia.com>, "Edwin Peer" <epeer@nvidia.com>,
	"Zhi Wang" <zhiw@nvidia.com>, "David Airlie" <airlied@gmail.com>,
	"Simona Vetter" <simona@ffwll.ch>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Dirk Behme" <dirk.behme@gmail.com>,
	nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	rust-for-linux@vger.kernel.org, linux-riscv@lists.infradead.org
Subject: Re: [PATCH v2 1/5] gpu: nova-core: use checked arithmetic in FWSEC firmware parsing
Date: Wed, 28 Jan 2026 16:58:44 -0800	[thread overview]
Message-ID: <8859d68f-ca38-469e-8345-84059573ac97@nvidia.com> (raw)
In-Reply-To: <DG0N5PNS1ZQ0.13WMT4XYDXNUS@kernel.org>

On 1/28/26 4:20 PM, Danilo Krummrich wrote:
> On Wed Jan 28, 2026 at 4:14 PM CET, Joel Fernandes wrote:
>> On 1/28/2026 5:53 AM, Danilo Krummrich wrote:
>>> On Mon Jan 26, 2026 at 9:23 PM CET, Joel Fernandes wrote:
>>>> @@ -267,7 +264,12 @@ fn new_fwsec(dev: &Device<device::Bound>, bios: &Vbios, cmd: FwsecCommand) -> Re
>>>>            let ucode = bios.fwsec_image().ucode(&desc)?;
>>>>            let mut dma_object = DmaObject::from_data(dev, ucode)?;
>>>>    
>>>> -        let hdr_offset = usize::from_safe_cast(desc.imem_load_size() + desc.interface_offset());
>>>> +        // Compute hdr_offset = imem_load_size + interface_offset.
>>>
>>> I do get the idea behind those comments, but are we sure that's really a good
>>> idea? How do we ensure to keep them up to date in case we have to change the
>>> code?
>>>
>>> If we really want this, I'd at least chose a common syntax, e.g.
>>>
>>> 	// CALC: `imem_load_size + interface_offset`
>>>
>>> without the variable name the resulting value is assigned to.
>>>
>>> But I'd rather prefer to just drop those comments.
>> The idea of adding these comments was to improve readability. However, I
>> can drop them in the v3, that's fine with me.
> 
> Yeah, that's why I wrote "I get the idea". :) But as I write above, I'm
> concerned about the comments getting outdated or inconsistent over time.
> 
> Besides that, it more seems like something your favorite editor should help with
> instead.
> 
>> Do you want me to wait for additional comments on this series, or should
>> I make the update and repost it?  Thanks,
> 
> As mentioned, I tend to think we should just drop them, but I'm happy to hear
> some more opinions on this if any.

Yes, please drop the comments. They were just echoing the code for
the most part, so the code itself will be easier to read without
them.

I think this is something that, when you are writing the code and
comments, it's really hard to see. But when you come back a while
later (or you are reviewing someone else's patch), then it is much
easier to spot.

thanks,
-- 
John Hubbard

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv