[PATCH 0/2] drm/amdgpu: fix MES queue init wptr reset on atomic64 carriers

[PATCH 0/2] drm/amdgpu: fix MES queue init wptr reset on atomic64 carriers

[Runyu Xiao]

This series fixes two MES queue-init reset paths that still clear
ring->wptr_cpu_addr with a plain 32-bit store even though the same
carrier is accessed with atomic64_set()/atomic64_read() and
support_64bit_ptrs is enabled.

This is not just a missing atomic annotation. The write pointer carrier
is used as a shared 64-bit value, so a plain *ring->wptr_cpu_addr = 0
only clears the low 32 bits. A later atomic64_read() can then observe
stale high 32 bits instead of a real zeroed reset state.

Use atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0) in both reset
paths so the full 64-bit MES wptr is cleared with the same access
family as the existing readers and writers.

This issue was first flagged by our static analysis tool while scanning
for 64-bit atomic write pointer carriers mixed with plain reset stores,
then manually audited on Linux v6.18.21.

It was further confirmed with a QEMU no-device stand-in that preserved
the same access contract: atomic64_set() write, plain low-32 reset, and
atomic64_read() readback. In that stand-in, a buggy reset turned
0x12345678abcdeff0 into 0x1234567800000000, while replacing the reset
with atomic64_set(..., 0) read back 0x0.

Build-tested by compiling mes_v11_0.o and mes_v12_0.o.

No AMDGPU hardware was available for end-to-end runtime testing.

Runyu Xiao (2):
  drm/amdgpu/mes11: fix queue init wptr reset
  drm/amdgpu/mes12: fix queue init wptr reset

 drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/mes_v12_0.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.34.1

[PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[Runyu Xiao]

mes_v11_0_queue_init() resets ring->wptr_cpu_addr with a plain 32-bit
store in the reset/suspend path even though the same carrier is
accessed with atomic64_set()/atomic64_read() and support_64bit_ptrs is
enabled.

This is not just a missing atomic annotation. The MES queue write
pointer is a shared 64-bit carrier, and *ring->wptr_cpu_addr = 0 only
clears the low 32 bits. A later atomic64_read() can then observe stale
high 32 bits instead of a real zeroed reset state.

Use atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0) so the reset
path updates the full 64-bit wptr with the same access family as the
existing readers and writers.

Build-tested by compiling mes_v11_0.o.

No AMDGPU hardware was available for end-to-end runtime testing.

