| Message ID | 20170825023024.10565-15-igor.mitsyanko.os@quantenna.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
I am ok with removal of CSA chandef info. It was kept mainly to warn about channel switch to the frequency that differs from original CSA request. > - if (vif->vifid != 0) { > - if (!(mac->status & QTNF_MAC_CSA_ACTIVE)) > - return -EOPNOTSUPP; > - > - if (!cfg80211_chandef_identical(¶ms->chandef, > - &mac->csa_chandef)) > - return -EINVAL; > - > - return 0; > - } This particular CSA_ACTIVE status check was introduced for compatibility with hostapd behaviour. Currently hostapd goes through all the virtual interfaces and sends CSA for each of them. So the idea was to send CSA for primary interface and confirm success for the others. If this snipped is dropped then we end up in multiple identical CSA requests queued in firmware. I suggest to remove chandef_identical check, but to keep the logic for secondary virtual interface handling. Regards, Sergey
On 08/29/2017 08:44 AM, Sergey Matyukevich wrote: > I am ok with removal of CSA chandef info. It was kept mainly to warn > about channel switch to the frequency that differs from original > CSA request. > >> - if (vif->vifid != 0) { >> - if (!(mac->status & QTNF_MAC_CSA_ACTIVE)) >> - return -EOPNOTSUPP; >> - >> - if (!cfg80211_chandef_identical(¶ms->chandef, >> - &mac->csa_chandef)) >> - return -EINVAL; >> - >> - return 0; >> - } > > > This particular CSA_ACTIVE status check was introduced for compatibility with > hostapd behaviour. Currently hostapd goes through all the virtual interfaces > and sends CSA for each of them. So the idea was to send CSA for primary > interface and confirm success for the others. If this snipped is dropped > then we end up in multiple identical CSA requests queued in firmware. > > I suggest to remove chandef_identical check, but to keep the logic for > secondary virtual interface handling. > > Regards, > Sergey > The idea was that driver doesn't have to keep track of this, but instead it will just forward directly to card, and card will return success for CSA request to the same channel, because card knows that already. As a bonus, in case it will ever support virtual concurrent (virtual interfaces on different physical channels), no changes to driver will be required.
On Tue, Aug 29, 2017 at 06:48:23PM -0700, Igor Mitsyanko wrote: > On 08/29/2017 08:44 AM, Sergey Matyukevich wrote: > > I am ok with removal of CSA chandef info. It was kept mainly to warn > > about channel switch to the frequency that differs from original > > CSA request. > > > > > - if (vif->vifid != 0) { > > > - if (!(mac->status & QTNF_MAC_CSA_ACTIVE)) > > > - return -EOPNOTSUPP; > > > - > > > - if (!cfg80211_chandef_identical(¶ms->chandef, > > > - &mac->csa_chandef)) > > > - return -EINVAL; > > > - > > > - return 0; > > > - } > > > > > > This particular CSA_ACTIVE status check was introduced for compatibility with > > hostapd behaviour. Currently hostapd goes through all the virtual interfaces > > and sends CSA for each of them. So the idea was to send CSA for primary > > interface and confirm success for the others. If this snipped is dropped > > then we end up in multiple identical CSA requests queued in firmware. > > > > I suggest to remove chandef_identical check, but to keep the logic for > > secondary virtual interface handling. > > The idea was that driver doesn't have to keep track of this, but instead it > will just forward directly to card, and card will return success for CSA > request to the same channel, because card knows that already. > > As a bonus, in case it will ever support virtual concurrent (virtual > interfaces on different physical channels), no changes to driver will be > required. If we drop this check from driver, then we have to add similar check into the firmware for the time being. We should drop subsequent CSA requests for secondary virtual interfaces. Regards, Sergey
diff --git a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c index b33c4fc..8542c16 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c +++ b/drivers/net/wireless/quantenna/qtnfmac/cfg80211.c @@ -736,7 +736,6 @@ qtnf_get_channel(struct wiphy *wiphy, struct wireless_dev *wdev, static int qtnf_channel_switch(struct wiphy *wiphy, struct net_device *dev, struct cfg80211_csa_settings *params) { - struct qtnf_wmac *mac = wiphy_priv(wiphy); struct qtnf_vif *vif = qtnf_netdev_get_priv(dev); int ret; @@ -744,17 +743,6 @@ static int qtnf_channel_switch(struct wiphy *wiphy, struct net_device *dev, params->chandef.chan->hw_value, params->count, params->radar_required, params->block_tx); - if (vif->vifid != 0) { - if (!(mac->status & QTNF_MAC_CSA_ACTIVE)) - return -EOPNOTSUPP; - - if (!cfg80211_chandef_identical(¶ms->chandef, - &mac->csa_chandef)) - return -EINVAL; - - return 0; - } - if (!cfg80211_chandef_valid(¶ms->chandef)) { pr_err("%s: invalid channel\n", dev->name); return -EINVAL; diff --git a/drivers/net/wireless/quantenna/qtnfmac/commands.c b/drivers/net/wireless/quantenna/qtnfmac/commands.c index cfbb636..7a616f9 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/commands.c +++ b/drivers/net/wireless/quantenna/qtnfmac/commands.c @@ -2347,8 +2347,6 @@ int qtnf_cmd_send_chan_switch(struct qtnf_vif *vif, switch (res_code) { case QLINK_CMD_RESULT_OK: - memcpy(&mac->csa_chandef, ¶ms->chandef, - sizeof(mac->csa_chandef)); mac->status |= QTNF_MAC_CSA_ACTIVE; ret = 0; break; diff --git a/drivers/net/wireless/quantenna/qtnfmac/core.h b/drivers/net/wireless/quantenna/qtnfmac/core.h index aa1a92f..5997915 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/core.h +++ b/drivers/net/wireless/quantenna/qtnfmac/core.h @@ -132,7 +132,6 @@ struct qtnf_wmac { struct qtnf_vif iflist[QTNF_MAX_INTF]; struct cfg80211_scan_request *scan_req; struct cfg80211_chan_def chandef; - struct cfg80211_chan_def csa_chandef; struct mutex mac_lock; /* lock during wmac speicific ops */ struct timer_list scan_timeout; }; diff --git a/drivers/net/wireless/quantenna/qtnfmac/event.c b/drivers/net/wireless/quantenna/qtnfmac/event.c index 7435986..b9ceeed 100644 --- a/drivers/net/wireless/quantenna/qtnfmac/event.c +++ b/drivers/net/wireless/quantenna/qtnfmac/event.c @@ -368,13 +368,7 @@ qtnf_event_handle_freq_change(struct qtnf_wmac *mac, mac->macid, chdef.chan->hw_value, chdef.center_freq1, chdef.center_freq2, chdef.width); - if (mac->status & QTNF_MAC_CSA_ACTIVE) { - mac->status &= ~QTNF_MAC_CSA_ACTIVE; - if (chdef.chan->hw_value != mac->csa_chandef.chan->hw_value) - pr_warn("unexpected switch to %u during CSA to %u\n", - chdef.chan->hw_value, - mac->csa_chandef.chan->hw_value); - } + mac->status &= ~QTNF_MAC_CSA_ACTIVE; memcpy(&mac->chandef, &chdef, sizeof(mac->chandef));