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

Commit Message

Evan Quan Aug. 18, 2023, 3:26 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_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 <[e-mail]>
Signed-off-by: Mario Limonciello <[e-mail]>
Co-developed-by: Evan Quan <[e-mail]>
Signed-off-by: Evan Quan <[e-mail]>
--
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

Comments

Rafael J. Wysocki Aug. 18, 2023, 4:41 p.m. UTC | #1

On Fri, Aug 18, 2023 at 5:27 AM Evan Quan <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Mario Limonciello <[e-mail]>
> Co-developed-by: Evan Quan <[e-mail]>
> Signed-off-by: Evan Quan <[e-mail]>

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.
> +                       auto    Up 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 000000000000..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 or what the
"exclusion ranges" are below.

> + */
> +
> +#include <linux/wbrf.h>
> +
> +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];
> +       u64                     ref_counter[MAX_NUM_OF_WBRF_RANGES];
> +} 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;

It never returns anything else.  Should it be void?

> +}
> +
> +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;
> +       }
> +
> +       out->num_of_ranges = out_idx;
> +
> +       return 0;

Same here.

> +}
> +
> +/**
> + * wbrf_supported_system - Determine if the system supports WBRF features
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will determine if the platform is able to support the
> + * WBRF features.

The code doesn't quite match the description above.  I guess the code
is temporary?

> + */
> +static bool wbrf_supported_system(void)
> +{
> +       switch (wbrf_policy) {
> +       case WBRF_POLICY_FORCE_ENABLE:
> +               return true;
> +       case WBRF_POLICY_FORCE_DISABLE:
> +               return false;
> +       case WBRF_POLICY_AUTO:
> +               return false;
> +       }
> +
> +       return false;
> +}
> +
> +/**
> + * wbrf_supported_producer - Determine if the device should report frequencies
> + *
> + * @dev: device pointer
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.
> + * This function will determine if this device should report such frequencies.

It is not clear how "harmonic interference" is related to "such
frequencies" from the above.

> + */
> +bool wbrf_supported_producer(struct device *dev)
> +{
> +       if (!wbrf_supported_system())
> +               return false;
> +
> +       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.

Well, the above isn't particularly helpful IMV.

What's "the exclusion list"?  What are "supported consumers" and how
are they going to "react" and to what exactly (the exclusion list or
the frequencies)?

Why is the notifier chain not mentioned in the kerneldoc description
of the function?

> + */
> +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.

This has the same problems as the above.

> + */
> +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 should react to frequencies
> + *
> + * @dev: device pointer
> + *
> + * WBRF is used to mitigate devices that cause harmonic interference.

What does this mean?  How can a device be "mitigated" and what
"harmonic interference" is this about?

> + * This function will determine if this device should react to reports from
> + * other devices for such frequencies.

What are "such frequencies"?

> + */
> +bool wbrf_supported_consumer(struct device *dev)
> +{
> +       if (!wbrf_supported_system())
> +               return false;
> +
> +       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.

What's a "frequency notification"?

> + */
> +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

Excluded from what?

> + *
> + * Retrieve the current exclusion list

What's "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..476a28fec27a
> --- /dev/null
> +++ b/include/linux/wbrf.h
> @@ -0,0 +1,47 @@
> +/* 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 */

I would put the comment above the whole struct definition and use the
kerneldoc format for it.

> +       u64             start;
> +       u64             end;
> +};
> +
> +struct wbrf_ranges_in {
> +       /* valid entry: `start` and `end` filled with non-zero values */

Same here.

> +       struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
> +};
> +
> +struct wbrf_ranges_out {
> +       u64                     num_of_ranges;
> +       struct exclusion_range  band_list[MAX_NUM_OF_WBRF_RANGES];
> +};
> +
> +enum wbrf_notifier_actions {
> +       WBRF_CHANGED,
> +};

No description.

> +
> +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);
> +
> +#endif /* _LINUX_WBRF_H */
> --

Overall, readers should be able to understand stuff they read.  The
above doesn't follow this rule IMO.

Greg KH Aug. 18, 2023, 9:24 p.m. UTC | #2

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

Mario Limonciello Aug. 18, 2023, 10:49 p.m. UTC | #3

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.

Greg KH Aug. 19, 2023, 10:50 a.m. UTC | #4

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

Mario Limonciello Aug. 22, 2023, 3:13 a.m. UTC | #5

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.

Greg KH Aug. 22, 2023, 6:39 a.m. UTC | #6

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

Kalle Valo Aug. 23, 2023, 7:53 a.m. UTC | #7

Greg KH <[e-mail]> 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.

Greg KH Aug. 23, 2023, 8:15 a.m. UTC | #8

On Wed, Aug 23, 2023 at 10:53:43AM +0300, Kalle Valo wrote:
> Greg KH <[e-mail]> 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

Patch

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.
+			auto	Up 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 000000000000..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 <linux/wbrf.h>
+
+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];
+	u64			ref_counter[MAX_NUM_OF_WBRF_RANGES];
+} 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;
+	}
+
+	out->num_of_ranges = out_idx;
+
+	return 0;
+}
+
+/**
+ * wbrf_supported_system - Determine if the system supports WBRF features
+ *
+ * WBRF is used to mitigate devices that cause harmonic interference.
+ * This function will determine if the platform is able to support the
+ * WBRF features.
+ */
+static bool wbrf_supported_system(void)
+{
+	switch (wbrf_policy) {
+	case WBRF_POLICY_FORCE_ENABLE:
+		return true;
+	case WBRF_POLICY_FORCE_DISABLE:
+		return false;
+	case WBRF_POLICY_AUTO:
+		return false;
+	}
+
+	return false;
+}
+
+/**
+ * wbrf_supported_producer - Determine if the device should report frequencies
+ *
+ * @dev: device pointer
+ *
+ * WBRF is used to mitigate devices that cause harmonic interference.
+ * This function will determine if this device should report such frequencies.
+ */
+bool wbrf_supported_producer(struct device *dev)
+{
+	if (!wbrf_supported_system())
+		return false;
+
+	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 should react to frequencies
+ *
+ * @dev: device pointer
+ *
+ * WBRF is used to mitigate devices that cause harmonic interference.
+ * This function will determine if this device should react to reports from
+ * other devices for such frequencies.
+ */
+bool wbrf_supported_consumer(struct device *dev)
+{
+	if (!wbrf_supported_system())
+		return false;
+
+	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..476a28fec27a
--- /dev/null
+++ b/include/linux/wbrf.h
@@ -0,0 +1,47 @@ 
+/* 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 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 {
+	u64			num_of_ranges;
+	struct exclusion_range	band_list[MAX_NUM_OF_WBRF_RANGES];
+};
+
+enum wbrf_notifier_actions {
+	WBRF_CHANGED,
+};
+
+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);
+
+#endif /* _LINUX_WBRF_H */