Hi,
On 3/1/23 16:31, 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?
>
>>
>> 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.
I'm fine with keeping the order as is, I'm also fine with swapping it :)
Regards,
Hans
