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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.