[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?
>
> 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
