Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/xe: short-circuit in xe_device_probe_early() suppresses survivability log --
commit 61c74e2642fe230ed9329d7da9f94eb01df2fc3a Author: Alan Previn <[email protected]> drm/xe/survivability: broadcast dmesg when engaging survivability This commit renames xe_survivability_mode_is_requested() to xe_survivability_mode_is_required() and moves the survivability dmesg logging from enable_boot_survivability_mode() into a shared helper survivability_mode_required(). > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c > index 7e6027f49a4ca..bb0ccae7f13a8 100644 > --- a/drivers/gpu/drm/xe/xe_device.c > +++ b/drivers/gpu/drm/xe/xe_device.c > @@ -727,7 +727,7 @@ int xe_device_probe_early(struct xe_device *xe) > * the driver will be blocked > */ > err = xe_pcode_probe_early(xe); > - if (err || xe_survivability_mode_is_requested(xe)) { > + if (err || xe_survivability_mode_is_required(xe)) { > int save_err = err; > > /* > diff --git a/drivers/gpu/drm/xe/xe_survivability_mode.c > b/drivers/gpu/drm/xe/xe_survivability_mode.c > index 45d44ebe288b4..f8619c0db3d95 100644 > --- a/drivers/gpu/drm/xe/xe_survivability_mode.c > +++ b/drivers/gpu/drm/xe/xe_survivability_mode.c > @@ -363,11 +363,6 @@ static int enable_boot_survivability_mode(struct pci_dev > *pdev) > if (ret) > goto failed; > > - if (check_boot_failure(xe)) > - xe_log_err_fatal(pdev, SURVIVABILITY, 0, "Boot Mode > enabled!\n"); > - else > - xe_log_info(pdev, SURVIVABILITY, "Boot Mode enabled!\n"); > - > return 0; > > failed: > @@ -389,17 +384,7 @@ bool xe_survivability_mode_is_boot_enabled(struct > xe_device *xe) > return survivability->mode && survivability->type == > XE_SURVIVABILITY_TYPE_BOOT; > } > > -/** > - * xe_survivability_mode_is_requested - check if it's possible to enable > survivability > - * mode that was requested by firmware or > userspace > - * @xe: xe device instance > - * > - * This function reads configfs and boot status from Pcode. > - * > - * Return: true if platform support is available and boot status indicates > - * failure or if survivability mode is requested, false otherwise. > - */ > -bool xe_survivability_mode_is_requested(struct xe_device *xe) > +static bool survivability_mode_required(struct xe_device *xe, bool > log_on_action) > { > struct xe_survivability *survivability = &xe->survivability; > struct xe_mmio *mmio = xe_root_tile_mmio(xe); > @@ -412,13 +397,36 @@ bool xe_survivability_mode_is_requested(struct > xe_device *xe) > > survivability_mode = xe_configfs_get_survivability_mode(pdev); > /* Enable survivability mode if set via configfs */ > - if (survivability_mode) > + if (survivability_mode) { > + if (log_on_action) > + xe_log_info(xe, SURVIVABILITY, "Boot mode > requested.\n"); > return true; > + } > > data = xe_mmio_read32(mmio, PCODE_SCRATCH(0)); > survivability->boot_status = REG_FIELD_GET(BOOT_STATUS, data); > > - return check_boot_failure(xe); > + if (check_boot_failure(xe)) { > + if (log_on_action) > + xe_log_err_fatal(pdev, SURVIVABILITY, 0, "Boot Mode > required!\n"); > + return true; > + } > + return false; > +} > + > +/** > + * xe_survivability_mode_is_required - check if survivability mode is > required based on > + * state of firmware or userspace > request. > + * @xe: xe device instance > + * > + * This function reads configfs and boot status from Pcode. > + * > + * Return: true if platform support is available and boot status indicates > + * failure or if survivability mode is requested, false otherwise. > + */ > +bool xe_survivability_mode_is_required(struct xe_device *xe) > +{ > + return survivability_mode_required(xe, true); > } > > /** > @@ -468,7 +476,7 @@ 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)) > + if (!survivability_mode_required(xe, false)) > return 0; [Severity: Medium] As reported by Sashiko on v1 [1], moving the survivability messages out of enable_boot_survivability_mode() and gating them on log_on_action in survivability_mode_required() causes a few issues depending on whether xe_pcode_probe_early() succeeds or fails. In drivers/gpu/drm/xe/xe_device.c:xe_device_probe_early(): err = xe_pcode_probe_early(xe); if (err || xe_survivability_mode_is_required(xe)) { int save_err = err; err = xe_survivability_mode_boot_enable(xe); When xe_pcode_probe_early() returns a non-zero error code (which is the normal hardware boot-survivability path when PCODE initialization fails), the logical OR short-circuits and xe_survivability_mode_is_required() is never called. Execution then proceeds to xe_survivability_mode_boot_enable(), which calls survivability_mode_required(xe, false) with log_on_action set to false, and then enable_boot_survivability_mode(), where the "Boot Mode enabled!" logs were removed. Does this cause boot survivability mode to be enabled without emitting any survivability dmesg or XE_SIGID_SURVIVABILITY message at all whenever xe_pcode_probe_early() fails? Conversely, when xe_pcode_probe_early() returns 0 and check_boot_failure() is true, xe_survivability_mode_is_required() calls survivability_mode_required(xe, true) and emits the fatal XE_SIGID_SURVIVABILITY log before xe_survivability_mode_boot_enable() populates the survivability info and checks the breadcrumb version: populate_survivability_info(xe); /* * v2 supports survivability mode for critical errors */ if (survivability->version < 2 && survivability->boot_status == CRITICAL_FAILURE) { log_survivability_info(xe); return -ENXIO; } In that case, the fatal survivability log is emitted even when xe_survivability_mode_boot_enable() aborts with -ENXIO, and when it does succeed, there is no longer a message confirming that boot survivability mode was enabled. Would it work better to log inside xe_survivability_mode_boot_enable() or enable_boot_survivability_mode() so both the err != 0 and err == 0 paths are covered without duplicate checks? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
