Message-ID: <3c3dccd6-6772-d62f-f183-bd84e109a366@amd.com>
Date:   Sun, 18 Jun 2023 21:17:29 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Evan Quan <evan.quan@....com>
Cc:     linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-wireless@...r.kernel.org, rafael@...nel.org, lenb@...nel.org,
        Alexander.Deucher@....com, Christian.Koenig@....com,
        Xinhui.Pan@....com, airlied@...il.com, daniel@...ll.ch,
        kvalo@...nel.org, nbd@....name, lorenzo@...nel.org,
        ryder.lee@...iatek.com, shayne.chen@...iatek.com,
        sean.wang@...iatek.com, matthias.bgg@...il.com,
        angelogioacchino.delregno@...labora.com, Lijo.Lazar@....com
Subject: Re: [PATCH V3 2/7] wifi: mac80211: Add support for ACPI WBRF

On 6/16/23 01:57, Evan Quan wrote:
> From: Mario Limonciello <mario.limonciello@....com>
> 
> To support AMD's WBRF interference mitigation mechanism, Wifi adapters
> utilized in the system must register the frequencies in use(or unregister
> those frequencies no longer used) via the dedicated APCI calls. So that,
> other drivers responding to the frequencies can take proper actions to
> mitigate possible interference.
> 
> To make WBRF feature functional, the kernel needs to be configured with
> CONFIG_ACPI_WBRF and the platform is equipped with WBRF support(from
> BIOS and drivers).
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
> Co-developed-by: Evan Quan <evan.quan@....com>
> Signed-off-by: Evan Quan <evan.quan@....com>
> --
> v1->v2:
>    - place the new added member(`wbrf_supported`) in
>      ieee80211_local(Johannes)
>    - handle chandefs change scenario properly(Johannes)
>    - some minor fixes around code sharing and possible invalid input
>      checks(Johannes)
> ---
>   include/net/cfg80211.h     |   8 +++
>   net/mac80211/Makefile      |   2 +
>   net/mac80211/chan.c        |  11 +++
>   net/mac80211/ieee80211_i.h |  19 +++++
>   net/mac80211/main.c        |   2 +
>   net/mac80211/wbrf.c        | 137 +++++++++++++++++++++++++++++++++++++
>   net/wireless/chan.c        |   3 +-
>   7 files changed, 181 insertions(+), 1 deletion(-)
>   create mode 100644 net/mac80211/wbrf.c
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 9e04f69712b1..c6dc337eafce 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -920,6 +920,14 @@ const struct cfg80211_chan_def *
>   cfg80211_chandef_compatible(const struct cfg80211_chan_def *chandef1,
>   			    const struct cfg80211_chan_def *chandef2);
>   
> +/**
> + * nl80211_chan_width_to_mhz - get the channel width in Mhz
> + * @chan_width: the channel width from &enum nl80211_chan_width
> + * Return: channel width in Mhz if the chan_width from &enum nl80211_chan_width
> + * is valid. -1 otherwise.
> + */
> +int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width);
> +

It's up to mac80211 maintainers, but I would think that the changes to 
change nl80211_chan_width_to_mhz from static to exported should be 
separate from the patch to introduced WBRF support in the series.

