diff mbox series

[V6,1/9] drivers core: Add support for Wifi band RF mitigations

Message ID 20230710083641.2132264-2-evan.quan@amd.com
State New
Headers show
Series Enable Wifi RFI interference mitigation feature support | expand

Commit Message

Evan Quan July 10, 2023, 8:36 a.m. UTC 
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  |  70 +++++++++++++
 4 files changed, 306 insertions(+)
 create mode 100644 drivers/base/wbrf.c
 create mode 100644 include/linux/wbrf.h

Comments

Andrew Lunn July 12, 2023, 11:12 p.m. UTC | #1 
> +/**
> + * 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.

How is the WBRF core supposed to answer this question? That it knows
there is at least one device which has registered with WBRF saying it
can change its behaviour to avoid causing interference?

Rather than "Determine if the device can report frequencies" should it be
"Determine if the device should report frequencies"

A WiFi device can always report frequencies, since it knows what
frequency is it currently using. However, it is pointless making such
reports if there is no device which can actually make use of the
information. 

> +bool wbrf_supported_producer(struct device *dev)
> +{
> +	return true;
> +}

I found the default implementation of true being odd. It makes me
wounder, what is the point of this call. I would expect this to see if
a linked list is empty or not.

> +/**
> + * wbrf_supported_consumer - Determine if the device can react to frequencies

This again seems odd. A device should know if it can react to
frequencies or not. WBRF core should not need to tell it. What makes
more sense to me is that this call is about a device telling the WBRF
core it is able to react to frequencies. The WBRF core then can give a
good answer to wbrf_supported_producer(), yes, i know of some other
device who might be able to do something to avoid causing interference
to you, so please do tell me about frequencies you want to use.

What is missing here in this API is policy information. The WBRF core
knows it has zero or more devices which can report what frequencies
they are using, and it has zero or more devices which maybe can do
something. But then you need policy to say this particular board needs
any registered devices to actually do something because of poor
shielding. Should this policy be as simple as a bool, or should it
actually say the board has shielding issues for a list of frequencies?
I think the answer to what will depend on the cost of taking action
when no action is actually required.

> + * 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);
> +}

What are the timing requirements for the handler? Should the handler
block until the device has finished doing what it needs to do and the
frequency response has settled? We don't want the WiFi device doing a
SNR measurement until we know local noise is at a minimum. I think it
would be good to document things like this here.

> +struct wbrf_ranges_out {
> +	u32			num_of_ranges;
> +	struct exclusion_range	band_list[MAX_NUM_OF_WBRF_RANGES];
> +} __packed;

Seems odd using packed here. It is the only structure which is
packed. I would also move the u32 after the struct so it is naturally
aligned on 64 bit systems.

	Andrew
Mario Limonciello July 12, 2023, 11:28 p.m. UTC | #2 
On 7/12/23 18:12, Andrew Lunn wrote:
>> +/**
>> + * 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.
> 
> How is the WBRF core supposed to answer this question? That it knows
> there is at least one device which has registered with WBRF saying it
> can change its behaviour to avoid causing interference?
> 
Potential producers are supposed to be the ones asking the question.
> Rather than "Determine if the device can report frequencies" should it be
> "Determine if the device should report frequencies"
Agree.
> 
> A WiFi device can always report frequencies, since it knows what
> frequency is it currently using. However, it is pointless making such
> reports if there is no device which can actually make use of the
> information.

Which is why this function exists.

With the AMD ACPI implementation the platform will dictate this information.

If a future device tree implementation is added it would work similarly.

> 
>> +bool wbrf_supported_producer(struct device *dev)
>> +{
>> +	return true;
>> +}
> 
> I found the default implementation of true being odd. It makes me
> wounder, what is the point of this call. I would expect this to see if
> a linked list is empty or not.

The point is a lot clearer when you look at the description for the 
Kconfig included in patch 2.

+	  Ideally it is the hardware designer/provider who should provide a
+	  solution for the possible RF interference issue. Since they know
+	  well whether there could be RF interference issue with their
+	  platforms.
+
+	  Say Y to enable this generic WBRF solution for diagnosing potential
+	  interference issues on systems without the ACPI mechanism and
+	  developing solutions.

WBRF_AMD_ACPI and WBRF_GENERIC are mutually exclusive.  I don't expect 
the average user to enable WBRF_GENERIC, but there isn't anything to 
stop them from doing so.

It may also aide in developing a WBRF_DEVICE_TREE or similar.

> 
>> +/**
>> + * wbrf_supported_consumer - Determine if the device can react to frequencies
> 
> This again seems odd. A device should know if it can react to
> frequencies or not. WBRF core should not need to tell it. What makes
> more sense to me is that this call is about a device telling the WBRF
> core it is able to react to frequencies. The WBRF core then can give a
> good answer to wbrf_supported_producer(), yes, i know of some other
> device who might be able to do something to avoid causing interference
> to you, so please do tell me about frequencies you want to use.
> 
> What is missing here in this API is policy information. The WBRF core
> knows it has zero or more devices which can report what frequencies
> they are using, and it has zero or more devices which maybe can do
> something. But then you need policy to say this particular board needs
> any registered devices to actually do something because of poor
> shielding. Should this policy be as simple as a bool, or should it
> actually say the board has shielding issues for a list of frequencies?
> I think the answer to what will depend on the cost of taking action
> when no action is actually required.

Again, look at patch 2 and the purpose of WBRF_GENERIC.  I suppose an 
argument can be made to bring WBRF_GENERIC into patch 1.

> 
>> + * 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);
>> +}
> 
> What are the timing requirements for the handler? Should the handler
> block until the device has finished doing what it needs to do and the
> frequency response has settled? We don't want the WiFi device doing a
> SNR measurement until we know local noise is at a minimum. I think it
> would be good to document things like this here.
> 
>> +struct wbrf_ranges_out {
>> +	u32			num_of_ranges;
>> +	struct exclusion_range	band_list[MAX_NUM_OF_WBRF_RANGES];
>> +} __packed;
> 
> Seems odd using packed here. It is the only structure which is
> packed. I would also move the u32 after the struct so it is naturally
> aligned on 64 bit systems.
> 
> 	Andrew
Andrew Lunn July 18, 2023, 2:16 p.m. UTC | #3 
> The wbrf_supported_producer and wbrf_supported_consumer APIs seem
> unnecessary for the generic implementation.

I'm happy with these, once the description is corrected. As i said in
another comment, 'can' should be replaced with 'should'. The device
itself knows if it can, only the core knows if it should, based on the
policy of if actions need to be taken, and there are both providers
and consumers registered with the core.

   Andrew
Evan Quan July 19, 2023, 2:29 a.m. UTC | #4 
[AMD Official Use Only - General]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, July 18, 2023 10:16 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 V6 1/9] drivers core: Add support for Wifi band RF
> mitigations
>
> > The wbrf_supported_producer and wbrf_supported_consumer APIs seem
> > unnecessary for the generic implementation.
>
> I'm happy with these, once the description is corrected. As i said in another
> comment, 'can' should be replaced with 'should'. The device itself knows if it
> can, only the core knows if it should, based on the policy of if actions need to
> be taken, and there are both providers and consumers registered with the
> core.
Sure, will update that in V7.

Evan
>
>    Andrew
diff mbox series

Patch

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..32c017232874
--- /dev/null
+++ b/include/linux/wbrf.h
@@ -0,0 +1,70 @@ 
+/* 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 */
+	u64		start;
+	u64		end;
+};
+
+struct exclusion_range_pool {
+	struct exclusion_range	band_list[MAX_NUM_OF_WBRF_RANGES];
+	u64			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 {
+	u32			num_of_ranges;
+	struct exclusion_range	band_list[MAX_NUM_OF_WBRF_RANGES];
+} __packed;
+
+enum wbrf_notifier_actions {
+	WBRF_CHANGED,
+};
+
+#if IS_ENABLED(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 0; }
+static inline int wbrf_remove_exclusion(struct device *dev,
+					struct wbrf_ranges_in *in) { return 0; }
+static inline int wbrf_retrieve_exclusions(struct device *dev,
+					   struct wbrf_ranges_out *out)
+{
+	memset(out, 0, sizeof(*out));
+
+	return 0;
+}
+static inline bool wbrf_supported_consumer(struct device *dev) { return false; }
+static inline int wbrf_register_notifier(struct notifier_block *nb) { return 0; }
+static inline int wbrf_unregister_notifier(struct notifier_block *nb) { return 0; }
+#endif
+
+#endif /* _LINUX_WBRF_H */