Message-ID: <83068c44-a4a7-41b9-84e2-b751deb1e093@amd.com>
Date:   Wed, 6 Sep 2023 14:57:59 -0500
From:   Mario Limonciello <mario.limonciello@....com>
To:     Jeff Johnson <quic_jjohnson@...cinc.com>,
        Evan Quan <evan.quan@....com>, lenb@...nel.org,
        johannes@...solutions.net, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
        alexander.deucher@....com, rafael@...nel.org, Lijo.Lazar@....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, netdev@...r.kernel.org
Subject: Re: [V11 3/8] wifi: mac80211: Add support for WBRF features

On 9/1/2023 09:32, Jeff Johnson wrote:
> On 8/30/2023 11:20 PM, Evan Quan wrote:
>> To support the WBRF mechanism, Wifi adapters utilized in the system must
> 
> Since this is the first mention of WBRF in the core wireless code IMO 
> you should indicate what this is an acronym for and briefly describe it
> (or add a lore link).

A lot of information is captured in the cover letter and earlier 
commits.  I think you raise a good point that 10 years from now someone 
looking at random commits will have a hard time understanding what 
exactly WBRF stands for.

How about if we introduce a wbrf.rst somewhere in Documentation/ that 
explains the basic principles of how/why for it.  This Documentation 
patch could be the first in the series and then the commit message for 
wireless subsystem can tell people to look at that path for more 
information.

> 
> I'm wondering if WBRF is just a special case of frequency avoidance, and 
> that more generic naming/terminology should be used in core wireless.
> For example, I know there are vendor-specific solutions which allow 
> Wi-Fi to avoid using channels which may conflict with cellular or 
> BlueTooth, and those may benefit from a more generic
>

It seems to me that most vendor solutions that exist don't operate in 
the kernel code but usually in firmware based solutions, right?

I think to come up with a generic solution we need to first have a 
vendor that "wants" to participate in a generic solution to design it 
properly.

>> register the frequencies in use(or unregister those frequencies no longer
>> used) via the dedicated calls. So that, other drivers responding to the
>> frequencies can take proper actions to mitigate possible interference.
>>
>> Co-developed-by: Mario Limonciello <mario.limonciello@....com>
>> 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)
>> v2->v3:
>>    - drop unnecessary input checks and intermediate APIs(Mario)
>>    - Separate some mac80211 common code(Mario, Johannes)
>> v3->v4:
>>    - some minor fixes around return values(Johannes)
>> v9->v10:
>>    - get ranges_in->num_of_ranges set and passed in(Johannes)
>> ---
>>   include/linux/ieee80211.h  |   1 +
>>   net/mac80211/Makefile      |   2 +
>>   net/mac80211/chan.c        |   9 ++++
>>   net/mac80211/ieee80211_i.h |   9 ++++
>>   net/mac80211/main.c        |   2 +
>>   net/mac80211/wbrf.c        | 105 +++++++++++++++++++++++++++++++++++++
>>   6 files changed, 128 insertions(+)
>>   create mode 100644 net/mac80211/wbrf.c
>>
>> diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
>> index 4b998090898e..f995d06da87f 100644
>> --- a/include/linux/ieee80211.h
>> +++ b/include/linux/ieee80211.h
>> @@ -4335,6 +4335,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..d46c36f55fd3 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-y += wbrf.o
>> +
>>   ccflags-y += -DDEBUG
>> diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
>> index 68952752b599..458469c224ae 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,8 @@ static int ieee80211_add_chanctx(struct 
>> ieee80211_local *local,
>>       lockdep_assert_held(&local->mtx);
>>       lockdep_assert_held(&local->chanctx_mtx);
>> +    ieee80211_add_wbrf(local, &ctx->conf.def);
>> +
>>       if (!local->use_chanctx)
>>           local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
>> @@ -748,6 +755,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 91633a0b723e..719f2c892132 100644
>> --- a/net/mac80211/ieee80211_i.h
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1600,6 +1600,8 @@ struct ieee80211_local {
>>       /* extended capabilities provided by mac80211 */
>>       u8 ext_capa[8];
>> +
>> +    bool wbrf_supported;
>>   };
>>   static inline struct ieee80211_sub_if_data *
>> @@ -2638,4 +2640,11 @@ 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);
>> +
>> +void ieee80211_check_wbrf_support(struct ieee80211_local *local);
>> +void 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);
>> +
>>   #endif /* IEEE80211_I_H */
>> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
>> index 24315d7b3126..b20bdaac84db 100644
>> --- a/net/mac80211/main.c
>> +++ b/net/mac80211/main.c
>> @@ -1396,6 +1396,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..63978c7d2bcb
>> --- /dev/null
>> +++ b/net/mac80211/wbrf.c
>> @@ -0,0 +1,105 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Wifi Band Exclusion Interface for WWAN
>> + * Copyright (C) 2023 Advanced Micro Devices
>> + *
>> + */
>> +
>> +#include <linux/acpi_amd_wbrf.h>
>> +#include <net/cfg80211.h>
>> +#include "ieee80211_i.h"
>> +
>> +void ieee80211_check_wbrf_support(struct ieee80211_local *local)
>> +{
>> +    struct wiphy *wiphy = local->hw.wiphy;
>> +    struct device *dev;
>> +
>> +    if (!wiphy)
>> +        return;
>> +
>> +    dev = wiphy->dev.parent;
>> +    if (!dev)
>> +        return;
>> +
>> +    local->wbrf_supported = acpi_amd_wbrf_supported_producer(dev);
> 
> 
> I haven't been looking at this series closely so perhaps this has 
> already been answered, but is this layered correctly? I'm surprised to 
> see core wireless code directly invoking a vendor-specific API.
> Instead should there be a registration mechanism with associated callbacks?
> 
> As I mentioned above I could envision multiple registrants for such an 
> interface.

This feedback was previously shared and earlier versions of the series 
had added another layer to make it generic.

The major complexity is that the semantics of how such a registration 
and notification would work are highly implementation specific.  So the 
generic solution just guessed at how it could work generically and fit 
the ACPI one into it.

The key tenants for a generic solution are:
1) Knowing that you need to notify frequencies
2) Knowing that you need to know about frequencies
3) Knowing which devices in the specific hardware design can be problematic.

