Re: [PATCH v6 6/7] drm/xe: Introduce xe_wedge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe: data race and inconsistent GT state in xe_wedge_set_mode()
- [Low] perf: xe_wedged() out-of-lining adds function call overhead in hot paths
--

commit 6fd8783d76a985325bbdd99f638c5f6528cd1918
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe: Introduce xe_wedge

This commit consolidates the wedging implementation into a dedicated xe_wedge
component for better maintainability. It moves the wedged state tracking and
related helpers out of xe_device and renames them to match the new component
namespace.

> diff --git a/drivers/gpu/drm/xe/xe_wedge.c b/drivers/gpu/drm/xe/xe_wedge.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_wedge.c

[ ... ]

> +/**
> + * xe_wedged() - Check for wedged device
> + * @xe: xe device instance
> + *
> + * Returns: %true if device is wedged, %false otherwise.
> + */
> +bool xe_wedged(struct xe_device *xe)
> +{
> +	return atomic_read(&xe->wedged.flag);
> +}

[Severity: Low]
Does moving this trivial atomic read out-of-line introduce unnecessary
overhead?

Hot paths like xe_bo_cpu_fault() and g2h_read() check xe_wedged() on every
invocation. Previously, xe_device_wedged() was a static inline function in
a header, allowing the compiler to optimize it. Moving it to xe_wedge.c
forces a function call for a single memory read. Should this be moved back
to a header file as a static inline?

[ ... ]

> +static int wedge_mode_set_reset_policy(struct xe_device *xe, enum xe_wedged_mode mode)
> +{
> +	struct xe_gt *gt;
> +	int ret;
> +	u8 id;
> +
> +	guard(xe_pm_runtime)(xe);
> +	for_each_gt(gt, xe, id) {
> +		ret = __wedge_mode_set_reset_policy(gt, mode);
> +		if (ret) {
> +			if (id > 0) {
> +				xe->wedged.inconsistent_reset = true;
> +				xe_err(xe, "Inconsistent reset policy state between GTs\n");
> +			}
> +			return ret;
> +		}
> +	}
> +
> +	xe->wedged.inconsistent_reset = false;
> +
> +	return 0;
> +}

[Severity: Medium]
Is there a risk of concurrent writers leaving the GTs in an inconsistent
state here?

Multiple user-space threads could simultaneously write conflicting modes to
/sys/kernel/debug/dri/*/wedged_mode, as the fops->write handler for debugfs
does not serialize concurrent invocations. If their updates to
__wedge_mode_set_reset_policy() interleave, it could leave hardware in a
mixed state (e.g., one GT configured for engine resets and another not)
while still erroneously clearing the xe->wedged.inconsistent_reset flag.

[ ... ]

> +int xe_wedge_set_mode(struct xe_device *xe, enum xe_wedged_mode mode)
> +{
> +	int ret;
> +
> +	ret = wedge_validate_mode(xe, mode);
> +	if (ret)
> +		return ret;
> +
> +	if (wedge_mode_needs_policy_update(xe, mode)) {
> +		ret = wedge_mode_set_reset_policy(xe, mode);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	xe->wedged.mode = mode;

[Severity: Medium]
Could this assignment cause a data race?

Without a lock protecting xe->wedged.mode, concurrent writes could result
in the stored mode no longer matching the actual hardware state set on the
GTs. Should these updates be synchronized to protect both the variable and
the GT policy updates?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014055215.330408-1-raag.jadav@intel.com?part=6