From: Johannes Berg <johannes@sipsolutions.net>
To: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [PATCH RFC] wifi: cfg80211: Refactor interface combination input parameter
Date: Tue, 07 May 2024 11:47:29 +0200 [thread overview]
Message-ID: <3f8e4d6d0f2facde80ad82b5b3060eb0af0958a4.camel@sipsolutions.net> (raw)
In-Reply-To: <20240427031503.22870-1-quic_periyasa@quicinc.com>
On Sat, 2024-04-27 at 08:45 +0530, Karthikeyan Periyasamy wrote:
> Currently, the interface combination input parameter num_different_channels
> and iftype_num are directly filled in by the caller under the assumption
> that all channels and interfaces belong to a single hardware device. This
> assumption is incorrect for multi-device interface combinations because
> each device supports a different set of channels and interfaces. As
> discussed in [1], need to refactor the input parameters to encode enough
> data to handle both single and multiple device interface combinations.
> This can be achieved by encoding the frequency and interface type under
> the interface entity itself. With this new input parameter structure, the
> cfg80211 can classify and construct the device parameters, then verify them
> against the device specific interface combinations.
^^ This should probably mention _something_ about links too :)
> [1]: https://lore.kernel.org/linux-wireless/ca70eeb3cdee1e8c3caee69db595bc8160eb4115.camel@sipsolutions.net/
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
> ---
> drivers/net/wireless/ath/wil6210/cfg80211.c | 44 +++++--
> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 60 +++++++--
> .../net/wireless/quantenna/qtnfmac/cfg80211.c | 32 +++--
> include/net/cfg80211.h | 37 +++++-
> net/mac80211/util.c | 124 +++++++++++++++---
> net/wireless/util.c | 56 ++++++--
> 6 files changed, 276 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c
> index 8993028709ec..3f9f5f56bd19 100644
> --- a/drivers/net/wireless/ath/wil6210/cfg80211.c
> +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c
> @@ -625,17 +625,25 @@ static int wil_cfg80211_validate_add_iface(struct wil6210_priv *wil,
> {
> int i;
> struct wireless_dev *wdev;
> - struct iface_combination_params params = {
> - .num_different_channels = 1,
> - };
> + struct iface_combination_params params = { 0 };
nit: just use "= {}".
> + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL);
> + if (!ifaces)
> + return -ENOMEM;
> +
> + list_for_each_entry(pos, &cfg->vif_list, list) {
> + if (params.num_iface >= total_iface)
> + continue;
??
Seems like that should be a WARN_ON or something?
> + struct iface_combination_interface *ifaces = NULL;
> + u16 total_iface = 0;
> + int ret;
>
> list_for_each_entry(pos, &cfg->vif_list, list)
> - params.iftype_num[pos->wdev.iftype]++;
> + total_iface++;
>
> - params.iftype_num[new_type]++;
> - return cfg80211_check_combinations(cfg->wiphy, ¶ms);
> + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL);
No point in "= NULL" if you overwrite it immediately.
> +/**
> + * struct iface_combination_iface_link - Interface combination link parameter
> + *
> + * Used to pass link specific interface combination parameters
> + *
> + * @freq: center frequency used for verification against the different channels
> + */
> +struct iface_combination_iface_link {
> + u32 freq;
> +};
> +
> +/**
> + * struct iface_combination_interface - Interface parameter for iface combination
> + *
> + * Used to pass interface specific parameter for iface combination
> + *
> + * @iftype: interface type as specified in &enum nl80211_iftype.
> + * @links: array with the number of link parameter used for verification
> + * @num_link: the length of the @links parameter used in this interface
> + */
> +struct iface_combination_interface {
> + enum nl80211_iftype iftype;
> + struct iface_combination_iface_link links[IEEE80211_MLD_MAX_NUM_LINKS];
> + u8 num_link;
Might be simpler (for the producers at least, but not really much more
difficult for the consumer) to just remove num_link, use the link ID as
the index, and declare freq==0 means unused?
> - * @num_different_channels: the number of different channels we want
> - * to use for verification
> * @radar_detect: a bitmap where each bit corresponds to a channel
> * width where radar detection is needed, as in the definition of
> * &struct ieee80211_iface_combination.@radar_detect_widths
> - * @iftype_num: array with the number of interfaces of each interface
> - * type. The index is the interface type as specified in &enum
> - * nl80211_iftype.
> * @new_beacon_int: set this to the beacon interval of a new interface
> * that's not operating yet, if such is to be checked as part of
> * the verification
> + * @ifaces: array with the number of interface parameter use for verification
> + * @num_iface: the length of the @ifaces interface parameter
> */
> struct iface_combination_params {
> - int num_different_channels;
> u8 radar_detect;
> - int iftype_num[NUM_NL80211_IFTYPES];
> u32 new_beacon_int;
> + const struct iface_combination_interface *ifaces;
> + u16 num_iface;
The "new_beacon_int" also needs to be for a specific link, witha a
specific freq, so you can check for *that* part of the wiphy? Similarly
for radar_detect?
> + if (iftype != NL80211_IFTYPE_UNSPECIFIED || chandef) {
> + struct iface_combination_interface *iface;
> +
> + iface = &ifaces[params.num_iface];
> + iface->iftype = iftype;
> +
> + if (chandef && cfg80211_chandef_valid(chandef)) {
> + iface->links[0].freq = chandef->chan->center_freq;
> + iface->num_link++;
> }
Not sure I understand this.
> @@ -4009,14 +4029,37 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> wdev_iter->iftype, 0, 1))
> continue;
>
> - params.iftype_num[wdev_iter->iftype]++;
> + iface = &ifaces[params.num_iface];
> + iface->iftype = wdev_iter->iftype;
> +
> + rcu_read_lock();
> + for_each_vif_active_link(&sdata_iter->vif, link_conf, link_id) {
> + struct ieee80211_chanctx_conf *chanctx_conf;
> + struct iface_combination_iface_link *link;
> +
> + chanctx_conf = rcu_dereference(link_conf->chanctx_conf);
> + if (chanctx_conf &&
> + cfg80211_chandef_valid(&chanctx_conf->def)) {
Why the valid check, btw? How could that possibly *not* be valid?
> + link = &iface->links[iface->num_link];
> + link->freq = chanctx_conf->def.chan->center_freq;
> + iface->num_link++;
> + }
> + }
> + rcu_read_unlock();
when you also have this?
But maybe separating out actual logic changes in mac80211 to a separate
patch would be good.
> list_for_each_entry_rcu(sdata, &local->interfaces, list)
> - params.iftype_num[sdata->wdev.iftype]++;
> + total_iface++;
> +
> + if (!total_iface)
> + goto skip;
> +
> + ifaces = kcalloc(total_iface, sizeof(*ifaces), GFP_KERNEL);
> + if (!ifaces)
> + return -ENOMEM;
> +
> + list_for_each_entry_rcu(sdata, &local->interfaces, list) {
> + struct iface_combination_interface *iface;
> +
> + if (params.num_iface >= total_iface)
> + continue;
> +
> + iface = &ifaces[params.num_iface];
> + iface->iftype = sdata->wdev.iftype;
> +
> + rcu_read_lock();
> + for_each_vif_active_link(&sdata->vif, link_conf, link_id) {
> + struct ieee80211_chanctx_conf *chanctx_conf;
> + struct iface_combination_iface_link *link;
> +
> + chanctx_conf = rcu_dereference(link_conf->chanctx_conf);
> + if (chanctx_conf &&
> + cfg80211_chandef_valid(&chanctx_conf->def)) {
> + link = &iface->links[iface->num_link];
> + link->freq = chanctx_conf->def.chan->center_freq;
> + iface->num_link++;
> + }
> + }
> + rcu_read_unlock();
> +
> + params.num_iface++;
> + }
Please don't add the same code twice.
johannes
next prev parent reply other threads:[~2024-05-07 9:47 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-27 3:15 [PATCH RFC] wifi: cfg80211: Refactor interface combination input parameter Karthikeyan Periyasamy
2024-05-07 9:47 ` Johannes Berg [this message]
2024-05-07 14:35 ` Karthikeyan Periyasamy
2024-05-07 15:35 ` Karthikeyan Periyasamy
2024-05-07 17:41 ` Karthikeyan Periyasamy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3f8e4d6d0f2facde80ad82b5b3060eb0af0958a4.camel@sipsolutions.net \
--to=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
--cc=quic_periyasa@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).