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

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.

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


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

Thanks
Riana


>
>> Thanks
>> Riana
>>
>>>>>         survivability->mode = false;
>>>>>         return ret;
>>>>>     }