[RFC 1/2] ath10k: move create-ht-cap methods above set-antenna.

[RFC 1/2] ath10k: move create-ht-cap methods above set-antenna.

[greearb]

From: Ben Greear <greearb@candelatech.com>

We will need to use these in set-antenna, so move them
so that we do not have to define the method signatures.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This appears to work, but needs more testing before it should
be applied.  I'll do that testing tomorrow.

 drivers/net/wireless/ath/ath10k/mac.c | 172 +++++++++++++++++-----------------
 1 file changed, 86 insertions(+), 86 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index bde3a2f..dbef84a 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2408,6 +2408,92 @@ void ath10k_halt(struct ath10k *ar)
 	spin_unlock_bh(&ar->data_lock);
 }
 
+static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar)
+{
+	struct ieee80211_sta_vht_cap vht_cap = {0};
+	u16 mcs_map;
+	int i;
+
+	vht_cap.vht_supported = 1;
+	vht_cap.cap = ar->vht_cap_info;
+
+	mcs_map = 0;
+	for (i = 0; i < 8; i++) {
+		if (i < ar->num_rf_chains)
+			mcs_map |= IEEE80211_VHT_MCS_SUPPORT_0_9 << (i*2);
+		else
+			mcs_map |= IEEE80211_VHT_MCS_NOT_SUPPORTED << (i*2);
+	}
+
+	vht_cap.vht_mcs.rx_mcs_map = cpu_to_le16(mcs_map);
+	vht_cap.vht_mcs.tx_mcs_map = cpu_to_le16(mcs_map);
+
+	return vht_cap;
+}
+
+static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar)
+{
+	int i;
+	struct ieee80211_sta_ht_cap ht_cap = {0};
+
+	if (!(ar->ht_cap_info & WMI_HT_CAP_ENABLED))
+		return ht_cap;
+
+	ht_cap.ht_supported = 1;
+	ht_cap.ampdu_factor = IEEE80211_HT_MAX_AMPDU_64K;
+	ht_cap.ampdu_density = IEEE80211_HT_MPDU_DENSITY_8;
+	ht_cap.cap |= IEEE80211_HT_CAP_SUP_WIDTH_20_40;
+	ht_cap.cap |= IEEE80211_HT_CAP_DSSSCCK40;
+	ht_cap.cap |= WLAN_HT_CAP_SM_PS_STATIC << IEEE80211_HT_CAP_SM_PS_SHIFT;
+
+	if (ar->ht_cap_info & WMI_HT_CAP_HT20_SGI)
+		ht_cap.cap |= IEEE80211_HT_CAP_SGI_20;
+
+	if (ar->ht_cap_info & WMI_HT_CAP_HT40_SGI)
+		ht_cap.cap |= IEEE80211_HT_CAP_SGI_40;
+
+	if (ar->ht_cap_info & WMI_HT_CAP_DYNAMIC_SMPS) {
+		u32 smps;
+
+		smps   = WLAN_HT_CAP_SM_PS_DYNAMIC;
+		smps <<= IEEE80211_HT_CAP_SM_PS_SHIFT;
+
+		ht_cap.cap |= smps;
+	}
+
+	if (ar->ht_cap_info & WMI_HT_CAP_TX_STBC)
+		ht_cap.cap |= IEEE80211_HT_CAP_TX_STBC;
+
+	if (ar->ht_cap_info & WMI_HT_CAP_RX_STBC) {
+		u32 stbc;
+
+		stbc   = ar->ht_cap_info;
+		stbc  &= WMI_HT_CAP_RX_STBC;
+		stbc >>= WMI_HT_CAP_RX_STBC_MASK_SHIFT;
+		stbc <<= IEEE80211_HT_CAP_RX_STBC_SHIFT;
+		stbc  &= IEEE80211_HT_CAP_RX_STBC;
+
+		ht_cap.cap |= stbc;
+	}
+
+	if (ar->ht_cap_info & WMI_HT_CAP_LDPC)
+		ht_cap.cap |= IEEE80211_HT_CAP_LDPC_CODING;
+
+	if (ar->ht_cap_info & WMI_HT_CAP_L_SIG_TXOP_PROT)
+		ht_cap.cap |= IEEE80211_HT_CAP_LSIG_TXOP_PROT;
+
+	/* max AMSDU is implicitly taken from vht_cap_info */
+	if (ar->vht_cap_info & WMI_VHT_CAP_MAX_MPDU_LEN_MASK)
+		ht_cap.cap |= IEEE80211_HT_CAP_MAX_AMSDU;
+
+	for (i = 0; i < ar->num_rf_chains; i++)
+		ht_cap.mcs.rx_mask[i] = 0xFF;
+
+	ht_cap.mcs.tx_params |= IEEE80211_HT_MCS_TX_DEFINED;
+
+	return ht_cap;
+}
+
 static int ath10k_get_antenna(struct ieee80211_hw *hw, u32 *tx_ant, u32 *rx_ant)
 {
 	struct ath10k *ar = hw->priv;
@@ -4735,92 +4821,6 @@ static const struct ieee80211_iface_combination ath10k_10x_if_comb[] = {
 	},
 };
 
