[RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[Janusz Dziedzic]

Allow to setup maximum subframes for AMSDU and AMPDU aggregation.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
Please check this patches, if help when MacBook Pro Retina used.

 drivers/net/wireless/ath/ath10k/htt.h    |   15 +++++-----
 drivers/net/wireless/ath/ath10k/htt_tx.c |   44 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 645a563..3ad0e09 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -240,16 +240,14 @@ struct htt_oob_sync_req {
 	__le16 rsvd0;
 } __packed;
 
-#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F
-#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  0
+#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F00
+#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  8
+#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_MASK 0xFF
+#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_LSB  0
 
 struct htt_aggr_conf {
 	u8 max_num_ampdu_subframes;
-	union {
-		/* dont use bitfields; undefined behaviour */
-		u8 flags; /* see %HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_ */
-		u8 max_num_amsdu_subframes:5;
-	} __packed;
+	u8 max_num_amsdu_subframes;
 } __packed;
 
 #define HTT_MGMT_FRM_HDR_DOWNLOAD_LEN 32
@@ -1341,6 +1339,9 @@ void ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb);
 int ath10k_htt_h2t_ver_req_msg(struct ath10k_htt *htt);
 int ath10k_htt_h2t_stats_req(struct ath10k_htt *htt, u8 mask, u64 cookie);
 int ath10k_htt_send_rx_ring_cfg_ll(struct ath10k_htt *htt);
+int ath10k_htt_h2t_aggr_cfg_msg(struct ath10k_htt *htt,
+				u8 max_subfrms_ampdu,
+				u8 max_subfrms_amsdu);
 
 void __ath10k_htt_tx_dec_pending(struct ath10k_htt *htt);
 int ath10k_htt_tx_alloc_msdu_id(struct ath10k_htt *htt);
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c b/drivers/net/wireless/ath/ath10k/htt_tx.c
index 7a3e2e4..c3ec9d1 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -307,6 +307,50 @@ int ath10k_htt_send_rx_ring_cfg_ll(struct ath10k_htt *htt)
 	return 0;
 }
 
+int ath10k_htt_h2t_aggr_cfg_msg(struct ath10k_htt *htt,
+				u8 max_subfrms_ampdu,
+				u8 max_subfrms_amsdu)
+{
+	struct htt_aggr_conf *aggr_conf;
+	struct sk_buff *skb;
+	struct htt_cmd *cmd;
+	int len = 0;
+	int ret;
+
+	/* By default FW setup amsdu = 3 and ampdu = 64 */
+	if (max_subfrms_ampdu == 0 || max_subfrms_ampdu > 64)
+		return -EINVAL;
+	if (max_subfrms_amsdu == 0 || max_subfrms_amsdu > 31)
+		return -EINVAL;
+
+	len += sizeof(cmd->hdr);
+	len += sizeof(cmd->aggr_conf);
+
+	skb = ath10k_htc_alloc_skb(len);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_put(skb, len);
+	cmd = (struct htt_cmd *)skb->data;
+	cmd->hdr.msg_type = HTT_H2T_MSG_TYPE_AGGR_CFG;
+
+	aggr_conf = &cmd->aggr_conf;
+	aggr_conf->max_num_ampdu_subframes = max_subfrms_ampdu;
+	aggr_conf->max_num_amsdu_subframes = max_subfrms_amsdu;
+
+	ath10k_dbg(ATH10K_DBG_HTT, "htt h2t aggr cfg msg ampdu %d, amsdu %d",
+		   aggr_conf->max_num_ampdu_subframes,
+		   aggr_conf->max_num_ampdu_subframes);
+
+	ret = ath10k_htc_send(&htt->ar->htc, htt->eid, skb);
+	if (ret) {
+		dev_kfree_skb_any(skb);
+		return ret;
+	}
+
+	return 0;
+}
+
 int ath10k_htt_mgmt_tx(struct ath10k_htt *htt, struct sk_buff *msdu)
 {
 	struct device *dev = htt->ar->dev;
-- 
1.7.9.5


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[RFC 2/2] ath10k: setup AMSDU subframes to 1

[Janusz Dziedzic]

Fore test purpose only.

Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index 75b3dfb..1e492a1 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -847,6 +847,8 @@ int ath10k_core_start(struct ath10k *ar)
 	if (status)
 		goto err_disconnect_htc;
 
+	WARN_ON(ath10k_htt_h2t_aggr_cfg_msg(&ar->htt, 64, 1));
+
 	status = ath10k_debug_start(ar);
 	if (status)
 		goto err_disconnect_htc;
-- 
1.7.9.5


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 2/2] ath10k: setup AMSDU subframes to 1

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> Fore test purpose only.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
> ---
>  drivers/net/wireless/ath/ath10k/core.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
> index 75b3dfb..1e492a1 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -847,6 +847,8 @@ int ath10k_core_start(struct ath10k *ar)
>  	if (status)
>  		goto err_disconnect_htc;
>  
> +	WARN_ON(ath10k_htt_h2t_aggr_cfg_msg(&ar->htt, 64, 1));