It's a lot of complexity that you need deep knowledge of the design to 
do properly.  When this generic concept was brought to drivers/base 
however there was no interest in a generic layer at that time.

So this version of the series returns back to only introducing the AMD 
implementation.  The idea being if another implementation is proposed we 
can look at the overlap between how the two work and could jointly 
introduce a new generic layer.

Hope that explains it well.  As products that need this solution are 
entering the marketplace and can benefit from this kernel work, I hope 
this can be reviewed again soon.

> 
>> +    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 void get_ranges_from_chandef(struct cfg80211_chan_def *chandef,
>> +                    struct wbrf_ranges_in_out *ranges_in)
>> +{
>> +    u64 start_freq1, end_freq1;
>> +    u64 start_freq2, end_freq2;
>> +    int bandwidth;
>> +
>> +    bandwidth = nl80211_chan_width_to_mhz(chandef->width);
>> +
>> +    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;
>> +    ranges_in->num_of_ranges = 1;
>> +
>> +    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;
>> +        ranges_in->num_of_ranges++;
>> +    }
>> +}
>> +
>> +void ieee80211_add_wbrf(struct ieee80211_local *local,
>> +            struct cfg80211_chan_def *chandef)
>> +{
>> +    struct wbrf_ranges_in_out ranges_in = {0};
>> +    struct device *dev;
>> +
>> +    if (!local->wbrf_supported)
>> +        return;
>> +
>> +    dev = local->hw.wiphy->dev.parent;
>> +
>> +    get_ranges_from_chandef(chandef, &ranges_in);
>> +
>> +    acpi_amd_wbrf_add_exclusion(dev, &ranges_in);
>> +}
>> +
>> +void ieee80211_remove_wbrf(struct ieee80211_local *local,
>> +               struct cfg80211_chan_def *chandef)
>> +{
>> +    struct wbrf_ranges_in_out ranges_in = {0};
>> +    struct device *dev;
>> +
>> +    if (!local->wbrf_supported)
>> +        return;
>> +
>> +    dev = local->hw.wiphy->dev.parent;
>> +
>> +    get_ranges_from_chandef(chandef, &ranges_in);
>> +
>> +    acpi_amd_wbrf_remove_exclusion(dev, &ranges_in);
>> +}
> 

Powered by blists - more mailing lists