Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On Wed, Aug 23, 2023 at 10:53:43AM +0300, Kalle Valo wrote: > Greg KH writes: > > > On Mon, Aug 21, 2023 at 10:13:45PM -0500, Limonciello, Mario wrote: > >> So I wonder if the right answer is to put it in drivers/net/wireless > >> initially and if we come up with a need later for non wifi producers we can > >> discuss moving it at that time. > > > > Please do so. > > Sorry, I haven't been able to follow the discussion in detail but just a > quick comment: if there's supposed to be code which is shared with > different wifi drivers then drivers/net/wireless sounds wrong, > net/wireless or net/mac80211 would be more approriate location. That's fine with me as well, just not drivers/core/ please :) thanks, greg k-h
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
Greg KH writes: > On Mon, Aug 21, 2023 at 10:13:45PM -0500, Limonciello, Mario wrote: >> So I wonder if the right answer is to put it in drivers/net/wireless >> initially and if we come up with a need later for non wifi producers we can >> discuss moving it at that time. > > Please do so. Sorry, I haven't been able to follow the discussion in detail but just a quick comment: if there's supposed to be code which is shared with different wifi drivers then drivers/net/wireless sounds wrong, net/wireless or net/mac80211 would be more approriate location. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On Mon, Aug 21, 2023 at 10:13:45PM -0500, Limonciello, Mario wrote: > So I wonder if the right answer is to put it in drivers/net/wireless > initially and if we come up with a need later for non wifi producers we can > discuss moving it at that time. Please do so. thanks, greg k-h
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On 8/19/2023 5:50 AM, Greg KH wrote: On Fri, Aug 18, 2023 at 05:49:14PM -0500, Limonciello, Mario wrote: On 8/18/2023 4:24 PM, Greg KH wrote: On Fri, Aug 18, 2023 at 11:26:11AM +0800, Evan Quan wrote: drivers/base/Makefile | 1 + drivers/base/wbrf.c | 280 ++ Why is a wifi-specific thing going into drivers/base/? confused, greg k-h The original problem statement was at a high level 'there can be interference between different devices operating at high frequencies'. The original patches introduced some ACPI library code that enabled a mitigated for this interference between mac80211 devices and amdgpu devices. Andrew Lunn wanted to see something more generic, so the series has morphed into base code for things to advertise frequencies in use and other things to listen to frequencies in use and react. The idea is supposed to be that if the platform knows that these mitigations are needed then the producers send the frequencies in use, consumers react to them. The AMD implementation of getting this info from the platform plugs into the base code (patch 2). If users don't want this behavior they can turn it off on kernel command line. If the platform doesn't know mitigations are needed but user wants to turn them on anyway they can turn it on kernel command line. That's all fine, I don't object to that at all. But bus/device-specific stuff should NOT be in drivers/base/ if at all possible (yes, we do have some exceptions with hypervisor.c and memory and cpu stuff) but for a frequency thing like this, why can't it live with the other wifi/frequency code in drivers/net/wireless/? In other words, what's the benefit to having me be the maintainer of this, someone who knows nothing about this subsystem, other than you passing off that work to me? :) thanks, greg k-h The reason drivers/base was proposed was because although the initial implementation is for producers from mac80211, Andrew pointed out that many other things can technically be producers and cause interference if not properly shielded. So by making it part of base that sets up the policy that if something "can" produce certain problematic harmonics that it can participate. Whether or not other devices /will/ use this is another question though. You need deep platform knowledge and proper equipment to diagnose a problem and conclude it can be helped with this kind of mitigation. So I wonder if the right answer is to put it in drivers/net/wireless initially and if we come up with a need later for non wifi producers we can discuss moving it at that time.
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On Fri, Aug 18, 2023 at 05:49:14PM -0500, Limonciello, Mario wrote: > > > On 8/18/2023 4:24 PM, Greg KH wrote: > > On Fri, Aug 18, 2023 at 11:26:11AM +0800, Evan Quan wrote: > > > drivers/base/Makefile | 1 + > > > drivers/base/wbrf.c | 280 ++ > > > > Why is a wifi-specific thing going into drivers/base/? > > > > confused, > > > > greg k-h > > The original problem statement was at a high level 'there can be > interference between different devices operating at high frequencies'. The > original patches introduced some ACPI library code that enabled a mitigated > for this interference between mac80211 devices and amdgpu devices. > > Andrew Lunn wanted to see something more generic, so the series has morphed > into base code for things to advertise frequencies in use and other things > to listen to frequencies in use and react. > > The idea is supposed to be that if the platform knows that these mitigations > are needed then the producers send the frequencies in use, consumers react > to them. The AMD implementation of getting this info from the platform > plugs into the base code (patch 2). > > If users don't want this behavior they can turn it off on kernel command > line. > > If the platform doesn't know mitigations are needed but user wants to turn > them on anyway they can turn it on kernel command line. That's all fine, I don't object to that at all. But bus/device-specific stuff should NOT be in drivers/base/ if at all possible (yes, we do have some exceptions with hypervisor.c and memory and cpu stuff) but for a frequency thing like this, why can't it live with the other wifi/frequency code in drivers/net/wireless/? In other words, what's the benefit to having me be the maintainer of this, someone who knows nothing about this subsystem, other than you passing off that work to me? :) thanks, greg k-h
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On Fri, Aug 18, 2023 at 11:26:11AM +0800, Evan Quan wrote: > drivers/base/Makefile | 1 + > drivers/base/wbrf.c | 280 ++ Why is a wifi-specific thing going into drivers/base/? confused, greg k-h
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On 8/18/2023 4:24 PM, Greg KH wrote: On Fri, Aug 18, 2023 at 11:26:11AM +0800, Evan Quan wrote: drivers/base/Makefile | 1 + drivers/base/wbrf.c | 280 ++ Why is a wifi-specific thing going into drivers/base/? confused, greg k-h The original problem statement was at a high level 'there can be interference between different devices operating at high frequencies'. The original patches introduced some ACPI library code that enabled a mitigated for this interference between mac80211 devices and amdgpu devices. Andrew Lunn wanted to see something more generic, so the series has morphed into base code for things to advertise frequencies in use and other things to listen to frequencies in use and react. The idea is supposed to be that if the platform knows that these mitigations are needed then the producers send the frequencies in use, consumers react to them. The AMD implementation of getting this info from the platform plugs into the base code (patch 2). If users don't want this behavior they can turn it off on kernel command line. If the platform doesn't know mitigations are needed but user wants to turn them on anyway they can turn it on kernel command line.
Re: [V9 1/9] drivers core: Add support for Wifi band RF mitigations
On Fri, Aug 18, 2023 at 5:27 AM 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_register_notifier` to register for notifications of
>frequency changes from other devices.
> 3) Call the `wbrf_retrieve_exclusions` to retrieve the current exclusions
>range on receiving a notification and response correspondingly.
>
> Meanwhile a kernel parameter `wbrf` with default setting as "auto" is
> introduced to specify what the policy is.
> - With `wbrf=on`, the WBRF features will be enabled forcely.
> - With `wbrf=off`, the WBRF features will be disabled forcely.
> - With `wbrf=auto`, it will be up to the system to do proper checks
> to determine the WBRF features should be enabled or not.
>
> Co-developed-by: Mario Limonciello
> Signed-off-by: Mario Limonciello
> Co-developed-by: Evan Quan
> Signed-off-by: Evan Quan
In the first place, this requires quite a bit of driver API
documentation that is missing.
To a minimum, it should explain what the interface is for and how it
is supposed to be used by drivers (both "producers" and "consumers").
And how to determine whether or not a given device is a "producer" or
"consumer" from the WBRF perspective.
> --
> 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)
> v6->v7:
> - revised the `struct wbrf_ranges_out` to be naturally aligned(Andrew)
> - revised some code comments(Andrew)
> v8->v9:
> - update the document to be more readable(Randy)
> ---
> .../admin-guide/kernel-parameters.txt | 8 +
> drivers/base/Makefile | 1 +
> drivers/base/wbrf.c | 280 ++
> include/linux/wbrf.h | 47 +++
> 4 files changed, 336 insertions(+)
> create mode 100644 drivers/base/wbrf.c
> create mode 100644 include/linux/wbrf.h
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index a1457995fd41..5566fefeffdf 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -7152,3 +7152,11 @@
> xmon commands.
> off xmon is disabled.
>
> + wbrf= [KNL]
> + Format: { on | auto (default) | off }
> + Controls if WBRF features should be forced on or off.
> + on Force enable the WBRF features.
> + autoUp to the system to do proper checks to
> + determine the WBRF features should be enabled
> + or not.
> + off Force disable the WBRF features.
Well, how's a casual reader of this file supposed to find out what
WBRF is and whether or not they should care?
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 3079bfe53d04..7b3cef898c19 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-y += wbrf.o
>
> obj-y += test/
>
> diff --git a/drivers/base/wbrf.c b/drivers/base/wbrf.c
> new file mode 100644
> index ..678f245c12c6
> --- /dev/null
> +++ b/drivers/base/wbrf.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wifi Band Exclusion Interface
> + * Copyright (C) 2023 Advanced Micro Devices
> + *
I would expect some explanation of the interface design and purpose here.
So I don't have to wonder what WBRF_POLICY_MODE is
[V9 1/9] drivers core: Add support for Wifi band RF mitigations
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_register_notifier` to register for notifications of
frequency changes from other devices.
3) Call the `wbrf_retrieve_exclusions` to retrieve the current exclusions
range on receiving a notification and response correspondingly.
Meanwhile a kernel parameter `wbrf` with default setting as "auto" is
introduced to specify what the policy is.
- With `wbrf=on`, the WBRF features will be enabled forcely.
- With `wbrf=off`, the WBRF features will be disabled forcely.
- With `wbrf=auto`, it will be up to the system to do proper checks
to determine the WBRF features should be enabled or not.
Co-developed-by: Mario Limonciello
Signed-off-by: Mario Limonciello
Co-developed-by: Evan Quan
Signed-off-by: Evan Quan
--
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)
v6->v7:
- revised the `struct wbrf_ranges_out` to be naturally aligned(Andrew)
- revised some code comments(Andrew)
v8->v9:
- update the document to be more readable(Randy)
---
.../admin-guide/kernel-parameters.txt | 8 +
drivers/base/Makefile | 1 +
drivers/base/wbrf.c | 280 ++
include/linux/wbrf.h | 47 +++
4 files changed, 336 insertions(+)
create mode 100644 drivers/base/wbrf.c
create mode 100644 include/linux/wbrf.h
diff --git a/Documentation/admin-guide/kernel-parameters.txt
b/Documentation/admin-guide/kernel-parameters.txt
index a1457995fd41..5566fefeffdf 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -7152,3 +7152,11 @@
xmon commands.
off xmon is disabled.
+ wbrf= [KNL]
+ Format: { on | auto (default) | off }
+ Controls if WBRF features should be forced on or off.
+ on Force enable the WBRF features.
+ autoUp to the system to do proper checks to
+ determine the WBRF features should be enabled
+ or not.
+ off Force disable the WBRF features.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 3079bfe53d04..7b3cef898c19 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-y += wbrf.o
obj-y += test/
diff --git a/drivers/base/wbrf.c b/drivers/base/wbrf.c
new file mode 100644
index ..678f245c12c6
--- /dev/null
+++ b/drivers/base/wbrf.c
@@ -0,0 +1,280 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Wifi Band Exclusion Interface
+ * Copyright (C) 2023 Advanced Micro Devices
+ *
+ */
+
+#include
+
+static BLOCKING_NOTIFIER_HEAD(wbrf_chain_head);
+static DEFINE_MUTEX(wbrf_mutex);
+static enum WBRF_POLICY_MODE {
+ WBRF_POLICY_FORCE_DISABLE,
+ WBRF_POLICY_AUTO,
+ WBRF_POLICY_FORCE_ENABLE,
+} wbrf_policy = WBRF_POLICY_AUTO;
+
+static int __init parse_wbrf_policy_mode(char *p)
+{
+ if (!strncmp(p, "auto", 4))
+ wbrf_policy = WBRF_POLICY_AUTO;
+ else if (!strncmp(p, "on", 2))
+ wbrf_policy = WBRF_POLICY_FORCE_ENABLE;
+ else if (!strncmp(p, "off", 3))
+ wbrf_policy = WBRF_POLICY_FORCE_DISABLE;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+early_param("wbrf", parse_wbrf_policy_mode);
+
+static struct exclusion_range_pool {
+ struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
+
