Re: [PATCH v14 8/9] drm/xe: Introduce xe_wedge

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: xe_wedge_init() registers dev-bound cleanup via drmm instead of devm
--

commit 57c0533228e6ea2dcc2823e52de07399929877cf
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe: Introduce xe_wedge

This commit consolidates the device wedging implementation into a dedicated
xe_wedge component and renames the wedge helper functions to match the
component namespace.

> diff --git a/drivers/gpu/drm/xe/xe_wedge.c b/drivers/gpu/drm/xe/xe_wedge.c
> new file mode 100644
> index 0000000000000..2f35937c5e661
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_wedge.c
> @@ -0,0 +1,262 @@

[ ... ]

> +static void xe_wedge_fini(struct drm_device *drm, void *arg)
> +{
> +	struct xe_device *xe = arg;
> +
> +	disable_work_sync(&xe->wedged.work);
> +
> +	if (atomic_read(&xe->wedged.flag))
> +		xe_pm_runtime_put(xe);
> +}
> +
> +/**
> + * xe_wedge_init() - Initialize wedge
> + * @xe: xe device instance
> + *
> + * Returns: 0 on success, negative error code otherwise.
> + */
> +int xe_wedge_init(struct xe_device *xe)
> +{
> +	INIT_WORK(&xe->wedged.work, wedge_work);
> +
> +	xe->wedged.mode = wedge_validate_mode(xe, xe_modparam.wedged_mode) ?
> +			  XE_DEFAULT_WEDGED_MODE : xe_modparam.wedged_mode;
> +	xe_dbg(xe, "wedged_mode: setting mode (%u) %s\n",
> +	       xe->wedged.mode, wedge_mode_to_string(xe->wedged.mode));
> +
> +	return drmm_add_action_or_reset(&xe->drm, xe_wedge_fini, xe);
> +}

[Severity: Medium]
Should xe_wedge_init() in drivers/gpu/drm/xe/xe_wedge.c register
xe_wedge_fini() via devm_add_action_or_reset(xe->drm.dev, ...) rather than
drmm_add_action_or_reset(&xe->drm, ...)?

When the device is wedged via xe_wedge_declare() or
prepare_device_for_reset() in drivers/gpu/drm/xe/xe_pci_error.c,
xe->wedged.flag is set to 1, xe_pm_runtime_get_noresume(xe) increments the
runtime pm usage count on the underlying pci device (xe->drm.dev), and
xe_wedge_declare() schedules xe->wedged.work:

drivers/gpu/drm/xe/xe_pci_error.c:prepare_device_for_reset() {
	if (!atomic_xchg(&xe->wedged.flag, 1))
		xe_pm_runtime_get_noresume(xe);
}

If any userspace process still holds an open drm file descriptor (or a
dma-buf reference) when the driver is unbound for recovery
(DRM_WEDGE_RECOVERY_REBIND) or when xe_pci_error_slot_reset() runs:

drivers/gpu/drm/xe/xe_pci_error.c:xe_pci_error_slot_reset() {
	...
	xe_device_clear_in_reset(xe);
	pdev->driver->remove(pdev);
	devres_release_group(&pdev->dev, xe->devres_group);

	if (pdev->driver->probe(pdev, ent))
		return PCI_ERS_RESULT_DISCONNECT;
	...
}

the open drm file keeps a reference on xe->drm, so drm_managed_release() and
xe_wedge_fini() do not run during driver teardown.

Can this delay disable_work_sync(&xe->wedged.work) past drm_dev_unplug() and
leak the runtime pm reference on pdev->dev across driver unbind or slot
reset, only decrementing pdev->dev.power.usage_count later when the stale
drm file descriptor is closed?

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