On Tue, 2024-09-10 at 13:45 -0700, Muna Sinada wrote:
>
> For MLO MBSSID, if the transmitted profile if part of an MLD, then the
> transmitted profile is a specific link of that MLD. Utilizing only Tx
> index is no longer sufficient to identify transmitted profile for MLO.
"Tx index"? Did you mean "interface index" or something?
> Add a new attribute to specify link id of the transmitted profile of
> MBSSID group if the profile is part of an MLD. It is required to map
> the nontransmitted link with corresponding transmitted link.
s/with/to/ or something?
> * @tx_wdev: pointer to the transmitted interface in the MBSSID set
> + * @tx_link_id: link ID of the transmitted interface if it is part of an MLD.
document the value for non-MLO?
> * @index: index of this AP in the multi bssid group.
> * @ema: set to true if the beacons should be sent out in EMA mode.
> */
> struct cfg80211_mbssid_config {
> struct wireless_dev *tx_wdev;
> + int tx_link_id;
> u8 index;
> bool ema;
> };
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index f97f5adc8d51..6bd46b4998c9 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -7987,6 +7987,10 @@ enum nl80211_sar_specs_attrs {
> * Setting this flag is permitted only if the driver advertises EMA support
> * by setting wiphy->ema_max_profile_periodicity to non-zero.
> *
> + * @NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID: Mandatory parameter for a non-transmitted
> + * profile which provides the link ID (u8) of the transmitted profile when the latter
> + * is part of an MLD.
It's probably better to rewrite this, the qualification of the first
word ("mandatory") comes at the very end of the sentence. German
speakers are probably used to that, but otherwise it seems a bit hard to
understand? ;)
> + [NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID] =
> + NLA_POLICY_MAX(NLA_U8, IEEE80211_MLD_MAX_NUM_LINKS),
why not just indent one tab?
> @@ -5477,6 +5479,8 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
> u8 num_elems)
> {
> struct nlattr *tb[NL80211_MBSSID_CONFIG_ATTR_MAX + 1];
> + struct net_device *tx_netdev = NULL;
> + int err = -EINVAL;
I don't much like this pattern of initializing the error first, can't
you initialize it in every place? I even wondered looking at the below
if it was correct everywhere.
> if (!wiphy->mbssid_max_interfaces)
> return -EOPNOTSUPP;
> @@ -5509,9 +5513,7 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
> return -EINVAL;
>
> if (tx_ifindex != dev->ifindex) {
> - struct net_device *tx_netdev =
> - dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
> -
> + tx_netdev = dev_get_by_index(wiphy_net(wiphy), tx_ifindex);
> if (!tx_netdev || !tx_netdev->ieee80211_ptr ||
> tx_netdev->ieee80211_ptr->wiphy != wiphy ||
> tx_netdev->ieee80211_ptr->iftype !=
> @@ -5530,7 +5532,28 @@ static int nl80211_parse_mbssid_config(struct wiphy *wiphy,
> return -EINVAL;
> }
>
> + config->tx_link_id = 0;
> + if (config->tx_wdev->valid_links) {
> + if (!tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID])
> + goto err;
> +
> + config->tx_link_id = nla_get_u8(tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]);
> + if (!(config->tx_wdev->valid_links & BIT(config->tx_link_id))) {
> + err = -ENOLINK;
> + goto err;
> + }
> + } else if (tb[NL80211_MBSSID_CONFIG_ATTR_TX_LINK_ID]) {
> + goto err;
> + }
> +
> return 0;
> +
> +err:
> + if (tx_netdev) {
> + config->tx_wdev = NULL;
> + dev_put(tx_netdev);
> + }
Why not use config->tx_wdev and avoid changes around tx_netdev?
There's also an existing error path that does dev_put(), so you should
unify that.
johannes
