Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking minimum SMU version
From: Mario Limonciello
Date: Wed Mar 01 2023 - 21:54:55 EST
On 3/1/23 20:50, Shyam Sundar S K wrote:
On 3/2/2023 8:12 AM, Mario Limonciello wrote:
On 3/1/23 20:39, Shyam Sundar S K wrote:
Hi Mario,
On 3/1/2023 9:01 PM, Limonciello, Mario wrote:
[Public]
-----Original Message-----
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Sent: Wednesday, March 1, 2023 09:28
To: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; S-k, Shyam-sundar
<Shyam-sundar.S-k@xxxxxxx>
Cc: Mark Gross <markgross@xxxxxxxxxx>; platform-driver-
x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
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@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.
@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().
OK yeah, then I'll split this correction of the logic off to that in a
separate patch to make this one "really no intended functional changes".
Like we discussed off-list, we should have this boilerplate in place, so
that the future checks would not be duplicated.
Something else I noticed that we probably need to consider is that there
is no examination for the "program" version which may be important.
We don't have any version checks for YC, but if we did for example YC A0
and YC B0 use program "0" or program "4" respectively so version checks
could fall over.
Checking for "program" version may not be required as A0/B0 are never
meant for production and IMO its a logical overhead.
Do you have a specific case, were you felt the real usage of "program"
version?
A0/B0 of YC was just one example. The reason I mention it is I think
technically it's possible that A0 is leveraged for an embedded product.
I know similar thing had occurred for Renoir leveraged products too.
I'll add something like this in for v2 of the patch as well.
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