Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/xe: unreachable branch and wrong log in xe_device_probe_early() --
commit 9d8c24e19d32b177027b35cc906adcdcf29823ef Author: Alan Previn <[email protected]> drm/xe/survivability: broadcast dmesg when engaging survivability This commit adds dmesg logging in xe_device_probe_early() when the driver encounters early pcode errors or when survivability mode is requested. > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > index e1979bbcf7bc6..49df992253a9a 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c > @@ -702,7 +702,8 @@ static int xe_device_vram_alloc(struct xe_device *xe) > */ > int xe_device_probe_early(struct xe_device *xe) > { > - int err; > + int err, pcode_err; > + bool survival_requested; > > xe_wa_device_init(xe); > xe_wa_process_device_oob(xe); > @@ -726,20 +727,37 @@ int xe_device_probe_early(struct xe_device *xe) > * initialization is complete. Until done, any MMIO or lmem access from > * the driver will be blocked > */ > - err = xe_pcode_probe_early(xe); > - if (err || xe_survivability_mode_is_requested(xe)) { > - int save_err = err; > - > + pcode_err = xe_pcode_probe_early(xe); > + survival_requested = xe_survivability_mode_is_requested(xe); > + if (pcode_err || survival_requested) { > /* > * Try to leave device in survivability mode if device is > * possible, but still return the previous error for error > * propagation > */ > - err = xe_survivability_mode_boot_enable(xe); > - if (err) > - return err; > - > - return save_err; > + int survive_err = xe_survivability_mode_boot_enable(xe); > + > + if (survive_err) { > + if (pcode_err && survival_requested) > + xe_err(xe, "Early pcode err(%pe), survivability > requested but unavailable(%pe)\n", > + ERR_PTR(pcode_err), > ERR_PTR(survive_err)); > + else if (!pcode_err) > + xe_warn(xe, "Early pcode ok, survivability > requested but unavailable(%pe)\n", > + ERR_PTR(survive_err)); > + else > + xe_warn(xe, "Early pcode err(%pe), > survivability not requested and unavailable(%pe)\n", > + ERR_PTR(pcode_err), > ERR_PTR(survive_err)); > + return survive_err; > + } > + if (pcode_err && survival_requested) > + xe_notice(xe, "Early pcode err(%pe), survivability > requested\n", > + ERR_PTR(pcode_err)); > + else if (!pcode_err) > + xe_notice(xe, "Early pcode ok but survivability > requested\n"); > + else > + xe_warn(xe, "Early pcode err(%pe), survivability not > requested. Consider xe_configfs\n", > + ERR_PTR(pcode_err)); [Severity: Medium] Can the first else branch inside if (survive_err) ever be reached, and does the second else branch log a misleading message on unsupported platforms? In drivers/gpu/drm/xe/xe_survivability_mode.c, xe_survivability_mode_boot_enable() immediately re-checks xe_survivability_mode_is_requested(xe) and returns 0 when it is false: int xe_survivability_mode_boot_enable(struct xe_device *xe) { struct xe_survivability *survivability = &xe->survivability; struct pci_dev *pdev = to_pci_dev(xe->drm.dev); if (!xe_survivability_mode_is_requested(xe)) return 0; And xe_survivability_mode_is_requested() returns false both when survivability is not requested and when the platform or VF does not support survivability mode: bool xe_survivability_mode_is_requested(struct xe_device *xe) { ... if (!IS_DGFX(xe) || IS_SRIOV_VF(xe) || xe->info.platform < XE_BATTLEMAGE) return false; Because xe_survivability_mode_boot_enable() returns 0 whenever xe_survivability_mode_is_requested() is false: 1. In xe_device_probe_early(), when pcode_err is non-zero and survival_requested is false, survive_err will be 0 (unless the redundant second call to xe_survivability_mode_is_requested() reads a changed configfs or PCODE_SCRATCH(0) state, in which case survivability was actually requested). That makes the "Early pcode err(%pe), survivability not requested and unavailable(%pe)" branch unreachable under normal execution. 2. On integrated GPUs, SR-IOV VFs, or dGPUs prior to XE_BATTLEMAGE where survivability mode is unsupported (and where xe_config_device_is_visible() in drivers/gpu/drm/xe/xe_configfs.c hides attr_survivability_mode), an early pcode failure will have survive_err == 0 and survival_requested == false, falling through to log "Consider xe_configfs" even though the survivability_mode configfs attribute is not available on that device. Should xe_survivability_mode_boot_enable() or its helper distinguish between "unsupported" and "not requested", and avoid calling xe_survivability_mode_is_requested() twice? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
