Re: [PATCH v4 22/32] drm/xe/survivability: Report 'Boot Mode enabled' status using SIGID

On 14-08-2026 16:10, Michal Wajdeczko wrote:
>
> On 8/14/2026 12:20 PM, Tauro, Riana wrote:
>> On 14-08-2026 15:36, Michal Wajdeczko wrote:
>>> On 8/14/2026 8:31 AM, Tauro, Riana wrote:
>>>> On 13-08-2026 16:31, Michal Wajdeczko wrote:
>>>>> On 8/13/2026 12:52 PM, Mallesh, Koujalagi wrote:
>>>>>> On 13-08-2026 12:44 am, Michal Wajdeczko wrote:
>>>>>>> Report 'Boot Mode' status or failure using various xe_log() helpers.
>>>>>>>
>>>>>>> Signed-off-by: Michal Wajdeczko <[email protected]>
>>>>>>> Cc: Rodrigo Vivi <[email protected]>
>>>>>>> Cc: Riana Tauro <[email protected]>
>>>>>>> Cc: Aravind Iddamsetty <[email protected]>
>>>>>>> Cc: Mallesh Koujalagi <[email protected]>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/xe/xe_survivability_mode.c | 21 +++++++++++++--------
>>>>>>>      1 file changed, 13 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>>> index 2d8c532157fd..ebd288986c11 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>>> @@ -304,14 +304,13 @@ static int create_survivability_sysfs(struct pci_dev *pdev)
>>>>>>>        static int enable_boot_survivability_mode(struct pci_dev *pdev)
>>>>>>>      {
>>>>>>> -    struct device *dev = &pdev->dev;
>>>>>>>          struct xe_device *xe = pdev_to_xe_device(pdev);
>>>>>>>          struct xe_survivability *survivability = &xe->survivability;
>>>>>>> -    int ret = 0;
>>>>>>> +    int ret;
>>>>>>>            ret = create_survivability_sysfs(pdev);
>>>>>>>          if (ret)
>>>>>>> -        return ret;
>>>>>>> +        goto failed;
>>>>>>>            /* Make sure xe_heci_gsc_init() and xe_i2c_probe() are aware of survivability */
>>>>>>>          survivability->mode = true;
>>>>>>> @@ -323,19 +322,25 @@ static int enable_boot_survivability_mode(struct pci_dev *pdev)
>>>>>>>          if (survivability->fdo_mode) {
>>>>>>>              ret = xe_nvm_init(xe);
>>>>>>>              if (ret)
>>>>>>> -            goto err;
>>>>>>> +            goto failed;
>>>>>>>          }
>>>>>>>            ret = xe_i2c_probe(xe);
>>>>>>>          if (ret)
>>>>>>> -        goto err;
>>>>>>> +        goto failed;
>>>>>>>      -    dev_err(dev, "In Survivability Mode\n");
>>>>>>> +    if (check_boot_failure(xe))
>>>>>>> +        xe_log_comp_fatal(pdev, SURVIVABILITY,
>>>>>>> +                  &survivability->boot_status,
>>>>>>> +                  sizeof(survivability->boot_status),
>>>>>>> +                  "Boot Mode enabled!\n");
>>>> Do we need this check? This function is only called if it is a boot failure.
>>> are you sure?
>>>
>>> in xe_device_probe_early() there is:
>>>
>>>      err = xe_pcode_probe_early(xe);
>>>      if (err || xe_survivability_mode_is_requested(xe)) {
>>>          err = xe_survivability_mode_boot_enable(xe);
>>>
>>> and xe_survivability_mode_is_requested() may return true based
>>> on the xe_configfs_get_survivability_mode() settings
>>
>> Configfs also is provided for users to enable boot survivability mode.
>> So one log should be sufficient.
> there will be still one log entry but it will be with different
> severity based on the configfs vs bootstatus origin

configfs should also result in fatal because it also enters 
survivability mode and it was user's decision.

>
>> If we do need to add a blob instead of error no we should go ahead with all
>> the scratch registers as they contain the error details instead of just boot status.
>> Can't we just add 0 for now and come up with something that we can decode in future.?
> including boot_status value here was requested by Mallesh in [1]

Don't agree with [1].

[2] prints only for critical errors. As responded on that patch, it was 
a intentional change
based on review feedback.

No error no should be good here or a hardware error (-EIO). Lets not use 
boot status

Thanks
Riana

>
> and logging all boot status registers is already done in [2] but
> only if boot mode was not triggered by configfs
>
> [1] https://patchwork.freedesktop.org/patch/743324/?series=171022&rev=3#comment_1372191
> [2] https://patchwork.freedesktop.org/patch/746002/?series=171022&rev=4
>
>
>>
>>>> We can have a error log with the type here. We don't need if else.
>>> based on [1] all survivability mode SIGID are FATAL
>>> and based on 21] it was an arch choice to go with info level for all
>>>
>>> but IMO if the boot mode was selected via configfs it doesn't
>>> make sense to call it FATAL, but if there was real problem detected
>>> we should make it FATAL (like we do with failed PROBE)
>>>
>>> [1] https://patchwork.freedesktop.org/patch/732271/?series=168333&rev=1
>>> [2] https://patchwork.freedesktop.org/patch/743324/?series=171022&rev=3#comment_1373731
>>>
>>>> How about something like this? and remove else
>>>>
>>>>
>>>> <3> xe 0000:03:00.0: [drm] *ERROR* SIGID=<N> FATAL (01000000) SURVIVABILITY: mode=Boot
>> I am still trying to understand this series. Apologies if it is wrong, i tried to generate log using AI.
>>
>>>                                                         ^^^^^^^^
>>> boot_status is u8 so it will be at most (01)
>>>
>>> and I don't think we should be so cryptic in the user facing error messages
>>>
>>>>>> In case of fatal, will make sense to return "0" OR we can add return as -ENXIO right? any thoughts?
>>>> Mallesh, you cannot change return codes here. This defeats the purpose of survivability mode
>>>>
>>>>> you're a reviewer here ;)
>>>>>
>>>>> but seriously, enable_boot_survivability_mode() is called from
>>>>> xe_survivability_mode_boot_enable() which says:
>>>>>
>>>>>     * Return: 0 if boot survivability mode is enabled or not requested, negative error
>>>>>     * code otherwise.
>>>>>
>>>>> so returning 0 as success code in enabling boot mode is the correct one
>>>>> returning -ENXIO will be no different than failing to enter boot mode
>>>>>
>>>>> @Riana, this is your code, can you please confirm that
>>>>>
>>>>>> After handling fatal case
>>>>>>
>>>>>> Reviewed-by: Mallesh Koujalagi <[email protected]>
>>>>>>
>>>>>>> +    else
>>>>>>> +        xe_log_info(pdev, SURVIVABILITY, "Boot Mode enabled!\n");
>>>>>>>            return 0;
>>>>>>>      -err:
>>>>>>> -    dev_err(dev, "Failed to enable Survivability Mode\n");
>>>>>>> +failed:
>>>>>>> +    xe_log_err_fatal(pdev, SURVIVABILITY, ret, "Failed to enable Boot Mode!\n");
>>>> Can we retain the previous dmesg?
>>> there will be already "SURVIVABILITY: " prefix included,
>>> so IMO instead of generic:
>>>
>>>     [drm] *ERROR* SIGID=103 (-EXXX) SURVIVABILITY: Failed to enable Survivability Mode
>>>
>>> it's better to have more clearer message:
>>>
>>>     [drm] *ERROR* SIGID=103 (-EXXX) SURVIVABILITY: Failed to enable Boot Mode!
>> Boot Mode and Runtime mode doesn't sound right.
> both "Boot Mode" and "Runtime mode" were existing names in the code ;)
>
>> But i don't have any better suggestions here due
>> to repetition.Since its message and can be changed . Will replace it if i can come up with something better in
>> the future.
> I take it as an ack-by then
>
>> Thanks
>> Riana
>>
>>
>>>> Thanks
>>>> Riana
>>>>
>>>>>>>          survivability->mode = false;
>>>>>>>          return ret;
>>>>>>>      }