linux-kernel - Re: [PATCH v5 08/13] gpu: nova-core: sequencer: Add register opcodes

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: <DE99UZ5ONZ4G.13Y7574M0YYHU@nvidia.com>
Date: Sat, 15 Nov 2025 21:37:05 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Lyude Paul" <lyude@...hat.com>, "Joel Fernandes"
 <joelagnelf@...dia.com>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 "Danilo Krummrich" <dakr@...nel.org>, "Alexandre Courbot"
 <acourbot@...dia.com>
Cc: "Alistair Popple" <apopple@...dia.com>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 <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>, "Timur Tabi" <ttabi@...dia.com>,
 <joel@...lfernandes.org>, "Daniel Almeida" <daniel.almeida@...labora.com>,
 <nouveau@...ts.freedesktop.org>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH v5 08/13] gpu: nova-core: sequencer: Add register
 opcodes

Hi Lyude,

On Sat Nov 15, 2025 at 6:55 AM JST, Lyude Paul wrote:
> We're very close! I see a few things we still need to fix though
>
> On Fri, 2025-11-14 at 14:55 -0500, Joel Fernandes wrote:
>> ...
>> -impl GspSeqCmdRunner for GspSeqCmd {
>> -    fn run(&self, _seq: &GspSequencer<'_>) -> Result {
>> +impl GspSeqCmdRunner for fw::RegWritePayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        let val = self.val();
>> +        let _ = sequencer.bar.try_write32(val, addr);
>
> We're still not handling the possible error from try_write32() here
> Also - addr/val seem a bit superfluous

I wonder whether this could be on purpose? As in, the register sequence
might try to write to non-existing registers on some models but this is
not necessarily a fatal error. Still, I can complete initialization even
when handling the error, so let's err on the side or correctness unless
Joel signals that this is indeed what we want (in which case a comment
would ensure there cannot be any confusion as to the purpose).

For now, changed this to:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_write32(self.val(), addr)
>
>>          Ok(())
>>      }
>>  }
>>  
>> +impl GspSeqCmdRunner for fw::RegModifyPayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        if let Ok(temp) = sequencer.bar.try_read32(addr) {
>> +            let _ = sequencer
>> +                .bar
>> +                .try_write32((temp & !self.mask()) | self.val(), addr);
>
> Same here

This is now:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_read32(addr).and_then(|val| {
        sequencer
            .bar
            .try_write32((val & !self.mask()) | self.val(), addr)
    })

>
>> +        }
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegPollPayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +
>> +        // Default timeout to 4 seconds.
>> +        let timeout_us = if self.timeout() == 0 {
>> +            4_000_000
>> +        } else {
>> +            i64::from(self.timeout())
>> +        };
>> +
>> +        // First read.
>> +        sequencer.bar.try_read32(addr)?;
>> +
>> +        // Poll the requested register with requested timeout.
>> +        read_poll_timeout(
>> +            || sequencer.bar.try_read32(addr),
>> +            |current| (current & self.mask()) == self.val(),
>> +            Delta::ZERO,
>> +            Delta::from_micros(timeout_us),
>> +        )
>> +        .map(|_| ())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegStorePayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        let _index = self.index();
>> +
>> +        let _val = sequencer.bar.try_read32(addr)?;
>> +
>> +        Ok(())
>
> These variables still seem superfluous - we don't use _index at all.
>
> This function should just be rewritten as:
>
>     fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>         sequencer.bar.try_read32(self.addr() as usize)?;

Actually I thought this is also a good opportunity to replace the `as`
with the safer `FromSafeCast` interface we introduced recently precisely
for that (you probably have already noticed from the previous fixes :)).
Only this would look a bit heavy if done inline, so for readability I'll
keep the temporary variable and change this to:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_read32(addr).map(|_| ())

This makes me wonder about the `index` member of this op - it is not
used at all! Joel, do you know what it is for?

Thanks for spotting these!