Re: [PATCH v2 11/11] drm/xe/xe_pci_error: Process errors in mmio_enabled

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

Hi Mallesh

On 3/11/2026 12:40 PM, Mallesh, Koujalagi wrote:
> 
> On 02-03-2026 03:52 pm, Riana Tauro wrote:
>> Query system controller when any non fatal error occurs to check
>> the type of the error, contain and recover.
>>
>> The system controller is queried in the mmio_enabled callback.
>>
>> Signed-off-by: Riana Tauro <riana.tauro@intel.com>
>> ---
>> v2: use ras recovery enum (Raag)
>> ---
>>   drivers/gpu/drm/xe/xe_pci_error.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_pci_error.c b/drivers/gpu/drm/xe/ 
>> xe_pci_error.c
>> index ba62868f00d4..35bc5b8cee99 100644
>> --- a/drivers/gpu/drm/xe/xe_pci_error.c
>> +++ b/drivers/gpu/drm/xe/xe_pci_error.c
>> @@ -9,6 +9,7 @@
>>   #include "xe_device.h"
>>   #include "xe_gt.h"
>>   #include "xe_pci.h"
>> +#include "xe_ras.h"
>>   #include "xe_uc.h"
>>   static void xe_pci_error_handling(struct pci_dev *pdev)
>> @@ -26,6 +27,12 @@ static void xe_pci_error_handling(struct pci_dev 
>> *pdev)
>>       pci_disable_device(pdev);
>>   }
> Explain the mapping rationale

Do you mean commit message?
In the code the naming is self explanatory

>> +static pci_ers_result_t ras_recovery_action_to_pci_result[] = {
> The mapping array should be constant, never modified.
>> +    [XE_RAS_RECOVERY_ACTION_RECOVERED] = PCI_ERS_RESULT_RECOVERED,
>> +    [XE_RAS_RECOVERY_ACTION_RESET] = PCI_ERS_RESULT_NEED_RESET,
>> +    [XE_RAS_RECOVERY_ACTION_DISCONNECT] = PCI_ERS_RESULT_DISCONNECT,
>> +};
>> +
>>   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);
>> @@ -50,9 +57,13 @@ static pci_ers_result_t 
>> xe_pci_error_detected(struct pci_dev *pdev, pci_channel_
>>   static pci_ers_result_t xe_pci_error_mmio_enabled(struct pci_dev *pdev)
>>   {
>> +    struct xe_device *xe = pdev_to_xe_device(pdev);
>> +    enum xe_ras_recovery_action action;
> Add action_max @ end of enum to validate.

Answered below, but for consistency i can add max

>> +
>>       dev_err(&pdev->dev, "Xe Pci error recovery: MMIO enabled\n");
>> +    action = xe_ras_process_errors(xe);
> What will happen, if RAS processing takes significant time?

System controller times out after a duration. Refer patch
https://patchwork.freedesktop.org/patch/710637/?series=159554&rev=10

If the ret is negative in process_errors

+		ret = xe_sysctrl_send_command(xe, &command, &rlen);
+		if (ret || !rlen) {
+			xe_err(xe, "[RAS]: Sysctrl error ret %d\n", ret);
+			goto err;
+		}
+

We request for a reset assuming a SC error.


>> -    return PCI_ERS_RESULT_NEED_RESET;
> 
> Use array bound check before using action to avoid out of bounds array 
> access

The action here comes from a xe_ras_process_errors which also returns
enum. This action is explicitly sent and is not coming from user
or firmware.

Adding a check here seems unnecessary.

Thanks
Riana

> 
> Thanks,
> 
> -/Mallesh
> 
>> +    return ras_recovery_action_to_pci_result[action];
>>   }
>>   static pci_ers_result_t xe_pci_error_slot_reset(struct pci_dev *pdev)