-static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar)
-{
-	struct ieee80211_sta_vht_cap vht_cap = {0};
-	u16 mcs_map;
-	int i;
-
-	vht_cap.vht_supported = 1;
-	vht_cap.cap = ar->vht_cap_info;
-
-	mcs_map = 0;
-	for (i = 0; i < 8; i++) {
-		if (i < ar->num_rf_chains)
-			mcs_map |= IEEE80211_VHT_MCS_SUPPORT_0_9 << (i*2);
-		else
-			mcs_map |= IEEE80211_VHT_MCS_NOT_SUPPORTED << (i*2);
-	}
-
-	vht_cap.vht_mcs.rx_mcs_map = cpu_to_le16(mcs_map);
-	vht_cap.vht_mcs.tx_mcs_map = cpu_to_le16(mcs_map);
-
-	return vht_cap;
-}
-
-static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar)
-{
-	int i;
-	struct ieee80211_sta_ht_cap ht_cap = {0};
-
-	if (!(ar->ht_cap_info & WMI_HT_CAP_ENABLED))
-		return ht_cap;
-
-	ht_cap.ht_supported = 1;
-	ht_cap.ampdu_factor = IEEE80211_HT_MAX_AMPDU_64K;
-	ht_cap.ampdu_density = IEEE80211_HT_MPDU_DENSITY_8;
-	ht_cap.cap |= IEEE80211_HT_CAP_SUP_WIDTH_20_40;
-	ht_cap.cap |= IEEE80211_HT_CAP_DSSSCCK40;
-	ht_cap.cap |= WLAN_HT_CAP_SM_PS_STATIC << IEEE80211_HT_CAP_SM_PS_SHIFT;
-
-	if (ar->ht_cap_info & WMI_HT_CAP_HT20_SGI)
-		ht_cap.cap |= IEEE80211_HT_CAP_SGI_20;
-
-	if (ar->ht_cap_info & WMI_HT_CAP_HT40_SGI)
-		ht_cap.cap |= IEEE80211_HT_CAP_SGI_40;
-
-	if (ar->ht_cap_info & WMI_HT_CAP_DYNAMIC_SMPS) {
-		u32 smps;
-
-		smps   = WLAN_HT_CAP_SM_PS_DYNAMIC;
-		smps <<= IEEE80211_HT_CAP_SM_PS_SHIFT;
-
-		ht_cap.cap |= smps;
-	}
-
-	if (ar->ht_cap_info & WMI_HT_CAP_TX_STBC)
-		ht_cap.cap |= IEEE80211_HT_CAP_TX_STBC;
-
-	if (ar->ht_cap_info & WMI_HT_CAP_RX_STBC) {
-		u32 stbc;
-
-		stbc   = ar->ht_cap_info;
-		stbc  &= WMI_HT_CAP_RX_STBC;
-		stbc >>= WMI_HT_CAP_RX_STBC_MASK_SHIFT;
-		stbc <<= IEEE80211_HT_CAP_RX_STBC_SHIFT;
-		stbc  &= IEEE80211_HT_CAP_RX_STBC;
-
-		ht_cap.cap |= stbc;
-	}
-
-	if (ar->ht_cap_info & WMI_HT_CAP_LDPC)
-		ht_cap.cap |= IEEE80211_HT_CAP_LDPC_CODING;
-
-	if (ar->ht_cap_info & WMI_HT_CAP_L_SIG_TXOP_PROT)
-		ht_cap.cap |= IEEE80211_HT_CAP_LSIG_TXOP_PROT;
-
-	/* max AMSDU is implicitly taken from vht_cap_info */
-	if (ar->vht_cap_info & WMI_VHT_CAP_MAX_MPDU_LEN_MASK)
-		ht_cap.cap |= IEEE80211_HT_CAP_MAX_AMSDU;
-
-	for (i = 0; i < ar->num_rf_chains; i++)
-		ht_cap.mcs.rx_mask[i] = 0xFF;
-
-	ht_cap.mcs.tx_params |= IEEE80211_HT_MCS_TX_DEFINED;
-
-	return ht_cap;
-}
-
 static void ath10k_get_arvif_iter(void *data, u8 *mac,
 				  struct ieee80211_vif *vif)
 {
-- 
1.7.11.7

[RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This lets the mac80211 stack use the proper ht-caps when
negotiating with the peer.  Note that all vdevs are guaranteed
to be down when antenna changes, so we can set the ht_caps
without worrying about messing up existing stations.

This patch also moves the get_nss_from_chainmask up in
the file.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 57 ++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index dbef84a..8bd47ea 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -2408,18 +2408,34 @@ void ath10k_halt(struct ath10k *ar)
 	spin_unlock_bh(&ar->data_lock);
 }
 
-static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar)
+static u32 get_nss_from_chainmask(u16 chain_mask)
+{
+	if ((chain_mask & 0x15) == 0x15)
+		return 4;
+	else if ((chain_mask & 0x7) == 0x7)
+		return 3;
+	else if ((chain_mask & 0x3) == 0x3)
+		return 2;
+	return 1;
+}
+
+static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar,
+							  bool use_cfg_chains)
 {
 	struct ieee80211_sta_vht_cap vht_cap = {0};
 	u16 mcs_map;
 	int i;
+	int nrf = ar->num_rf_chains;
+
+	if (use_cfg_chains && ar->cfg_tx_chainmask)
+		nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
 
 	vht_cap.vht_supported = 1;
 	vht_cap.cap = ar->vht_cap_info;
 
 	mcs_map = 0;
 	for (i = 0; i < 8; i++) {
-		if (i < ar->num_rf_chains)
+		if (i < nrf)
 			mcs_map |= IEEE80211_VHT_MCS_SUPPORT_0_9 << (i*2);
 		else
 			mcs_map |= IEEE80211_VHT_MCS_NOT_SUPPORTED << (i*2);
@@ -2431,10 +2447,15 @@ static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar)
 	return vht_cap;
 }
 
