| Message ID | 20230609072846.1552238-1-evan.quan@amd.com |
|---|---|
| Headers | show |
| Series | Support Wifi RFI interference mitigation feature | expand |
On Fri, 2023-06-09 at 15:28 +0800, Evan Quan wrote: > --- a/include/net/cfg80211.h > +++ b/include/net/cfg80211.h > @@ -5551,6 +5551,10 @@ struct wiphy { > > u16 hw_timestamp_max_peers; > > +#ifdef CONFIG_ACPI_WBRF > + bool wbrf_supported; > +#endif This should be in some private struct in mac80211, ieee80211_local I think. > char priv[] __aligned(NETDEV_ALIGN); > }; > > @@ -9067,4 +9071,18 @@ static inline int cfg80211_color_change_notify(struct net_device *dev) > bool cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap, > const struct cfg80211_chan_def *chandef); > > +#ifdef CONFIG_ACPI_WBRF > +void ieee80211_check_wbrf_support(struct wiphy *wiphy); > +int ieee80211_add_wbrf(struct wiphy *wiphy, > + struct cfg80211_chan_def *chandef); > +void ieee80211_remove_wbrf(struct wiphy *wiphy, > + struct cfg80211_chan_def *chandef); > +#else > +static inline void ieee80211_check_wbrf_support(struct wiphy *wiphy) { } > +static inline int ieee80211_add_wbrf(struct wiphy *wiphy, > + struct cfg80211_chan_def *chandef) { return 0; } > +static inline void ieee80211_remove_wbrf(struct wiphy *wiphy, > + struct cfg80211_chan_def *chandef) { } > +#endif /* CONFIG_ACPI_WBRF */ Same here, not the right place. This should even be in an internal mac80211 header (such as net/mac80211/ieee80211_i.h or create a new net/mac80211/wrbf.h or so if you prefer.) > --- a/net/mac80211/chan.c > +++ b/net/mac80211/chan.c > @@ -668,6 +668,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->hw.wiphy, &ctx->conf.def); > + if (err) > + return err; > + > if (!local->use_chanctx) > local->hw.conf.radar_enabled = ctx->conf.radar_enabled; > > @@ -748,6 +752,8 @@ static void ieee80211_del_chanctx(struct ieee80211_local *local, > } > > ieee80211_recalc_idle(local); > + > + ieee80211_remove_wbrf(local->hw.wiphy, &ctx->conf.def); > } > > static void ieee80211_free_chanctx(struct ieee80211_local *local, > This is tricky, and quite likely incorrect. First of all, chandefs can actually _change_, see _ieee80211_change_chanctx(). You'd probably have to call this add/remove (or have modify) whenever we call drv_change_chanctx() to change the width (not if radar/rx chains change). Secondly, you don't know if the driver will actually use ctx->conf.def, or ctx->conf.mindef. For client mode that doesn't matter, but for AP mode if the AP is configured to say 160 MHz, it might actually configure down to 20 MHz when no stations are connected (or only 20 MHz stations are). I don't know if you really care about taking that into account, I also don't know how dynamic this really should be. Stations can connect and disconnect quickly, so perhaps the WBRF should actually take the full potential bandwidth into account all the time, in which case taking ctx->conf.def would be correct. I'll note that your previous in-driver approach had all the same problems the way you had implemented it, though I don't know if that driver ever can use mindef or not. > +void ieee80211_check_wbrf_support(struct wiphy *wiphy) > +{ > + struct device *dev = wiphy->dev.parent; > + struct acpi_device *acpi_dev; > + > + acpi_dev = ACPI_COMPANION(dev); Can this cope with 'dev' being NULL? Just not sure nothing like hwsim or so always even has a parent. I guess it should, but ... > +static int chan_width_to_mhz(enum nl80211_chan_width chan_width) > +{ > + int mhz; > + > + switch (chan_width) { > + case NL80211_CHAN_WIDTH_1: > + mhz = 1; > + break; > + case NL80211_CHAN_WIDTH_2: > + mhz = 2; > + break; > + case NL80211_CHAN_WIDTH_4: > + mhz = 4; > + break; > + case NL80211_CHAN_WIDTH_8: > + mhz = 8; > + break; > + case NL80211_CHAN_WIDTH_16: > + mhz = 16; > + break; > + case NL80211_CHAN_WIDTH_5: > + mhz = 5; > + break; > + case NL80211_CHAN_WIDTH_10: > + mhz = 10; > + break; > + case NL80211_CHAN_WIDTH_20: > + case NL80211_CHAN_WIDTH_20_NOHT: > + mhz = 20; > + break; > + case NL80211_CHAN_WIDTH_40: > + mhz = 40; > + break; > + case NL80211_CHAN_WIDTH_80P80: > + case NL80211_CHAN_WIDTH_80: > + mhz = 80; > + break; > + case NL80211_CHAN_WIDTH_160: > + mhz = 160; > + break; > + case NL80211_CHAN_WIDTH_320: > + mhz = 320; > + break; > + default: > + WARN_ON_ONCE(1); > + return -1; > + } > + return mhz; This might be more generally useful as a function in cfg80211 that's exported - hwsim has exactly the same function today, for example. > +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); > +} Similar patterns are probably elsewhere too for this, but I guess we can always refactor later too. johannes
[AMD Official Use Only - General] Thanks Johannes. Comment in-line > -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Friday, June 9, 2023 4:21 PM > To: Quan, Evan <Evan.Quan@amd.com>; 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; kvalo@kernel.org; nbd@nbd.name; > lorenzo@kernel.org; ryder.lee@mediatek.com; shayne.chen@mediatek.com; > sean.wang@mediatek.com; matthias.bgg@gmail.com; > angelogioacchino.delregno@collabora.com; Limonciello, Mario > <Mario.Limonciello@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com> > Cc: 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 > Subject: Re: [PATCH V2 2/7] wifi: mac80211: Add support for ACPI WBRF > > On Fri, 2023-06-09 at 15:28 +0800, Evan Quan wrote: > > > --- a/include/net/cfg80211.h > > +++ b/include/net/cfg80211.h > > @@ -5551,6 +5551,10 @@ struct wiphy { > > > > u16 hw_timestamp_max_peers; > > > > +#ifdef CONFIG_ACPI_WBRF > > + bool wbrf_supported; > > +#endif > > This should be in some private struct in mac80211, ieee80211_local I think. There was indeed a proposal from Mario to put this in ieee80211_local. But I thought "wbrf_supported" stands for a device specific feature and "struct wiphy" seemed the right place. Anyway I can update this as suggested. > > > char priv[] __aligned(NETDEV_ALIGN); }; > > > > @@ -9067,4 +9071,18 @@ static inline int > > cfg80211_color_change_notify(struct net_device *dev) bool > cfg80211_valid_disable_subchannel_bitmap(u16 *bitmap, > > const struct cfg80211_chan_def > *chandef); > > > > +#ifdef CONFIG_ACPI_WBRF > > +void ieee80211_check_wbrf_support(struct wiphy *wiphy); int > > +ieee80211_add_wbrf(struct wiphy *wiphy, > > + struct cfg80211_chan_def *chandef); void > > +ieee80211_remove_wbrf(struct wiphy *wiphy, > > + struct cfg80211_chan_def *chandef); #else static > inline void > > +ieee80211_check_wbrf_support(struct wiphy *wiphy) { } static inline > > +int ieee80211_add_wbrf(struct wiphy *wiphy, > > + struct cfg80211_chan_def *chandef) > { return 0; } static > > +inline void ieee80211_remove_wbrf(struct wiphy *wiphy, > > + struct cfg80211_chan_def *chandef) > { } #endif /* > > +CONFIG_ACPI_WBRF */ > > Same here, not the right place. This should even be in an internal > mac80211 header (such as net/mac80211/ieee80211_i.h or create a new > net/mac80211/wrbf.h or so if you prefer.) Will update this altogether. > > > > --- a/net/mac80211/chan.c > > +++ b/net/mac80211/chan.c > > @@ -668,6 +668,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->hw.wiphy, &ctx->conf.def); > > + if (err) > > + return err; > > + > > if (!local->use_chanctx) > > local->hw.conf.radar_enabled = ctx->conf.radar_enabled; > > > > @@ -748,6 +752,8 @@ static void ieee80211_del_chanctx(struct > ieee80211_local *local, > > } > > > > ieee80211_recalc_idle(local); > > + > > + ieee80211_remove_wbrf(local->hw.wiphy, &ctx->conf.def); > > } > > > > static void ieee80211_free_chanctx(struct ieee80211_local *local, > > > > This is tricky, and quite likely incorrect. > > First of all, chandefs can actually _change_, see _ieee80211_change_chanctx(). > You'd probably have to call this add/remove (or have modify) whenever we > call drv_change_chanctx() to change the width (not if radar/rx chains change). Thanks for pointing this out. Unfortunately I have limit knowledge about network related. Can you help me to list all those APIs? Any others except the four below? _ieee80211_change_chanctx ieee80211_recalc_chanctx_min_def (Just curious why "!local->use_chanctx" is not cover in this API like others?) ieee80211_recalc_radar_chanctx (do you mean this one can be ignored ?) ieee80211_recalc_smps_chanctx > > Secondly, you don't know if the driver will actually use ctx->conf.def, or ctx- > >conf.mindef. For client mode that doesn't matter, but for AP mode if the AP is > configured to say 160 MHz, it might actually configure down to 20 MHz when > no stations are connected (or only 20 MHz stations are). I don't know if you > really care about taking that into account, I also don't know how dynamic this > really should be. Stations can connect and disconnect quickly, so perhaps the > WBRF should actually take the full potential bandwidth into account all the > time, in which case taking > ctx->conf.def would be correct. OK. If we cannot figure out what's the exact bandwidth in use for AP, I assume using ctx->conf.def should be OK. @Limonciello, Mario what's your opinion? > > I'll note that your previous in-driver approach had all the same problems the > way you had implemented it, though I don't know if that driver ever can use > mindef or not. > > > > +void ieee80211_check_wbrf_support(struct wiphy *wiphy) { > > + struct device *dev = wiphy->dev.parent; > > + struct acpi_device *acpi_dev; > > + > > + acpi_dev = ACPI_COMPANION(dev); > > Can this cope with 'dev' being NULL? Just not sure nothing like hwsim or so > always even has a parent. I guess it should, but ... OK, I see. I will add input checks for unexpected NULL. > > > +static int chan_width_to_mhz(enum nl80211_chan_width chan_width) { > > + int mhz; > > + > > + switch (chan_width) { > > + case NL80211_CHAN_WIDTH_1: > > + mhz = 1; > > + break; > > + case NL80211_CHAN_WIDTH_2: > > + mhz = 2; > > + break; > > + case NL80211_CHAN_WIDTH_4: > > + mhz = 4; > > + break; > > + case NL80211_CHAN_WIDTH_8: > > + mhz = 8; > > + break; > > + case NL80211_CHAN_WIDTH_16: > > + mhz = 16; > > + break; > > + case NL80211_CHAN_WIDTH_5: > > + mhz = 5; > > + break; > > + case NL80211_CHAN_WIDTH_10: > > + mhz = 10; > > + break; > > + case NL80211_CHAN_WIDTH_20: > > + case NL80211_CHAN_WIDTH_20_NOHT: > > + mhz = 20; > > + break; > > + case NL80211_CHAN_WIDTH_40: > > + mhz = 40; > > + break; > > + case NL80211_CHAN_WIDTH_80P80: > > + case NL80211_CHAN_WIDTH_80: > > + mhz = 80; > > + break; > > + case NL80211_CHAN_WIDTH_160: > > + mhz = 160; > > + break; > > + case NL80211_CHAN_WIDTH_320: > > + mhz = 320; > > + break; > > + default: > > + WARN_ON_ONCE(1); > > + return -1; > > + } > > + return mhz; > > This might be more generally useful as a function in cfg80211 that's exported - > hwsim has exactly the same function today, for example. Yes, this shares exactly the same implementation as nl80211_chan_width_to_mhz. Let me get nl80211_chan_width_to_mhz exposed so that other parts can share. > > > +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); > > +} > > Similar patterns are probably elsewhere too for this, but I guess we can always > refactor later too. I did check this before and I did not find similar patterns in other parts of the kernel. Although some of them can provide the information about the center frequency point (e.g. control_freq/ oper_freq). None of them can be used to tell the frequency of the start/end point of the range. Anyway, yes, we can refactor this later if we see such need. Evan > > johannes