Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a80c215a-c1d9-4c76-d4a8-9b5fd320a2b1@amd.com>
Date:   Wed, 21 Jun 2023 12:43:48 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.2
Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF
 mitigations
Content-Language: en-US
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Johannes Berg <johannes@sipsolutions.net>,
        Evan Quan <evan.quan@amd.com>, rafael@kernel.org,
        lenb@kernel.org, alexander.deucher@amd.com,
        christian.koenig@amd.com, Xinhui.Pan@amd.com, airlied@gmail.com,
        daniel@ffwll.ch, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, mdaenzer@redhat.com,
        maarten.lankhorst@linux.intel.com, tzimmermann@suse.de,
        hdegoede@redhat.com, jingyuwang_vip@163.com, lijo.lazar@amd.com,
        jim.cromie@gmail.com, bellosilicio@gmail.com,
        andrealmeid@igalia.com, trix@redhat.com, jsg@jsg.id.au,
        arnd@arndb.de, linux-kernel@vger.kernel.org,
        linux-acpi@vger.kernel.org, amd-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org
References: <20230621054603.1262299-1-evan.quan@amd.com>
 <20230621054603.1262299-2-evan.quan@amd.com>
 <3a7c8ffa-de43-4795-ae76-5cd9b00c52b5@lunn.ch>
 <216f3c5aa1299100a0009ddf4e95b019855a32be.camel@sipsolutions.net>
 <b1abec47-04df-4481-d680-43c5ff3cbb48@amd.com>
 <36902dda-9e51-41b3-b5fc-c641edf6f1fb@lunn.ch>
 <33d80292-e639-91d0-4d0f-3ed973f89e14@amd.com>
 <9159c3a5-390f-4403-854d-9b5e87b58d8c@lunn.ch>
