Received-SPF: pass (google.com: domain of linux-wireless+bounces-412-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <67fe48be-8e10-47f7-bf47-e819ed02e732@amd.com>
Date: Tue, 5 Dec 2023 15:22:07 +0800
User-Agent: Mozilla Thunderbird
Cc: majun@amd.com, Evan Quan <quanliangl@hotmail.com>
Subject: Re: [PATCH v14 2/9] platform/x86/amd: Add support for AMD ACPI based
 Wifi band RFI mitigation feature
To: Hans de Goede <hdegoede@redhat.com>, Ma Jun <Jun.Ma2@amd.com>,
 amd-gfx@lists.freedesktop.org, lenb@kernel.org, johannes@sipsolutions.net,
 davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
 pabeni@redhat.com, alexander.deucher@amd.com, Lijo.Lazar@amd.com,
 mario.limonciello@amd.com, netdev@vger.kernel.org,
 linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-doc@vger.kernel.org, platform-driver-x86@vger.kernel.org
References: <20231129091348.3972539-1-Jun.Ma2@amd.com>
 <20231129091348.3972539-3-Jun.Ma2@amd.com>
 <99de0223-6d28-4379-ac2a-ef093ee0386c@redhat.com>
Content-Language: en-US
From: "Ma, Jun" <majun@amd.com>
In-Reply-To: <99de0223-6d28-4379-ac2a-ef093ee0386c@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?dk15T1h0Q2gyU2tleWZQbG9qaWZoUExHWCtacmdXTUMxN1d3eGFWVllnVnFO?=
 =?utf-8?B?ZUxUZGZwclY2aUUwYVh2bXhsZ09oZUdIS3p5ZGVSN1VuVDdza2RGaTVYOUoz?=
 =?utf-8?B?ZGRiVlZvdWE5UnN5L1IwOVg2eVBoa2E2N2tnUHBoTVE4cEx1UVJhZ2JxVXc3?=
 =?utf-8?B?TWwrMWJ6MmFNUDRrSzRUY0FRQWdYODlYQ0IzZFZac1YxU29tVFN3OE51OVRy?=
 =?utf-8?B?UlJwaUdYcGtWeXd4NjRrUE96MElialNGQysydEJCZ3JZNENWZ0Y3TG1wNTRy?=
 =?utf-8?B?UkNZUXRzTFlkMnVEa3dCVDYzRW5ERXVxVUdqQkI5V2MyWUtQYnBrdVYrMk1n?=
 =?utf-8?B?WkJmODNtcDF1MnNXWHl6Zy8zblRhUG1Ca0Y1UCtnVTBYNk1lVW9FOGV2bW4w?=
 =?utf-8?B?RUpORmRzMzBnZVEvbmpyUytxbnpGeWpHR2JXUDdCakJQWmtzMXNidi9LQkF0?=
 =?utf-8?B?TmZNbXVtZXhKZkcxSis0Z0FXdGRpb3RIYkVZY0M5cGZpQkY0Ynp1VVQvejBT?=
 =?utf-8?B?bUhpWmNjQ3FFYkpQdUZ6TVFHZm9xbmx2elJOQlg5LzA0V0ZkQmw4aWNiQlhE?=
 =?utf-8?B?ckNZZUgwM3RUYVdTMzFodUsySkNoRVRhbTBpUlJ0amxRNTMrYjcyY3ZTaE02?=
 =?utf-8?B?TE40R25nWHVBQ09iN0FLZG92UytzanRjRHkrNEtaNjJ5WEIxckRiSWZnSE9o?=
 =?utf-8?B?bmZMd3R0eDRPRGhWTnlkc3VXRlNhNk11YWgweVRtTFZYdUdNamdoUC9tSlRD?=
 =?utf-8?B?RnVJRWQzRCtIR3BnNFd6dGZrbmJpR1VuTDNrRXFaZ1dGYzMzbkRkVGY3cXpR?=
 =?utf-8?B?WEVDVno2alNRcHlyUmkzNkJiaVo5VHR1V1F5YUdiRzJwdkt1bUZ2WjRxUEdH?=
 =?utf-8?B?OUdLUDdqcFkvTnBNa1F4dms3RTQ2SExJNTlyeTZxc1dMSTNhOFZkc1FEVDlk?=
 =?utf-8?B?cE42aFBjUlNiUnVheDZZOEUrTWRCb1lkei95MlBXb1NzZjZTbGZoUU9ZMlJN?=
 =?utf-8?B?VlNUcUx5Ukw0ZDI1d2Qwekd2WVMzT1BFdE5zY2JvbTI2L0FXSVAxSFM4TXE3?=
 =?utf-8?B?REdhSmJPbHBJS2tlR256Zkp2ZThCeDg4bWoxOWdMVGdsdkN2NUlGcFYxM1h5?=
 =?utf-8?B?MFg0bE05UUlhbmkyWGNpYmxEZVd6M0xISlJoZXZnMEtnZHhEVURPckhCYlAv?=
 =?utf-8?B?U25wdEJzZGt5SHRBbXRndyt0QVhyeHJiMnV2Rzh2Y0lZK0NqdjJvckZnOTQ3?=
 =?utf-8?B?dDhYQlVTcVBrN05lU0k0OFc2VVZSZ1pnaW0rOFdLdytJVzAvVXEyMlhZMzBy?=
 =?utf-8?B?a2RML2EzV1FGZzEzcEMwclREN2NwakFxNDBUNTBiTUxZSkh3Y2RtMnhxdWxu?=
 =?utf-8?B?d1ppZjNLemhmWkRQWkk0R050eDNNL0k2Y2p4Qzg2TnEvL3ZBckZRYVFkRXZP?=
 =?utf-8?B?SXhOYU04K3o0OFpkRk5PNlhPRUE0RkdKTTNmOHFsRlpmZlA5WldpeDBZVDRk?=
 =?utf-8?B?d3B2RGZEWG1vVlZNakdGSWFvRWNqZmkyN0VOTEcxOWptd29ia0NMM255OEhS?=
 =?utf-8?B?cnEvbUJlOFNScCt3N1F0NGlwZUJ6a3lXSVQwYzBYMVBqSGd4VEZQMGliTGhk?=
 =?utf-8?B?NGlxWlBoOEdreXkyeEd0VjFvVnQzTmEvdjVIRWRpT2p3MlE5d2xycTJLNDdD?=
 =?utf-8?B?aFhKNndxUmJnUlA2NkI0N0U4c1F4S05pTVdKY2k0ZU53NE04cFVOTmVseWdO?=
 =?utf-8?B?cEZ0N3ZiSysrL0RnNGZQWWNheUNBMGIxQ0h2V2dLeWNYUEk5WTJOWE5uTlc1?=
 =?utf-8?B?Y2lMaGxjQ1dFTVNzNGNaenl3WENZS01tall6S2lSQzE1ZHp4M0ZqckVwRGwz?=
 =?utf-8?B?VXR1cW8rVUJEaEp3N1VrM3F2ZlExcjQvQTd0RzBoUUlNWnZOUnhlSkNOa3NQ?=
 =?utf-8?B?YVgwRlNqUUJUeW9MTjBTYThFem9hOTZQN0twSEpybmpvODVBU2k0ZFhKZ1VG?=
 =?utf-8?B?Q1hzK2VTbTl4T0t5VDl0TGFNWThnZG4vd0dhOVRzVTlSWEtJdHFqL3NBcTN1?=
 =?utf-8?B?UXhpWDlhUFg0V1M0bUVlOWprZkFtY053bVhjYjl1aDh5V0J4T09jMHhGd3gw?=
 =?utf-8?Q?b2oo8KXryhj0GBhXWSBTU5SJr?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6b29fc74-337c-4d8d-f8cf-08dbf562eae6
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB6351.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Dec 2023 07:22:20.1019
 (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: VPndJ1BryngIR45G1rfY9+DLFRPMBmIB3oXXrrqY8GyVBOSuP0IDaPIYVD74g+J8
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR12MB4317

Hi Hans,

On 12/4/2023 9:00 PM, Hans de Goede wrote:
> Hi,
> 
> On 11/29/23 10:13, Ma Jun wrote:
>> Due to electrical and mechanical constraints in certain platform designs
>> there may be likely interference of relatively high-powered harmonics of
>> the (G-)DDR memory clocks with local radio module frequency bands used
>> by Wifi 6/6e/7.
>>
>> To mitigate this, AMD has introduced a mechanism that devices can use to
>> notify active use of particular frequencies so that other devices can make
>> relative internal adjustments as necessary to avoid this resonance.
>>
>> Co-developed-by: Evan Quan <quanliangl@hotmail.com>
>> Signed-off-by: Evan Quan <quanliangl@hotmail.com>
>> Signed-off-by: Ma Jun <Jun.Ma2@amd.com>
>>
>> --
>> v11:
>>  - fix typo(Simon)
>> v12:
>>  - Fix the code logic (Rafael)
>>  - Move amd_wbrf.c to drivers/platform/x86/amd/wbrf.c
>>  - Updated Evan's email because he's no longer at AMD.Thanks
>> for his work in earlier versions.
>> v13:
>>  - Fix the format issue (IIpo Jarvinen)
>>  - Add comment for some functions
>> v14:
>>  - Use the apci_check_dsm and acpi_evaluate_dsm (Hans de Goede)
> 
> Thank you this is much better.
> 
> I notice that the #define ACPI_AMD_WBRF_METHOD	"\\WBRF"
> still exists though and that this is still used in
> static bool acpi_amd_wbrf_supported_system(void).
> 
> I think it might be better to just remove
> these 2 all together.
> 
> Checking if a DSM with the expected GUID is present
> and if that has the correct bits set in its supported
> mask should be enough.
> 
> And on future systems the implementer may decide to
> not have a WBRF helper function at all and instead
> handle everything in the _DSM method.
> 
> So the "\\WBRF" check seems to be checking for
> what really is an implementation detail.
> 
Yes,you are right. I will fix these issues.
Thanks for your review and suggestion. 

Regards,
Ma Jun
> 2 other very small remark
> 
>> +/**
>> + * acpi_amd_wbrf_supported_producer - determine if the WBRF can be enabled
>> + *                                    for the device as a producer
>> + *
>> + * @dev: device pointer
>> + *
>> + * Check if the platform equipped with necessary implementations to
>> + * support WBRF for the device as a producer.
>> + *
>> + * Return:
>> + * true if WBRF is supported, otherwise returns false
>> + */
>> +bool acpi_amd_wbrf_supported_producer(struct device *dev)
>> +{
>> +	struct acpi_device *adev;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +	if (!adev)
>> +		return false;
>> +
>> +	if (!acpi_amd_wbrf_supported_system())
>> +		return false;
>> +
>> +
>> +	return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
>> +			      WBRF_REVISION, BIT(WBRF_RECORD));
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_producer);
> 
> Please don't use double empty lines, one empty line to separate things
> is enough.
> 
>> +
>> +/**
>> + * acpi_amd_wbrf_supported_consumer - determine if the WBRF can be enabled
>> + *                                    for the device as a consumer
>> + *
>> + * @dev: device pointer
>> + *
>> + * Determine if the platform equipped with necessary implementations to
>> + * support WBRF for the device as a consumer.
>> + *
>> + * Return:
>> + * true if WBRF is supported, otherwise returns false.
>> + */
>> +bool acpi_amd_wbrf_supported_consumer(struct device *dev)
>> +{
>> +	struct acpi_device *adev;
>> +
>> +	adev = ACPI_COMPANION(dev);
>> +	if (!adev)
>> +		return false;
>> +
>> +	if (!acpi_amd_wbrf_supported_system())
>> +		return false;
>> +
>> +	return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
>> +			      WBRF_REVISION, BIT(WBRF_RETRIEVE));
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_consumer);
>> +
>> +/**
>> + * amd_wbrf_retrieve_freq_band - retrieve current active frequency
>> + *                                     bands
> 
> You may go a bit over the 80 chars limit, please just make this
> a single line:
> 
>  * amd_wbrf_retrieve_freq_band - retrieve current active frequency bands
> 
>> + *
>> + * @dev: device pointer
>> + * @out: output structure containing all the active frequency bands
>> + *
>> + * Retrieve the current active frequency bands which were broadcasted
>> + * by other producers. The consumer who calls this API should take
>> + * proper actions if any of the frequency band may cause RFI with its
>> + * own frequency band used.
>> + *
>> + * Return:
>> + * 0 for getting wifi freq band successfully.
>> + * Returns a negative error code for failure.
>> + */
> 
> Regards,
> 
> Hans
>