linux-kernel - Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF mitigations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a80c215a-c1d9-4c76-d4a8-9b5fd320a2b1@amd.com>
Date:   Wed, 21 Jun 2023 12:43:48 -0500
From:   "Limonciello, Mario" <mario.limonciello@....com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Johannes Berg <johannes@...solutions.net>,
        Evan Quan <evan.quan@....com>, rafael@...nel.org,
        lenb@...nel.org, alexander.deucher@....com,
        christian.koenig@....com, Xinhui.Pan@....com, airlied@...il.com,
        daniel@...ll.ch, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, mdaenzer@...hat.com,
        maarten.lankhorst@...ux.intel.com, tzimmermann@...e.de,
        hdegoede@...hat.com, jingyuwang_vip@....com, lijo.lazar@....com,
        jim.cromie@...il.com, bellosilicio@...il.com,
        andrealmeid@...lia.com, trix@...hat.com, jsg@....id.au,
        arnd@...db.de, linux-kernel@...r.kernel.org,
        linux-acpi@...r.kernel.org, amd-gfx@...ts.freedesktop.org,
        dri-devel@...ts.freedesktop.org, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH V4 1/8] drivers/acpi: Add support for Wifi band RF
 mitigations


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.