Search Linux Wireless

Re: [PATCH v2 2/2] wifi: mac80211: restructure vif and link conf for mlo mbssid support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> +++ b/include/net/mac80211.h
> @@ -682,6 +682,8 @@ struct ieee80211_parsed_tpe {
>   *	responder functionality.
>   * @ftmr_params: configurable lci/civic parameter when enabling FTM responder.
>   * @nontransmitted: this BSS is a nontransmitted BSS profile
> + * @mbssid_tx_bss: Pointer to the BSS configuration of transmitting interface
> + *	if MBSSID is enabled.
>   * @transmitter_bssid: the address of transmitter AP
>   * @bssid_index: index inside the multiple BSSID set
>   * @bssid_indicator: 2^bssid_indicator is the maximum number of APs in set
> @@ -790,6 +792,7 @@ struct ieee80211_bss_conf {
>  	struct ieee80211_ftm_responder_params *ftmr_params;
>  	/* Multiple BSSID data */
>  	bool nontransmitted;
> +	struct ieee80211_bss_conf *mbssid_tx_bss;

Please say a few words about the safety of accessing this pointer.

It doesn't _feel_ safe to me, if the vif/bss_conf itself may have been
reached via RCU for example. But I won't say that my gut feeling here is
necessarily correct. Please check and comment.

> @@ -2032,8 +2034,6 @@ struct ieee80211_vif {
>  	bool probe_req_reg;
>  	bool rx_mcast_action_reg;
>  
> -	struct ieee80211_vif *mbssid_tx_vif;

The same would've been true before, I guess, though perhaps less likely
to have been an issue since the other netdev must obviously be unlinked
first. Still pointer races could happen if they both go away at the same
time and we have a stale one via RCU?

> @@ -3636,23 +3651,43 @@ void ieee80211_csa_finish(struct ieee80211_vif *vif, unsigned int link_id)
>  		return;
>  	}
>  
> -	/* TODO: MBSSID with MLO changes */
> -	if (vif->mbssid_tx_vif == vif) {
> +	if (link_data->conf->mbssid_tx_bss == vif->link_conf[link_id]) {

And this shouldn't even build without (sparse) warnings (for similar
reasons)?

> +			valid_links = iter->vif.valid_links | BIT(0);
> +			for_each_set_bit(link_id_iter, &valid_links,
> +					 IEEE80211_MLD_MAX_NUM_LINKS) {

for_each_vif_active_link() is probably enough, on AP side you can't have
active != valid links I think. Or for_each_valid_link()?

> +				link_iter =
> +				      rcu_dereference(iter->link[link_id_iter]);
> +				if (!link_iter)
> +					continue;
> +				/* Check if any of link of iterator sdata
> +				 * belongs to same mbssid group as the tx link
> +				 */
> +				if (link_iter->conf->mbssid_tx_bss !=
> +				    vif->link_conf[link_id])
> +					continue;
> +
> +				wiphy_work_queue(iter->local->hw.wiphy,
> +						 &link_iter->csa.finalize_work);

This really got indented a bit too far anyway though - hardly readable
at all. Please refactor.

Or you could use for_each_sdata_link() I guess, at the small expense of
iterating all links of the interfaces you're going to skip anyway, but
that seems hardly important here?

> +			}
>  		}
>  	}
>  	wiphy_work_queue(local->hw.wiphy, &link_data->csa.finalize_work);
> @@ -4757,15 +4792,35 @@ ieee80211_color_change_bss_config_notify(struct ieee80211_link_data *link,
>  
>  	ieee80211_link_info_change_notify(sdata, link, changed);
>  
> -	if (!sdata->vif.bss_conf.nontransmitted && sdata->vif.mbssid_tx_vif) {
> +	if (!link->conf->nontransmitted && link->conf->mbssid_tx_bss) {
>  		struct ieee80211_sub_if_data *child;
> +		unsigned int link_id_iter;
> +		unsigned long valid_links;
> +		struct ieee80211_link_data *link_iter;
>  
>  		list_for_each_entry(child, &sdata->local->interfaces, list) {
> -			if (child != sdata && child->vif.mbssid_tx_vif == &sdata->vif) {
> -				child->vif.bss_conf.he_bss_color.color = color;
> -				child->vif.bss_conf.he_bss_color.enabled = enable;
> +			if (child == sdata)
> +				continue;

All the same comments here ...

> +	lockdep_assert_wiphy(sdata->local->hw.wiphy);
> +	/* Check link 0 by default for non MLO. */
> +	iter_valid_links = sdata->vif.valid_links | BIT(0);
> +	/* Check if any of the links of current sdata is an MBSSID. */
> +	for_each_set_bit(iter_link_id, &iter_valid_links,
> +			 IEEE80211_MLD_MAX_NUM_LINKS) {

And also here.

> +++ b/net/wireless/sme.c
> @@ -1581,7 +1581,7 @@ void cfg80211_autodisconnect_wk(struct work_struct *work)
>  		container_of(work, struct wireless_dev, disconnect_wk);
>  	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
>  
> -	wiphy_lock(wdev->wiphy);
> +	wiphy_lock(&rdev->wiphy);
>  
>  	if (wdev->conn_owner_nlportid) {
>  		switch (wdev->iftype) {
> @@ -1618,5 +1618,5 @@ void cfg80211_autodisconnect_wk(struct work_struct *work)
>  		}
>  	}
>  
> -	wiphy_unlock(wdev->wiphy);
> +	wiphy_unlock(&rdev->wiphy);
>  }

?????

johannes





[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux