Re: [PATCH v3 02/10] drm/xe/xe_pci_error: Implement PCI error recovery callbacks

["Mallesh, Koujalagi" <mallesh.koujalagi@intel.com>]

On 13-04-2026 02:30 pm, Tauro, Riana wrote:
>
> On 4/7/2026 10:20 AM, Matthew Brost wrote:
>> On Thu, Apr 02, 2026 at 12:31:33PM +0530, Riana Tauro wrote:
>>> Add error_detected, mmio_enabled, slot_reset and resume
>>> recovery callbacks to handle PCIe Advanced Error Reporting
>>> (AER) errors.
>>>
>>> For fatal errors, the device is wedged and becomes
>>> inaccessible. Return PCI_ERS_RESULT_SLOT_RESET from
>>> error_detected to request a Secondary Bus Reset (SBR).
>>>
>>> For non-fatal errors, return PCI_ERS_RESULT_CAN_RECOVER from
>>> error_detected to trigger the mmio_enabled callback. In this callback,
>>> the device is queried to determine the error cause and attempt
>>> recovery based on the error type.
>>>
>>> Once the secondary bus reset(SBR) is completed the slot_reset callback
>>> cleanly removes and reprobe the device to restore functionality.
>>>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Matt Roper <matthew.d.roper@intel.com>
>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>> ---
>>> v2: re-order linux headers
>>>      reword error messages
>>>      do not clear in_recovery after remove
>>>      return PCI_ERS_RESULT_DISCONNECT if probe fails (Michal)
>>>      only wedge device do not send uevent (Raag)
>>>      set recovery flag in error_detected and clear on resume
>>>      add default switch case (Mallesh)
>>>
>>> v3: do not set in_recovery for disconnect (Mallesh)
>>>      return if already wedged or in survivability mode
>>> ---
>>>   drivers/gpu/drm/xe/Makefile          |   1 +
>>>   drivers/gpu/drm/xe/xe_device.h       |  15 ++++
>>>   drivers/gpu/drm/xe/xe_device_types.h |   3 +
>>>   drivers/gpu/drm/xe/xe_pci.c          |   3 +
>>>   drivers/gpu/drm/xe/xe_pci_error.c    | 104 
>>> +++++++++++++++++++++++++++
>>>   5 files changed, 126 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/xe/xe_pci_error.c
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 9dacb0579a7d..7f03f06df186 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -100,6 +100,7 @@ xe-y += xe_bb.o \
>>>       xe_page_reclaim.o \
>>>       xe_pat.o \
>>>       xe_pci.o \
>>> +    xe_pci_error.o \
>>>       xe_pci_rebar.o \
>>>       xe_pcode.o \
>>>       xe_pm.o \
>>> diff --git a/drivers/gpu/drm/xe/xe_device.h 
>>> b/drivers/gpu/drm/xe/xe_device.h
>>> index e4b9de8d8e95..60db2492cb92 100644
>>> --- a/drivers/gpu/drm/xe/xe_device.h
>>> +++ b/drivers/gpu/drm/xe/xe_device.h
>>> @@ -43,6 +43,21 @@ static inline struct xe_device 
>>> *ttm_to_xe_device(struct ttm_device *ttm)
>>>       return container_of(ttm, struct xe_device, ttm);
>>>   }
>>>   +static inline bool xe_device_is_in_recovery(struct xe_device *xe)
>>> +{
>>> +    return atomic_read(&xe->in_recovery);
>>> +}
>>> +
>>> +static inline void xe_device_set_in_recovery(struct xe_device *xe)
>>> +{
>>> +    atomic_set(&xe->in_recovery, 1);
>>> +}
>>> +
>>> +static inline void xe_device_clear_in_recovery(struct xe_device *xe)
>>> +{
>>> +     atomic_set(&xe->in_recovery, 0);
nit: Remove white space
>>> +}
>>> +
>>>   struct xe_device *xe_device_create(struct pci_dev *pdev,
>>>                      const struct pci_device_id *ent);
>>>   int xe_device_probe_early(struct xe_device *xe);
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
>>> b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 150c76b2acaf..c9fe86b670bd 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -494,6 +494,9 @@ struct xe_device {
>>>           bool inconsistent_reset;
>>>       } wedged;
>>>   +    /** @in_recovery: Indicates if device is in recovery */
>>> +    atomic_t in_recovery;
>>> +
>>>       /** @bo_device: Struct to control async free of BOs */
>>>       struct xe_bo_dev {
>>>           /** @bo_device.async_free: Free worker */
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index 1df3f08e2e1c..30d71795dd2e 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -1323,6 +1323,8 @@ static const struct dev_pm_ops xe_pm_ops = {
>>>   };
>>>   #endif
>>>   +extern const struct pci_error_handlers xe_pci_error_handlers;
>>> +
>>>   static struct pci_driver xe_pci_driver = {
>>>       .name = DRIVER_NAME,
>>>       .id_table = pciidlist,
>>> @@ -1330,6 +1332,7 @@ static struct pci_driver xe_pci_driver = {
>>>       .remove = xe_pci_remove,
>>>       .shutdown = xe_pci_shutdown,
>>>       .sriov_configure = xe_pci_sriov_configure,
>>> +    .err_handler = &xe_pci_error_handlers,
>>>   #ifdef CONFIG_PM_SLEEP
>>>       .driver.pm = &xe_pm_ops,
>>>   #endif
>>> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c 
>>> b/drivers/gpu/drm/xe/xe_pci_error.c
>>> new file mode 100644
>>> index 000000000000..cd9f39010278
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
>>> @@ -0,0 +1,104 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2026 Intel Corporation
>>> + */
>>> +#include <linux/pci.h>
>>> +
>>> +#include <drm/drm_drv.h>
>>> +
>>> +#include "xe_device.h"
>>> +#include "xe_gt.h"
>>> +#include "xe_pci.h"
>>> +#include "xe_survivability_mode.h"
>>> +#include "xe_uc.h"
>>> +
>>> +static void xe_pci_error_handling(struct pci_dev *pdev)
>>> +{
>>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>>> +    struct xe_gt *gt;
>>> +    u8 id;
>>> +
>>> +    /* Return if device is wedged or in survivability mode */
>>> +    if (xe_survivability_mode_is_boot_enabled(xe) || 
>>> xe_device_wedged(xe))
>>> +        return;
>>> +
>>> +    /* Wedge the device to prevent userspace access but don't send 
>>> the event yet */
>>> +    atomic_set(&xe->wedged.flag, 1);
>> We can't blindly set '&xe->wedged.flag, 1' as this is tied to a PM ref
>> [1], [2]. The existing sematic might be wrong but we to normalize
>> adjustmets to the '&xe->wedged.flag' field with uniform rules, or the
>> cases when we wedge we also take a PM ref
>>
>
> If the device was already wedged from xe_device_declare_wedged, this 
> function returns.
> And the ref is released in fini.
>
> PM ref was added to prevent runtime suspend during wedging. But in 
> case of error_callbacks
> this is already taken by PCI core drivers/pci/pcie/err.c
>
> pci_walk_bridge(bridge, pci_pm_runtime_get_sync, NULL);
>
> I will add a comment here.
>
> Thanks
> Riana
>
>>   Matt
>>
>> [1] https://patchwork.freedesktop.org/patch/714622/?series=163948&rev=1
>> [2] 
>> https://patchwork.freedesktop.org/patch/715028/?series=162055&rev=4#comment_1315905
>>
>>> +
>>> +    for_each_gt(gt, xe, id)
>>> +        xe_gt_declare_wedged(gt);
>>> +
>>> +    pci_disable_device(pdev);
>>> +}
>>> +
>>> +static pci_ers_result_t xe_pci_error_detected(struct pci_dev *pdev, 
>>> pci_channel_state_t state)
>>> +{
>>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>>> +
>>> +    dev_err(&pdev->dev, "Xe Pci error recovery: error detected 
>>> state %d\n", state);
>>> +
>>> +    if (state == pci_channel_io_perm_failure)
>>> +        return PCI_ERS_RESULT_DISCONNECT;
>>> +
>>> +    xe_device_set_in_recovery(xe);
>>> +
>>> +    switch (state) {
>>> +    case pci_channel_io_normal:
>>> +        return PCI_ERS_RESULT_CAN_RECOVER;
>>> +    case pci_channel_io_frozen:
>>> +        xe_pci_error_handling(pdev);
>>> +        return PCI_ERS_RESULT_NEED_RESET;
>>> +    default:
>>> +        dev_err(&pdev->dev, "Unknown state %d\n", state);
>>> +        return PCI_ERS_RESULT_NEED_RESET;
>>> +    }
>>> +}
>>> +
>>> +static pci_ers_result_t xe_pci_error_mmio_enabled(struct pci_dev 
>>> *pdev)
>>> +{
>>> +    dev_err(&pdev->dev, "Xe Pci error recovery: MMIO enabled\n");
>>> +
>>> +    return PCI_ERS_RESULT_NEED_RESET;
>>> +}
>>> +
>>> +static pci_ers_result_t xe_pci_error_slot_reset(struct pci_dev *pdev)
>>> +{
>>> +    const struct pci_device_id *ent = 
>>> pci_match_id(pdev->driver->id_table, pdev);
>>> +
>>> +    dev_err(&pdev->dev, "Xe Pci error recovery: Slot reset\n");
>>> +
>>> +    pci_restore_state(pdev);

Is pci_restore_state() needed here? before invoking slot_reset, the PCI 
core already calls

pci_restore_state() right.

Thanks,

-/Mallesh

>>> +
>>> +    if (pci_enable_device(pdev)) {
>>> +        dev_err(&pdev->dev,
>>> +            "Cannot re-enable PCI device after reset\n");
>>> +        return PCI_ERS_RESULT_DISCONNECT;
>>> +    }
>>> +
>>> +    /*
>>> +     * Secondary Bus Reset wipes out all device memory
>>> +     * requiring XE KMD to perform a device removal and reprobe.
>>> +     */
>>> +    pdev->driver->remove(pdev);
>>> +
>>> +    if (!pdev->driver->probe(pdev, ent))
>>> +        return PCI_ERS_RESULT_RECOVERED;
>>> +
>>> +    return PCI_ERS_RESULT_DISCONNECT;
>>> +}
>>> +
>>> +static void xe_pci_error_resume(struct pci_dev *pdev)
>>> +{
>>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>>> +
>>> +    dev_info(&pdev->dev, "Xe Pci error recovery: Recovered\n");
>>> +
>>> +    xe_device_clear_in_recovery(xe);
>>> +}
>>> +
>>> +const struct pci_error_handlers xe_pci_error_handlers = {
>>> +    .error_detected    = xe_pci_error_detected,
>>> +    .mmio_enabled    = xe_pci_error_mmio_enabled,
>>> +    .slot_reset    = xe_pci_error_slot_reset,
>>> +    .resume        = xe_pci_error_resume,
>>> +};
>>> -- 
>>> 2.47.1
>>>