>   /**
>    * cfg80211_chandef_valid - check if a channel definition is valid
>    * @chandef: the channel definition to check
> diff --git a/net/mac80211/Makefile b/net/mac80211/Makefile
> index b8de44da1fb8..709eb678f42a 100644
> --- a/net/mac80211/Makefile
> +++ b/net/mac80211/Makefile
> @@ -65,4 +65,6 @@ rc80211_minstrel-$(CONFIG_MAC80211_DEBUGFS) += \
>   
>   mac80211-$(CONFIG_MAC80211_RC_MINSTREL) += $(rc80211_minstrel-y)
>   
> +mac80211-$(CONFIG_ACPI_WBRF) += wbrf.o
> +
>   ccflags-y += -DDEBUG
> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
> index 77c90ed8f5d7..0c5289a9aa6c 100644
> --- a/net/mac80211/chan.c
> +++ b/net/mac80211/chan.c
> @@ -506,11 +506,16 @@ static void _ieee80211_change_chanctx(struct ieee80211_local *local,
>   
>   	WARN_ON(!cfg80211_chandef_compatible(&ctx->conf.def, chandef));
>   
> +	ieee80211_remove_wbrf(local, &ctx->conf.def);
> +
>   	ctx->conf.def = *chandef;
>   
>   	/* check if min chanctx also changed */
>   	changed = IEEE80211_CHANCTX_CHANGE_WIDTH |
>   		  _ieee80211_recalc_chanctx_min_def(local, ctx, rsvd_for);
> +
> +	ieee80211_add_wbrf(local, &ctx->conf.def);
> +
>   	drv_change_chanctx(local, ctx, changed);
>   
>   	if (!local->use_chanctx) {
> @@ -668,6 +673,10 @@ static int ieee80211_add_chanctx(struct ieee80211_local *local,
>   	lockdep_assert_held(&local->mtx);
>   	lockdep_assert_held(&local->chanctx_mtx);
>   
> +	err = ieee80211_add_wbrf(local, &ctx->conf.def);
> +	if (err)
> +		return err;
> +
>   	if (!local->use_chanctx)
>   		local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
>   
> @@ -748,6 +757,8 @@ static void ieee80211_del_chanctx(struct ieee80211_local *local,
>   	}
>   
>   	ieee80211_recalc_idle(local);
> +
> +	ieee80211_remove_wbrf(local, &ctx->conf.def);
>   }
>   
>   static void ieee80211_free_chanctx(struct ieee80211_local *local,
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
> index b0372e76f373..f832de16073b 100644
> --- a/net/mac80211/ieee80211_i.h
> +++ b/net/mac80211/ieee80211_i.h
> @@ -1591,6 +1591,10 @@ struct ieee80211_local {
>   
>   	/* extended capabilities provided by mac80211 */
>   	u8 ext_capa[8];
> +
> +#ifdef CONFIG_ACPI_WBRF
> +	bool wbrf_supported;
> +#endif
>   };
>   
>   static inline struct ieee80211_sub_if_data *
> @@ -2615,4 +2619,19 @@ ieee80211_eht_cap_ie_to_sta_eht_cap(struct ieee80211_sub_if_data *sdata,
>   				    const struct ieee80211_eht_cap_elem *eht_cap_ie_elem,
>   				    u8 eht_cap_len,
>   				    struct link_sta_info *link_sta);
> +
> +#ifdef CONFIG_ACPI_WBRF
> +void ieee80211_check_wbrf_support(struct ieee80211_local *local);
> +int ieee80211_add_wbrf(struct ieee80211_local *local,
> +		       struct cfg80211_chan_def *chandef);
> +void ieee80211_remove_wbrf(struct ieee80211_local *local,
> +			   struct cfg80211_chan_def *chandef);
> +#else
> +static inline void ieee80211_check_wbrf_support(struct ieee80211_local *local) { }
> +static inline int ieee80211_add_wbrf(struct ieee80211_local *local,
> +				     struct cfg80211_chan_def *chandef) { return 0; }
> +static inline void ieee80211_remove_wbrf(struct ieee80211_local *local,
> +					 struct cfg80211_chan_def *chandef) { }
> +#endif /* CONFIG_ACPI_WBRF */
> +
>   #endif /* IEEE80211_I_H */
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index 55cdfaef0f5d..0a55626b1546 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -1395,6 +1395,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
>   	debugfs_hw_add(local);
>   	rate_control_add_debugfs(local);
>   
> +	ieee80211_check_wbrf_support(local);
> +
>   	rtnl_lock();
>   	wiphy_lock(hw->wiphy);
>   
> diff --git a/net/mac80211/wbrf.c b/net/mac80211/wbrf.c
> new file mode 100644
> index 000000000000..2e1a58cf4dbf
> --- /dev/null
> +++ b/net/mac80211/wbrf.c
> @@ -0,0 +1,137 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AMD Wifi Band Exclusion Interface
> + * Copyright (C) 2023 Advanced Micro Devices
> + *
> + */
> +
> +#include <linux/wbrf.h>
> +#include <net/cfg80211.h>
> +#include "ieee80211_i.h"
> +
> +#define KHZ_TO_HZ(freq)		((freq) * 1000ULL)

I think this new macro probably should live in include/linux/ieee80211.h.

> +
> +void ieee80211_check_wbrf_support(struct ieee80211_local *local)
> +{
> +	struct device *dev = local->hw.wiphy->dev.parent;
> +	struct acpi_device *acpi_dev;
> +
> +	if (!dev)
> +		return;
> +
> +	acpi_dev = ACPI_COMPANION(dev);
> +	if (!acpi_dev) {
> +		dev_dbg(dev, "ACPI companion not found\n");
> +		return;
> +	}
> +
> +	local->wbrf_supported = wbrf_supported_producer(acpi_dev);
> +	dev_dbg(dev, "WBRF is %s supported\n",
> +		local->wbrf_supported ? "" : "not");
> +}
> +
> +static void get_chan_freq_boundary(u32 center_freq,
> +				   u32 bandwidth,
> +				   u64 *start,
> +				   u64 *end)
> +{
> +	bandwidth = MHZ_TO_KHZ(bandwidth);
> +	center_freq = MHZ_TO_KHZ(center_freq);
> +
> +	*start = center_freq - bandwidth / 2;
> +	*end = center_freq + bandwidth / 2;

What do you think about using cfg80211_get_start_freq and 
cfg80211_get_end_freq and then converting the result from them to HZ 
instead?

> +
> +	/* Frequency in HZ is expected */
> +	*start = KHZ_TO_HZ(*start);
> +	*end = KHZ_TO_HZ(*end);
> +}
> +
> +static int wbrf_get_ranges_from_chandef(struct cfg80211_chan_def *chandef,
> +					struct wbrf_ranges_in *ranges_in)
> +{
> +	u64 start_freq1, end_freq1;
> +	u64 start_freq2, end_freq2;
> +	int bandwidth;
> +
> +	bandwidth = nl80211_chan_width_to_mhz(chandef->width);
> +	if (bandwidth < 0)
> +		return -EINVAL;
> +
> +	get_chan_freq_boundary(chandef->center_freq1,
> +			       bandwidth,
> +			       &start_freq1,
> +			       &end_freq1);
> +
> +	ranges_in->band_list[0].start = start_freq1;
> +	ranges_in->band_list[0].end = end_freq1;
> +
> +	if (chandef->width == NL80211_CHAN_WIDTH_80P80) {
> +		get_chan_freq_boundary(chandef->center_freq2,
> +				       bandwidth,
> +				       &start_freq2,
> +				       &end_freq2);
> +
> +		ranges_in->band_list[1].start = start_freq2;
> +		ranges_in->band_list[1].end = end_freq2;
> +	}
> +
> +	return 0;
> +}
> +
> +static int wbrf_add_exclusion_wlan(struct acpi_device *adev,
> +				   struct cfg80211_chan_def *chandef)
> +{
> +	struct wbrf_ranges_in ranges_in = {0};
> +	int ret;
> +
> +	ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
> +	if (ret)
> +		return ret;
> +
> +	return wbrf_add_exclusion(adev, &ranges_in);
> +}
> +
> +static int wbrf_remove_exclusion_wlan(struct acpi_device *adev,
> +				      struct cfg80211_chan_def *chandef)
> +{
> +	struct wbrf_ranges_in ranges_in = {0};
> +	int ret;
> +
> +	ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in);
> +	if (ret)
> +		return ret;
> +
> +	return wbrf_remove_exclusion(adev, &ranges_in);
> +}

I don't really see a good reason for wbrf_remove_exclusion_wlan and 
wbrf_add_exclusion_wlan to be static functions.
In the earlier verisons they were both in the ACPI file so it made sense 
as an exported symbol.

But they each only have a single calling site now and I think they 
should collapse into those functions.

> +
> +int ieee80211_add_wbrf(struct ieee80211_local *local,
> +		       struct cfg80211_chan_def *chandef)
> +{
> +	struct device *dev = local->hw.wiphy->dev.parent;
> +	struct acpi_device *acpi_dev;
> +
> +	if (!local->wbrf_supported)
> +		return 0;
> +
> +	acpi_dev = ACPI_COMPANION(dev);
> +	if (!acpi_dev)
> +		return -ENODEV;

ACPI devices won't go away, this should be an impossible failure.
When wbrf_supported was populated earlier on the ACPI device was checked.

> +
> +	return wbrf_add_exclusion_wlan(acpi_dev, chandef); > +}
> +
> +void ieee80211_remove_wbrf(struct ieee80211_local *local,
> +			   struct cfg80211_chan_def *chandef)
> +{
> +	struct device *dev = local->hw.wiphy->dev.parent;
> +	struct acpi_device *acpi_dev;
> +
> +	if (!local->wbrf_supported)
> +		return;
> +
> +	acpi_dev = ACPI_COMPANION(dev);
> +	if (!acpi_dev)
> +		return;
> +

ACPI devices won't go away, this should be an impossible failure.
When wbrf_supported was populated earlier on the ACPI device was checked.

> +	wbrf_remove_exclusion_wlan(acpi_dev, chandef);
> +}
> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> index 0b7e81db383d..227db04eac42 100644
> --- a/net/wireless/chan.c
> +++ b/net/wireless/chan.c
> @@ -141,7 +141,7 @@ static bool cfg80211_edmg_chandef_valid(const struct cfg80211_chan_def *chandef)
>   	return true;
>   }
>   
> -static int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
> +int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
>   {
>   	int mhz;
>   
> @@ -190,6 +190,7 @@ static int nl80211_chan_width_to_mhz(enum nl80211_chan_width chan_width)
>   	}
>   	return mhz;
>   }
> +EXPORT_SYMBOL(nl80211_chan_width_to_mhz);
>   
>   static int cfg80211_chandef_get_width(const struct cfg80211_chan_def *c)
>   {

Powered by blists - more mailing lists