Re: [PATCH v2 4/9] drm/xe/xe_hw_error: Handle non-fatal errors routed via direct IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe: ignored recovery action in hw_error_work() and premature DB_ECC wedge
- [High] drm/xe: race between hw_error_work() and PCIe AER reset and removal paths
--

commit 599d1ef3076a9398588bb132729ba7fcc7f8f463
Author: Riana Tauro <riana.tauro@intel.com>

drm/xe/xe_hw_error: Handle non-fatal errors routed via direct IRQ

This patch schedules hw_error_work() when non-fatal hardware errors are
routed via direct IRQ on platforms with a system controller, invokes
xe_ras_process_errors() from the worker, and declares the device wedged
with a bus reset if double-bit ECC page offlining fails.

> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c b/drivers/gpu/drm/xe/xe_hw_error.c
> index f7799c3c08aac..269ce6a19a80b 100644
> --- a/drivers/gpu/drm/xe/xe_hw_error.c
> +++ b/drivers/gpu/drm/xe/xe_hw_error.c

[ ... ]

> @@ -166,12 +170,36 @@ static_assert(ARRAY_SIZE(pvc_master_local_nonfatal_err_reg) == XE_RAS_REG_SIZE);

[ ... ]

>  static void hw_error_work(struct work_struct *work)
>  {
>  	struct xe_tile *tile = container_of(work, typeof(*tile), hw_error_work.work);
>  	struct xe_device *xe = tile_to_xe(tile);
>  
>  	guard(xe_pm_runtime)(xe);
> +
> +	if (xe->info.has_sysctrl && XE_DEVICE_WA(xe, 14028434957)) {
> +		/* If the device is in reset in AER path, skip processing hardware errors */
> +		if (xe_device_is_in_reset(xe))
> +			return;

[Severity: High]
Can hw_error_work() race with the PCIe AER reset and removal paths here?

In hw_error_source_handler(), a non-fatal hardware error IRQ schedules
tile->hw_error_work with a 1-second delay, and hw_error_work() checks
xe_device_is_in_reset(xe) without synchronization before calling
xe_ras_process_errors(xe).

Although the later commit "drm/xe/xe_ras: Disable hw error worker when AER
handling is in progress" calls xe_hw_error_work_disable(xe) in
xe_pci_error_detected(), that call is gated on:

    if (XE_DEVICE_WA(xe, 14028434957) && is_aer_internal_error(pdev))

which only checks PCI_ERR_UNC_INTN on the upstream switch port, and is
also skipped if xe_wedged(xe) is already true.

When an AER event occurs without PCI_ERR_UNC_INTN set on the upstream
switch port (such as a fatal link or protocol error with
state == pci_channel_io_frozen, or a non-fatal error where
xe_pci_error_mmio_enabled() runs xe_ras_process_errors(xe) and triggers
XE_RAS_RECOVERY_ACTION_RESET), xe_hw_error_work_disable(xe) is not called:

1. In drivers/gpu/drm/xe/xe_pci_error.c:prepare_device_for_reset():

    xe_device_set_in_reset(xe);

    for_each_gt(gt, xe, id) {
        xe_gt_wedge_declare(gt);
        flush_work(&gt->wedge.worker);
    }

    pci_disable_device(pdev);

If hw_error_work() passes the xe_device_is_in_reset(xe) check just before
xe_device_set_in_reset(xe) is called, xe_ras_process_errors(xe) can run
system controller MMIO mailbox transactions and TTM VRAM page offlining
concurrently with pci_disable_device(pdev) and secondary bus reset.

2. In 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);

Because xe_device_clear_in_reset(xe) clears xe->in_reset before
pdev->driver->remove(pdev) and devres_release_group() run, if the delayed
tile->hw_error_work fires in this window, xe_device_is_in_reset(xe) will
return false and xe_ras_process_errors(xe) will execute concurrently with
driver teardown.

Should prepare_device_for_reset() also cancel or flush tile->hw_error_work,
and should in_reset remain set across removal in xe_pci_error_slot_reset()?

> +
> +		xe_ras_process_errors(xe);
> +		return;
> +	}
> +
>  	xe_survivability_mode_runtime_enable(xe);
>  }

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index db3cbc186a514..e6a0ee468b17b 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
> @@ -628,8 +628,15 @@ static u8 handle_device_memory_errors(struct xe_device *xe, struct xe_ras_error_
>  		break;
>  	case XE_RAS_MEMORY_DB_ECC:
>  		ret = handle_page_offline(xe, info->sw_address, true);
> -		if (ret)
> +		/*
> +		 * Double-bit ECC errors are non-fatal errors and are reported via IRQ.
> +		 * Wedge the device with a bus reset for errors that require SBR to recover.
> +		 * TODO: Trigger SBR using pci_reset_bus
> +		 */
> +		if (ret) {
> +			xe_wedge_declare(xe, DRM_WEDGE_RECOVERY_BUS_RESET);
>  			return XE_RAS_RECOVERY_ACTION_RESET;
> +		}

[Severity: High]
Should theDRM_WEDGE_RECOVERY_BUS_RESET wedging be performed by the caller
based on the enum xe_ras_recovery_action return value of
xe_ras_process_errors(xe) in hw_error_work(), rather than directly inside
handle_device_memory_errors()?

Right now, hw_error_work() in drivers/gpu/drm/xe/xe_hw_error.c calls
xe_ras_process_errors(xe) and ignores its return value. Any error processed
on the direct IRQ path that returns XE_RAS_RECOVERY_ACTION_RESET outside of
XE_RAS_MEMORY_DB_ECC (such as GLOBAL_UNCORR_ERROR in
handle_core_compute_errors(), non-CSC/non-Punit errors in
handle_soc_internal_errors(), other device memory error categories, unknown
RAS components, or a sysctrl mailbox failure/flood jumping to err:) will
not trigger any reset or wedge recovery action.

Conversely, calling xe_wedge_declare(xe, DRM_WEDGE_RECOVERY_BUS_RESET)
directly inside handle_device_memory_errors() affects all callers of
xe_ras_process_errors(xe):

1. Multi-error severity aggregation in xe_ras_process_errors():

    /* Process and log all errors and then trigger highest recovery action */
    if (action > final_action)
        final_action = action;

Because xe_wedge_declare() is one-shot via atomic_xchg(&xe->wedged.flag, 1),
if a XE_RAS_MEMORY_DB_ECC entry with a failing handle_page_offline() is
processed before a higher-severity XE_RAS_RECOVERY_ACTION_DISCONNECT entry
in the batch (such as a Punit error requiring DRM_WEDGE_RECOVERY_COLD_RESET
in punit_error_handler() or a CSC firmware error requiring
DRM_WEDGE_RECOVERY_VENDOR in xe_survivability_mode_runtime_enable()),
xe->wedged.method is latched to DRM_WEDGE_RECOVERY_BUS_RESET and the
higher-severity recovery method is masked.

2. Driver probe path via xe_device_probe() -> xe_ras_init() ->
xe_ras_process_errors(xe):
This runs before xe_gt_init() and drm_dev_register() to log boot-time
errors. If a boot-time XE_RAS_MEMORY_DB_ECC error fails
handle_page_offline() (for example with -EIO on a critical BO/stolen page,
-ENOMEM, or -ENOSPC), handle_device_memory_errors() calls
xe_wedge_declare(), scheduling drm_dev_wedged_event() before
drm_dev_register() and setting xe->wedged.flag, which causes
__guc_ct_send_locked() in xe_gt_init() to fail with -ENOTRECOVERABLE and
aborts probe.

3. PCIe AER path in xe_pci_error_mmio_enabled():
In the AER path, XE_RAS_RECOVERY_ACTION_RESET is handled by
ras_action_to_pci_result() -> prepare_device_for_reset(), which sets
wedged.flag without sending a wedged uevent or setting needs_flr_on_fini
because AER performs an automatic slot reset. Although the later commit
"drm/xe/ras: Skip double-bit ECC page offlining on AER path" skips DB_ECC
when ras->aer_path is set for WA 14028434957 internal errors, ras->aer_path
remains false on non-internal AER errors and on platforms without
WA 14028434957, where calling xe_wedge_declare() emits an unintended
WEDGED=bus-reset uevent and sets xe->needs_flr_on_fini = true.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026060459.771448-11-riana.tauro@intel.com?part=4