Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version

[Hans de Goede]

Hi,

On 3/1/23 16:08, Mario Limonciello wrote:
> In a few locations there is some boilerplate code for checking
> minimum SMU version.  Switch this to a helper for this check.
> 
> No intended functional changes.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
>  drivers/platform/x86/amd/pmc.c | 49 +++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmc.c b/drivers/platform/x86/amd/pmc.c
> index 2edaae04a691..c42fa47381c3 100644
> --- a/drivers/platform/x86/amd/pmc.c
> +++ b/drivers/platform/x86/amd/pmc.c
> @@ -418,6 +418,22 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
>  	return 0;
>  }
>  
> +static bool amd_pmc_verify_min_version(struct amd_pmc_dev *pdev, int major, int minor)
> +{
> +	if (!pdev->major) {
> +		int rc = amd_pmc_get_smu_version(pdev);
> +
> +		if (rc) {
> +			dev_warn(pdev->dev, "failed to read SMU version: %d\n", rc);
> +			return false;
> +		}
> +	}
> +	if (pdev->major > major)
> +		return true;
> +
> +	return pdev->major == major && pdev->minor >= minor;
> +}
> +
>  static ssize_t smu_fw_version_show(struct device *d, struct device_attribute *attr,
>  				   char *buf)
>  {
> @@ -526,14 +542,7 @@ static int amd_pmc_idlemask_show(struct seq_file *s, void *unused)
>  	struct amd_pmc_dev *dev = s->private;
>  	int rc;
>  
> -	/* we haven't yet read SMU version */
> -	if (!dev->major) {
> -		rc = amd_pmc_get_smu_version(dev);
> -		if (rc)
> -			return rc;
> -	}
> -
> -	if (dev->major > 56 || (dev->major >= 55 && dev->minor >= 37)) {

The 2 major checks here originally were not in sync, so for major == 55
*and* major == 56 it would only succeed if minor >= 37.

Where as after this patch for major == 56 it will now always succeed.

This feels like a bug in the original code, but might have been
intentional ? Please verify this.

After verifying please post a v2 updating the commit message to
point out this functional change.

> +	if (amd_pmc_verify_min_version(dev, 55, 37)) {
>  		rc = amd_pmc_idlemask_read(dev, NULL, s);
>  		if (rc)
>  			return rc;
> @@ -686,15 +695,8 @@ static int amd_pmc_get_os_hint(struct amd_pmc_dev *dev)
>  static int amd_pmc_czn_wa_irq1(struct amd_pmc_dev *pdev)
>  {
>  	struct device *d;
> -	int rc;
>  
> -	if (!pdev->major) {
> -		rc = amd_pmc_get_smu_version(pdev);
> -		if (rc)
> -			return rc;
> -	}
> -
> -	if (pdev->major > 64 || (pdev->major == 64 && pdev->minor > 65))
> +	if (amd_pmc_verify_min_version(pdev, 64, 66))
>  		return 0;
>  
>  	d = bus_find_device_by_name(&serio_bus, NULL, "serio0");
> @@ -718,14 +720,10 @@ static int amd_pmc_verify_czn_rtc(struct amd_pmc_dev *pdev, u32 *arg)
>  	struct rtc_time tm;
>  	int rc;
>  
> -	/* we haven't yet read SMU version */
> -	if (!pdev->major) {
> -		rc = amd_pmc_get_smu_version(pdev);
> -		if (rc)
> -			return rc;
> -	}
> +	if (disable_workarounds)
> +		return 0;
>  
> -	if (pdev->major < 64 || (pdev->major == 64 && pdev->minor < 53))
> +	if (!amd_pmc_verify_min_version(pdev, 64, 53))
>  		return 0;
>  
>  	rtc_device = rtc_class_open("rtc0");
> @@ -772,13 +770,14 @@ static void amd_pmc_s2idle_prepare(void)
>  	/* Reset and Start SMU logging - to monitor the s0i3 stats */
>  	amd_pmc_setup_smu_logging(pdev);
>  
> -	/* Activate CZN specific platform bug workarounds */
> -	if (pdev->cpu_id == AMD_CPU_ID_CZN && !disable_workarounds) {
> +	switch (pdev->cpu_id) {
> +	case AMD_CPU_ID_CZN:
>  		rc = amd_pmc_verify_czn_rtc(pdev, &arg);
>  		if (rc) {
>  			dev_err(pdev->dev, "failed to set RTC: %d\n", rc);
>  			return;
>  		}
> +		break;
>  	}
>  
>  	msg = amd_pmc_get_os_hint(pdev);


Patch 2/2 looks good to me.

Should I queue v2 (once posted) up as a fix for 6.3-rc#  ?

Regards,

Hans