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

[Limonciello, Mario]

[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