Re: [PATCH v5 2/5] drm/xe: Drop xe_device_set_wedged_method()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: sleeping in atomic context in xe_mert_irq_handler
- [Low] drm/xe: Stale kernel-doc for xe_device_declare_wedged()
--

commit 92170b825df41bc94e7e282489a7558acba677d1
Author: Raag Jadav <raag.jadav@intel.com>

drm/xe: Drop xe_device_set_wedged_method()

Commit 60439ac3f235 ("drm/xe: Add a helper function to set recovery
method") introduced xe_device_set_wedged_method() helper to allow setting
preferred recovery method before wedging the device, but also introduced
a flaw where the previously set method can be overwritten by concurrent
racing wedge call. This can result in unintended recovery method to be
sent to userspace and possibly failed recovery.

Drop xe_device_set_wedged_method() entirely and pass the recovery method
as part of xe_device_declare_wedged() argument, which places the preferred
recovery method on stack and avoids this issue.

Fixes: 60439ac3f235 ("drm/xe: Add a helper function to set recovery method")
Signed-off-by: Raag Jadav <raag.jadav@intel.com>

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 014bc5b0216dd..5b98b35cdbe8a 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -1371,24 +1371,13 @@ u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address)
>   *   firmware and restore device to normal operation.
>   */
>  
> -/**
> - * xe_device_set_wedged_method - Set wedged recovery method
> - * @xe: xe device instance
> - * @method: recovery method to set
> - *
> - * Set wedged recovery method to be sent in drm wedged uevent.
> - */
> -void xe_device_set_wedged_method(struct xe_device *xe, unsigned long method)
> -{
> -	xe->wedged.method = method;
> -}
> -
>  #define WEDGED_URL	"https://docs.kernel.org/gpu/drm-uapi.html#device-wedging"
>  #define XE_BUG_URL	"https://gitlab.freedesktop.org/drm/xe/kernel/issues/new"
>  
>  /**
>   * xe_device_declare_wedged - Declare device wedged
>   * @xe: xe device instance
> + * @method: method(s) to be used for recovery
>   *
>   * This is a final state that can only be cleared with the recovery method
>   * specified in the drm wedged uevent. The method can be set using

[Severity: Low]
Does this kernel-doc need to be updated to match the new behavior?

The documentation block for xe_device_declare_wedged() still instructs
developers to use the deleted xe_device_set_wedged_method() function and
describes a removed fallback behavior.

> @@ -1404,7 +1393,7 @@ void xe_device_set_wedged_method(struct xe_device *xe, unsigned long method)
>   * snapshot capture. In this mode, GT reset won't be attempted so the state of
>   * the issue is preserved for further debugging.
>   */
> -void xe_device_declare_wedged(struct xe_device *xe)
> +void xe_device_declare_wedged(struct xe_device *xe, unsigned long method)
>  {
>  	struct xe_gt *gt;
>  	u8 id;

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_mert.c b/drivers/gpu/drm/xe/xe_mert.c
> index 32700c19a1df3..934be933d57f7 100644
> --- a/drivers/gpu/drm/xe/xe_mert.c
> +++ b/drivers/gpu/drm/xe/xe_mert.c
> @@ -74,7 +74,8 @@ static void mert_handle_cat_error(struct xe_device *xe)
>  		break;
>  	case CATERR_UNMAPPED_GGTT:
>  		xe_sriov_err(xe, "MERT: CAT_ERR: Access to an unmapped GGTT!\n");
> -		xe_device_declare_wedged(xe);
> +		xe_device_declare_wedged(xe,
> +					 DRM_WEDGE_RECOVERY_REBIND | DRM_WEDGE_RECOVERY_BUS_RESET);

[Severity: High]
Does this code sleep in an atomic context?

The mert_handle_cat_error() function is executed from the top-half hard IRQ
handler xe_mert_irq_handler() (via dg1_irq_handler()).

When a CATERR_UNMAPPED_GGTT or unexpected error code is detected, it calls
xe_device_declare_wedged(). This function performs synchronous wait
operations by calling xe_gt_declare_wedged(), which blocks on
flush_work(&gt->wedge.worker). It also allocates memory with GFP_KERNEL via
drm_dev_wedged_event() -> kobject_uevent_env().

Executing these blocking operations in a hardirq context triggers a
BUG: scheduling while atomic panic.

>  		break;
>  	case CATERR_LMTT_FAULT:
>  		xe_sriov_dbg_ratelimited(xe, "MERT: CAT_ERR: VF%u LMTT fault!\n", vfid);
> @@ -82,7 +83,8 @@ static void mert_handle_cat_error(struct xe_device *xe)
>  		break;
>  	default:
>  		xe_sriov_err(xe, "MERT: Unexpected CAT_ERR code=%#x!\n", code);
> -		xe_device_declare_wedged(xe);
> +		xe_device_declare_wedged(xe,
> +					 DRM_WEDGE_RECOVERY_REBIND | DRM_WEDGE_RECOVERY_BUS_RESET);
>  		break;
>  	}
>  }

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