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

["Tauro, Riana" <riana.tauro@intel.com>]

On 4/14/2026 6:59 PM, Mallesh, Koujalagi wrote:
>
> 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

I have already responded to this in v1. [2/8] drm/xe/xe_pci_error: 
Implement PCI error recovery callbacks - Patchwork 
<https://patchwork.freedesktop.org/patch/700296/?series=160482&rev=1>.

We have seen issues if we don't restore it after SBR. Maybe i missed 
something.
Can you please point to the line of code? I don't see it in 
report_slot_reset or aer_root_reset.

err.c - drivers/pci/pcie/err.c - Linux source code v7.0 - Bootlin Elixir 
Cross Referencer

<https://elixir.bootlin.com/linux/v7.0/source/drivers/pci/pcie/err.c#L148>Thanks
Riana

>
> 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
>>>>