Re: [PATCH] drm/amd/pm: correct the checks for fan attributes support

["Lazar, Lijo" <lijo.lazar@xxxxxxx>]

On 1/11/2022 3:36 PM, Quan, Evan wrote:
> [AMD Official Use Only]
>
>
>
> > -----Original Message-----
> > From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> > Sent: Tuesday, January 11, 2022 4:14 PM
> > To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Chen, Guchun
> > <Guchun.Chen@xxxxxxx>
> > Subject: Re: [PATCH] drm/amd/pm: correct the checks for fan attributes
> > support
> >
> >
> >
> > On 1/11/2022 1:17 PM, Evan Quan wrote:
> > > On functionality unsupported, -EOPNOTSUPP will be returned. And we
> > > rely on that to determine the fan attributes support.
> > >
> > > Fixes: 801771de0331 ("drm/amd/pm: do not expose power
> > implementation
> > > details to
> > > amdgpu_pm.c")
> > >
> > > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > > Change-Id: I95e7e0beebd678a446221a72234cd356e14f0fcd
> > > ---
> > >    .../gpu/drm/amd/include/kgd_pp_interface.h    |   4 +-
> > >    drivers/gpu/drm/amd/pm/amdgpu_dpm.c           |  31 ++++-
> > >    drivers/gpu/drm/amd/pm/amdgpu_pm.c            |  21 ++--
> > >    drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c    |  10 +-
> > >    .../gpu/drm/amd/pm/powerplay/amd_powerplay.c  |  59 ++++-----
> > >    drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c     | 115 +++++++++----
> > -----
> > >    6 files changed, 132 insertions(+), 108 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > index a8eec91c0995..387120099493 100644
> > > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h
> > > @@ -315,8 +315,8 @@ struct amd_pm_funcs {
> > >    				void  *rps,
> > >    				bool  *equal);
> > >    /* export for sysfs */
> > > -	void (*set_fan_control_mode)(void *handle, u32 mode);
> > > -	u32 (*get_fan_control_mode)(void *handle);
> > > +	int (*set_fan_control_mode)(void *handle, u32 mode);
> > > +	int (*get_fan_control_mode)(void *handle, u32 *fan_mode);
> > >    	int (*set_fan_speed_pwm)(void *handle, u32 speed);
> > >    	int (*get_fan_speed_pwm)(void *handle, u32 *speed);
> > >    	int (*force_clock_level)(void *handle, enum pp_clock_type type,
> > > uint32_t mask); diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > > b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > > index 68d2e80a673b..fe69785df403 100644
> > > --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > > +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > > @@ -1030,15 +1030,20 @@ int
> > amdgpu_dpm_get_fan_control_mode(struct amdgpu_device *adev,
> > >    				    uint32_t *fan_mode)
> > >    {
> > >    	const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> > > +	int ret = 0;
> > >
> > >    	if (!pp_funcs->get_fan_control_mode)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (!fan_mode)
> > > +		return -EINVAL;
> > > +
> >
> > pp_funcs most likely will be there for smu/powerplay subsystem. I think the
> > checks should be at one layer down - smu_get_fan_control_mode() and
> > pp_dpm_get_fan_control_mode()
> >
> > First one will check if ppt func is implemented and second one will check if
> > hwmgr func is implemented for the specific ASIC.
> [Quan, Evan] Yes, I agree. And if you go through the changes below, you will see the checks (for the layers down) there.
> This patch checks not only those amdgpu_dpm_xxxx APIs, but also those downstream interfaces(smu_xxxx and pp_dpm_xxxx).
>   

Say you call amdgpu_dpm_get_fan_control_mode(adev, NULL) from amdgpu_pm

pp_funcs->get_fan_control_mode => this will point to 
smu_get_fan_control_mode for all swsmu ASICs. So  "if 
(!pp_funcs->get_fan_control_mode)" will be false.

The next statement is NULL check and it will return -EINVAL.

What we want to know is ppt_func is implemented or not for the 
particualr swsmu ASIC. Isn't that the case or am I reading it differently?

Thanks,
Lijo

> BR
> Evan
> > Thanks,
> > Lijo
> >
> > >    	mutex_lock(&adev->pm.mutex);
> > > -	*fan_mode = pp_funcs->get_fan_control_mode(adev-
> > > powerplay.pp_handle);
> > > +	ret = pp_funcs->get_fan_control_mode(adev-
> > > powerplay.pp_handle,
> > > +					     fan_mode);
> > >    	mutex_unlock(&adev->pm.mutex);
> > >
> > > -	return 0;
> > > +	return ret;
> > >    }
> > >
> > >    int amdgpu_dpm_set_fan_speed_pwm(struct amdgpu_device *adev,
> > @@
> > > -1048,6 +1053,9 @@ int amdgpu_dpm_set_fan_speed_pwm(struct
> > amdgpu_device *adev,
> > >    	int ret = 0;
> > >
> > >    	if (!pp_funcs->set_fan_speed_pwm)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (speed == U32_MAX)
> > >    		return -EINVAL;
> > >
> > >    	mutex_lock(&adev->pm.mutex);
> > > @@ -1065,6 +1073,9 @@ int amdgpu_dpm_get_fan_speed_pwm(struct
> > amdgpu_device *adev,
> > >    	int ret = 0;
> > >
> > >    	if (!pp_funcs->get_fan_speed_pwm)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (!speed)
> > >    		return -EINVAL;
> > >
> > >    	mutex_lock(&adev->pm.mutex);
> > > @@ -1082,6 +1093,9 @@ int amdgpu_dpm_get_fan_speed_rpm(struct
> > amdgpu_device *adev,
> > >    	int ret = 0;
> > >
> > >    	if (!pp_funcs->get_fan_speed_rpm)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (!speed)
> > >    		return -EINVAL;
> > >
> > >    	mutex_lock(&adev->pm.mutex);
> > > @@ -1099,6 +1113,9 @@ int amdgpu_dpm_set_fan_speed_rpm(struct
> > amdgpu_device *adev,
> > >    	int ret = 0;
> > >
> > >    	if (!pp_funcs->set_fan_speed_rpm)
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (speed == U32_MAX)
> > >    		return -EINVAL;
> > >
> > >    	mutex_lock(&adev->pm.mutex);
> > > @@ -1113,16 +1130,20 @@ int
> > amdgpu_dpm_set_fan_control_mode(struct amdgpu_device *adev,
> > >    				    uint32_t mode)
> > >    {
> > >    	const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> > > +	int ret = 0;
> > >
> > >    	if (!pp_funcs->set_fan_control_mode)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (mode == U32_MAX)
> > > +		return -EINVAL;
> > > +
> > >    	mutex_lock(&adev->pm.mutex);
> > > -	pp_funcs->set_fan_control_mode(adev->powerplay.pp_handle,
> > > -				       mode);
> > > +	ret = pp_funcs->set_fan_control_mode(adev-
> > > powerplay.pp_handle,
> > > +					     mode);
> > >    	mutex_unlock(&adev->pm.mutex);
> > >
> > > -	return 0;
> > > +	return ret;
> > >    }
> > >
> > >    int amdgpu_dpm_get_power_limit(struct amdgpu_device *adev, diff
> > > --git a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > > b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > > index d3eab245e0fe..940cbe751163 100644
> > > --- a/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > > +++ b/drivers/gpu/drm/amd/pm/amdgpu_pm.c
> > > @@ -3197,7 +3197,6 @@ static umode_t hwmon_attributes_visible(struct
> > kobject *kobj,
> > >    	struct device *dev = kobj_to_dev(kobj);
> > >    	struct amdgpu_device *adev = dev_get_drvdata(dev);
> > >    	umode_t effective_mode = attr->mode;
> > > -	uint32_t speed = 0;
> > >
> > >    	/* under multi-vf mode, the hwmon attributes are all not supported
> > */
> > >    	if (amdgpu_sriov_vf(adev) && !amdgpu_sriov_is_pp_one_vf(adev))
> > @@
> > > -3263,15 +3262,15 @@ static umode_t hwmon_attributes_visible(struct
> > kobject *kobj,
> > >    		return 0;
> > >
> > >    	/* mask fan attributes if we have no bindings for this asic to expose
> > */
> > > -	if (((amdgpu_dpm_get_fan_speed_pwm(adev, &speed) == -EINVAL)
> > &&
> > > +	if (((amdgpu_dpm_get_fan_speed_pwm(adev, NULL) == -
> > EOPNOTSUPP) &&
> > >    	      attr == &sensor_dev_attr_pwm1.dev_attr.attr) || /* can't query
> > fan */
> > > -	    ((amdgpu_dpm_get_fan_control_mode(adev, &speed) == -
> > EOPNOTSUPP) &&
> > > +	    ((amdgpu_dpm_get_fan_control_mode(adev, NULL) == -
> > EOPNOTSUPP) &&
> > >    	     attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr)) /* can't
> > query state */
> > >    		effective_mode &= ~S_IRUGO;
> > >
> > > -	if (((amdgpu_dpm_set_fan_speed_pwm(adev, speed) == -EINVAL)
> > &&
> > > +	if (((amdgpu_dpm_set_fan_speed_pwm(adev, U32_MAX) == -
> > EOPNOTSUPP) &&
> > >    	      attr == &sensor_dev_attr_pwm1.dev_attr.attr) || /* can't
> > manage fan */
> > > -	      ((amdgpu_dpm_set_fan_control_mode(adev, speed) == -
> > EOPNOTSUPP) &&
> > > +	      ((amdgpu_dpm_set_fan_control_mode(adev, U32_MAX) ==
> > > +-EOPNOTSUPP) &&
> > >    	      attr == &sensor_dev_attr_pwm1_enable.dev_attr.attr)) /* can't
> > manage state */
> > >    		effective_mode &= ~S_IWUSR;
> > >
> > > @@ -3291,16 +3290,16 @@ static umode_t
> > hwmon_attributes_visible(struct kobject *kobj,
> > >    		return 0;
> > >
> > >    	/* hide max/min values if we can't both query and manage the fan */
> > > -	if (((amdgpu_dpm_set_fan_speed_pwm(adev, speed) == -EINVAL)
> > &&
> > > -	      (amdgpu_dpm_get_fan_speed_pwm(adev, &speed) == -EINVAL)
> > &&
> > > -	      (amdgpu_dpm_set_fan_speed_rpm(adev, speed) == -EINVAL)
> > &&
> > > -	      (amdgpu_dpm_get_fan_speed_rpm(adev, &speed) == -EINVAL))
> > &&
> > > +	if (((amdgpu_dpm_set_fan_speed_pwm(adev, U32_MAX) == -
> > EOPNOTSUPP) &&
> > > +	      (amdgpu_dpm_get_fan_speed_pwm(adev, NULL) == -
> > EOPNOTSUPP) &&
> > > +	      (amdgpu_dpm_set_fan_speed_rpm(adev, U32_MAX) == -
> > EOPNOTSUPP) &&
> > > +	      (amdgpu_dpm_get_fan_speed_rpm(adev, NULL) == -
> > EOPNOTSUPP)) &&
> > >    	    (attr == &sensor_dev_attr_pwm1_max.dev_attr.attr ||
> > >    	     attr == &sensor_dev_attr_pwm1_min.dev_attr.attr))
> > >    		return 0;
> > >
> > > -	if ((amdgpu_dpm_set_fan_speed_rpm(adev, speed) == -EINVAL)
> > &&
> > > -	     (amdgpu_dpm_get_fan_speed_rpm(adev, &speed) == -EINVAL)
> > &&
> > > +	if ((amdgpu_dpm_set_fan_speed_rpm(adev, U32_MAX) == -
> > EOPNOTSUPP) &&
> > > +	     (amdgpu_dpm_get_fan_speed_rpm(adev, NULL) == -
> > EOPNOTSUPP) &&
> > >    	     (attr == &sensor_dev_attr_fan1_max.dev_attr.attr ||
> > >    	     attr == &sensor_dev_attr_fan1_min.dev_attr.attr))
> > >    		return 0;
> > > diff --git a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > index 92b987fb31d4..7677d3a21f8c 100644
> > > --- a/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > +++ b/drivers/gpu/drm/amd/pm/legacy-dpm/si_dpm.c
> > > @@ -6669,7 +6669,7 @@ static int si_dpm_set_fan_speed_pwm(void
> > *handle,
> > >    	return 0;
> > >    }
> > >
> > > -static void si_dpm_set_fan_control_mode(void *handle, u32 mode)
> > > +static int si_dpm_set_fan_control_mode(void *handle, u32 mode)
> > >    {
> > >    	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > >
> > > @@ -6685,9 +6685,11 @@ static void si_dpm_set_fan_control_mode(void
> > *handle, u32 mode)
> > >    		else
> > >    			si_fan_ctrl_set_default_mode(adev);
> > >    	}
> > > +
> > > +	return 0;
> > >    }
> > >
> > > -static u32 si_dpm_get_fan_control_mode(void *handle)
> > > +static int si_dpm_get_fan_control_mode(void *handle, u32 *fan_mode)
> > >    {
> > >    	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > >    	struct si_power_info *si_pi = si_get_pi(adev); @@ -6697,7 +6699,9
> > > @@ static u32 si_dpm_get_fan_control_mode(void *handle)
> > >    		return 0;
> > >
> > >    	tmp = RREG32(CG_FDO_CTRL2) & FDO_PWM_MODE_MASK;
> > > -	return (tmp >> FDO_PWM_MODE_SHIFT);
> > > +	*fan_mode = (tmp >> FDO_PWM_MODE_SHIFT);
> > > +
> > > +	return 0;
> > >    }
> > >
> > >    #if 0
> > > diff --git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > > b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > > index 89341729744d..57bc9405d6a9 100644
> > > --- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > > +++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > > @@ -488,38 +488,37 @@ static enum amd_pm_state_type
> > pp_dpm_get_current_power_state(void *handle)
> > >    	return pm_type;
> > >    }
> > >
> > > -static void pp_dpm_set_fan_control_mode(void *handle, uint32_t mode)
> > > +static int pp_dpm_set_fan_control_mode(void *handle, uint32_t mode)
> > >    {
> > >    	struct pp_hwmgr *hwmgr = handle;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (hwmgr->hwmgr_func->set_fan_control_mode == NULL)
> > > +		return -EOPNOTSUPP;
> > >
> > > -	if (hwmgr->hwmgr_func->set_fan_control_mode == NULL) {
> > > -		pr_info_ratelimited("%s was not implemented.\n",
> > __func__);
> > > -		return;
> > > -	}
> > >    	mutex_lock(&hwmgr->smu_lock);
> > >    	hwmgr->hwmgr_func->set_fan_control_mode(hwmgr, mode);
> > >    	mutex_unlock(&hwmgr->smu_lock);
> > > +
> > > +	return 0;
> > >    }
> > >
> > > -static uint32_t pp_dpm_get_fan_control_mode(void *handle)
> > > +static int pp_dpm_get_fan_control_mode(void *handle, uint32_t
> > > +*fan_mode)
> > >    {
> > >    	struct pp_hwmgr *hwmgr = handle;
> > > -	uint32_t mode = 0;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return 0;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (hwmgr->hwmgr_func->get_fan_control_mode == NULL)
> > > +		return -EOPNOTSUPP;
> > >
> > > -	if (hwmgr->hwmgr_func->get_fan_control_mode == NULL) {
> > > -		pr_info_ratelimited("%s was not implemented.\n",
> > __func__);
> > > -		return 0;
> > > -	}
> > >    	mutex_lock(&hwmgr->smu_lock);
> > > -	mode = hwmgr->hwmgr_func->get_fan_control_mode(hwmgr);
> > > +	*fan_mode = hwmgr->hwmgr_func-
> > > get_fan_control_mode(hwmgr);
> > >    	mutex_unlock(&hwmgr->smu_lock);
> > > -	return mode;
> > > +	return 0;
> > >    }
> > >
> > >    static int pp_dpm_set_fan_speed_pwm(void *handle, uint32_t speed)
> > @@
> > > -528,12 +527,11 @@ static int pp_dpm_set_fan_speed_pwm(void *handle,
> > uint32_t speed)
> > >    	int ret = 0;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (hwmgr->hwmgr_func->set_fan_speed_pwm == NULL)
> > > +		return -EOPNOTSUPP;
> > >
> > > -	if (hwmgr->hwmgr_func->set_fan_speed_pwm == NULL) {
> > > -		pr_info_ratelimited("%s was not implemented.\n",
> > __func__);
> > > -		return 0;
> > > -	}
> > >    	mutex_lock(&hwmgr->smu_lock);
> > >    	ret = hwmgr->hwmgr_func->set_fan_speed_pwm(hwmgr, speed);
> > >    	mutex_unlock(&hwmgr->smu_lock);
> > > @@ -546,12 +544,10 @@ static int pp_dpm_get_fan_speed_pwm(void
> > *handle, uint32_t *speed)
> > >    	int ret = 0;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > >
> > > -	if (hwmgr->hwmgr_func->get_fan_speed_pwm == NULL) {
> > > -		pr_info_ratelimited("%s was not implemented.\n",
> > __func__);
> > > -		return 0;
> > > -	}
> > > +	if (hwmgr->hwmgr_func->get_fan_speed_pwm == NULL)
> > > +		return -EOPNOTSUPP;
> > >
> > >    	mutex_lock(&hwmgr->smu_lock);
> > >    	ret = hwmgr->hwmgr_func->get_fan_speed_pwm(hwmgr, speed);
> > @@
> > > -565,10 +561,10 @@ static int pp_dpm_get_fan_speed_rpm(void *handle,
> > uint32_t *rpm)
> > >    	int ret = 0;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > >
> > >    	if (hwmgr->hwmgr_func->get_fan_speed_rpm == NULL)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > >
> > >    	mutex_lock(&hwmgr->smu_lock);
> > >    	ret = hwmgr->hwmgr_func->get_fan_speed_rpm(hwmgr, rpm);
> > @@ -582,12
> > > +578,11 @@ static int pp_dpm_set_fan_speed_rpm(void *handle,
> > uint32_t rpm)
> > >    	int ret = 0;
> > >
> > >    	if (!hwmgr || !hwmgr->pm_en)
> > > -		return -EINVAL;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (hwmgr->hwmgr_func->set_fan_speed_rpm == NULL)
> > > +		return -EOPNOTSUPP;
> > >
> > > -	if (hwmgr->hwmgr_func->set_fan_speed_rpm == NULL) {
> > > -		pr_info_ratelimited("%s was not implemented.\n",
> > __func__);
> > > -		return 0;
> > > -	}
> > >    	mutex_lock(&hwmgr->smu_lock);
> > >    	ret = hwmgr->hwmgr_func->set_fan_speed_rpm(hwmgr, rpm);
> > >    	mutex_unlock(&hwmgr->smu_lock);
> > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > index c374c3067496..474f1f04cc96 100644
> > > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > @@ -59,7 +59,7 @@ static int smu_handle_task(struct smu_context *smu,
> > >    			   bool lock_needed);
> > >    static int smu_reset(struct smu_context *smu);
> > >    static int smu_set_fan_speed_pwm(void *handle, u32 speed); -static
> > > int smu_set_fan_control_mode(struct smu_context *smu, int value);
> > > +static int smu_set_fan_control_mode(void *handle, u32 value);
> > >    static int smu_set_power_limit(void *handle, uint32_t limit);
> > >    static int smu_set_fan_speed_rpm(void *handle, uint32_t speed);
> > >    static int smu_set_gfx_cgpg(struct smu_context *smu, bool enabled);
> > > @@ -407,7 +407,7 @@ static void smu_restore_dpm_user_profile(struct
> > smu_context *smu)
> > >    	if (smu->user_dpm_profile.fan_mode ==
> > AMD_FAN_CTRL_MANUAL ||
> > >    	    smu->user_dpm_profile.fan_mode == AMD_FAN_CTRL_NONE) {
> > >    		ret = smu_set_fan_control_mode(smu, smu-
> > > user_dpm_profile.fan_mode);
> > > -		if (ret) {
> > > +		if (ret != -EOPNOTSUPP) {
> > >    			smu->user_dpm_profile.fan_speed_pwm = 0;
> > >    			smu->user_dpm_profile.fan_speed_rpm = 0;
> > >    			smu->user_dpm_profile.fan_mode =
> > AMD_FAN_CTRL_AUTO; @@ -416,13
> > > +416,13 @@ static void smu_restore_dpm_user_profile(struct
> > smu_context
> > > *smu)
> > >
> > >    		if (smu->user_dpm_profile.fan_speed_pwm) {
> > >    			ret = smu_set_fan_speed_pwm(smu, smu-
> > > user_dpm_profile.fan_speed_pwm);
> > > -			if (ret)
> > > +			if (ret != -EOPNOTSUPP)
> > >    				dev_err(smu->adev->dev, "Failed to set
> > manual fan speed in pwm\n");
> > >    		}
> > >
> > >    		if (smu->user_dpm_profile.fan_speed_rpm) {
> > >    			ret = smu_set_fan_speed_rpm(smu, smu-
> > > user_dpm_profile.fan_speed_rpm);
> > > -			if (ret)
> > > +			if (ret != -EOPNOTSUPP)
> > >    				dev_err(smu->adev->dev, "Failed to set
> > manual fan speed in rpm\n");
> > >    		}
> > >    	}
> > > @@ -2218,18 +2218,19 @@ static int smu_set_fan_speed_rpm(void
> > *handle, uint32_t speed)
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (!smu->ppt_funcs->set_fan_speed_rpm)
> > > +		return -EOPNOTSUPP;
> > > +
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->set_fan_speed_rpm) {
> > > -		ret = smu->ppt_funcs->set_fan_speed_rpm(smu, speed);
> > > -		if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > -			smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_RPM;
> > > -			smu->user_dpm_profile.fan_speed_rpm = speed;
> > > +	ret = smu->ppt_funcs->set_fan_speed_rpm(smu, speed);
> > > +	if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > +		smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_RPM;
> > > +		smu->user_dpm_profile.fan_speed_rpm = speed;
> > >
> > > -			/* Override custom PWM setting as they cannot co-
> > exist */
> > > -			smu->user_dpm_profile.flags &=
> > ~SMU_CUSTOM_FAN_SPEED_PWM;
> > > -			smu->user_dpm_profile.fan_speed_pwm = 0;
> > > -		}
> > > +		/* Override custom PWM setting as they cannot co-exist */
> > > +		smu->user_dpm_profile.flags &=
> > ~SMU_CUSTOM_FAN_SPEED_PWM;
> > > +		smu->user_dpm_profile.fan_speed_pwm = 0;
> > >    	}
> > >
> > >    	mutex_unlock(&smu->mutex);
> > > @@ -2562,60 +2563,59 @@ static int smu_set_power_profile_mode(void
> > *handle,
> > >    }
> > >
> > >
> > > -static u32 smu_get_fan_control_mode(void *handle)
> > > +static int smu_get_fan_control_mode(void *handle, u32 *fan_mode)
> > >    {
> > >    	struct smu_context *smu = handle;
> > > -	u32 ret = 0;
> > >
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > > -		return AMD_FAN_CTRL_NONE;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (!smu->ppt_funcs->get_fan_control_mode)
> > > +		return -EOPNOTSUPP;
> > >
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->get_fan_control_mode)
> > > -		ret = smu->ppt_funcs->get_fan_control_mode(smu);
> > > +	*fan_mode = smu->ppt_funcs->get_fan_control_mode(smu);
> > >
> > >    	mutex_unlock(&smu->mutex);
> > >
> > > -	return ret;
> > > +	return 0;
> > >    }
> > >
> > > -static int smu_set_fan_control_mode(struct smu_context *smu, int
> > > value)
> > > +static int smu_set_fan_control_mode(void *handle, u32 value)
> > >    {
> > > +	struct smu_context *smu = handle;
> > >    	int ret = 0;
> > >
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > > -		return  -EOPNOTSUPP;
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	if (!smu->ppt_funcs->set_fan_control_mode)
> > > +		return -EOPNOTSUPP;
> > >
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->set_fan_control_mode) {
> > > -		ret = smu->ppt_funcs->set_fan_control_mode(smu, value);
> > > -		if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE))
> > > -			smu->user_dpm_profile.fan_mode = value;
> > > -	}
> > > +	ret = smu->ppt_funcs->set_fan_control_mode(smu, value);
> > > +	if (ret)
> > > +		goto out;
> > >
> > > -	mutex_unlock(&smu->mutex);
> > > +	if (!(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > +		smu->user_dpm_profile.fan_mode = value;
> > >
> > > -	/* reset user dpm fan speed */
> > > -	if (!ret && value != AMD_FAN_CTRL_MANUAL &&
> > > -			!(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > -		smu->user_dpm_profile.fan_speed_pwm = 0;
> > > -		smu->user_dpm_profile.fan_speed_rpm = 0;
> > > -		smu->user_dpm_profile.flags &=
> > ~(SMU_CUSTOM_FAN_SPEED_RPM | SMU_CUSTOM_FAN_SPEED_PWM);
> > > +		/* reset user dpm fan speed */
> > > +		if (value != AMD_FAN_CTRL_MANUAL) {
> > > +			smu->user_dpm_profile.fan_speed_pwm = 0;
> > > +			smu->user_dpm_profile.fan_speed_rpm = 0;
> > > +			smu->user_dpm_profile.flags &=
> > ~(SMU_CUSTOM_FAN_SPEED_RPM | SMU_CUSTOM_FAN_SPEED_PWM);
> > > +		}
> > >    	}
> > >
> > > -	return ret;
> > > -}
> > > -
> > > -static void smu_pp_set_fan_control_mode(void *handle, u32 value) -{
> > > -	struct smu_context *smu = handle;
> > > +out:
> > > +	mutex_unlock(&smu->mutex);
> > >
> > > -	smu_set_fan_control_mode(smu, value);
> > > +	return ret;
> > >    }
> > >
> > > -
> > >    static int smu_get_fan_speed_pwm(void *handle, u32 *speed)
> > >    {
> > >    	struct smu_context *smu = handle;
> > > @@ -2624,10 +2624,12 @@ static int smu_get_fan_speed_pwm(void
> > *handle, u32 *speed)
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (!smu->ppt_funcs->get_fan_speed_pwm)
> > > +		return -EOPNOTSUPP;
> > > +
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->get_fan_speed_pwm)
> > > -		ret = smu->ppt_funcs->get_fan_speed_pwm(smu, speed);
> > > +	ret = smu->ppt_funcs->get_fan_speed_pwm(smu, speed);
> > >
> > >    	mutex_unlock(&smu->mutex);
> > >
> > > @@ -2642,18 +2644,19 @@ static int smu_set_fan_speed_pwm(void
> > *handle, u32 speed)
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (!smu->ppt_funcs->set_fan_speed_pwm)
> > > +		return -EOPNOTSUPP;
> > > +
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->set_fan_speed_pwm) {
> > > -		ret = smu->ppt_funcs->set_fan_speed_pwm(smu, speed);
> > > -		if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > -			smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_PWM;
> > > -			smu->user_dpm_profile.fan_speed_pwm = speed;
> > > +	ret = smu->ppt_funcs->set_fan_speed_pwm(smu, speed);
> > > +	if (!ret && !(smu->user_dpm_profile.flags &
> > SMU_DPM_USER_PROFILE_RESTORE)) {
> > > +		smu->user_dpm_profile.flags |=
> > SMU_CUSTOM_FAN_SPEED_PWM;
> > > +		smu->user_dpm_profile.fan_speed_pwm = speed;
> > >
> > > -			/* Override custom RPM setting as they cannot co-
> > exist */
> > > -			smu->user_dpm_profile.flags &=
> > ~SMU_CUSTOM_FAN_SPEED_RPM;
> > > -			smu->user_dpm_profile.fan_speed_rpm = 0;
> > > -		}
> > > +		/* Override custom RPM setting as they cannot co-exist */
> > > +		smu->user_dpm_profile.flags &=
> > ~SMU_CUSTOM_FAN_SPEED_RPM;
> > > +		smu->user_dpm_profile.fan_speed_rpm = 0;
> > >    	}
> > >
> > >    	mutex_unlock(&smu->mutex);
> > > @@ -2669,10 +2672,12 @@ static int smu_get_fan_speed_rpm(void
> > *handle, uint32_t *speed)
> > >    	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled)
> > >    		return -EOPNOTSUPP;
> > >
> > > +	if (!smu->ppt_funcs->get_fan_speed_rpm)
> > > +		return -EOPNOTSUPP;
> > > +
> > >    	mutex_lock(&smu->mutex);
> > >
> > > -	if (smu->ppt_funcs->get_fan_speed_rpm)
> > > -		ret = smu->ppt_funcs->get_fan_speed_rpm(smu, speed);
> > > +	ret = smu->ppt_funcs->get_fan_speed_rpm(smu, speed);
> > >
> > >    	mutex_unlock(&smu->mutex);
> > >
> > > @@ -3101,7 +3106,7 @@ static int smu_get_prv_buffer_details(void
> > > *handle, void **addr, size_t *size)
> > >
> > >    static const struct amd_pm_funcs swsmu_pm_funcs = {
> > >    	/* export for sysfs */
> > > -	.set_fan_control_mode    = smu_pp_set_fan_control_mode,
> > > +	.set_fan_control_mode    = smu_set_fan_control_mode,
> > >    	.get_fan_control_mode    = smu_get_fan_control_mode,
> > >    	.set_fan_speed_pwm   = smu_set_fan_speed_pwm,
> > >    	.get_fan_speed_pwm   = smu_get_fan_speed_pwm,