Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
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 Sent: Tuesday, July 4, 2023 9:07 PM To: Quan, Evan Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander ; Koenig, Christian ; Pan, Xinhui ; airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; da...@davemloft.net; eduma...@google.com; k...@kernel.org; pab...@redhat.com; Limonciello, Mario ; mdaen...@redhat.com; maarten.lankho...@linux.intel.com; tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo ; jim.cro...@gmail.com; bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux- a...@vger.kernel.org; amd-gfx@lists.freedesktop.org; dri- de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org; net...@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
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[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 > Sent: Tuesday, July 4, 2023 9:07 PM > To: Quan, Evan > Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander > ; Koenig, Christian > ; Pan, Xinhui ; > airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; > da...@davemloft.net; eduma...@google.com; k...@kernel.org; > pab...@redhat.com; Limonciello, Mario ; > mdaen...@redhat.com; maarten.lankho...@linux.intel.com; > tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com; > Lazar, Lijo ; jim.cro...@gmail.com; > bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com; > j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux- > a...@vger.kernel.org; amd-gfx@lists.freedesktop.org; dri- > de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org; > net...@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
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
Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
On 7/3/23 22:40, Quan, Evan wrote:
[AMD Official Use Only - General]
-Original Message-
From: Limonciello, Mario
Sent: Saturday, July 1, 2023 12:41 AM
To: Quan, Evan ; raf...@kernel.org; l...@kernel.org;
Deucher, Alexander ; Koenig, Christian
; Pan, Xinhui ;
airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net;
da...@davemloft.net; eduma...@google.com; k...@kernel.org;
pab...@redhat.com; mdaen...@redhat.com;
maarten.lankho...@linux.intel.com; tzimmerm...@suse.de;
hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo
; jim.cro...@gmail.com; bellosili...@gmail.com;
andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de
Cc: linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; amd-
g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-
wirel...@vger.kernel.org; net...@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
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)
---
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 ..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
+
+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;
+
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[AMD Official Use Only - General]
> -Original Message-
> From: Simon Horman
> Sent: Friday, June 30, 2023 9:39 PM
> To: Quan, Evan
> Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander
> ; Koenig, Christian
> ; Pan, Xinhui ;
> airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net;
> da...@davemloft.net; eduma...@google.com; k...@kernel.org;
> pab...@redhat.com; Limonciello, Mario ;
> mdaen...@redhat.com; maarten.lankho...@linux.intel.com;
> tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com;
> Lazar, Lijo ; jim.cro...@gmail.com;
> bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com;
> j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux-
> a...@vger.kernel.org; amd-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org;
> net...@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 ..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
> > +
> > +/* 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_tstart;
> > + uint64_tend;
> > +};
> > +
> > +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_tnum_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
>
> ...
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[AMD Official Use Only - General] > -Original Message- > From: Limonciello, Mario > Sent: Saturday, July 1, 2023 12:41 AM > To: Quan, Evan ; raf...@kernel.org; l...@kernel.org; > Deucher, Alexander ; Koenig, Christian > ; Pan, Xinhui ; > airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; > da...@davemloft.net; eduma...@google.com; k...@kernel.org; > pab...@redhat.com; mdaen...@redhat.com; > maarten.lankho...@linux.intel.com; tzimmerm...@suse.de; > hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo > ; jim.cro...@gmail.com; bellosili...@gmail.com; > andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de > Cc: linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; amd- > g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux- > wirel...@vger.kernel.org; net...@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 > > 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) > > --- > > 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 ..2163a8ec8a9a > > --- /dev/null > > +++ b/drivers/base/wbrf.c > > @@ -0,0 +1,227 @@ > > +// SPDX-License-I
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[AMD Official Use Only - General]
> -Original Message-
> From: Andrew Lunn
> Sent: Saturday, July 1, 2023 8:20 AM
> To: Quan, Evan
> Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander
> ; Koenig, Christian
> ; Pan, Xinhui ;
> airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net;
> da...@davemloft.net; eduma...@google.com; k...@kernel.org;
> pab...@redhat.com; Limonciello, Mario ;
> mdaen...@redhat.com; maarten.lankho...@linux.intel.com;
> tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com;
> Lazar, Lijo ; jim.cro...@gmail.com;
> bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com;
> j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux-
> a...@vger.kernel.org; amd-gfx@lists.freedesktop.org; dri-
> de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org;
> net...@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
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[AMD Official Use Only - General]
> -Original Message-
> From: Andrew Lunn
> Sent: Saturday, July 1, 2023 8:25 AM
> To: Limonciello, Mario
> Cc: Quan, Evan ; raf...@kernel.org; l...@kernel.org;
> Deucher, Alexander ; Koenig, Christian
> ; Pan, Xinhui ;
> airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net;
> da...@davemloft.net; eduma...@google.com; k...@kernel.org;
> pab...@redhat.com; mdaen...@redhat.com;
> maarten.lankho...@linux.intel.com; tzimmerm...@suse.de;
> hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo
> ; jim.cro...@gmail.com; bellosili...@gmail.com;
> andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de;
> linux-ker...@vger.kernel.org; linux-a...@vger.kernel.org; amd-
> g...@lists.freedesktop.org; dri-de...@lists.freedesktop.org; linux-
> wirel...@vger.kernel.org; net...@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
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?
> +#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
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.
Andrew
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
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)
---
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 ..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
+
+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 &&
+
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 ..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
> +
> +/* 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_tstart;
> + uint64_tend;
> +};
> +
> +struct exclusion_range_pool {
> + struct exclusion_range band_list[MAX_NUM_OF_WBRF_RANGES];
> + uint64_tref_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_tnum_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.
...
[PATCH V5 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_retrieve_exclusions` to retrieve the current
exclusions on receiving an ACPI notification for a new frequency
change.
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)
---
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 ..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
+
+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)
+