From:   "Limonciello, Mario" <mario.limonciello@amd.com>
In-Reply-To: <9159c3a5-390f-4403-854d-9b5e87b58d8c@lunn.ch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bTVHRFV5K1h1d0lBK1FiRVdrYXVtTmhWWFFsdUVIZW5PdG02bkZrQ1lES1Zn?=
 =?utf-8?B?NWFKY3BJT3V2ZDZjTENvSko4WENtVG9BVnduM3g0Sm1vUnRSRUdSTVBUMlI2?=
 =?utf-8?B?eU5xc1U0YWIwNVd3dlpZcVg1eXpKVHdKY3hjK0NLb0twOFJtTWF4NVZpTHdX?=
 =?utf-8?B?WnpqSVRJb0Y1VDJncGVTZCtJenlZY1ZDSFZhY3V2bWNwNHdianA2NHQ1NmFH?=
 =?utf-8?B?QmpTRjkvZFdrdFZCSmRUakQvZ1ZRWGpIR0pJMWZMSllqc2JUMEREMnNoamJu?=
 =?utf-8?B?cFdvOE1UUk1JWVBMekxyZXA2eEdLeGR3MisybmFYMGowQzlsMXBRbWFjbUJY?=
 =?utf-8?B?ak84NFZvY3VtRTNlL2ZRMCtvVzc4UTFvdTI1LzJMdkJHbkNjekRrYlJzc0tT?=
 =?utf-8?B?V0w1TFd2N3FlT3NrblRpWWdRYmF2NWM2b3JROC9CdmwydGZ2aldPelRaRmVw?=
 =?utf-8?B?eU5PRTJtK3RjQ1FjeldPUmJVUEJqWHYrd0t4R1R4Q1BLSkdJZFRWK20vN2Y5?=
 =?utf-8?B?OU9TTE5JRTlYSloxS3VuYjJxeUV6bWJOWmthRFZmSGcrT290ZnJiQ3AvUUlI?=
 =?utf-8?B?eXRGN1ZPYnlQMzRBVmFLZHAxcyt4K012QjY1eXMrdC9UK2U1STdyLzJQTDcy?=
 =?utf-8?B?bldsWHVYcWd3Sy9IN1VCSk1GM0I2UUhsR3BTOG85VjRxdGJYM2d6ellWWXdU?=
 =?utf-8?B?K0hoRnJ3MVpYWmZKS2N5YWZiMGZKTXFLTXRQaG1OeDhKSnNHcXNrZm9qV0lG?=
 =?utf-8?B?UkU4dC9oMWVBMWQ5OVBnejFmVitXdFBCKzMzZFEyYVZkNWh0ZXhXNGFaL3lS?=
 =?utf-8?B?RklnK2JlbVdHdEtZMk15UC9NK29rY29IcHg3V3A2S0g1ZGUxUXMzRzBIL3lu?=
 =?utf-8?B?MHV3amdpSUkrU3FHQmFDVW9vOHF6eGNxb2JzM3V6SW53Vm50bWZvcFRyYlh3?=
 =?utf-8?B?RlFPeUxPbC9TUk1WRTlJZkJjQ3l1UGcrNFBhTFBaUkNBV2h3MU92cUhCMUNK?=
 =?utf-8?B?TW5sbHR1aGRjSXJjL01LYWhvR1dpUE5LUDVWamVzY1JZMzZ5blhJTG5aU2lp?=
 =?utf-8?B?RFNaR29JMjArZytwV0lxOWJ2cW01bjVrYlY4WWVaaHY4dlJvQW5RdzF1SG45?=
 =?utf-8?B?YkxHVDRoNk04NFJSRUtac0RIcElQT1Z2TktaWlRScGp1blByQXBjNUZlb2M4?=
 =?utf-8?B?Qkt4eTk3RTE1dXlUdGoyaytydEtxcW1KNytUeENPQlhKVUN6S2pNUFc2YUFP?=
 =?utf-8?B?dG1zbHBHNTg1NnRobjVpOXl6cDR5U0xUdmN2R3htd2Eva3luUHJFMThuQ3Z2?=
 =?utf-8?B?QW1zUTNoYTFIQmtFOVlmZ3dTRUJPS0k1TUZON0FoVjNvNnVJMGtsSzd6RExS?=
 =?utf-8?B?ZjU0dC80TDNheURxU1loMGZUemR0bVlFZVg1SFR5SmtWaHR6aUpmZkE3WTdw?=
 =?utf-8?B?NVRlQ2Y5WEdjN2w2a214b0t0YVpSNk04ZU9YcnZCaUlTNTM4OW5lSG9pbGFh?=
 =?utf-8?B?V0lzekk3RTdnRnVhWmpYNWJQSmFodzh1V2oyOW1qY1Rsclh2RncxVmI4bTdW?=
 =?utf-8?B?ZTB3RjBBSG1YRjEwalFPVlNqRzdUQXl3SjZGL1d3L3l6V0pUY2RMSDE2MVBp?=
 =?utf-8?B?THpvK2VJNlovSldhcGw3YkloTWRnWjlGNkJFNFFrNmFSSW9KK1BodXRYTmlS?=
 =?utf-8?B?VE5jZTgyUkQ0K1J5ZHR4T0oyYVNkNW40aDdTcmIrYnI3RnM1cHBpMlhFcWxj?=
 =?utf-8?B?b1VOOEpGQ0VvRkcvdVJmUUNVbXpTYUowaG1rbXp2Ukkxd2ErVGovTk9GcTIv?=
 =?utf-8?B?SEt4MGZBZ0FPRk9nOUtHUjN4eVNpdXh6dW9zSXorVkUxckRrWnJpNnUxdXIz?=
 =?utf-8?B?eS8zcUF4S1g2QVFJcVNkQWhzLzBLUHhKVktUQjZNM3BLS1EwZis2am1PY2o2?=
 =?utf-8?B?TVZJUTljU0dqNGt5Smt6VU5YelIydWtxb29MMGlaanFDWlg3Nzh4dVFEMkVF?=
 =?utf-8?B?VHQzK01sQ1pRNnc5UlpMRGJUeVpJeW5FQXZOdWNxWk9raVloaEo2cGQ5djRW?=
 =?utf-8?B?Y2t0eUpmc3h2Zzk5eHB5M0NITTByVDNpR0RmOFg5VFJ5OWRxbWorcUpWRk93?=
 =?utf-8?Q?5IwETTuwGcygnovam3MpX9aPg?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 688a88bf-48f9-4bad-06e8-08db727f1432
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jun 2023 17:43:52.9839
 (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: yIeV8f5e05QJv3DoPHJ/eS3vJwZi2HP+TsRiF2J7p17DhnrszqIUsIoBRwH1UCYtewPdbo0he72P7wMGFjiNXA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5804
X-Spam-Status: No, score=-1.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org


On 6/21/2023 12:26 PM, Andrew Lunn wrote:
>> I think what you're asking for is another layer of indirection
>> like CONFIG_WBRF in addition to CONFIG_ACPI_WBRF.
>>
>> Producers would call functions like wbrf_supported_producer()
>> where the source file is not guarded behind CONFIG_ACPI_WBRF,
>> but instead by CONFIG_WBRF and locally use CONFIG_ACPI_WBRF within
>> it.  So a producer could look like this:
>>
>> bool wbrf_supported_producer(struct device *dev)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>      struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>      if (adev)
>>          return check_acpi_wbrf(adev->handle,
>>                         WBRF_REVISION,
>>                         1ULL << WBRF_RECORD);
>> #endif
>>      return -ENODEV;
>>
>> }
>> EXPORT_SYMBOL_GPL(wbrf_supported_producer);
>>
>> And then adding/removing could look something like this
>>
>> int wbrf_add_exclusion(struct device *dev,
>>                 struct wbrf_ranges_in *in)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>      struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>      if (adev)
>>          return wbrf_record(adev, WBRF_RECORD_ADD, in);
>> #endif
>>      return -ENODEV;
>> }
>> EXPORT_SYMBOL_GPL(wbrf_add_exclusion);
>>
>> int wbrf_remove_exclusion(struct device *dev,
>>                 struct wbrf_ranges_in *in)
>> {
>> #ifdef CONFIG_ACPI_WBRF
>>      struct acpi_device *adev = ACPI_COMPANION(dev);
>>
>>      if (adev)
>>          return wbrf_record(adev, WBRF_RECORD_REMOVE, in);
>> #endif
>>      return -ENODEV;
>> }
>> EXPORT_SYMBOL_GPL(wbrf_remove_exclusion);
> Yes, this looks a lot better.
>
> But what about notifications?
Once you implement this it gets a lot more complex and the driver 
consumers would need
to know more about the kernel's implementation.  For example consumers 
need a
notifier block like:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index e3e2e6e3b485..146fe3c43343 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1066,6 +1066,8 @@ struct amdgpu_device {

         bool                            job_hang;
         bool                            dc_enabled;
+
+       struct notifier_block           wbrf_notifier;
  };

  static inline struct amdgpu_device *drm_to_adev(struct drm_device *ddev)

And then would need matching notifier functions like:

static int amdgpu_wbrf_frequencies_notifier(struct notifier_block *nb,
                                     unsigned long action, void *_arg)

And we'd need to set up a chain to be used in this case in the WBRF code:

static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);

int wbrf_register_notifier(struct notifier_block *nb)
{
     return blocking_notifier_chain_register(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_register_notifier);

int wbrf_unregister_notifier(struct notifier_block *nb)
{
     return blocking_notifier_chain_unregister(&wbrf_chain_head, nb);
}
EXPORT_SYMBOL_GPL(wbrf_unregister_notifier);

And consumer would need to call it, but only if CONFIG_WBRF_ACPI isn't set.

Add/remove functions can easily call something like:

blocking_notifier_call_chain(&wbrf_chain_head, action, data);

With all of this complexity and (effectively) dead code for ACPI vs non-ACPI
path I really have to ask why wouldn't a non-AMD implementation be able to
do this as ACPI?

I don't see why it couldn't be a DT/ACPI hybrid solution for ARM64.