| Message ID | 20230630103240.1557100-1-evan.quan@amd.com |
|---|---|
| Headers | show |
| Series | Enable Wifi RFI interference mitigation feature support | expand |
On Fri, Jun 30, 2023 at 06:32:32PM +0800, Evan Quan wrote: ... > diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h > new file mode 100644 > index 000000000000..3ca95786cef5 > --- /dev/null > +++ b/include/linux/wbrf.h > @@ -0,0 +1,65 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Wifi Band Exclusion Interface > + * Copyright (C) 2023 Advanced Micro Devices > + */ > + > +#ifndef _LINUX_WBRF_H > +#define _LINUX_WBRF_H > + > +#include <linux/device.h> > + > +/* Maximum number of wbrf ranges */ > +#define MAX_NUM_OF_WBRF_RANGES 11 > + > +struct exclusion_range { > + /* start and end point of the frequency range in Hz */ > + uint64_t start; > + uint64_t end; > +}; > + > +struct exclusion_range_pool { > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > + uint64_t ref_counter[MAX_NUM_OF_WBRF_RANGES]; > +}; > + > +struct wbrf_ranges_in { > + /* valid entry: `start` and `end` filled with non-zero values */ > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > +}; > + > +struct wbrf_ranges_out { > + uint32_t num_of_ranges; > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > +} __packed; > + > +enum wbrf_notifier_actions { > + WBRF_CHANGED, > +}; Hi Evan, checkpatch suggests that u64 and u32 might be more appropriate types here, as they are Kernel types, whereas the ones use are user-space types. ...
> Drivers/subsystems contributing frequencies: > > 1) During probe, check `wbrf_supported_producer` to see if WBRF supported > for the device. What is the purpose of this stage? Why would it not be supported for this device? > +#ifdef CONFIG_WBRF > +bool wbrf_supported_producer(struct device *dev); > +int wbrf_add_exclusion(struct device *adev, > + struct wbrf_ranges_in *in); > +int wbrf_remove_exclusion(struct device *dev, > + struct wbrf_ranges_in *in); > +int wbrf_retrieve_exclusions(struct device *dev, > + struct wbrf_ranges_out *out); > +bool wbrf_supported_consumer(struct device *dev); > + > +int wbrf_register_notifier(struct notifier_block *nb); > +int wbrf_unregister_notifier(struct notifier_block *nb); > +#else > +static inline bool wbrf_supported_producer(struct device *dev) { return false; } > +static inline int wbrf_add_exclusion(struct device *adev, > + struct wbrf_ranges_in *in) { return -ENODEV; } > +static inline int wbrf_remove_exclusion(struct device *dev, > + struct wbrf_ranges_in *in) { return -ENODEV; } The normal aim of stubs is that so long as it is not expected to be fatal if the functionality is missing, the caller should not care if it is missing. So i would expect these to return 0, indicating everything worked as expected. > +static inline int wbrf_retrieve_exclusions(struct device *dev, > + struct wbrf_ranges_out *out) { return -ENODEV; } This is more complex. Ideally you want to return an empty set, so there is nothing to do. So i think the stub probably wants to do a memset and then return 0. > +static inline bool wbrf_supported_consumer(struct device *dev) { return false; } > +static inline int wbrf_register_notifier(struct notifier_block *nb) { return -ENODEV; } > +static inline int wbrf_unregister_notifier(struct notifier_block *nb) { return -ENODEV; } And these can just return 0. Andrew
> Right now there are stubs for non CONFIG_WBRF as well as other patches are > using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 patch > looks for #ifdef CONFIG_WBRF. > > I think we should pick one or the other. > > Having other subsystems #ifdef CONFIG_WBRF will make the series easier to > land through multiple trees; so I have a slight leaning in that direction. #ifdef in C files is generally not liked because it makes build testing harder. There are more permutations to build. It is better to use if (IS_ENABLED(CONFIG_WBTR)) { } so that the code is compiled, and them throw away because IS_ENABLED(CONFIG_WBTR) evaluates to false. However, if the stubs are done correctly, the driver should not care. I doubt this is used in any sort of hot path where every instruction counts. Andrew
[AMD Official Use Only - General] > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Saturday, July 1, 2023 8:25 AM > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Quan, Evan <Evan.Quan@amd.com>; rafael@kernel.org; lenb@kernel.org; > Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; > 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; Lazar, Lijo > <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 > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > > Right now there are stubs for non CONFIG_WBRF as well as other patches > > are using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 > > patch looks for #ifdef CONFIG_WBRF. > > > > I think we should pick one or the other. > > > > Having other subsystems #ifdef CONFIG_WBRF will make the series easier > > to land through multiple trees; so I have a slight leaning in that direction. > > #ifdef in C files is generally not liked because it makes build testing harder. > There are more permutations to build. It is better to use > > if (IS_ENABLED(CONFIG_WBTR)) { > } > > so that the code is compiled, and them throw away because > IS_ENABLED(CONFIG_WBTR) evaluates to false. > > However, if the stubs are done correctly, the driver should not care. I doubt > this is used in any sort of hot path where every instruction counts. OK, will update as suggested. Evan > > Andrew
[AMD Official Use Only - General] > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Saturday, July 1, 2023 8:20 AM > To: Quan, Evan <Evan.Quan@amd.com> > Cc: rafael@kernel.org; lenb@kernel.org; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Limonciello, Mario <Mario.Limonciello@amd.com>; > mdaenzer@redhat.com; maarten.lankhorst@linux.intel.com; > tzimmermann@suse.de; hdegoede@redhat.com; jingyuwang_vip@163.com; > Lazar, Lijo <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 > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > > Drivers/subsystems contributing frequencies: > > > > 1) During probe, check `wbrf_supported_producer` to see if WBRF > supported > > for the device. > > What is the purpose of this stage? Why would it not be supported for this > device? This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) does not support the wbrf adding/removing for some device, it should speak that out so that the device can be aware of that. > > > +#ifdef CONFIG_WBRF > > +bool wbrf_supported_producer(struct device *dev); int > > +wbrf_add_exclusion(struct device *adev, > > + struct wbrf_ranges_in *in); > > +int wbrf_remove_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in); > > +int wbrf_retrieve_exclusions(struct device *dev, > > + struct wbrf_ranges_out *out); bool > > +wbrf_supported_consumer(struct device *dev); > > + > > +int wbrf_register_notifier(struct notifier_block *nb); int > > +wbrf_unregister_notifier(struct notifier_block *nb); #else static > > +inline bool wbrf_supported_producer(struct device *dev) { return > > +false; } static inline int wbrf_add_exclusion(struct device *adev, > > + struct wbrf_ranges_in *in) { return - > ENODEV; } static inline > > +int wbrf_remove_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in) { return - > ENODEV; } > > The normal aim of stubs is that so long as it is not expected to be fatal if the > functionality is missing, the caller should not care if it is missing. So i would > expect these to return 0, indicating everything worked as expected. Sure, that makes sense. > > > +static inline int wbrf_retrieve_exclusions(struct device *dev, > > + struct wbrf_ranges_out *out) > { return -ENODEV; } > > This is more complex. Ideally you want to return an empty set, so there is > nothing to do. So i think the stub probably wants to do a memset and then > return 0. Right, will update it accordingly. > > > +static inline bool wbrf_supported_consumer(struct device *dev) { > > +return false; } static inline int wbrf_register_notifier(struct > > +notifier_block *nb) { return -ENODEV; } static inline int > > +wbrf_unregister_notifier(struct notifier_block *nb) { return -ENODEV; > > +} > > And these can just return 0. Will update it. Evan > > Andrew
[AMD Official Use Only - General] > -----Original Message----- > From: Limonciello, Mario <Mario.Limonciello@amd.com> > Sent: Saturday, July 1, 2023 12:41 AM > To: Quan, Evan <Evan.Quan@amd.com>; rafael@kernel.org; lenb@kernel.org; > Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; > 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; Lazar, Lijo > <Lijo.Lazar@amd.com>; jim.cromie@gmail.com; bellosilicio@gmail.com; > andrealmeid@igalia.com; trix@redhat.com; jsg@jsg.id.au; arnd@arndb.de > Cc: 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 > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > On 6/30/2023 05:32, Evan Quan 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. > > > > In order for a device to support this, the expected flow for device > > driver or subsystems: > > > > Drivers/subsystems contributing frequencies: > > > > 1) During probe, check `wbrf_supported_producer` to see if WBRF > supported > > for the device. > > 2) If adding frequencies, then call `wbrf_add_exclusion` with the > > start and end ranges of the frequencies. > > 3) If removing frequencies, then call `wbrf_remove_exclusion` with > > start and end ranges of the frequencies. > > > > Drivers/subsystems responding to frequencies: > > > > 1) During probe, check `wbrf_supported_consumer` to see if WBRF is > supported > > for the device. > > 2) Call the `wbrf_retrieve_exclusions` to retrieve the current > > exclusions on receiving an ACPI notification for a new frequency > > change. > > > > Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > Co-developed-by: Evan Quan <evan.quan@amd.com> > > Signed-off-by: Evan Quan <evan.quan@amd.com> > > -- > > v4->v5: > > - promote this to be a more generic solution with input argument taking > > `struct device` and provide better scalability to support non-ACPI > > scenarios(Andrew) > > - update the APIs naming and some other minor fixes(Rafael) > > --- > > drivers/base/Kconfig | 8 ++ > > drivers/base/Makefile | 1 + > > drivers/base/wbrf.c | 227 > ++++++++++++++++++++++++++++++++++++++++++ > > include/linux/wbrf.h | 65 ++++++++++++ > > 4 files changed, 301 insertions(+) > > create mode 100644 drivers/base/wbrf.c > > create mode 100644 include/linux/wbrf.h > > > > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index > > 2b8fd6bb7da0..5b441017b225 100644 > > --- a/drivers/base/Kconfig > > +++ b/drivers/base/Kconfig > > @@ -242,4 +242,12 @@ config FW_DEVLINK_SYNC_STATE_TIMEOUT > > command line option on every system/board your kernel is expected > to > > work on. > > > > +config WBRF > > + bool "Wifi band RF mitigation mechanism" > > + default n > > + help > > + Wifi band RF mitigation mechanism allows multiple drivers from > > + different domains to notify the frequencies in use so that hardware > > + can be reconfigured to avoid harmonic conflicts. > > + > > endmenu > > diff --git a/drivers/base/Makefile b/drivers/base/Makefile index > > 3079bfe53d04..c844f68a6830 100644 > > --- a/drivers/base/Makefile > > +++ b/drivers/base/Makefile > > @@ -26,6 +26,7 @@ obj-$(CONFIG_GENERIC_MSI_IRQ) += platform-msi.o > > obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o > > obj-$(CONFIG_GENERIC_ARCH_NUMA) += arch_numa.o > > obj-$(CONFIG_ACPI) += physical_location.o > > +obj-$(CONFIG_WBRF) += wbrf.o > > > > obj-y += test/ > > > > diff --git a/drivers/base/wbrf.c b/drivers/base/wbrf.c new file mode > > 100644 index 000000000000..2163a8ec8a9a > > --- /dev/null > > +++ b/drivers/base/wbrf.c > > @@ -0,0 +1,227 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Wifi Band Exclusion Interface > > + * Copyright (C) 2023 Advanced Micro Devices > > + * > > + */ > > + > > +#include <linux/wbrf.h> > > + > > +static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head); > > +static DEFINE_MUTEX(wbrf_mutex); > > +static struct exclusion_range_pool wbrf_pool; > > + > > +static int _wbrf_add_exclusion_ranges(struct wbrf_ranges_in *in) { > > + int i, j; > > + > > + for (i = 0; i < ARRAY_SIZE(in->band_list); i++) { > > + if (!in->band_list[i].start && > > + !in->band_list[i].end) > > + continue; > > + > > + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { > > + if (wbrf_pool.band_list[j].start == in- > >band_list[i].start && > > + wbrf_pool.band_list[j].end == in->band_list[i].end) { > > + wbrf_pool.ref_counter[j]++; > > + break; > > + } > > + } > > + if (j < ARRAY_SIZE(wbrf_pool.band_list)) > > + continue; > > + > > + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { > > + if (!wbrf_pool.band_list[j].start && > > + !wbrf_pool.band_list[j].end) { > > + wbrf_pool.band_list[j].start = in- > >band_list[i].start; > > + wbrf_pool.band_list[j].end = in- > >band_list[i].end; > > + wbrf_pool.ref_counter[j] = 1; > > + break; > > + } > > + } > > + if (j >= ARRAY_SIZE(wbrf_pool.band_list)) > > + return -ENOSPC; > > + } > > + > > + return 0; > > +} > > + > > +static int _wbrf_remove_exclusion_ranges(struct wbrf_ranges_in *in) { > > + int i, j; > > + > > + for (i = 0; i < ARRAY_SIZE(in->band_list); i++) { > > + if (!in->band_list[i].start && > > + !in->band_list[i].end) > > + continue; > > + > > + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { > > + if (wbrf_pool.band_list[j].start == in- > >band_list[i].start && > > + wbrf_pool.band_list[j].end == in->band_list[i].end) { > > + wbrf_pool.ref_counter[j]--; > > + if (!wbrf_pool.ref_counter[j]) { > > + wbrf_pool.band_list[j].start = 0; > > + wbrf_pool.band_list[j].end = 0; > > + } > > + break; > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > +static int _wbrf_retrieve_exclusion_ranges(struct wbrf_ranges_out > > +*out) { > > + int out_idx = 0; > > + int i; > > + > > + memset(out, 0, sizeof(*out)); > > + > > + for (i = 0; i < ARRAY_SIZE(wbrf_pool.band_list); i++) { > > + if (!wbrf_pool.band_list[i].start && > > + !wbrf_pool.band_list[i].end) > > + continue; > > + > > + out->band_list[out_idx].start = wbrf_pool.band_list[i].start; > > + out->band_list[out_idx++].end = wbrf_pool.band_list[i].end; > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * wbrf_supported_producer - Determine if the device can report > > +frequencies > > + * > > + * @dev: device pointer > > + * > > + * WBRF is used to mitigate devices that cause harmonic interference. > > + * This function will determine if this device needs to report such > frequencies. > > + */ > > +bool wbrf_supported_producer(struct device *dev) { > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(wbrf_supported_producer); > > + > > +/** > > + * wbrf_add_exclusion - Add frequency ranges to the exclusion list > > + * > > + * @dev: device pointer > > + * @in: input structure containing the frequency ranges to be added > > + * > > + * Add frequencies into the exclusion list for supported consumers > > + * to react to. > > + */ > > +int wbrf_add_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in) > > +{ > > + int r; > > + > > + mutex_lock(&wbrf_mutex); > > + > > + r = _wbrf_add_exclusion_ranges(in); > > + > > + mutex_unlock(&wbrf_mutex); > > + if (r) > > + return r; > > + > > + blocking_notifier_call_chain(&wbrf_chain_head, WBRF_CHANGED, > NULL); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(wbrf_add_exclusion); > > + > > +/** > > + * wbrf_remove_exclusion - Remove frequency ranges from the exclusion > > +list > > + * > > + * @dev: device pointer > > + * @in: input structure containing the frequency ranges to be removed > > + * > > + * Remove frequencies from the exclusion list for supported consumers > > + * to react to. > > + */ > > +int wbrf_remove_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in) > > +{ > > + int r; > > + > > + mutex_lock(&wbrf_mutex); > > + > > + r = _wbrf_remove_exclusion_ranges(in); > > + > > + mutex_unlock(&wbrf_mutex); > > + if (r) > > + return r; > > + > > + blocking_notifier_call_chain(&wbrf_chain_head, WBRF_CHANGED, > NULL); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(wbrf_remove_exclusion); > > + > > +/** > > + * wbrf_supported_consumer - Determine if the device can react to > > +frequencies > > + * > > + * @dev: device pointer > > + * > > + * WBRF is used to mitigate devices that cause harmonic interference. > > + * This function will determine if this device needs to react to > > +reports from > > + * other devices for such frequencies. > > + */ > > +bool wbrf_supported_consumer(struct device *dev) { > > + return true; > > +} > > +EXPORT_SYMBOL_GPL(wbrf_supported_consumer); > > + > > +/** > > + * wbrf_register_notifier - Register for notifications of frequency > > +changes > > + * > > + * @nb: driver notifier block > > + * > > + * WBRF is used to mitigate devices that cause harmonic interference. > > + * This function will allow consumers to register for frequency notifications. > > + */ > > +int wbrf_register_notifier(struct notifier_block *nb) { > > + return blocking_notifier_chain_register(&wbrf_chain_head, nb); } > > +EXPORT_SYMBOL_GPL(wbrf_register_notifier); > > + > > +/** > > + * wbrf_unregister_notifier - Unregister for notifications of > > +frequency changes > > + * > > + * @nb: driver notifier block > > + * > > + * WBRF is used to mitigate devices that cause harmonic interference. > > + * This function will allow consumers to unregister for frequency > notifications. > > + */ > > +int wbrf_unregister_notifier(struct notifier_block *nb) { > > + return blocking_notifier_chain_unregister(&wbrf_chain_head, nb); } > > +EXPORT_SYMBOL_GPL(wbrf_unregister_notifier); > > + > > +/** > > + * wbrf_retrieve_exclusions - Retrieve the exclusion list > > + * > > + * @dev: device pointer > > + * @out: output structure containing the frequency ranges to be > > +excluded > > + * > > + * Retrieve the current exclusion list */ int > > +wbrf_retrieve_exclusions(struct device *dev, > > + struct wbrf_ranges_out *out) > > +{ > > + int r; > > + > > + mutex_lock(&wbrf_mutex); > > + > > + r = _wbrf_retrieve_exclusion_ranges(out); > > + > > + mutex_unlock(&wbrf_mutex); > > + > > + return r; > > +} > > +EXPORT_SYMBOL_GPL(wbrf_retrieve_exclusions); > > diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h new file mode > > 100644 index 000000000000..3ca95786cef5 > > --- /dev/null > > +++ b/include/linux/wbrf.h > > @@ -0,0 +1,65 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Wifi Band Exclusion Interface > > + * Copyright (C) 2023 Advanced Micro Devices */ > > + > > +#ifndef _LINUX_WBRF_H > > +#define _LINUX_WBRF_H > > + > > +#include <linux/device.h> > > + > > +/* Maximum number of wbrf ranges */ > > +#define MAX_NUM_OF_WBRF_RANGES 11 > > + > > +struct exclusion_range { > > + /* start and end point of the frequency range in Hz */ > > + uint64_t start; > > + uint64_t end; > > +}; > > + > > +struct exclusion_range_pool { > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > + uint64_t > ref_counter[MAX_NUM_OF_WBRF_RANGES]; > > +}; > > + > > +struct wbrf_ranges_in { > > + /* valid entry: `start` and `end` filled with non-zero values */ > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > +}; > > + > > +struct wbrf_ranges_out { > > + uint32_t num_of_ranges; > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > +} __packed; > > + > > +enum wbrf_notifier_actions { > > + WBRF_CHANGED, > > +}; > > + > > +#ifdef CONFIG_WBRF > > +bool wbrf_supported_producer(struct device *dev); int > > +wbrf_add_exclusion(struct device *adev, > > + struct wbrf_ranges_in *in); > > +int wbrf_remove_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in); > > +int wbrf_retrieve_exclusions(struct device *dev, > > + struct wbrf_ranges_out *out); bool > > +wbrf_supported_consumer(struct device *dev); > > + > > +int wbrf_register_notifier(struct notifier_block *nb); int > > +wbrf_unregister_notifier(struct notifier_block *nb); #else static > > +inline bool wbrf_supported_producer(struct device *dev) { return > > +false; } static inline int wbrf_add_exclusion(struct device *adev, > > + struct wbrf_ranges_in *in) { return - > ENODEV; } static inline > > +int wbrf_remove_exclusion(struct device *dev, > > + struct wbrf_ranges_in *in) { return - > ENODEV; } static inline int > > +wbrf_retrieve_exclusions(struct device *dev, > > + struct wbrf_ranges_out *out) > { return -ENODEV; } static > > +inline bool wbrf_supported_consumer(struct device *dev) { return > > +false; } static inline int wbrf_register_notifier(struct > > +notifier_block *nb) { return -ENODEV; } static inline int > > +wbrf_unregister_notifier(struct notifier_block *nb) { return -ENODEV; > > +} #endif > > + > > Right now there are stubs for non CONFIG_WBRF as well as other patches are > using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 patch > looks for #ifdef CONFIG_WBRF. > > I think we should pick one or the other. Right.. > > Having other subsystems #ifdef CONFIG_WBRF will make the series easier to > land through multiple trees; so I have a slight leaning in that direction. I kind of expecting to use the other way. That is to make CONFIG_WBRF agnostic to other subsystems or drivers. They (other subsystems or drivers) can always assume those wbrf_xxxxx interfaces are available. What they need to care only are the return values of those interfaces. How do you think? Evan > > > +#endif /* _LINUX_WBRF_H */
[AMD Official Use Only - General] > -----Original Message----- > From: Simon Horman <simon.horman@corigine.com> > Sent: Friday, June 30, 2023 9:39 PM > To: Quan, Evan <Evan.Quan@amd.com> > Cc: rafael@kernel.org; lenb@kernel.org; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Limonciello, Mario <Mario.Limonciello@amd.com>; > mdaenzer@redhat.com; maarten.lankhorst@linux.intel.com; > tzimmermann@suse.de; hdegoede@redhat.com; jingyuwang_vip@163.com; > Lazar, Lijo <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 > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > On Fri, Jun 30, 2023 at 06:32:32PM +0800, Evan Quan wrote: > > ... > > > diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h > > new file mode 100644 > > index 000000000000..3ca95786cef5 > > --- /dev/null > > +++ b/include/linux/wbrf.h > > @@ -0,0 +1,65 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Wifi Band Exclusion Interface > > + * Copyright (C) 2023 Advanced Micro Devices > > + */ > > + > > +#ifndef _LINUX_WBRF_H > > +#define _LINUX_WBRF_H > > + > > +#include <linux/device.h> > > + > > +/* Maximum number of wbrf ranges */ > > +#define MAX_NUM_OF_WBRF_RANGES 11 > > + > > +struct exclusion_range { > > + /* start and end point of the frequency range in Hz */ > > + uint64_t start; > > + uint64_t end; > > +}; > > + > > +struct exclusion_range_pool { > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > + uint64_t > ref_counter[MAX_NUM_OF_WBRF_RANGES]; > > +}; > > + > > +struct wbrf_ranges_in { > > + /* valid entry: `start` and `end` filled with non-zero values */ > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > +}; > > + > > +struct wbrf_ranges_out { > > + uint32_t num_of_ranges; > > + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; > > +} __packed; > > + > > +enum wbrf_notifier_actions { > > + WBRF_CHANGED, > > +}; > > Hi Evan, > > checkpatch suggests that u64 and u32 might be more appropriate types here, > as they are Kernel types, whereas the ones use are user-space types. Thanks for pointing this out. Will update them accordingly. Evan > > ...
On 7/3/23 22:40, Quan, Evan wrote: > [AMD Official Use Only - General] > >> -----Original Message----- >> From: Limonciello, Mario <Mario.Limonciello@amd.com> >> Sent: Saturday, July 1, 2023 12:41 AM >> To: Quan, Evan <Evan.Quan@amd.com>; rafael@kernel.org; lenb@kernel.org; >> Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian >> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; >> airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; >> 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; Lazar, Lijo >> <Lijo.Lazar@amd.com>; jim.cromie@gmail.com; bellosilicio@gmail.com; >> andrealmeid@igalia.com; trix@redhat.com; jsg@jsg.id.au; arnd@arndb.de >> Cc: 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 >> Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF >> mitigations >> >> On 6/30/2023 05:32, Evan Quan 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. >>> >>> In order for a device to support this, the expected flow for device >>> driver or subsystems: >>> >>> Drivers/subsystems contributing frequencies: >>> >>> 1) During probe, check `wbrf_supported_producer` to see if WBRF >> supported >>> for the device. >>> 2) If adding frequencies, then call `wbrf_add_exclusion` with the >>> start and end ranges of the frequencies. >>> 3) If removing frequencies, then call `wbrf_remove_exclusion` with >>> start and end ranges of the frequencies. >>> >>> Drivers/subsystems responding to frequencies: >>> >>> 1) During probe, check `wbrf_supported_consumer` to see if WBRF is >> supported >>> for the device. >>> 2) Call the `wbrf_retrieve_exclusions` to retrieve the current >>> exclusions on receiving an ACPI notification for a new frequency >>> change. >>> >>> Co-developed-by: Mario Limonciello <mario.limonciello@amd.com> >>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >>> Co-developed-by: Evan Quan <evan.quan@amd.com> >>> Signed-off-by: Evan Quan <evan.quan@amd.com> >>> -- >>> v4->v5: >>> - promote this to be a more generic solution with input argument taking >>> `struct device` and provide better scalability to support non-ACPI >>> scenarios(Andrew) >>> - update the APIs naming and some other minor fixes(Rafael) >>> --- >>> drivers/base/Kconfig | 8 ++ >>> drivers/base/Makefile | 1 + >>> drivers/base/wbrf.c | 227 >> ++++++++++++++++++++++++++++++++++++++++++ >>> include/linux/wbrf.h | 65 ++++++++++++ >>> 4 files changed, 301 insertions(+) >>> create mode 100644 drivers/base/wbrf.c >>> create mode 100644 include/linux/wbrf.h >>> >>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig index >>> 2b8fd6bb7da0..5b441017b225 100644 >>> --- a/drivers/base/Kconfig >>> +++ b/drivers/base/Kconfig >>> @@ -242,4 +242,12 @@ config FW_DEVLINK_SYNC_STATE_TIMEOUT >>> command line option on every system/board your kernel is expected >> to >>> work on. >>> >>> +config WBRF >>> + bool "Wifi band RF mitigation mechanism" >>> + default n >>> + help >>> + Wifi band RF mitigation mechanism allows multiple drivers from >>> + different domains to notify the frequencies in use so that hardware >>> + can be reconfigured to avoid harmonic conflicts. >>> + >>> endmenu >>> diff --git a/drivers/base/Makefile b/drivers/base/Makefile index >>> 3079bfe53d04..c844f68a6830 100644 >>> --- a/drivers/base/Makefile >>> +++ b/drivers/base/Makefile >>> @@ -26,6 +26,7 @@ obj-$(CONFIG_GENERIC_MSI_IRQ) += platform-msi.o >>> obj-$(CONFIG_GENERIC_ARCH_TOPOLOGY) += arch_topology.o >>> obj-$(CONFIG_GENERIC_ARCH_NUMA) += arch_numa.o >>> obj-$(CONFIG_ACPI) += physical_location.o >>> +obj-$(CONFIG_WBRF) += wbrf.o >>> >>> obj-y += test/ >>> >>> diff --git a/drivers/base/wbrf.c b/drivers/base/wbrf.c new file mode >>> 100644 index 000000000000..2163a8ec8a9a >>> --- /dev/null >>> +++ b/drivers/base/wbrf.c >>> @@ -0,0 +1,227 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Wifi Band Exclusion Interface >>> + * Copyright (C) 2023 Advanced Micro Devices >>> + * >>> + */ >>> + >>> +#include <linux/wbrf.h> >>> + >>> +static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head); >>> +static DEFINE_MUTEX(wbrf_mutex); >>> +static struct exclusion_range_pool wbrf_pool; >>> + >>> +static int _wbrf_add_exclusion_ranges(struct wbrf_ranges_in *in) { >>> + int i, j; >>> + >>> + for (i = 0; i < ARRAY_SIZE(in->band_list); i++) { >>> + if (!in->band_list[i].start && >>> + !in->band_list[i].end) >>> + continue; >>> + >>> + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { >>> + if (wbrf_pool.band_list[j].start == in- >>> band_list[i].start && >>> + wbrf_pool.band_list[j].end == in->band_list[i].end) { >>> + wbrf_pool.ref_counter[j]++; >>> + break; >>> + } >>> + } >>> + if (j < ARRAY_SIZE(wbrf_pool.band_list)) >>> + continue; >>> + >>> + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { >>> + if (!wbrf_pool.band_list[j].start && >>> + !wbrf_pool.band_list[j].end) { >>> + wbrf_pool.band_list[j].start = in- >>> band_list[i].start; >>> + wbrf_pool.band_list[j].end = in- >>> band_list[i].end; >>> + wbrf_pool.ref_counter[j] = 1; >>> + break; >>> + } >>> + } >>> + if (j >= ARRAY_SIZE(wbrf_pool.band_list)) >>> + return -ENOSPC; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int _wbrf_remove_exclusion_ranges(struct wbrf_ranges_in *in) { >>> + int i, j; >>> + >>> + for (i = 0; i < ARRAY_SIZE(in->band_list); i++) { >>> + if (!in->band_list[i].start && >>> + !in->band_list[i].end) >>> + continue; >>> + >>> + for (j = 0; j < ARRAY_SIZE(wbrf_pool.band_list); j++) { >>> + if (wbrf_pool.band_list[j].start == in- >>> band_list[i].start && >>> + wbrf_pool.band_list[j].end == in->band_list[i].end) { >>> + wbrf_pool.ref_counter[j]--; >>> + if (!wbrf_pool.ref_counter[j]) { >>> + wbrf_pool.band_list[j].start = 0; >>> + wbrf_pool.band_list[j].end = 0; >>> + } >>> + break; >>> + } >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +static int _wbrf_retrieve_exclusion_ranges(struct wbrf_ranges_out >>> +*out) { >>> + int out_idx = 0; >>> + int i; >>> + >>> + memset(out, 0, sizeof(*out)); >>> + >>> + for (i = 0; i < ARRAY_SIZE(wbrf_pool.band_list); i++) { >>> + if (!wbrf_pool.band_list[i].start && >>> + !wbrf_pool.band_list[i].end) >>> + continue; >>> + >>> + out->band_list[out_idx].start = wbrf_pool.band_list[i].start; >>> + out->band_list[out_idx++].end = wbrf_pool.band_list[i].end; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> +/** >>> + * wbrf_supported_producer - Determine if the device can report >>> +frequencies >>> + * >>> + * @dev: device pointer >>> + * >>> + * WBRF is used to mitigate devices that cause harmonic interference. >>> + * This function will determine if this device needs to report such >> frequencies. >>> + */ >>> +bool wbrf_supported_producer(struct device *dev) { >>> + return true; >>> +} >>> +EXPORT_SYMBOL_GPL(wbrf_supported_producer); >>> + >>> +/** >>> + * wbrf_add_exclusion - Add frequency ranges to the exclusion list >>> + * >>> + * @dev: device pointer >>> + * @in: input structure containing the frequency ranges to be added >>> + * >>> + * Add frequencies into the exclusion list for supported consumers >>> + * to react to. >>> + */ >>> +int wbrf_add_exclusion(struct device *dev, >>> + struct wbrf_ranges_in *in) >>> +{ >>> + int r; >>> + >>> + mutex_lock(&wbrf_mutex); >>> + >>> + r = _wbrf_add_exclusion_ranges(in); >>> + >>> + mutex_unlock(&wbrf_mutex); >>> + if (r) >>> + return r; >>> + >>> + blocking_notifier_call_chain(&wbrf_chain_head, WBRF_CHANGED, >> NULL); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(wbrf_add_exclusion); >>> + >>> +/** >>> + * wbrf_remove_exclusion - Remove frequency ranges from the exclusion >>> +list >>> + * >>> + * @dev: device pointer >>> + * @in: input structure containing the frequency ranges to be removed >>> + * >>> + * Remove frequencies from the exclusion list for supported consumers >>> + * to react to. >>> + */ >>> +int wbrf_remove_exclusion(struct device *dev, >>> + struct wbrf_ranges_in *in) >>> +{ >>> + int r; >>> + >>> + mutex_lock(&wbrf_mutex); >>> + >>> + r = _wbrf_remove_exclusion_ranges(in); >>> + >>> + mutex_unlock(&wbrf_mutex); >>> + if (r) >>> + return r; >>> + >>> + blocking_notifier_call_chain(&wbrf_chain_head, WBRF_CHANGED, >> NULL); >>> + >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(wbrf_remove_exclusion); >>> + >>> +/** >>> + * wbrf_supported_consumer - Determine if the device can react to >>> +frequencies >>> + * >>> + * @dev: device pointer >>> + * >>> + * WBRF is used to mitigate devices that cause harmonic interference. >>> + * This function will determine if this device needs to react to >>> +reports from >>> + * other devices for such frequencies. >>> + */ >>> +bool wbrf_supported_consumer(struct device *dev) { >>> + return true; >>> +} >>> +EXPORT_SYMBOL_GPL(wbrf_supported_consumer); >>> + >>> +/** >>> + * wbrf_register_notifier - Register for notifications of frequency >>> +changes >>> + * >>> + * @nb: driver notifier block >>> + * >>> + * WBRF is used to mitigate devices that cause harmonic interference. >>> + * This function will allow consumers to register for frequency notifications. >>> + */ >>> +int wbrf_register_notifier(struct notifier_block *nb) { >>> + return blocking_notifier_chain_register(&wbrf_chain_head, nb); } >>> +EXPORT_SYMBOL_GPL(wbrf_register_notifier); >>> + >>> +/** >>> + * wbrf_unregister_notifier - Unregister for notifications of >>> +frequency changes >>> + * >>> + * @nb: driver notifier block >>> + * >>> + * WBRF is used to mitigate devices that cause harmonic interference. >>> + * This function will allow consumers to unregister for frequency >> notifications. >>> + */ >>> +int wbrf_unregister_notifier(struct notifier_block *nb) { >>> + return blocking_notifier_chain_unregister(&wbrf_chain_head, nb); } >>> +EXPORT_SYMBOL_GPL(wbrf_unregister_notifier); >>> + >>> +/** >>> + * wbrf_retrieve_exclusions - Retrieve the exclusion list >>> + * >>> + * @dev: device pointer >>> + * @out: output structure containing the frequency ranges to be >>> +excluded >>> + * >>> + * Retrieve the current exclusion list */ int >>> +wbrf_retrieve_exclusions(struct device *dev, >>> + struct wbrf_ranges_out *out) >>> +{ >>> + int r; >>> + >>> + mutex_lock(&wbrf_mutex); >>> + >>> + r = _wbrf_retrieve_exclusion_ranges(out); >>> + >>> + mutex_unlock(&wbrf_mutex); >>> + >>> + return r; >>> +} >>> +EXPORT_SYMBOL_GPL(wbrf_retrieve_exclusions); >>> diff --git a/include/linux/wbrf.h b/include/linux/wbrf.h new file mode >>> 100644 index 000000000000..3ca95786cef5 >>> --- /dev/null >>> +++ b/include/linux/wbrf.h >>> @@ -0,0 +1,65 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Wifi Band Exclusion Interface >>> + * Copyright (C) 2023 Advanced Micro Devices */ >>> + >>> +#ifndef _LINUX_WBRF_H >>> +#define _LINUX_WBRF_H >>> + >>> +#include <linux/device.h> >>> + >>> +/* Maximum number of wbrf ranges */ >>> +#define MAX_NUM_OF_WBRF_RANGES 11 >>> + >>> +struct exclusion_range { >>> + /* start and end point of the frequency range in Hz */ >>> + uint64_t start; >>> + uint64_t end; >>> +}; >>> + >>> +struct exclusion_range_pool { >>> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; >>> + uint64_t >> ref_counter[MAX_NUM_OF_WBRF_RANGES]; >>> +}; >>> + >>> +struct wbrf_ranges_in { >>> + /* valid entry: `start` and `end` filled with non-zero values */ >>> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; >>> +}; >>> + >>> +struct wbrf_ranges_out { >>> + uint32_t num_of_ranges; >>> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES]; >>> +} __packed; >>> + >>> +enum wbrf_notifier_actions { >>> + WBRF_CHANGED, >>> +}; >>> + >>> +#ifdef CONFIG_WBRF >>> +bool wbrf_supported_producer(struct device *dev); int >>> +wbrf_add_exclusion(struct device *adev, >>> + struct wbrf_ranges_in *in); >>> +int wbrf_remove_exclusion(struct device *dev, >>> + struct wbrf_ranges_in *in); >>> +int wbrf_retrieve_exclusions(struct device *dev, >>> + struct wbrf_ranges_out *out); bool >>> +wbrf_supported_consumer(struct device *dev); >>> + >>> +int wbrf_register_notifier(struct notifier_block *nb); int >>> +wbrf_unregister_notifier(struct notifier_block *nb); #else static >>> +inline bool wbrf_supported_producer(struct device *dev) { return >>> +false; } static inline int wbrf_add_exclusion(struct device *adev, >>> + struct wbrf_ranges_in *in) { return - >> ENODEV; } static inline >>> +int wbrf_remove_exclusion(struct device *dev, >>> + struct wbrf_ranges_in *in) { return - >> ENODEV; } static inline int >>> +wbrf_retrieve_exclusions(struct device *dev, >>> + struct wbrf_ranges_out *out) >> { return -ENODEV; } static >>> +inline bool wbrf_supported_consumer(struct device *dev) { return >>> +false; } static inline int wbrf_register_notifier(struct >>> +notifier_block *nb) { return -ENODEV; } static inline int >>> +wbrf_unregister_notifier(struct notifier_block *nb) { return -ENODEV; >>> +} #endif >>> + >> >> Right now there are stubs for non CONFIG_WBRF as well as other patches are >> using #ifdef CONFIG_WBRF or having their own stubs. Like mac80211 patch >> looks for #ifdef CONFIG_WBRF. >> >> I think we should pick one or the other. > Right.. >> >> Having other subsystems #ifdef CONFIG_WBRF will make the series easier to >> land through multiple trees; so I have a slight leaning in that direction. > I kind of expecting to use the other way. That is to make CONFIG_WBRF agnostic to other subsystems or drivers. > They (other subsystems or drivers) can always assume those wbrf_xxxxx interfaces are available. > What they need to care only are the return values of those interfaces. > How do you think? That's fine, thanks. > > Evan >> >>> +#endif /* _LINUX_WBRF_H */ >
> > What is the purpose of this stage? Why would it not be supported for this > > device? > This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) does not support the wbrf adding/removing for some device, > it should speak that out so that the device can be aware of that. How much overhead is this adding? How deep do you need to go to find the BIOS does not support it? And how often is this called? Where do we want to add complexity? In the generic API? Or maybe a little deeper in the ACPI specific code? Andrew
[AMD Official Use Only - General] Hi Andrew, I discussed with Mario about your proposal/concerns here. We believe some changes below might address your concerns. - place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion - place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped. We made some prototypes and even performed some tests which showed technically it is absolutely practicable. However, we found several issues with that. - The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion. Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of them might do in-use band/frequency switching frequently. - Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander, setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving, it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first. This could happen to other consumers and producers too. After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts. Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed. Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly. Please let us know your thoughts. BR, Evan > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, July 4, 2023 9:07 PM > To: Quan, Evan <Evan.Quan@amd.com> > Cc: rafael@kernel.org; lenb@kernel.org; Deucher, Alexander > <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; > airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Limonciello, Mario <Mario.Limonciello@amd.com>; > mdaenzer@redhat.com; maarten.lankhorst@linux.intel.com; > tzimmermann@suse.de; hdegoede@redhat.com; jingyuwang_vip@163.com; > Lazar, Lijo <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 > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > > > What is the purpose of this stage? Why would it not be supported for > > > this device? > > This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) > > does not support the wbrf adding/removing for some device, it should > speak that out so that the device can be aware of that. > > How much overhead is this adding? How deep do you need to go to find the > BIOS does not support it? And how often is this called? > > Where do we want to add complexity? In the generic API? Or maybe a little > deeper in the ACPI specific code? > > Andrew
On 7/5/23 21:58, Quan, Evan wrote: > [AMD Official Use Only - General] > > Hi Andrew, > > I discussed with Mario about your proposal/concerns here. > We believe some changes below might address your concerns. > - place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion > - place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions > So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped. > We made some prototypes and even performed some tests which showed technically it is absolutely practicable. > > However, we found several issues with that. > - The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion. > Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of > them might do in-use band/frequency switching frequently. One more piece of overhead that is in this same theme that Evan didn't mention is the case of a system "without" AMD's ACPI WBRF but the kernel was configured with it enabled. Think like a distro kernel. Moving it into add/remove exclusion would mean that every single time frequency changed by a producer the _DSM would attempt to be evaluated and fail. To avoid that extra call overhead after the first time would mean needing to keep a variable somewhere, and at that point what did you save? > - Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander, > setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving, > it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first. > This could happen to other consumers and producers too. > > After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts. > Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed. > Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly. > Please let us know your thoughts. > > BR, > Evan >> -----Original Message----- >> From: Andrew Lunn <andrew@lunn.ch> >> Sent: Tuesday, July 4, 2023 9:07 PM >> To: Quan, Evan <Evan.Quan@amd.com> >> Cc: rafael@kernel.org; lenb@kernel.org; Deucher, Alexander >> <Alexander.Deucher@amd.com>; Koenig, Christian >> <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; >> airlied@gmail.com; daniel@ffwll.ch; johannes@sipsolutions.net; >> davem@davemloft.net; edumazet@google.com; kuba@kernel.org; >> pabeni@redhat.com; Limonciello, Mario <Mario.Limonciello@amd.com>; >> mdaenzer@redhat.com; maarten.lankhorst@linux.intel.com; >> tzimmermann@suse.de; hdegoede@redhat.com; jingyuwang_vip@163.com; >> Lazar, Lijo <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 >> Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF >> mitigations >> >>>> What is the purpose of this stage? Why would it not be supported for >>>> this device? >>> This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) >>> does not support the wbrf adding/removing for some device, it should >> speak that out so that the device can be aware of that. >> >> How much overhead is this adding? How deep do you need to go to find the >> BIOS does not support it? And how often is this called? >> >> Where do we want to add complexity? In the generic API? Or maybe a little >> deeper in the ACPI specific code? >> >> Andrew >