Re: [PATCH V2 2/7] wifi: mac80211: Add support for ACPI WBRF

[Johannes Berg <johannes@sipsolutions.net>]

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