Message-ID: <727bdbb5-50cd-aebc-72e4-5849ae2c6f3f@amd.com>
Date:   Wed, 1 Mar 2023 20:42:30 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.1
Subject: Re: [PATCH 1/2] platform/x86/amd: pmc: Add a helper for checking
 minimum SMU version
Content-Language: en-US
To:     Shyam Sundar S K <Shyam-sundar.S-k@amd.com>,
        Hans de Goede <hdegoede@redhat.com>
Cc:     Mark Gross <markgross@kernel.org>,
        "platform-driver-x86@vger.kernel.org" 
        <platform-driver-x86@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <20230301150821.9791-1-mario.limonciello@amd.com>
 <846d6f46-07ab-4d4a-5623-417a469c3d0b@redhat.com>
 <MN0PR12MB6101781FC5089EA217CA48A9E2AD9@MN0PR12MB6101.namprd12.prod.outlook.com>
 <e4a8f2ef-d6bf-1b14-ddaf-057fcd5abf11@amd.com>
From:   Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <e4a8f2ef-d6bf-1b14-ddaf-057fcd5abf11@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U1FMMVZKeHV6ZHVXRSsranRKcTNpZ3pvRnJzOG84Vi9uMExRZXp1WUczYzE4?=
 =?utf-8?B?T1dtQzhrRjl3N2M1ZGtVS3FjSG9jcnZQVnVGbFZqVWk1c0xES256Y3V4Ujlk?=
 =?utf-8?B?UXhXSWpIMlVNcHJ6VGgrckRqQ0l1UDNzWDRxRlhkSWljRVNLK1JOaHVZOWFv?=
 =?utf-8?B?QUM4bkM3cDBodWRRM1ZITHgzbHg5YTJ2K1FNRFVYL0JxRE54Z2NMYVNrdlR1?=
 =?utf-8?B?Snpac0ZYTGt0R2JBUlpJWHVQUDlUU3hMUDdySmFzUUg2V0gxQUdBT2dzSEh5?=
 =?utf-8?B?VlVnZC9xUklXL2Q2cHBkaitrQVpTZ1hFUkpYanU2bDA5T2k4bGYwQTJzYmVN?=
 =?utf-8?B?TlF4Vzk3cFdmaldZOW9WYU5pL1JjRWUzQzZuV2dkQ2hVNmg4TzJ1ZVR0SURn?=
 =?utf-8?B?QjFJeHMzdDh5d1BsMyswNWRkVjQ1QWpTM1Qvb3RaWWlNeTJuNUpsUGlSSnNK?=
 =?utf-8?B?Y1VpUS9EOTFkalNScDlaMHc5Qm9VRkpML1FnejF3SlBXWHY5UktYcDRHRDFl?=
 =?utf-8?B?bk81NnIxYkw1Nkl4dm1adk84VHRtUm9BdHVoZGhrQUd0blV5N01VQzRiellz?=
 =?utf-8?B?RzJLRmRVVkw3eTB6eVl3QkhOVC90dmRrck95QzVYRXNXWHZINCtjVXU4aXB0?=
 =?utf-8?B?L1BrZ0FaWVVDcm9OcExDRXFReU12S0UvZXRuaEkwM2J2YmhVcmNEMEZjVUJm?=
 =?utf-8?B?cXMyWXIvYW0rZHhVNTVtcGcvdEE5NmZWcGF2ZmpyZkFaTVM3czFzcXZpc0cr?=
 =?utf-8?B?LysySWN3S0JhR05tdS9mc2ExK0ZubVV3U2xWNllpYUJxQmEvcnY2SHVTUVh0?=
 =?utf-8?B?dGRpUlVUNHpvZ2hTTEhpY0NJNndJVWpQenpNVERoNFAvSXdnUG56UTJGWmZu?=
 =?utf-8?B?bElzejBscklUREZrYUFyL0pHUVdpaGx1eG95Z3psZjBscDZabnJ2TUF5OTRU?=
 =?utf-8?B?Nk13MXRpbW43K1kzSlNqR1dka3ozL1FDOXdnbzZwK2MrVVFlUlZKaTkrazYy?=
 =?utf-8?B?cUhwK1hqR21ZZk9pODFVYWEwcGhFUERwaWQyOGhRZ1RmV3pJVGdBcjNlWWZ6?=
 =?utf-8?B?REliOVFlek1DUUV1aWFKSFJYVjZpTUttdzNsZVNsMitPSVhLZ1VyRWVXZlZs?=
 =?utf-8?B?M1VmbGF4VG5YSTNxMHhtd0JXMC95azZLZlB6b3BwUFkrZ3ZtWXp4MVBjdlRQ?=
 =?utf-8?B?RXZjY2FpbnQzNy9QQVVLaERzS2czbnMyczlEUzdjNjJuSU9URE04Z0Vib1VS?=
 =?utf-8?B?dlhIVElYUjNqTUUrUnV0enVNSDRmYkFHdjhDbDUwMG5oUlFzUk4zcWhWQllD?=
 =?utf-8?B?NG1VOTBhL0t3R2pRb0ZpTkRsSFZlc0NIMndrREZVSThvdlE0emE2UXlBYkkv?=
 =?utf-8?B?UGZ4aGFQT1N6M1AxUmZ5dVVQd2RKQmJERlpOQlorMTUzdzdFU2N5TC9HV1pE?=
 =?utf-8?B?N1lsT1lMVFZGMkZvK0VHejNOMDFiSVZxTklweDdRWWdOcHdNcERhd3JrUXNv?=
 =?utf-8?B?ZXZXMDVrSWVXelZHN00wRGlPV09zUVlTKytUMXRBcEVUV2NiZ21YSE1WYVY2?=
 =?utf-8?B?R25McXRubG9ZNWljRFdjWHNxS0ZKREVRSnBXY0pTU1pteFVIcTh2R0hFS0Jj?=
 =?utf-8?B?L3ZkczRWVkFvaXNmNytRUnJEL0RzWW01Qk5ESlovOUZaNEdZdjU5eUhtSjRN?=
 =?utf-8?B?aEM3Q2lJcVJCSktFRkRDT2JkSlN2NmFxVUE2VTdyYTcvVEJFcG91WVJmOS9r?=
 =?utf-8?B?c1pma3JpYkFzd240Y3FQWHJVOGlyODVIVEV3WW1MbmsyTHEwRlhwZ1prS3Ev?=
 =?utf-8?B?TW9GWkRwUjVPVU4rQUFtRWtrdWl0RlVDeEd5OUJ6NldGNzVFV1l2ZVVzMzgw?=
 =?utf-8?B?OWxoMGtEL1pEbDBuYmMzeVI2QUtpaVdqUlBhcnh0eUFmSjZQRnJoSEdlYURy?=
 =?utf-8?B?MlRENndCdExjdWw0Q2o2Qlo0Q2NwZkhXeVdRQk9saW1YN3pnNUNiTGE2cUNZ?=
 =?utf-8?B?Um5yS05jL1VWbzk5SVA1ZG1xOGxZdDh1cUxwMCtxeWFGY0c5SDZ3eGFpQUxo?=
 =?utf-8?B?OTJHL2R2Rk45OHBIZDZ5MGRMV1ZvWkE5V3ZyZlRIcnNIV2MyUWJhekMvTHJw?=
 =?utf-8?B?Njh6VXFGWDJ1WkNsd0ZkaDhWZk0wMmNlK1YwNlRHRUZ5UzZNUHFTRTJlM3dq?=
 =?utf-8?Q?gj4pZOGC5r3aFg0s3wkTWBPjZbX97YO8y7qzeE5qQFQI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 29a45a07-9973-4ae5-0202-08db1ac7c6a3
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Mar 2023 02:42:33.8161
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uPSPEcykSvTJBWWDWnssWrNWk5P6yFRYTit7G7g6IrOf/V1m9ylkodMW3nzsUSVhQ+PEyjWDrgJhCj3ukfs41A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7439
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@redhat.com>
>>> Sent: Wednesday, March 1, 2023 09:28
>>> To: Limonciello, Mario <Mario.Limonciello@amd.com>; S-k, Shyam-sundar
>>> <Shyam-sundar.S-k@amd.com>
>>> Cc: Mark Gross <markgross@kernel.org>; platform-driver-
>>> x86@vger.kernel.org; linux-kernel@vger.kernel.org
>>> 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@amd.com>
>>>> ---
>>>>   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?
> 
> 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.

I'll add something like this in for v2 of the patch as well.

> 
> Thanks,
> Shyam
> 
>>
>>>
>>> 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