[PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate

[PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The expression:
    MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB) ?: 0;

is parsed as:
    (MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW |
     (flush_flags & MI_INVALIDATE_TLB)) ?: 0;

Since the combined constant flags are always non-zero, the GNU extension
'?: 0' acts as a no-op and does not change the value. Remove this redundant
and confusing logic.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/gpu/drm/xe/xe_ring_ops.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c b/drivers/gpu/drm/xe/xe_ring_ops.c
index cfeb4fc..ee468d3 100644
--- a/drivers/gpu/drm/xe/xe_ring_ops.c
+++ b/drivers/gpu/drm/xe/xe_ring_ops.c
@@ -144,7 +144,7 @@ static int emit_bb_start(u64 batch_addr, u32 ppgtt_flag, u32 *dw, int i)
 static int emit_flush_invalidate(u32 addr, u32 val, u32 flush_flags, u32 *dw, int i)
 {
 	dw[i++] = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW |
-		  MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB) ?: 0;
+		  MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB);
 
 	dw[i++] = addr | MI_FLUSH_DW_USE_GTT;
 	dw[i++] = 0;
-- 
2.9.4

Re: [PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate

[Thomas Hellström]

On Wed, 2026-05-13 at 07:37 -0400, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> The expression:
>     MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB) ?: 0;
> 
> is parsed as:
>     (MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW |
>      (flush_flags & MI_INVALIDATE_TLB)) ?: 0;
> 
> Since the combined constant flags are always non-zero, the GNU
> extension
> '?: 0' acts as a no-op and does not change the value. Remove this
> redundant
> and confusing logic.
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>

Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Will push to drm-xe-next once CI is clean.


> ---
>  drivers/gpu/drm/xe/xe_ring_ops.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ring_ops.c
> b/drivers/gpu/drm/xe/xe_ring_ops.c
> index cfeb4fc..ee468d3 100644
> --- a/drivers/gpu/drm/xe/xe_ring_ops.c
> +++ b/drivers/gpu/drm/xe/xe_ring_ops.c
> @@ -144,7 +144,7 @@ static int emit_bb_start(u64 batch_addr, u32
> ppgtt_flag, u32 *dw, int i)
>  static int emit_flush_invalidate(u32 addr, u32 val, u32 flush_flags,
> u32 *dw, int i)
>  {
>  	dw[i++] = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW |
> -		  MI_FLUSH_IMM_DW | (flush_flags &
> MI_INVALIDATE_TLB) ?: 0;
> +		  MI_FLUSH_IMM_DW | (flush_flags &
> MI_INVALIDATE_TLB);
>  
>  	dw[i++] = addr | MI_FLUSH_DW_USE_GTT;
>  	dw[i++] = 0;

Claude review: Re: [PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate

[Claude Code Review Bot]

Overall Series Review

Subject: Re: [PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate
Author: Thomas =?ISO-8859-1?Q?Hellstr=F6m?= <thomas.hellstrom@linux.intel.com>
Patches: 2
Reviewed: 2026-05-16T11:45:23.554758

---

This is a single-patch series that removes a redundant GNU ternary extension (`?: 0`) from `emit_flush_invalidate()` in the Xe ring ops code. The patch is **correct** — the `?: 0` is provably a no-op regardless of how you interpret the author's original intent, and removing it improves readability.

The commit message's operator precedence analysis is accurate. The fix is trivially safe with no functional change.

**Verdict: Looks good to merge.**

---
Generated by Claude Code Patch Reviewer

Claude review: Re: [PATCH] drm/xe: Fix operator precedence bug in emit_flush_invalidate

[Claude Code Review Bot]

Patch Review

**Analysis of the original code:**

```c
dw[i++] = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW |
          MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB) ?: 0;
```

The GNU `?:` (Elvis operator) has the same precedence as the standard ternary `?:`, which is **lower** than bitwise OR `|`. So the expression parses as:

```c
(MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW | MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB)) ?: 0;
```

The left operand of `?:` includes the constant bits `MI_FLUSH_DW` (`__MI_INSTR(0x26)`), `MI_FLUSH_DW_OP_STOREDW` (`REG_BIT(14)`), and `MI_FLUSH_IMM_DW` — all non-zero. Therefore the combined expression is **always** non-zero, the `?: 0` fallback never triggers, and it's a no-op exactly as the commit message states.

**Even under the "generous" interpretation** — if the original author intended `(flush_flags & MI_INVALIDATE_TLB) ?: 0` to be grouped together — that would still be a no-op, since `x ?: 0` is always equal to `x` for any value: non-zero returns itself, zero returns zero.

**The fix:**

```c
dw[i++] = MI_FLUSH_DW | MI_FLUSH_DW_OP_STOREDW |
          MI_FLUSH_IMM_DW | (flush_flags & MI_INVALIDATE_TLB);
```

This is correct and strictly equivalent. No functional change.

**Minor nit on the commit message:** The subject says "Fix operator precedence bug" but this is more accurately a cleanup of dead/redundant code — the precedence quirk doesn't cause any incorrect behavior since the `?: 0` is a no-op either way. Calling it a "bug" is slightly misleading since there is no behavioral change. A subject like "Remove redundant ?: 0 in emit_flush_invalidate" would be more precise, but this is a stylistic point, not a blocker.

**Reviewed-by worthy:** Yes. The patch is safe, correct, and improves readability.

---
Generated by Claude Code Patch Reviewer