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

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

[-- Attachment #1: Type: text/plain, Size: 11415 bytes --]


On 16-04-2026 10:18 am, Tauro, Riana wrote:
>
> 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.

Yes, you’re absolutely right. In the AER recovery path, the 
responsibility to call pci_restore_state() lies with the driver, and in 
our case the reset happens through the AER path.
In contrast, for non‑AER paths, the PCI core itself handles state 
restoration via pci_dev_restore() (which internally calls 
pci_restore_state()).

Thanks,

-/Mallesh

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

[-- Attachment #2: Type: text/html, Size: 22739 bytes --]