| Message ID | 20230621054603.1262299-4-evan.quan@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support Wifi RFI interference mitigation feature | expand |
On Wed, 2023-06-21 at 13:45 +0800, Evan Quan wrote: > 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@amd.com> > Co-developed-by: Evan Quan <evan.quan@amd.com> > Signed-off-by: Evan Quan <evan.quan@amd.com> I was going to say this looks good ... but still have a few nits, sorry. But then the next question anyway is how we merge this? The wifi parts sort of depend on the first patch, although technically I guess I could merge them since it's all hidden behind the CONFIG_ symbol, assuming you get that in via some other tree it can combine upstream. I'd also say you can merge those parts elsewhere but I'm planning to also land some locking rework that I've been working on, so it will probably conflict somewhere. > +++ 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); You ignore the return value here. > @@ -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; But not here. In the code, there are basically two error paths: > +int ieee80211_add_wbrf(struct ieee80211_local *local, > + struct cfg80211_chan_def *chandef) > +{ > + struct device *dev = local->hw.wiphy->dev.parent; > + struct wbrf_ranges_in ranges_in = {0}; > + int ret; > + > + if (!local->wbrf_supported) > + return 0; > + > + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in); > + if (ret) > + return ret; This really won't fail, just if the bandwidth calculation was bad, but that's an internal error that WARNs anyway and we can ignore it. > + return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in); This I find a bit confusing, why do we even propagate the error? If the platform has some issue with it, should we really fail the connection? I think it seems better to me to just make this void, and have it be only a notification interface? johannes
On 6/21/2023 5:22 AM, Johannes Berg wrote: > On Wed, 2023-06-21 at 13:45 +0800, Evan Quan wrote: >> 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@amd.com> >> Co-developed-by: Evan Quan <evan.quan@amd.com> >> Signed-off-by: Evan Quan <evan.quan@amd.com> > I was going to say this looks good ... but still have a few nits, sorry. > > But then the next question anyway is how we merge this? The wifi parts > sort of depend on the first patch, although technically I guess I could > merge them since it's all hidden behind the CONFIG_ symbol, assuming you > get that in via some other tree it can combine upstream. > > I'd also say you can merge those parts elsewhere but I'm planning to > also land some locking rework that I've been working on, so it will > probably conflict somewhere. Since it's all gated by CONFIG_ACPI_WBRF for each subsystem that it touches, my take is that we should merge like this: 1) Get A-b/R-b on patch 1 (ACPI patch) from Rafael. 2) Merge mac80211 bits through WLAN trees 3) Merge AMDGPU bits *and* ACPI bits through amd-staging-drm-next followed by drm tree Since WLAN and AMDGPU bits are using the exported ACPI functions from patch 1, we need to make sure that it is accepted and won't change interface before merging other bits. Everything can come together in the upstream tree and the bots will be able to test linux-next as well this way. By bringing ACPI bits through amd-staging-drm-next we can also enable the new Kconfig option in AMD's CI system to make sure that all the amdgpu bits are going through CI testing too earlier before it even hits linux-next. >> +++ 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); > You ignore the return value here. > > >> @@ -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; > But not here. > > In the code, there are basically two error paths: > >> +int ieee80211_add_wbrf(struct ieee80211_local *local, >> + struct cfg80211_chan_def *chandef) >> +{ >> + struct device *dev = local->hw.wiphy->dev.parent; >> + struct wbrf_ranges_in ranges_in = {0}; >> + int ret; >> + >> + if (!local->wbrf_supported) >> + return 0; >> + >> + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in); >> + if (ret) >> + return ret; > This really won't fail, just if the bandwidth calculation was bad, but > that's an internal error that WARNs anyway and we can ignore it. > >> + return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in); > This I find a bit confusing, why do we even propagate the error? If the > platform has some issue with it, should we really fail the connection? > > > I think it seems better to me to just make this void, and have it be > only a notification interface? > > johannes
On Wed, 2023-06-21 at 09:12 -0500, Limonciello, Mario wrote: > > > > But then the next question anyway is how we merge this? The wifi parts > > sort of depend on the first patch, although technically I guess I could > > merge them since it's all hidden behind the CONFIG_ symbol, assuming you > > get that in via some other tree it can combine upstream. > > > > I'd also say you can merge those parts elsewhere but I'm planning to > > also land some locking rework that I've been working on, so it will > > probably conflict somewhere. > Since it's all gated by CONFIG_ACPI_WBRF for each subsystem that it touches, > my take is that we should merge like this: > > 1) Get A-b/R-b on patch 1 (ACPI patch) from Rafael. > 2) Merge mac80211 bits through WLAN trees > 3) Merge AMDGPU bits *and* ACPI bits through amd-staging-drm-next > followed by drm tree > > Since WLAN and AMDGPU bits are using the exported ACPI functions from > patch 1, we need to make sure that it is accepted and won't change > interface before merging other bits. Right. > Everything can come together in the upstream tree and the bots > will be able to test linux-next as well this way. Yeah, that's what I thought. Sounds good to me! Oh, also, since it's pretty late in the cycle I'm assuming for now that you're not aiming this for 6.5 anymore. If you still are, it all would probably need to happen very quickly. johannes
diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h index c4cf296e7eaf..0703921547f5 100644 --- a/include/linux/ieee80211.h +++ b/include/linux/ieee80211.h @@ -4319,6 +4319,7 @@ static inline int ieee80211_get_tdls_action(struct sk_buff *skb, u32 hdr_size) /* convert frequencies */ #define MHZ_TO_KHZ(freq) ((freq) * 1000) #define KHZ_TO_MHZ(freq) ((freq) / 1000) +#define KHZ_TO_HZ(freq) ((freq) * 1000) #define PR_KHZ(f) KHZ_TO_MHZ(f), f % 1000 #define KHZ_F "%d.%03d" 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..a96f74ef6584 --- /dev/null +++ b/net/mac80211/wbrf.c @@ -0,0 +1,111 @@ +// 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" + +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; + + /* 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; +} + +int ieee80211_add_wbrf(struct ieee80211_local *local, + struct cfg80211_chan_def *chandef) +{ + struct device *dev = local->hw.wiphy->dev.parent; + struct wbrf_ranges_in ranges_in = {0}; + int ret; + + if (!local->wbrf_supported) + return 0; + + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in); + if (ret) + return ret; + + return wbrf_add_exclusion(ACPI_COMPANION(dev), &ranges_in); +} + +void ieee80211_remove_wbrf(struct ieee80211_local *local, + struct cfg80211_chan_def *chandef) +{ + struct device *dev = local->hw.wiphy->dev.parent; + struct wbrf_ranges_in ranges_in = {0}; + int ret; + + if (!local->wbrf_supported) + return; + + ret = wbrf_get_ranges_from_chandef(chandef, &ranges_in); + if (ret) + return; + + wbrf_remove_exclusion(ACPI_COMPANION(dev), &ranges_in); +}