Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/xe/xe_ras: unintended severity downgrade in handle_page_offline() --
commit 43f2f57f0adc520b740507efd524d932280d6544 Author: Ethan Tidmore <[email protected]> drm/xe/xe_ras: Use xe_log_info to avoid passing zero as error code This commit replaces xe_log_err() with xe_log_info() in the case 0 branch of handle_page_offline() in drivers/gpu/drm/xe/xe_ras.c to avoid passing a zero return value to xe_log_err(). > diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c > index 2c01c183d02b1..df157d521d640 100644 > --- a/drivers/gpu/drm/xe/xe_ras.c > +++ b/drivers/gpu/drm/xe/xe_ras.c > @@ -346,9 +346,9 @@ static int handle_page_offline(struct xe_device *xe, u64 > page_address, bool noti > switch (ret) { > case 0: > action = XE_RAS_PAGE_ACTION_REMOVE; > - xe_log_err(xe, DEVICE_MEMORY, ret, > - "Poison detected at physical address 0x%llx, page > soft-offlined\n", > - page_address); > + xe_log_info(xe, DEVICE_MEMORY, > + "Poison detected at physical address 0x%llx, page > soft-offlined\n", > + page_address); [Severity: Medium] Does switching to xe_log_info() here unintentionally downgrade the severity of a hardware memory poison event when xe_ttm_vram_handle_addr_fault() returns 0 in handle_page_offline()? In drivers/gpu/drm/xe/xe_log.h, xe_log_err() explicitly documents 0 as a valid argument when no negative errno applies, and emits the log entry with CPER_SEV_RECOVERABLE severity: * @err: negative errno for the failing operation, or 0 if not applicable ... * The log entry will be emitted with @CPER_SEV_RECOVERABLE severity. */ #define xe_log_err(any, TAG, err, fmt, args...) \ xe_log_comp_recoverable((any), TAG, ERR_PTR(err), 0, fmt, ##args) Meanwhile, xe_log_info() still passes 0 to ERR_PTR() via xe_log_err_info(), while downgrading the severity to CPER_SEV_INFORMATIONAL: #define xe_log_err_info(any, TAG, err, fmt, args...) \ xe_log_comp_info((any), TAG, ERR_PTR(err), 0, fmt, ##args) ... #define xe_log_info(any, TAG, fmt, args...) \ xe_log_err_info((any), TAG, 0, fmt, ##args) In drivers/gpu/drm/xe/xe_log.c, log_emit_dmesg() handles ERR_PTR(0) (which is NULL) in the final else branch by omitting the (%pe) output, while using cper_sev to select pci_err() vs pci_info() and whether to include the *ERROR* and [Hardware Error] prefixes: if (IS_ERR(data)) log_dmesg_printf(pdev, cper_sev, "SIGID=%u %s(%pe) %s%s%s%pV", sigid, sev_prefix, data, hwe_prefix, loc_prefix, comp_prefix, vaf); else if (data && len) log_dmesg_printf(pdev, cper_sev, "SIGID=%u %s(%*phN) %s%s%s%pV", sigid, sev_prefix, (int)len, data, hwe_prefix, loc_prefix, comp_prefix, vaf); else log_dmesg_printf(pdev, cper_sev, "SIGID=%u %s%s%s%s%pV", sigid, sev_prefix, hwe_prefix, loc_prefix, comp_prefix, vaf); In addition, send_page_offline_cmd() in drivers/gpu/drm/xe/xe_ras.c, which runs immediately after this switch on the case 0 path, already passes a literal 0 to xe_log_err(): if (action == XE_RAS_PAGE_ACTION_OFFLINE) xe_log_err(xe, DEVICE_MEMORY, 0, "Requesting firmware to offline page 0x%llx\n", page_address); else xe_log_err(xe, DEVICE_MEMORY, 0, "Requesting firmware to remove page 0x%llx from queue\n", page_address); If this change was intended to silence a static checker warning on passing the variable ret to ERR_PTR(ret) when ret is known to be 0, would passing a literal 0 to xe_log_err() instead preserve the CPER_SEV_RECOVERABLE severity? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
