Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] Changing `smu7_send_msg_to_smc` to return error codes (like `-EOPNOTSUPP`) breaks caller contracts across the powerplay subsystem, leading to severe initialization failures and incomplete hardware teardowns. --
commit 69f2b8c2068f5f9f311a575e997b86049cf9f826 Author: Michał Mirosław <[email protected]> Subject: drm/amd/pm/smu7: return error on message send failure This commit changes smu7_send_msg_to_smc() to return standard error codes like -EOPNOTSUPP and -EIO when the SMU message fails or is unsupported. Previously, the function always returned 0 regardless of the SMU response. > diff --git a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu7_smumgr.c > b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu7_smumgr.c > index ffe3ce87d9073..2e6dfe864306c 100644 > --- a/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu7_smumgr.c > +++ b/drivers/gpu/drm/amd/pm/powerplay/smumgr/smu7_smumgr.c > @@ -141,12 +141,16 @@ int smu7_send_msg_to_smc(struct pp_hwmgr *hwmgr, > uint16_t msg) > > ret = PHM_READ_FIELD(hwmgr->device, SMC_RESP_0, SMC_RESP); > > - if (ret == 0xFE) > + switch (ret) { > + case 1: > + return 0; > + case 0xFE: > dev_dbg(adev->dev, "SMU message %#x was not supported\n", msg); > - else if (ret != 1) > + return -EOPNOTSUPP; [Severity: High] Does returning -EOPNOTSUPP here break caller contracts across the powerplay subsystem? Many callers appear to rely on the previous behavior where unsupported messages (0xFE) still returned 0. For example, in smu7_stop_dpm(): drivers/gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c:smu7_stop_dpm() { ... PP_ASSERT_WITH_CODE( (smum_send_msg_to_smc(hwmgr, PPSMC_MSG_PCIeDPM_Disable, NULL) == 0), "Failed to disable pcie DPM during DPM Stop Function!", return -EINVAL); ... } If the hardware doesn't support PPSMC_MSG_PCIeDPM_Disable, the function now returns -EOPNOTSUPP. Will this cause smu7_stop_dpm() to abort early and skip critical teardown steps like smu7_disable_sclk_mclk_dpm()? Similarly, in fiji_thermal_setup_fan_table(), an unsupported PPSMC_MSG_SetFanMinPwm message will cause the return value to be non-zero, which might mistakenly disable microcode fan control entirely. > + default: > dev_info(adev->dev, "SMU message %#x failed: response is %d\n", > msg, ret); > - > - return 0; > + return ret != 0xFFFF ? -EIO : -ENXIO; > + } > } > -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