-static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar)
+static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar,
+						     bool use_cfg_chains)
 {
 	int i;
 	struct ieee80211_sta_ht_cap ht_cap = {0};
+	int nrf = ar->num_rf_chains;
+
+	if (use_cfg_chains && ar->cfg_tx_chainmask)
+		nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
 
 	if (!(ar->ht_cap_info & WMI_HT_CAP_ENABLED))
 		return ht_cap;
@@ -2486,7 +2507,7 @@ static struct ieee80211_sta_ht_cap ath10k_get_ht_cap(struct ath10k *ar)
 	if (ar->vht_cap_info & WMI_VHT_CAP_MAX_MPDU_LEN_MASK)
 		ht_cap.cap |= IEEE80211_HT_CAP_MAX_AMSDU;
 
-	for (i = 0; i < ar->num_rf_chains; i++)
+	for (i = 0; i < nrf; i++)
 		ht_cap.mcs.rx_mask[i] = 0xFF;
 
 	ht_cap.mcs.tx_params |= IEEE80211_HT_MCS_TX_DEFINED;
@@ -2528,6 +2549,8 @@ static void ath10k_check_chain_mask(struct ath10k *ar, u32 cm, const char* dbg)
 static int __ath10k_set_antenna(struct ath10k *ar, u32 tx_ant, u32 rx_ant)
 {
 	int ret;
+	struct ieee80211_sta_vht_cap vht_cap;
+	struct ieee80211_sta_ht_cap ht_cap;
 
 	lockdep_assert_held(&ar->conf_mutex);
 
@@ -2537,6 +2560,17 @@ static int __ath10k_set_antenna(struct ath10k *ar, u32 tx_ant, u32 rx_ant)
 	ar->cfg_tx_chainmask = tx_ant;
 	ar->cfg_rx_chainmask = rx_ant;
 
+	ht_cap = ath10k_get_ht_cap(ar, true);
+	vht_cap = ath10k_create_vht_cap(ar, true);
+
+	if (ar->phy_capability & WHAL_WLAN_11G_CAPABILITY)
+		ar->mac.sbands[IEEE80211_BAND_2GHZ].ht_cap = ht_cap;
+
+	if (ar->phy_capability & WHAL_WLAN_11A_CAPABILITY) {
+		ar->mac.sbands[IEEE80211_BAND_5GHZ].ht_cap = ht_cap;
+		ar->mac.sbands[IEEE80211_BAND_5GHZ].vht_cap = vht_cap;
+	}
+
 	if ((ar->state != ATH10K_STATE_ON) &&
 	    (ar->state != ATH10K_STATE_RESTARTED))
 		return 0;
@@ -2859,17 +2893,6 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 	return ret;
 }
 
-static u32 get_nss_from_chainmask(u16 chain_mask)
-{
-	if ((chain_mask & 0x15) == 0x15)
-		return 4;
-	else if ((chain_mask & 0x7) == 0x7)
-		return 3;
-	else if ((chain_mask & 0x3) == 0x3)
-		return 2;
-	return 1;
-}
-
 /*
  * TODO:
  * Figure out how to handle WMI_VDEV_SUBTYPE_P2P_DEVICE,
@@ -4864,8 +4887,8 @@ int ath10k_mac_register(struct ath10k *ar)
 
 	SET_IEEE80211_DEV(ar->hw, ar->dev);
 
-	ht_cap = ath10k_get_ht_cap(ar);
-	vht_cap = ath10k_create_vht_cap(ar);
+	ht_cap = ath10k_get_ht_cap(ar, false);
+	vht_cap = ath10k_create_vht_cap(ar, false);
 
 	if (ar->phy_capability & WHAL_WLAN_11G_CAPABILITY) {
 		channels = kmemdup(ath10k_2ghz_channels,
-- 
1.7.11.7

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Michal Kazior]

On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
[...]
> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar,
> +                                                         bool use_cfg_chains)
>  {
>         struct ieee80211_sta_vht_cap vht_cap = {0};
>         u16 mcs_map;
>         int i;
> +       int nrf = ar->num_rf_chains;
> +
> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);

Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
0x0 make any sense at all? Shouldn't we deny it or make it fallback to
the supported tx/rx chainmask values?


Michał

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Ben Greear]

On 09/24/2014 12:51 AM, Michal Kazior wrote:
> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
> [...]
>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar,
>> +                                                         bool use_cfg_chains)
>>   {
>>          struct ieee80211_sta_vht_cap vht_cap = {0};
>>          u16 mcs_map;
>>          int i;
>> +       int nrf = ar->num_rf_chains;
>> +
>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>
> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
> the supported tx/rx chainmask values?

It would cause the logic to flip back to the defaults, so seems mildly useful.  I'm not sure
upper layers would ever let it be < 1 though.

Thanks,
Ben

>
>
> Michał
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Michal Kazior]

On 24 September 2014 16:35, Ben Greear <greearb@candelatech.com> wrote:
> On 09/24/2014 12:51 AM, Michal Kazior wrote:
>> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
>> [...]
>>>
>>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k
>>> *ar,
>>> +                                                         bool
>>> use_cfg_chains)
>>>   {
>>>          struct ieee80211_sta_vht_cap vht_cap = {0};
>>>          u16 mcs_map;
>>>          int i;
>>> +       int nrf = ar->num_rf_chains;
>>> +
>>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>
>>
>> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
>> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
>> the supported tx/rx chainmask values?
>
> It would cause the logic to flip back to the defaults, so seems mildly
> useful.  I'm not sure
> upper layers would ever let it be < 1 though.

0 is a valid argument as far as upper layers are concerned and should
be treated as "use all available antennas" (see `iw list` output
before ever setting antenna, after setting to, e.g. 1 and then to 0).

This implies current set_antenna() implementation is actually buggy
(pdev param should involve using supp_tx/rx_chainmask). Your
assumption in recent patches is also incorrect as antenna mask = 0
should imply max nss, not 1.


Michał

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Ben Greear]

On 09/24/2014 08:05 AM, Michal Kazior wrote:
> On 24 September 2014 16:35, Ben Greear <greearb@candelatech.com> wrote:
>> On 09/24/2014 12:51 AM, Michal Kazior wrote:
>>> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
>>> [...]
>>>>
>>>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k
>>>> *ar,
>>>> +                                                         bool
>>>> use_cfg_chains)
>>>>    {
>>>>           struct ieee80211_sta_vht_cap vht_cap = {0};
>>>>           u16 mcs_map;
>>>>           int i;
>>>> +       int nrf = ar->num_rf_chains;
>>>> +
>>>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>>
>>>
>>> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
>>> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
>>> the supported tx/rx chainmask values?
>>
>> It would cause the logic to flip back to the defaults, so seems mildly
>> useful.  I'm not sure
>> upper layers would ever let it be < 1 though.
>
> 0 is a valid argument as far as upper layers are concerned and should
> be treated as "use all available antennas" (see `iw list` output
> before ever setting antenna, after setting to, e.g. 1 and then to 0).
>
> This implies current set_antenna() implementation is actually buggy
> (pdev param should involve using supp_tx/rx_chainmask). Your
> assumption in recent patches is also incorrect as antenna mask = 0
> should imply max nss, not 1.

I will test, but I think you are mis-understanding the logic in my
patches.  I should be using the max nss whenever configured value
is 0.

Thanks,
Ben

>
>
> Michał
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Ben Greear]

On 09/24/2014 08:05 AM, Michal Kazior wrote:
> On 24 September 2014 16:35, Ben Greear <greearb@candelatech.com> wrote:
>> On 09/24/2014 12:51 AM, Michal Kazior wrote:
>>> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
>>> [...]
>>>>
>>>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k
>>>> *ar,
>>>> +                                                         bool
>>>> use_cfg_chains)
>>>>   {
>>>>          struct ieee80211_sta_vht_cap vht_cap = {0};
>>>>          u16 mcs_map;
>>>>          int i;
>>>> +       int nrf = ar->num_rf_chains;
>>>> +
>>>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>>
>>>
>>> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
>>> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
>>> the supported tx/rx chainmask values?
>>
>> It would cause the logic to flip back to the defaults, so seems mildly
>> useful.  I'm not sure
>> upper layers would ever let it be < 1 though.
> 
> 0 is a valid argument as far as upper layers are concerned and should
> be treated as "use all available antennas" (see `iw list` output
> before ever setting antenna, after setting to, e.g. 1 and then to 0).
> 
> This implies current set_antenna() implementation is actually buggy
> (pdev param should involve using supp_tx/rx_chainmask). Your
> assumption in recent patches is also incorrect as antenna mask = 0
> should imply max nss, not 1.

I tested this using:

iw phy wiphy1 set antenna 0 0

This flips it back to 3x3 (I had previously configured it for 2x2),
so I think the patches are working properly.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Michal Kazior]

On 24 September 2014 18:30, Ben Greear <greearb@candelatech.com> wrote:
> On 09/24/2014 08:05 AM, Michal Kazior wrote:
>> On 24 September 2014 16:35, Ben Greear <greearb@candelatech.com> wrote:
>>> On 09/24/2014 12:51 AM, Michal Kazior wrote:
>>>> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
>>>> [...]
>>>>>
>>>>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k
>>>>> *ar,
>>>>> +                                                         bool
>>>>> use_cfg_chains)
>>>>>   {
>>>>>          struct ieee80211_sta_vht_cap vht_cap = {0};
>>>>>          u16 mcs_map;
>>>>>          int i;
>>>>> +       int nrf = ar->num_rf_chains;
>>>>> +
>>>>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>>>
>>>>
>>>> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
>>>> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
>>>> the supported tx/rx chainmask values?
>>>
>>> It would cause the logic to flip back to the defaults, so seems mildly
>>> useful.  I'm not sure
>>> upper layers would ever let it be < 1 though.
>>
>> 0 is a valid argument as far as upper layers are concerned and should
>> be treated as "use all available antennas" (see `iw list` output
>> before ever setting antenna, after setting to, e.g. 1 and then to 0).
>>
>> This implies current set_antenna() implementation is actually buggy
>> (pdev param should involve using supp_tx/rx_chainmask). Your
>> assumption in recent patches is also incorrect as antenna mask = 0
>> should imply max nss, not 1.
>
> I tested this using:
>
> iw phy wiphy1 set antenna 0 0
>
> This flips it back to 3x3 (I had previously configured it for 2x2),
> so I think the patches are working properly.

Mea culpa. It will flip back indeed.

But I still don't see why use_cfg_chains is necessary. I don't see how
cfg_tx_chainmask could be non-zero when ath10k is registering to mac.


Michał

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Ben Greear]

On 09/24/2014 11:23 PM, Michal Kazior wrote:
> On 24 September 2014 18:30, Ben Greear <greearb@candelatech.com> wrote:
>> On 09/24/2014 08:05 AM, Michal Kazior wrote:
>>> On 24 September 2014 16:35, Ben Greear <greearb@candelatech.com> wrote:
>>>> On 09/24/2014 12:51 AM, Michal Kazior wrote:
>>>>> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
>>>>> [...]
>>>>>>
>>>>>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k
>>>>>> *ar,
>>>>>> +                                                         bool
>>>>>> use_cfg_chains)
>>>>>>    {
>>>>>>           struct ieee80211_sta_vht_cap vht_cap = {0};
>>>>>>           u16 mcs_map;
>>>>>>           int i;
>>>>>> +       int nrf = ar->num_rf_chains;
>>>>>> +
>>>>>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>>>>>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>>>>>
>>>>>
>>>>> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
>>>>> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
>>>>> the supported tx/rx chainmask values?
>>>>
>>>> It would cause the logic to flip back to the defaults, so seems mildly
>>>> useful.  I'm not sure
>>>> upper layers would ever let it be < 1 though.
>>>
>>> 0 is a valid argument as far as upper layers are concerned and should
>>> be treated as "use all available antennas" (see `iw list` output
>>> before ever setting antenna, after setting to, e.g. 1 and then to 0).
>>>
>>> This implies current set_antenna() implementation is actually buggy
>>> (pdev param should involve using supp_tx/rx_chainmask). Your
>>> assumption in recent patches is also incorrect as antenna mask = 0
>>> should imply max nss, not 1.
>>
>> I tested this using:
>>
>> iw phy wiphy1 set antenna 0 0
>>
>> This flips it back to 3x3 (I had previously configured it for 2x2),
>> so I think the patches are working properly.
>
> Mea culpa. It will flip back indeed.
>
> But I still don't see why use_cfg_chains is necessary. I don't see how
> cfg_tx_chainmask could be non-zero when ath10k is registering to mac.

I was thinking we might want to re-register someday, like after loading
a new firmware, or tuning firmware differently so that the vdev limits
changed.

If you are sure we currently only register once per module load, then
I agree that use_cfg_chains should not be needed currently.

But, considering my desire to allow to re-register in the future, I'd
prefer the patch to remain as is unless you disagree.

Thanks,
Ben

>
>
> Michał
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 2/2] ath10k: re-config ht_caps when chainmask is modified.

[Ben Greear]

On 09/24/2014 12:51 AM, Michal Kazior wrote:
> On 24 September 2014 02:26,  <greearb@candelatech.com> wrote:
> [...]
>> +static struct ieee80211_sta_vht_cap ath10k_create_vht_cap(struct ath10k *ar,
>> +                                                         bool use_cfg_chains)
>>   {
>>          struct ieee80211_sta_vht_cap vht_cap = {0};
>>          u16 mcs_map;
>>          int i;
>> +       int nrf = ar->num_rf_chains;
>> +
>> +       if (use_cfg_chains && ar->cfg_tx_chainmask)
>> +               nrf = get_nss_from_chainmask(ar->cfg_tx_chainmask);
>
> Is use_cfg_chains really necessary here? Is setting tx/rx chainmask to
> 0x0 make any sense at all? Shouldn't we deny it or make it fallback to
> the supported tx/rx chainmask values?

I was thinking we should register with supported values, instead of
configured values.  That is the intention of the code.  In case we
ever re-register after user has configured the system, this should
retain that functionality.  If it is impossible to re-register the
wiphy, then this extra use_cfg_chains logic could go away.

On startup, before user ever configures anything (and most users never will),
the cfg-tx-chainmask is 0, so it would stay with chip's defaults.

Thanks,
Ben

>
>
> 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
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com