Re: [PATCH 2/8] drm/xe/xe_pci_error: Implement PCI error recovery callbacks

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

On 2/2/2026 6:49 PM, Nilawar, Badal wrote:
> Added few more comments
> 
> On 29-01-2026 14:39, Nilawar, Badal wrote:
>> Hi Riana,
>>
>> On 22-01-2026 15:36, 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.
>>>
>>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>>> ---
>>>   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    | 85 ++++++++++++++++++++++++++++
>>>   5 files changed, 107 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 f6650ec3ab42..5581f2180b5c 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -98,6 +98,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 58d7d8b2fea3..81480248eeff 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);
>>> +}
>>> +
>>>   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 944f909a86ad..2d140463dc5e 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -669,6 +669,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 c92cc176f669..e1ee393b7461 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -1255,6 +1255,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,
>>> @@ -1262,6 +1264,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..a3cc01afa179
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
>>> @@ -0,0 +1,85 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2026 Intel Corporation
>>> + */
>>> +#include <drm/drm_drv.h>
>>> +#include <linux/pci.h>
>>> +
>>> +#include "xe_device.h"
>>> +#include "xe_gt.h"
>>> +#include "xe_pci.h"
>>> +#include "xe_uc.h"
>>> +
>>> +static void xe_pci_error_handling(struct pci_dev *pdev)
>>> +{
>>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>>> +
>>> +    xe_device_set_in_recovery(xe);
>>> +    xe_device_declare_wedged(xe);
>>> +
>>> +    pci_disable_device(pdev);
>>> +}
>>> +
>>> +static pci_ers_result_t xe_pci_error_detected(struct pci_dev *pdev, 
>>> pci_channel_state_t state)
>>> +{
>>> +    dev_err(&pdev->dev, "PCI error detected, state %d\n", state);
>>> +
>>> +    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;
>>> +    case pci_channel_io_perm_failure:
>>> +        return PCI_ERS_RESULT_DISCONNECT;
>>> +    }
>>> +
>>> +    return PCI_ERS_RESULT_NEED_RESET;
>>> +}
>>> +
>>> +static pci_ers_result_t xe_pci_error_mmio_enabled(struct pci_dev *pdev)
>>> +{
>>> +    dev_err(&pdev->dev, "PCI 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);
>>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>>> +
>>> +    dev_err(&pdev->dev, "PCI slot reset\n");
>>> +
>>> +    pci_restore_state(pdev);
>> What is the significance of restore state here? In reset path any 
>> where pci_save_state() is happening?
>>> +
>>> +    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);
>>> +    xe_device_clear_in_recovery(xe);
>>> +
>>> +    if (!pdev->driver->probe(pdev, ent))
>>> +        return PCI_ERS_RESULT_RECOVERED;
>>> +
> Instead of invoking xe_pci_remove() and xe_pci_probe() for unbind-bind 
> operations, we can use device_release_driver() followed by device_attach().
> With this approach, the patch "Group all devres to release them on PCIe 
> slot reset" becomes unnecessary.
> 
>      device_release_driver(&pdev->dev);
>      xe_device_clear_in_recovery(xe);
>      ret = device_attach(&pdev->dev);
>      if (ret == 1)
>          return PCI_ERS_RESULT_RECOVERED;
> 
>      return PCI_ERS_RESULT_DISCONNECT;


I tried this before. This causes a deadlock. The device_attach and
release_driver hold a dev lock which is also held by the error handling 
functions.

static int report_slot_reset(struct pci_dev *dev, void *data)
{
....

	device_lock(&dev->dev);


static int __device_attach(struct device *dev, bool allow_async)
{
....

	device_lock(dev);

Thanks
Riana



> 
> Thanks
> Badal
> 
>>> +    return PCI_ERS_RESULT_RECOVERED;
>>
>> Is it correct to return PCI_ERS_RESULT_RECOVERED if driver probe fails?
>>
>> Thanks, Badal
>>
>>> +}
>>> +
>>> +static void xe_pci_error_resume(struct pci_dev *pdev)
>>> +{
>>> +    dev_info(&pdev->dev, "PCI error resume\n");
>>> +}
>>> +
>>> +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,
>>> +};