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

On 8/14/2026 12:51 PM, Tauro, Riana wrote:
> 
> 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.

then I guess we should be more consistent in reporting user
initiated actions, as in case of blocked PROBE (with modparam)
we are now using info [1] as requested by Aravind in [2] as it
was also an user initiated action

[1] https://patchwork.freedesktop.org/patch/746003/?series=171022&rev=4
[2] https://patchwork.freedesktop.org/patch/742259/?series=171022&rev=1#comment_1367587

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