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

[Shyam Sundar S K <Shyam-sundar.S-k@amd.com>]

Hi Mario,

On 3/1/2023 9:01 PM, Limonciello, Mario wrote:
> [Public]
> 
> 
> 
>> -----Original Message-----
>> From: Hans de Goede <hdegoede@redhat.com>
>> Sent: Wednesday, March 1, 2023 09:28
>> To: Limonciello, Mario <Mario.Limonciello@amd.com>; S-k, Shyam-sundar
>> <Shyam-sundar.S-k@amd.com>
>> Cc: Mark Gross <markgross@kernel.org>; platform-driver-
>> x86@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking
>> minimum SMU version
>>
>> 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@amd.com>
>>> ---
>>>  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.
> 
> @S-k, Shyam-sundar as the original author of that, can you please confirm?

I cannot completely recall :-) It was something like if the major
version is greater than 56, there is no need to check the other part of
the "OR".

which is kind of similar to what you are now doing in
amd_pmc_verify_min_version().

Like we discussed off-list, we should have this boilerplate in place, so
that the future checks would not be duplicated.

Thanks,
Shyam

> 
>>
>> After verifying please post a v2 updating the commit message to
>> point out this functional change.
>>
> 
> Sure, thanks.
> 
>>> +	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#  ?
> 
> Yes please.  If it makes it easier I can re-order the series so that
> we add a check in 1/2 and switch to the helper as 2/2.
> 
> This might make it easier to take the LTS kernel too for stable,
> but I don't feel strongly.
> 
>>
>> Regards,
>>
>> Hans