[AMD Official Use Only - General]
Thanks Johannes. Comment in-line
> -----Original Message-----
> From: Johannes Berg <johannes@xxxxxxxxxxxxxxxx>
> Sent: Friday, June 9, 2023 4:21 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; rafael@xxxxxxxxxx; lenb@xxxxxxxxxx;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Koenig, Christian
> <Christian.Koenig@xxxxxxx>; Pan, Xinhui <Xinhui.Pan@xxxxxxx>;
> airlied@xxxxxxxxx; daniel@xxxxxxxx; kvalo@xxxxxxxxxx; nbd@xxxxxxxx;
> lorenzo@xxxxxxxxxx; ryder.lee@xxxxxxxxxxxx; shayne.chen@xxxxxxxxxxxx;
> sean.wang@xxxxxxxxxxxx; matthias.bgg@xxxxxxxxx;
> angelogioacchino.delregno@xxxxxxxxxxxxx; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>; Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-acpi@xxxxxxxxxxxxxxx; amd-
> gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-
> wireless@xxxxxxxxxxxxxxx
> 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
