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.cb/drivers/platform/x86/amd/pmc.cindex 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(structamd_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, structdevice_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, ShyamAfter 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(structamd_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(structamd_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
