Re: [PATCH v3 1/2] gpu: nova-core: fix barrier usage in CPU->GSP messaging path

["Gary Guo" <gary@garyguo.net>]

On Tue Aug 25, 2026 at 1:38 AM BST, Eliot Courtney wrote:
> On Mon Aug 24, 2026 at 10:07 PM JST, Gary Guo wrote:
>> On Mon Aug 24, 2026 at 2:03 PM BST, Eliot Courtney wrote:
>>> On Mon Aug 24, 2026 at 9:56 PM JST, Gary Guo wrote:
>>>>>> @@ -683,6 +689,9 @@ fn send_single_command<M>(&mut self, bar: Bar0<'_>, command: M) -> Result
>>>>>>              dst.header.length(),
>>>>>>          );
>>>>>>  
>>>>>> +        // ORDERING: STORE->STORE ordering needed to order `cpu_write_ptr` write after data write.
>>>>>> +        dma_mb(Write);
>>>>>> +
>>>>>
>>>>> Is there a reason this can't go into `advance_cpu_write_ptr`?
>>>>
>>>> I think it's more clear to consider `advance_cpu_write_ptr` to just be the
>>>> pointer increment, and the ordering should be visible in code that performs both
>>>> memory ops.
>>>
>>> In the second patch, it looks like you're adding the memory barrier
>>> directly in `advance_cpu_read_ptr`. So we'd have one barrier directly in
>>> the code advancing the pointer and one not, which seems asymmetric. I
>>> think it's less error prone to put the barrier in the function so it
>>> can't be misused (and we already have evidence the barriers are easy to
>>> get wrong, since this code was already broken).
>>
>> In the second one `message.header.length()` is read, so if I move the barrier to
>> before the advance it'll be incorrect.
>>
>> Best,
>> Gary
>
> Yerp I mean move the barrier into `advance_cpu_write_ptr` not move the
> barrier out of `advance_cpu_read_ptr` - I agree that'd be incorrect. On
> clearness, it feels very odd to me to have these two functions
> (advance_cpu_read_ptr, advance_cpu_write_ptr) where one controls the
> memory barrier and one doesn't, purely based off the structure of the
> callers. And I still think it's less error prone (for future changes) to
> do it this way too.

Frankly I don't like the asymmetry that the advancing code does the barrier,
while the pointer reading code doesn't have the barrier. However, if we move the
barrier to the pointer read function, then the `driver_write_area_size` would
gain a unnecessary barrier. (Actually, `driver_read_area` code have a similar
issue, a failed pool would execute an unnecessary barrier).

As an alternative to move the barrier into the advancing code, alternatively we
can pull the `message.header.length()` to a separate line instead.

I think we should either always have barrier inside the pointer read/update
code, or always on the user side. Given the former would mean unnecessary
barriers, I am erring on the latter.

Best,
Gary