Re: [PATCH v4 20/32] drm/xe/survivability: Report 'boot status' using SIGID

On 8/14/2026 12:46 PM, Tauro, Riana wrote:
> 
> On 13-08-2026 15:48, Michal Wajdeczko wrote:
>>
>> On 8/13/2026 12:07 PM, Mallesh, Koujalagi wrote:
>>> On 13-08-2026 02:58 pm, Michal Wajdeczko wrote:
>>>> On 8/13/2026 10:38 AM, Mallesh, Koujalagi wrote:
>>>>> On 13-08-2026 12:44 am, Michal Wajdeczko wrote:
>>>>>> Report 'boot status' details using xe_log_err_fatal/info() macros.
>>>>>> While around, move static helper code closer to the caller and let
>>>>>> it take xe instead of pdev.
>>>>>>
>>>>>> 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 | 35 ++++++++++++----------
>>>>>>    1 file changed, 19 insertions(+), 16 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c b/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>> index 4c506027fa94..85b4c125a217 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c
>>>>>> @@ -14,6 +14,7 @@
>>>>>>    #include "xe_device.h"
>>>>>>    #include "xe_heci_gsc.h"
>>>>>>    #include "xe_i2c.h"
>>>>>> +#include "xe_log.h"
>>>>>>    #include "xe_mmio.h"
>>>>>>    #include "xe_nvm.h"
>>>>>>    #include "xe_pcode_api.h"
>>>>>> @@ -172,21 +173,6 @@ static void populate_survivability_info(struct xe_device *xe)
>>>>>>        }
>>>>>>    }
>>>>>>    -static void log_survivability_info(struct pci_dev *pdev)
>>>>>> -{
>>>>>> -    struct xe_device *xe = pdev_to_xe_device(pdev);
>>>>>> -    struct xe_survivability *survivability = &xe->survivability;
>>>>>> -    u32 *info = survivability->info;
>>>>>> -    int id;
>>>>>> -
>>>>>> -    dev_info(&pdev->dev, "Survivability Boot Status : Critical Failure (%d)\n",
>>>>>> -         survivability->boot_status);
>>>>>> -    for (id = 0; id < MAX_SCRATCH_REG; id++) {
>>>>>> -        if (info[id])
>>>>>> -            dev_info(&pdev->dev, "%s: 0x%x\n", reg_map[id], info[id]);
>>>>>> -    }
>>>>>> -}
>>>>>> -
>>>>>>    static int check_boot_failure(struct xe_device *xe)
>>>>>>    {
>>>>>>        struct xe_survivability *survivability = &xe->survivability;
>>>>>> @@ -429,6 +415,23 @@ void xe_survivability_mode_runtime_enable(struct xe_device *xe)
>>>>>>        dev_err(&pdev->dev, "Firmware flash required, Please refer to the userspace documentation for more details!\n");
>>>>>>    }
>>>>>>    +static void log_survivability_info(struct xe_device *xe)
>>>>>> +{
>>>>>> +    struct xe_survivability *survivability = &xe->survivability;
>>>>>> +    u32 *info = survivability->info;
>>>>>> +    int id;
>>>>>> +
>>>>>> +    xe_log_err_fatal(xe, SURVIVABILITY, -ENXIO, "Boot Status: %s (%u)\n",
>>>>>> +             survivability->boot_status == CRITICAL_FAILURE ?
>>>>>> +             "Critical Failure" : "Other", survivability->boot_status);
>>>>> Since log_survivability_info is called when survivability->boot_status == CRITICAL_FAILURE true, so please use "Critical Failure"  string directly.
>>>> well, that's the current usage and function name didn't strictly say "log critical failure" only
>>>>
>>>> btw, printing fixed "Critical Failure" string followed by flexible %d also doesn't make sense IMO
>>>>
>>>> that's why I decided to make this function more flexible and reusable if needed
>>> Totally agreed, u made it generic one, however current usage, we never going to hit "Other" case, which is dead here.
>>>
>>> btw, "Other" is "Non Critical Failure" right?
>> only if boot_status == 7
>>
>> but since BOOT_STATUS is REG_GENMASK(3, 1) I assume that there
>> might be other values, beyond currently documented 4 & 7
> 
> 
> Yeah there are other values apart from 4 and 7. When this patch was initially added,
> it was suggested not to log the duplicate information as part of dmesg as it is already part of sysfs.

what about the case when we failed to add sysfs files?
then user is left with nothing ...

> That is the reason we print this only in version 1 of survivability for critical errors.
> 
> In v1, we do not enter survivability mode for critical errors and only log additional information
> in dmesg to aid with debug.
> 
> So generic is not necessary based on previous review decisions. But will leave it upto you.

I'll add boot_status_str() helper to make log function even
more generic 7) and likely keep it in current place as a
preparation step for future patch to use it when sysfs fails 

> 
> Thanks
> Riana
> 
> 
>>
>> so even if we print "Other" there will be still numerical
>> value "(7)" or "(1)" that could be used for debug/triage
>>
>> but I can add helper
>>
>>     static const char *boot_status_str(u8 boot_status)
>>
>> to return friendly name for all currently known codes
>> (and "Other" or NULL for unknown codes)
>>>
>>> Thanks,
>>>
>>> -/Mallesh
>>>
>>>>> Reviewed-by: Mallesh Koujalagi <[email protected]>
>>>>>
>>>>>> +
>>>>>> +    for (id = 0; id < MAX_SCRATCH_REG; id++) {
>>>>>> +        if (!info[id])
>>>>>> +            continue;
>>>>>> +        xe_log_info(xe, SURVIVABILITY, "%s: %#x\n", reg_map[id], info[id]);
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>>>>    /**
>>>>>>     * xe_survivability_mode_boot_enable - Initialize and enable boot survivability mode
>>>>>>     * @xe: xe device instance
>>>>>> @@ -452,7 +455,7 @@ int xe_survivability_mode_boot_enable(struct xe_device *xe)
>>>>>>         * v2 supports survivability mode for critical errors
>>>>>>         */
>>>>>>        if (survivability->version < 2  && survivability->boot_status == CRITICAL_FAILURE) {
>>>>>> -        log_survivability_info(pdev);
>>>>>> +        log_survivability_info(xe);
>>>>>>            return -ENXIO;
>>>>>>        }
>>>>>>