Re: [PATCH 3/5] drm/amd/pm/smu7: return error on message send failure

[sashiko-bot@kernel.org]

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 <mirq-linux@rere.qmqm.pl>
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/cover.1783009338.git.mirq-linux@rere.qmqm.pl?part=3