Re: [PATCH v10 14/27] drm/xe/eudebug: Add hw enablement

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The overwrite logic in `xe_reg_sr_add` fails to clear previously set bits when `e->set_bits` is non-zero, corrupting register state.
- [Low] Redundant duplicate macro definition of `STALL_DOP_GATING_DISABLE`.
--

commit 5a2461057e026a59c99e9e1133f757b575f16e78
Author: Dominik Grzegorzek <dominik.grzegorzek@intel.com>
Subject: drm/xe/eudebug: Add hw enablement

This commit implements eudebug mode enabling by programming TD_CTL and other
registers. It introduces eudebug-related workarounds and modifies the
register save/restore logic to allow dynamic manipulation with overwrites.

> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 8056d007c23e2..7537fc1a9c3fa 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
[ ... ]
> @@ -546,11 +558,13 @@
>  #define   MDQ_ARBITRATION_MODE			REG_BIT(12)
>  #define   STALL_DOP_GATING_DISABLE		REG_BIT(5)
>  #define   EARLY_EOT_DIS				REG_BIT(1)
> +#define   STALL_DOP_GATING_DISABLE		REG_BIT(5)

[Severity: Low]
Does this code inadvertently redefine STALL_DOP_GATING_DISABLE? This isn't a
bug, but the macro is redefined with the exact same value immediately after
its original definition.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_reg_sr.c b/drivers/gpu/drm/xe/xe_reg_sr.c
> index e328f50725572..72919a5612a5a 100644
> --- a/drivers/gpu/drm/xe/xe_reg_sr.c
> +++ b/drivers/gpu/drm/xe/xe_reg_sr.c
[ ... ]
> @@ -114,15 +115,20 @@ int xe_reg_sr_add(struct xe_reg_sr *sr,
>  	reg = sanitize_mcr(sr, e, gt);
>  
>  	if (pentry) {
> -		if (!compatible_entries(pentry, e)) {
> +		if (!overwrite && !compatible_entries(pentry, e)) {
>  			ret = -EINVAL;
>  			goto fail;
>  		}
>  
>  		pentry->clr_bits |= e->clr_bits;
> -		pentry->set_bits |= e->set_bits;
>  		pentry->read_mask |= e->read_mask;
>  
> +		/* An overwrite with nothing to set clears instead */
> +		if (overwrite && !e->set_bits)
> +			pentry->set_bits &= ~e->clr_bits;
> +		else
> +			pentry->set_bits |= e->set_bits;
> +
>  		return 0;
>  	}

[Severity: High]
Does this logic bypass clearing e->clr_bits when e->set_bits is non-zero
during an overwrite in xe_reg_sr_add()?

If xe_reg_sr_add() is called with overwrite = true and e->set_bits != 0, the
old bits intersecting with e->clr_bits do not appear to be cleared from
pentry->set_bits before the new bits are logically OR-ed. Could this result
in hardware registers being programmed with corrupted, logically OR-ed values
instead of the requested explicit values?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903145952.848051-1-mika.kuoppala@linux.intel.com?part=14