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
