linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@qca.qualcomm.com>
To: Janusz Dziedzic <janusz.dziedzic@tieto.com>
Cc: <ath10k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] ath10k: add implementation for configure max amsdu, ampdu
Date: Tue, 27 May 2014 17:20:55 +0300	[thread overview]
Message-ID: <87zji39ubc.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <1400245332-6974-1-git-send-email-janusz.dziedzic@tieto.com> (Janusz Dziedzic's message of "Fri, 16 May 2014 15:02:12 +0200")

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

> Allow to setup maximum subframes for AMSDU and AMPDU aggregation,
> using debugfs htt_max_amsdu_ampdu file.
>
> Eg.
> echo "2 64" > htt_max_amsdu_ampdu
> will setup maximum amsdu subframes equal 2 and
> maximum ampdu subframes equal to 64.
>
> Signed-off-by: Janusz Dziedzic <janusz.dziedzic@tieto.com>

I saw two new warnings:

drivers/net/wireless/ath/ath10k/debug.c:683: WARNING: line over 80 characters
drivers/net/wireless/ath/ath10k/debug.c:685: WARNING: line over 80 characters

> I wasn't sure we have to protect
> debug.htt_max_amsdu, debug.htt_max_ampdu using conf_mutex.
> Anyway I can add such protection if required.

Yeah, we need to protect those variables.

> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -671,6 +671,61 @@ static const struct file_operations fops_htt_stats_mask = {
>  	.llseek = default_llseek,
>  };
>  
> +static ssize_t ath10k_read_htt_max_amsdu_ampdu(struct file *file,
> +					       char __user *user_buf,
> +					       size_t count, loff_t *ppos)
> +{
> +	struct ath10k *ar = file->private_data;
> +	char buf[64];
> +	unsigned int len;
> +
> +	if (!ar->debug.htt_max_amsdu || !ar->debug.htt_max_ampdu)
> +		len = scnprintf(buf, sizeof(buf), "using FW default max subframes configuration: amsdu 3, ampdu 64\n");
> +	else
> +		len = scnprintf(buf, sizeof(buf), "max subframes amsdu %u, ampdu %d\n",
> +				ar->debug.htt_max_amsdu,
> +				ar->debug.htt_max_ampdu);
> +
> +	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
> +}

I would prefer to have something more machine readable, for example
something like this:

if (ar->debug.htt_max_amsdu != 0) {
        amsdu = ar->debug.htt_max_amsdu;
} else {
        amsdu = 3;
        amsdu_default = " (default)";
}

len += scnprintf(buf + len, sizeof(buf) - len, 
                "max subframes amsdu %u%s\n", amsdu, amsdu_default);

And the same for ampdu. But I'm not actually sure if it makes any sense
to print the default at all?

Now I think more maybe we should just use the same format as we use as
input ("%u %u"). That would follow the KISS principle.

> @@ -757,6 +812,9 @@ void ath10k_debug_stop(struct ath10k *ar)
>  	 * warning from del_timer(). */
>  	if (ar->debug.htt_stats_mask != 0)
>  		cancel_delayed_work(&ar->debug.htt_stats_dwork);
> +
> +	ar->debug.htt_max_amsdu = 0;
> +	ar->debug.htt_max_ampdu = 0;
>  }

This is correct. But the problem now is that the value only is valid
until the interface is put down or firmware crashes. That's a bit
confusing for the user, but I guess we can fix that later if that will
become a problem. Users should not use this command normally.

> +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;
> +	int ret;
> +
> +	/* By default FW setup amsdu = 3 and ampdu = 64 */
> +	if (max_subfrms_ampdu == 0 || max_subfrms_ampdu > 64)
> +		return -EINVAL;

I didn't immediately understand the comment above. Is it just to
document what are the firmware defaults? In that case please add an
empty line after the comment and clear up the wording. For example,
"Firmware defaults are: amsdu = 3 and ampdu = 64 */".

> +
> +	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 amsdu %d, ampdu %d",
> +		   aggr_conf->max_num_amsdu_subframes,
> +		   aggr_conf->max_num_ampdu_subframes);

No comma in the debug message, please.

-- 
Kalle Valo

      reply	other threads:[~2014-05-27 14:21 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-16 13:02 [PATCH] ath10k: add implementation for configure max amsdu, ampdu Janusz Dziedzic
2014-05-27 14:20 ` Kalle Valo [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87zji39ubc.fsf@kamboji.qca.qualcomm.com \
    --to=kvalo@qca.qualcomm.com \
    --cc=ath10k@lists.infradead.org \
    --cc=janusz.dziedzic@tieto.com \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).