Fixes: d81d75c99936 ("drm/amdgpu/gfx11: enable kiq to map mes ring")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
index a926a3307..e2f762c2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
@@ -1308,7 +1308,7 @@ static int mes_v11_0_queue_init(struct amdgpu_device *adev,
 
 	if ((pipe == AMDGPU_MES_SCHED_PIPE) &&
 	    (amdgpu_in_reset(adev) || adev->in_suspend)) {
-		*(ring->wptr_cpu_addr) = 0;
+		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
 		*(ring->rptr_cpu_addr) = 0;
 		amdgpu_ring_clear_ring(ring);
 	}
-- 
2.34.1

Re: [PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[Christian König]

On 6/2/26 07:03, Runyu Xiao wrote:
> mes_v11_0_queue_init() resets ring->wptr_cpu_addr with a plain 32-bit
> store in the reset/suspend path even though the same carrier is
> accessed with atomic64_set()/atomic64_read() and support_64bit_ptrs is
> enabled.
> 
> This is not just a missing atomic annotation. The MES queue write
> pointer is a shared 64-bit carrier, and *ring->wptr_cpu_addr = 0 only
> clears the low 32 bits. A later atomic64_read() can then observe stale
> high 32 bits instead of a real zeroed reset state.
> 
> Use atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0) so the reset
> path updates the full 64-bit wptr with the same access family as the
> existing readers and writers.
> 
> Build-tested by compiling mes_v11_0.o.
> 
> No AMDGPU hardware was available for end-to-end runtime testing.

Clear NAK.

The atomic64_t cast hack is just something we did for older generations and is not something which is necessary not should be done here.

What could be possible is that we need to use amdgpu_ring_set_wptr() here to correctly distinct between queues with 32bit and 64bit wptrs.

Regards,
Christian.

> 
> Fixes: d81d75c99936 ("drm/amdgpu/gfx11: enable kiq to map mes ring")
> Cc: stable@vger.kernel.org
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
>  drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> index a926a3307..e2f762c2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> @@ -1308,7 +1308,7 @@ static int mes_v11_0_queue_init(struct amdgpu_device *adev,
> 
>         if ((pipe == AMDGPU_MES_SCHED_PIPE) &&
>             (amdgpu_in_reset(adev) || adev->in_suspend)) {
> -               *(ring->wptr_cpu_addr) = 0;
> +               atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
>                 *(ring->rptr_cpu_addr) = 0;
>                 amdgpu_ring_clear_ring(ring);
>         }
> --
> 2.34.1

Re:Re: [PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[肖润宇]

[-- Attachment #1: Type: text/plain, Size: 776 bytes --]

Hi Christian,


Thanks, understood.


To make sure I rework this in the right direction: would you expect
this reset path to do


  ring->wptr = 0;
  amdgpu_ring_set_wptr(ring);


instead of writing wptr_cpu_addr directly?


I am asking because amdgpu_ring_set_wptr() also updates the doorbell,
so I want to confirm that this is the intended sequence for the
reset/suspend case here.


Thanks,
Runyu


On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote:
> Clear NAK.
>
> The atomic64_t cast hack is just something we did for older
> generations and is not something which is necessary nor should
> be done here.
>
> What could be possible is that we need to use amdgpu_ring_set_wptr()
> here to correctly distinguish between queues with 32bit and 64bit
> wptrs.


[-- Attachment #2: Type: text/html, Size: 3510 bytes --]

Re: [PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[Christian König]

On 6/2/26 12:53, 肖润宇 wrote:
> 	
> Some people who received this message don't often get email from 220255722@seu.edu.cn. Learn why this is important <https://aka.ms/LearnAboutSenderIdentification>
> 	
> 
> Hi Christian,
> 
> Thanks, understood.
> 
> To make sure I rework this in the right direction: would you expect
> this reset path to do
> 
>   ring->wptr = 0;
>   amdgpu_ring_set_wptr(ring);
> 
> instead of writing wptr_cpu_addr directly?
> 
> I am asking because amdgpu_ring_set_wptr() also updates the doorbell,
> so I want to confirm that this is the intended sequence for the
> reset/suspend case here.

Yeah I was wondering the same thing.

I think the correct approach would be to make both rptr_cpu_addr and wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then cast that to either (u64*) or (u32*) depending on the ring type.

The atomic64_t hack should really be removed.

BTW Reading the rptr is wrong on multiple instances as well and should probably be fixed in the same patch set.

Regards,
Christian.

> 
> Thanks,
> Runyu
> 
> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote:
>> Clear NAK.
>>
>> The atomic64_t cast hack is just something we did for older
>> generations and is not something which is necessary nor should
>> be done here.
>>
>> What could be possible is that we need to use amdgpu_ring_set_wptr()
>> here to correctly distinguish between queues with 32bit and 64bit
>> wptrs.
> 

Re:Re: [PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[Runyu Xiao]

Hi Christian,


Thanks, that makes sense.


I will rework this around typed rptr/wptr_cpu_addr accesses instead of
the atomic64_t cast, and I will include the related rptr fixes in the
same patch set.


Thanks,
Runyu
>> 	
>> 
>> Hi Christian,
>> 
>> Thanks, understood.
>> 
>> To make sure I rework this in the right direction: would you expect
>> this reset path to do
>> 
>>   ring->wptr = 0;
>>   amdgpu_ring_set_wptr(ring);
>> 
>> instead of writing wptr_cpu_addr directly?
>> 
>> I am asking because amdgpu_ring_set_wptr() also updates the doorbell,
>> so I want to confirm that this is the intended sequence for the
>> reset/suspend case here.
>
>Yeah I was wondering the same thing.
>
>I think the correct approach would be to make both rptr_cpu_addr and wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then cast that to either (u64*) or (u32*) depending on the ring type.
>
>The atomic64_t hack should really be removed.
>
>BTW Reading the rptr is wrong on multiple instances as well and should probably be fixed in the same patch set.
>
>Regards,
>Christian.
>
>> 
>> Thanks,
>> Runyu
>> 
>> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote:
>>> Clear NAK.
>>>
>>> The atomic64_t cast hack is just something we did for older
>>> generations and is not something which is necessary nor should
>>> be done here.
>>>
>>> What could be possible is that we need to use amdgpu_ring_set_wptr()
>>> here to correctly distinguish between queues with 32bit and 64bit
>>> wptrs.
>> 
>
>

>> 	
>> 
>> Hi Christian,
>> 
>> Thanks, understood.
>> 
>> To make sure I rework this in the right direction: would you expect
>> this reset path to do
>> 
>>   ring->wptr = 0;
>>   amdgpu_ring_set_wptr(ring);
>> 
>> instead of writing wptr_cpu_addr directly?
>> 
>> I am asking because amdgpu_ring_set_wptr() also updates the doorbell,
>> so I want to confirm that this is the intended sequence for the
>> reset/suspend case here.
>
>Yeah I was wondering the same thing.
>
>I think the correct approach would be to make both rptr_cpu_addr and wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then cast that to either (u64*) or (u32*) depending on the ring type.
>
>The atomic64_t hack should really be removed.
>
>BTW Reading the rptr is wrong on multiple instances as well and should probably be fixed in the same patch set.
>
>Regards,
>Christian.
>
>> 
>> Thanks,
>> Runyu
>> 
>> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote:
>>> Clear NAK.
>>>
>>> The atomic64_t cast hack is just something we did for older
>>> generations and is not something which is necessary nor should
>>> be done here.
>>>
>>> What could be possible is that we need to use amdgpu_ring_set_wptr()
>>> here to correctly distinguish between queues with 32bit and 64bit
>>> wptrs.
>> 
>
>

Re:Re: [PATCH 1/2] drm/amdgpu/mes11: fix queue init wptr reset

[Runyu Xiao]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

>>
>> Some people who received this message don't often get email from 220255722@seu.edu.cn. Learn why this is important <https://aka.ms/LearnAboutSenderIdentification>
>> 	
>> 
>> Hi Christian,
>> 
>> Thanks, understood.
>> 
>> To make sure I rework this in the right direction: would you expect
>> this reset path to do
>> 
>>   ring->wptr = 0;
>>   amdgpu_ring_set_wptr(ring);
>> 
>> instead of writing wptr_cpu_addr directly?
>> 
>> I am asking because amdgpu_ring_set_wptr() also updates the doorbell,
>> so I want to confirm that this is the intended sequence for the
>> reset/suspend case here.
>
>Yeah I was wondering the same thing.
>
>I think the correct approach would be to make both rptr_cpu_addr and wptr_cpu_addr void* in the amdgpu_ring.h structure instead of u32* and then cast that to either (u64*) or (u32*) depending on the ring type.
>
>The atomic64_t hack should really be removed.
>
>BTW Reading the rptr is wrong on multiple instances as well and should probably be fixed in the same patch set.
>
>Regards,
>Christian.
>


Hi Christian,


Thanks for the guidance.


I reworked the original idea around changing rptr_cpu_addr and
wptr_cpu_addr to void * and using typed 32-bit / 64-bit accesses at the
call sites, rather than using the atomic64_t cast.


The patchset I have in mind is now roughly:


  1. add typed ring writeback helpers
  2. convert ring writeback users to those helpers
  3. make ring rptr_cpu_addr / wptr_cpu_addr typeless
  4. fix the MES queue-init reset path on top of that cleanup


The goal is to stop mixing direct u32-typed accesses with ad hoc 64-bit
casts in the writeback paths, and then handle the MES 64-bit reset case
through the same typed access model.


I also plan to include the related rptr read fixes in the same series,
as you pointed out.


Since this is a bit larger than the original 2-patch fix, I wanted to
check before posting v2 whether this overall direction and split look
reasonable to you, or whether you would prefer a smaller or differently
split series.


If you think any part of this cleanup should be handled differently,
please let me know and I will adjust before posting.


Thanks,
Runyu






>> 
>> Thanks,
>> Runyu
>> 
>> On Tue, Jun 2, 2026 at 11:49:05AM +0200, Christian König wrote:
>>> Clear NAK.
>>>
>>> The atomic64_t cast hack is just something we did for older
>>> generations and is not something which is necessary nor should
>>> be done here.
>>>
>>> What could be possible is that we need to use amdgpu_ring_set_wptr()
>>> here to correctly distinguish between queues with 32bit and 64bit
>>> wptrs.
>> 
>
>


[-- Attachment #2: Type: text/html, Size: 6331 bytes --]

Claude review: drm/amdgpu/mes11: fix queue init wptr reset

[Claude Code Review Bot]

Patch Review

**The change:**
```c
-		*(ring->wptr_cpu_addr) = 0;
+		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
```

At `mes_v11_0.c:1311`, in the reset/suspend path of `mes_v11_0_queue_init()`.

**Correctness:** The fix is correct. The normal writer `mes_v11_0_ring_set_wptr()` (line 79) uses `atomic64_set((atomic64_t *)ring->wptr_cpu_addr, ring->wptr)`, and the reader `mes_v11_0_ring_get_wptr()` (line 97) uses `atomic64_read((atomic64_t *)ring->wptr_cpu_addr)`. Using `atomic64_set()` in the reset path makes the reset consistent with these accessors.

**Fixes tag:** `Fixes: d81d75c99936 ("drm/amdgpu/gfx11: enable kiq to map mes ring")` — this identifies when the queue_init code with the plain store was introduced, which seems reasonable.

**Minor note:** The adjacent `*(ring->rptr_cpu_addr) = 0` is left as a 32-bit store, but this is fine because `mes_v11_0_ring_get_rptr()` (line 89) reads rptr via `return *ring->rptr_cpu_addr` — also a 32-bit access — so the rptr path is internally consistent at 32 bits.

**No issues with this patch.**

---
Generated by Claude Code Patch Reviewer

[PATCH 2/2] drm/amdgpu/mes12: fix queue init wptr reset

[Runyu Xiao]

mes_v12_0_queue_init() resets ring->wptr_cpu_addr with a plain 32-bit
store in the reset/suspend path even though the same carrier is
accessed with atomic64_set()/atomic64_read() and support_64bit_ptrs is
enabled.

This is not just a missing atomic annotation. The MES queue write
pointer is a shared 64-bit carrier, and *ring->wptr_cpu_addr = 0 only
clears the low 32 bits. A later atomic64_read() can then observe stale
high 32 bits instead of a real zeroed reset state.

Use atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0) so the reset
path updates the full 64-bit wptr with the same access family as the
existing readers and writers.

Build-tested by compiling mes_v12_0.o.

No AMDGPU hardware was available for end-to-end runtime testing.

Fixes: 785f0f9fe742 ("drm/amdgpu: Add mes v12_0 ip block support (v4)")
Cc: stable@vger.kernel.org
Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
---
 drivers/gpu/drm/amd/amdgpu/mes_v12_0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c b/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
index 023c7345e..22ed7bb51 100644
--- a/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/mes_v12_0.c
@@ -1476,7 +1476,7 @@ static int mes_v12_0_queue_init(struct amdgpu_device *adev,
 
 	if ((adev->enable_uni_mes || pipe == AMDGPU_MES_SCHED_PIPE) &&
 	    (amdgpu_in_reset(adev) || adev->in_suspend)) {
-		*(ring->wptr_cpu_addr) = 0;
+		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
 		*(ring->rptr_cpu_addr) = 0;
 		amdgpu_ring_clear_ring(ring);
 	}
-- 
2.34.1

Claude review: drm/amdgpu/mes12: fix queue init wptr reset

[Claude Code Review Bot]

Patch Review

**The change:**
```c
-		*(ring->wptr_cpu_addr) = 0;
+		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
```

At `mes_v12_0.c:1479`, in the reset/suspend path of `mes_v12_0_queue_init()`.

**Correctness:** Same analysis as patch 1. The writer `mes_v12_0_ring_set_wptr()` (line 58) uses `atomic64_set()` and the reader `mes_v12_0_ring_get_wptr()` (line 76) uses `atomic64_read()`. The fix aligns the reset path with these.

**Fixes tag:** `Fixes: 785f0f9fe742 ("drm/amdgpu: Add mes v12_0 ip block support (v4)")` — reasonable, identifies when the v12 queue_init was introduced.

**Same observation about rptr:** `mes_v12_0_ring_get_rptr()` (line 67) uses `return *ring->rptr_cpu_addr` — 32-bit read — so leaving the rptr reset as a 32-bit store is consistent.

**No issues with this patch.**

---

**Summary:** Both patches are correct, minimal, and well-scoped. The commit messages are clear and honest about testing limitations (build-tested only, no hardware). The series makes the reset path consistent with the normal read/write accessors. The only thing worth noting to the author is that the cover letter claims this was audited on "Linux v6.18.21" which doesn't exist — v6.18 hasn't had 21 point releases. This is likely a version numbering mistake and doesn't affect the patches themselves.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/amdgpu: fix MES queue init wptr reset on atomic64 carriers

[Claude Code Review Bot]

Overall Series Review

Subject: drm/amdgpu: fix MES queue init wptr reset on atomic64 carriers
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>
Patches: 5
Reviewed: 2026-06-04T13:17:09.171124

---

**Series: drm/amdgpu: fix MES queue init wptr reset on atomic64 carriers (2 patches)**

The premise of this series is that `*(ring->wptr_cpu_addr) = 0` only writes 32 bits because `wptr_cpu_addr` is typed as `u32 *`, while the normal read/write paths use `atomic64_set()`/`atomic64_read()` which treat the same memory as a 64-bit value. The claim that the plain store "only clears the low 32 bits" is **correct** — `wptr_cpu_addr` is `u32 *`, so `*wptr_cpu_addr = 0` is a 32-bit store, leaving the upper 32 bits untouched.

However, **the practical severity is questionable**. The write pointer (`ring->wptr`) is a ring buffer offset. On a reset/suspend path, the ring is being cleared entirely (`amdgpu_ring_clear_ring(ring)`), and the wptr value itself is typically well within 32-bit range for normal operation. The upper 32 bits would only be non-zero after the ring has wrapped an extraordinary number of times (past 4G entries). That said, the fix is trivially correct — using the same access width as the normal read/write paths is strictly better and removes a latent inconsistency.

**The `rptr_cpu_addr` reset on the adjacent line has the same pattern** but is not addressed. In `mes_v11_0` and `mes_v12_0`, `rptr_cpu_addr` is read via `*ring->rptr_cpu_addr` (32-bit read in `ring_get_rptr`), so the 32-bit store to rptr is at least consistent with its reader. The wptr case is different because its reader uses `atomic64_read()`. So the wptr fix is correctly scoped.

**Verdict:** The fix is correct and consistent with the existing access pattern. Low practical risk but a real inconsistency. Appropriate for stable backport if accepted.

---
Generated by Claude Code Patch Reviewer