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

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

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

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