| Message ID | 1411426820-4047-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 23 September 2014 01:00, <greearb@candelatech.com> wrote: > From: Ben Greear <greearb@candelatech.com> > > When re-associating a station, the nss was set back to > maximum value even if user had configured small number > of tx chains. So, pay attention to user's config in > this case as well. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > drivers/net/wireless/ath/ath10k/mac.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c > index 855c71c..c5d31cc 100644 > --- a/drivers/net/wireless/ath/ath10k/mac.c > +++ b/drivers/net/wireless/ath/ath10k/mac.c > @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, > u32 legacy = 0x00ff; > u8 ht = 0xff, i; > u16 vht = 0x3ff; > + u16 nrf = ar->num_rf_chains; > + > + if (ar->cfg_tx_chainmask) > + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); Oh, so you do update the peer nss value here. I think it might be a good idea to convey the limitation of tx/rx chainmask to the user: you can't change the tx/rx chainmask on the fly easily (while connected/have associated stations). Or do you plan to schedule peer reassoc in __ath10k_set_antenna() in a follow up patch(es) later? Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/23/2014 01:59 AM, Michal Kazior wrote: > On 23 September 2014 01:00, <greearb@candelatech.com> wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> When re-associating a station, the nss was set back to >> maximum value even if user had configured small number >> of tx chains. So, pay attention to user's config in >> this case as well. >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> drivers/net/wireless/ath/ath10k/mac.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c >> index 855c71c..c5d31cc 100644 >> --- a/drivers/net/wireless/ath/ath10k/mac.c >> +++ b/drivers/net/wireless/ath/ath10k/mac.c >> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, >> u32 legacy = 0x00ff; >> u8 ht = 0xff, i; >> u16 vht = 0x3ff; >> + u16 nrf = ar->num_rf_chains; >> + >> + if (ar->cfg_tx_chainmask) >> + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); > > Oh, so you do update the peer nss value here. Yeah, I found this was needed in further testing yesterday... > I think it might be a good idea to convey the limitation of tx/rx > chainmask to the user: you can't change the tx/rx chainmask on the fly > easily (while connected/have associated stations). Or do you plan to > schedule peer reassoc in __ath10k_set_antenna() in a follow up > patch(es) later? My user-space tools assume you have to re-connect (requested from user-space) after changing the chainmask. I was under the impression that this is how ath9k works, though I could be wrong. I would rather keep this requirement in user-space... I am not sure where to document this...just a comment in the code? Thanks, Ben
On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote: > On 09/23/2014 01:59 AM, Michal Kazior wrote: >> On 23 September 2014 01:00, <greearb@candelatech.com> wrote: >>> From: Ben Greear <greearb@candelatech.com> [...] >>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, >>> u32 legacy = 0x00ff; >>> u8 ht = 0xff, i; >>> u16 vht = 0x3ff; >>> + u16 nrf = ar->num_rf_chains; >>> + >>> + if (ar->cfg_tx_chainmask) >>> + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); [...] >> I think it might be a good idea to convey the limitation of tx/rx >> chainmask to the user: you can't change the tx/rx chainmask on the fly >> easily (while connected/have associated stations). Or do you plan to >> schedule peer reassoc in __ath10k_set_antenna() in a follow up >> patch(es) later? > > My user-space tools assume you have to re-connect (requested from user-space) > after changing the chainmask. I was under the impression that this > is how ath9k works, though I could be wrong. ath9k doesn't have firmware and it doesn't have a blackbox rate control so maybe it doesn't need to worry about this as much as ath10k needs to. But then this is not something you normally use except some very specific use cases so I think it might not be worth the hassle to implement full-blown reconfig after chainmask is updated. > I would rather keep this requirement in user-space... > > I am not sure where to document this...just a comment in the code? Hmm.. Perhaps in ath10k_set_antenna? Or maybe we should just put it up on the wiki? But it's still probably a good idea to comment the quoted code chunk above explaining why nrf is overriden by chainmask (i.e. due to firmware rate control issues, right?). Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/24/2014 12:09 AM, Michal Kazior wrote: > On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote: >> On 09/23/2014 01:59 AM, Michal Kazior wrote: >>> On 23 September 2014 01:00, <greearb@candelatech.com> wrote: >>>> From: Ben Greear <greearb@candelatech.com> > [...] >>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, >>>> u32 legacy = 0x00ff; >>>> u8 ht = 0xff, i; >>>> u16 vht = 0x3ff; >>>> + u16 nrf = ar->num_rf_chains; >>>> + >>>> + if (ar->cfg_tx_chainmask) >>>> + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); > [...] >>> I think it might be a good idea to convey the limitation of tx/rx >>> chainmask to the user: you can't change the tx/rx chainmask on the fly >>> easily (while connected/have associated stations). Or do you plan to >>> schedule peer reassoc in __ath10k_set_antenna() in a follow up >>> patch(es) later? >> >> My user-space tools assume you have to re-connect (requested from user-space) >> after changing the chainmask. I was under the impression that this >> is how ath9k works, though I could be wrong. > > ath9k doesn't have firmware and it doesn't have a blackbox rate > control so maybe it doesn't need to worry about this as much as ath10k > needs to. > > But then this is not something you normally use except some very > specific use cases so I think it might not be worth the hassle to > implement full-blown reconfig after chainmask is updated. See this in net/mac80211/cfg.c: static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) { struct ieee80211_local *local = wiphy_priv(wiphy); if (local->started) return -EOPNOTSUPP; return drv_set_antenna(local, tx_ant, rx_ant); } It ensures there are no active vdevs when this change is made, so you should not have to worry about block-ack or anything similar. > But it's still probably a good idea to comment the quoted code chunk > above explaining why nrf is overriden by chainmask (i.e. due to > firmware rate control issues, right?). I am not certain it is a bug in the firmware. The driver should not configure nss incorrectly as it was doing previous to my recent patches. Thanks, Ben
On 24 September 2014 18:09, Ben Greear <greearb@candelatech.com> wrote: > On 09/24/2014 12:09 AM, Michal Kazior wrote: >> On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote: >>> On 09/23/2014 01:59 AM, Michal Kazior wrote: >>>> On 23 September 2014 01:00, <greearb@candelatech.com> wrote: >>>>> From: Ben Greear <greearb@candelatech.com> >> [...] >>>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, >>>>> u32 legacy = 0x00ff; >>>>> u8 ht = 0xff, i; >>>>> u16 vht = 0x3ff; >>>>> + u16 nrf = ar->num_rf_chains; >>>>> + >>>>> + if (ar->cfg_tx_chainmask) >>>>> + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); >> [...] >>>> I think it might be a good idea to convey the limitation of tx/rx >>>> chainmask to the user: you can't change the tx/rx chainmask on the fly >>>> easily (while connected/have associated stations). Or do you plan to >>>> schedule peer reassoc in __ath10k_set_antenna() in a follow up >>>> patch(es) later? [...] > See this in net/mac80211/cfg.c: > > static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) > { > struct ieee80211_local *local = wiphy_priv(wiphy); > > if (local->started) > return -EOPNOTSUPP; Ah, thanks! So my argument is invalid :) >> But it's still probably a good idea to comment the quoted code chunk >> above explaining why nrf is overriden by chainmask (i.e. due to >> firmware rate control issues, right?). > > I am not certain it is a bug in the firmware. The driver should not configure > nss incorrectly as it was doing previous to my recent patches. Since firmware does rate control it just seems redundant for the driver to update both chainmask and nss (the first implies the other). But maybe that's just me. Micha? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/24/2014 11:16 PM, Michal Kazior wrote: > On 24 September 2014 18:09, Ben Greear <greearb@candelatech.com> wrote: >> On 09/24/2014 12:09 AM, Michal Kazior wrote: >>> On 23 September 2014 18:48, Ben Greear <greearb@candelatech.com> wrote: >>>> On 09/23/2014 01:59 AM, Michal Kazior wrote: >>>>> On 23 September 2014 01:00, <greearb@candelatech.com> wrote: >>>>>> From: Ben Greear <greearb@candelatech.com> >>> [...] >>>>>> @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, >>>>>> u32 legacy = 0x00ff; >>>>>> u8 ht = 0xff, i; >>>>>> u16 vht = 0x3ff; >>>>>> + u16 nrf = ar->num_rf_chains; >>>>>> + >>>>>> + if (ar->cfg_tx_chainmask) >>>>>> + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); >>> [...] >>>>> I think it might be a good idea to convey the limitation of tx/rx >>>>> chainmask to the user: you can't change the tx/rx chainmask on the fly >>>>> easily (while connected/have associated stations). Or do you plan to >>>>> schedule peer reassoc in __ath10k_set_antenna() in a follow up >>>>> patch(es) later? > [...] >> See this in net/mac80211/cfg.c: >> >> static int ieee80211_set_antenna(struct wiphy *wiphy, u32 tx_ant, u32 rx_ant) >> { >> struct ieee80211_local *local = wiphy_priv(wiphy); >> >> if (local->started) >> return -EOPNOTSUPP; > > Ah, thanks! So my argument is invalid :) > > >>> But it's still probably a good idea to comment the quoted code chunk >>> above explaining why nrf is overriden by chainmask (i.e. due to >>> firmware rate control issues, right?). >> >> I am not certain it is a bug in the firmware. The driver should not configure >> nss incorrectly as it was doing previous to my recent patches. > > Since firmware does rate control it just seems redundant for the > driver to update both chainmask and nss (the first implies the other). > But maybe that's just me. nss is really a per-station issue, where the chainmask is a per-radio issue. Think of an AP with 2x2 and 3x3 stations connected, for instance. Thanks, Ben > > > Micha? >
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index 855c71c..c5d31cc 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -4086,6 +4086,10 @@ ath10k_default_bitrate_mask(struct ath10k *ar, u32 legacy = 0x00ff; u8 ht = 0xff, i; u16 vht = 0x3ff; + u16 nrf = ar->num_rf_chains; + + if (ar->cfg_tx_chainmask) + nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask); switch (band) { case IEEE80211_BAND_2GHZ: @@ -4101,11 +4105,11 @@ ath10k_default_bitrate_mask(struct ath10k *ar, if (mask->control[band].legacy != legacy) return false; - for (i = 0; i < ar->num_rf_chains; i++) + for (i = 0; i < nrf; i++) if (mask->control[band].ht_mcs[i] != ht) return false; - for (i = 0; i < ar->num_rf_chains; i++) + for (i = 0; i < nrf; i++) if (mask->control[band].vht_mcs[i] != vht) return false; @@ -4356,6 +4360,9 @@ static int ath10k_set_bitrate_mask(struct ieee80211_hw *hw, u8 fixed_nss = ar->num_rf_chains; u8 force_sgi; + if (ar->cfg_tx_chainmask) + fixed_nss = get_nss_from_chainmask(ar->cfg_tx_chainmask); + force_sgi = mask->control[band].gi; if (force_sgi == NL80211_TXRATE_FORCE_LGI) return -EINVAL;