I anticipate we will need to change AMSDU values in the future and it
would be good to have some proper interface for this command. Maybe a
debugfs file which takes both values? Or something else?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> Allow to setup maximum subframes for AMSDU and AMPDU aggregation.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
> ---
> Please check this patches, if help when MacBook Pro Retina used.

Did you somehow check that this command works as it should?

> --- a/drivers/net/wireless/ath/ath10k/htt.h
> +++ b/drivers/net/wireless/ath/ath10k/htt.h
> @@ -240,16 +240,14 @@ struct htt_oob_sync_req {
>  	__le16 rsvd0;
>  } __packed;
>  
> -#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F
> -#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  0
> +#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F00
> +#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  8
> +#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_MASK 0xFF
> +#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_LSB  0

The masks here (0x1f and 0xff) look strange to me. Are they really
correct?

Ah, but we are not using them anywhere? Can you still double check that
the firmware interface is really like this, just for my sake?

> +int ath10k_htt_h2t_aggr_cfg_msg(struct ath10k_htt *htt,
> +				u8 max_subfrms_ampdu,
> +				u8 max_subfrms_amsdu)
> +{
> +	struct htt_aggr_conf *aggr_conf;
> +	struct sk_buff *skb;
> +	struct htt_cmd *cmd;
> +	int len = 0;
> +	int ret;

I prefer not to initialise variables here if possible, so this would be
better:

        int ret, len;

> +	/* By default FW setup amsdu = 3 and ampdu = 64 */
> +	if (max_subfrms_ampdu == 0 || max_subfrms_ampdu > 64)
> +		return -EINVAL;

Empty line here.

> +	if (max_subfrms_amsdu == 0 || max_subfrms_amsdu > 31)
> +		return -EINVAL;
> +
> +	len += sizeof(cmd->hdr);

len = sizeof(cmd->hdr);

(Because not initialising len)

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[Janusz Dziedzic]

On 15 May 2014 13:24, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>
>> Allow to setup maximum subframes for AMSDU and AMPDU aggregation.
>>
>> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>
>> ---
>> Please check this patches, if help when MacBook Pro Retina used.
>
> Did you somehow check that this command works as it should?
>
Added WMI_DEBUG_EVENT in FW (_htt_tgt_tx_aggr_cfg):

May 15 12:47:30 dell kernel: [ 6777.355444] ath10k: wmi event debug
print 'xxx amsdu: 3, ampdu: 64' // This is default :)
May 15 12:47:30 dell kernel: [ 6777.355477] ath10k: wmi event debug
print 'xxx data: 0x00014005, 81925'
May 15 12:47:30 dell kernel: [ 6777.355520] ath10k: wmi event debug
print 'xxx amsdu: 1, ampdu: 64'

>> --- a/drivers/net/wireless/ath/ath10k/htt.h
>> +++ b/drivers/net/wireless/ath/ath10k/htt.h
>> @@ -240,16 +240,14 @@ struct htt_oob_sync_req {
>>       __le16 rsvd0;
>>  } __packed;
>>
>> -#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F
>> -#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  0
>> +#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_MASK 0x1F00
>> +#define HTT_AGGR_CONF_MAX_NUM_AMSDU_SUBFRAMES_LSB  8
>> +#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_MASK 0xFF
>> +#define HTT_AGGR_CONF_MAX_NUM_AMPDU_SUBFRAMES_LSB  0
>
> The masks here (0x1f and 0xff) look strange to me. Are they really
> correct?
>
Yes, mask is correct, added only as a reference (will remove in final version)
And because only 0x1F ( < 32 limitation) for u8 max_num_amsdu_subframes.

> Ah, but we are not using them anywhere? Can you still double check that
> the firmware interface is really like this, just for my sake?
>
>> +int ath10k_htt_h2t_aggr_cfg_msg(struct ath10k_htt *htt,
>> +                             u8 max_subfrms_ampdu,
>> +                             u8 max_subfrms_amsdu)
>> +{
>> +     struct htt_aggr_conf *aggr_conf;
>> +     struct sk_buff *skb;
>> +     struct htt_cmd *cmd;
>> +     int len = 0;
>> +     int ret;
>
> I prefer not to initialise variables here if possible, so this would be
> better:
>
>         int ret, len;
>
>> +     /* By default FW setup amsdu = 3 and ampdu = 64 */
>> +     if (max_subfrms_ampdu == 0 || max_subfrms_ampdu > 64)
>> +             return -EINVAL;
>
> Empty line here.
>
>> +     if (max_subfrms_amsdu == 0 || max_subfrms_amsdu > 31)
>> +             return -EINVAL;
>> +
>> +     len += sizeof(cmd->hdr);
>
> len = sizeof(cmd->hdr);
>
> (Because not initialising len)
>
> --
> Kalle Valo
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[Kalle Valo]

Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:

> On 15 May 2014 13:24, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>
>> Did you somehow check that this command works as it should?
>
> Added WMI_DEBUG_EVENT in FW (_htt_tgt_tx_aggr_cfg):
>
> May 15 12:47:30 dell kernel: [ 6777.355444] ath10k: wmi event debug
> print 'xxx amsdu: 3, ampdu: 64' // This is default :)
> May 15 12:47:30 dell kernel: [ 6777.355477] ath10k: wmi event debug
> print 'xxx data: 0x00014005, 81925'
> May 15 12:47:30 dell kernel: [ 6777.355520] ath10k: wmi event debug
> print 'xxx amsdu: 1, ampdu: 64'

Ok, that's one way to test as well. I was more thinking about using
sniffer or another way to check what's really happening on the air.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC 1/2] ath10k: add implementation for configure max amsdu, ampdu

[Bartosz Markowski]

On 15 May 2014 14:24, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Janusz Dziedzic <janusz.dziedzic@tieto.com> writes:
>
>> On 15 May 2014 13:24, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>>
>>> Did you somehow check that this command works as it should?
>>
>> Added WMI_DEBUG_EVENT in FW (_htt_tgt_tx_aggr_cfg):
>>
>> May 15 12:47:30 dell kernel: [ 6777.355444] ath10k: wmi event debug
>> print 'xxx amsdu: 3, ampdu: 64' // This is default :)
>> May 15 12:47:30 dell kernel: [ 6777.355477] ath10k: wmi event debug
>> print 'xxx data: 0x00014005, 81925'
>> May 15 12:47:30 dell kernel: [ 6777.355520] ath10k: wmi event debug
>> print 'xxx amsdu: 1, ampdu: 64'
>
> Ok, that's one way to test as well. I was more thinking about using
> sniffer or another way to check what's really happening on the air.

ok, so we have finally testing this (V2:
http://permalink.gmane.org/gmane.linux.kernel.wireless.general/124128)
today as it looks like the root issue may be similar to the macBook
IOT problem reported by Denton here:
http://lists.infradead.org/pipermail/ath10k/2014-July/002576.html

Anyway the commands are working fine and we can set the desired
aggregation settings.

-Bartosz

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k