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

[Kalle Valo <kvalo@qca.qualcomm.com>]

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