From: Bob Copeland <me@bobcopeland.com>
To: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [PATCH 09/21] cw1200: txrx.*, implementation of datapath.
Date: Mon, 7 May 2012 08:53:16 -0400 [thread overview]
Message-ID: <20120507125316.GA25400@localhost> (raw)
In-Reply-To: <1330652495-25837-10-git-send-email-dmitry.tarnyagin@stericsson.com>
Hi Dmitry,
A couple of other quick notes follow. I'll make and send on those
other patches sometime this week as I get time.
On Fri, Mar 02, 2012 at 02:41:23AM +0100, Dmitry Tarnyagin wrote:
> + /* minstrel is buggy a little bit, so distille
> + * incoming rates first. */
Can you elaborate a bit on the bugs? It may be worth fixing them
in minstrel so that everyone benefits. From your code it looks
like you want to reorder them from fastest->slowest, eliminate
duplicate rates, and change retry counts to respect total retry
limits. I agree that the last two are problematic and should
probably be fixed in minstrel.
Changing the rate order may have adverse affects (I'm not sure).
Minstrel often puts 'probe' rates in early MRR slots to update
its statistics, these may well be lower than the current rate.
> + /* Sort rates in descending order. */
> + for (i = 1; i < count; ++i) {
> + if (rates[i].idx < 0) {
> + count = i;
> + break;
> + }
> + if (rates[i].idx > rates[i - 1].idx) {
> + struct ieee80211_tx_rate tmp = rates[i - 1];
> + rates[i - 1] = rates[i];
> + rates[i] = tmp;
> + }
> + }
This sort seems to be missing an inner loop. E.g. if input was 2-3-5-4,
I think you would end up with 2-5-4-3 instead of 5-4-3-2.
> + /* ">> 1" is an equivalent of "/ 2", but faster */
> + int mid_rate = (rates[0].idx + 4) >> 1;
(This is true, but it's only by a couple of instructions for signed
division since compilers still change divide-by-power-of-two to shifts.
Unsigned /2 typically has the same code as >> 1. I tend to prefer /2
for readability unless it's a super hot path, but that's your call.)
--
Bob Copeland %% www.bobcopeland.com
next prev parent reply other threads:[~2012-05-07 12:53 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <d53e019b1a0bcd29c2c367fbe5665413f2d33938-submit>
2012-03-02 1:41 ` [PATCH 00/21] cw1200: mac80211-based driver for ST-Ericsson CW1200 device Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 01/21] cw1200: cw1200.h, private driver data Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 02/21] cw1200: cw1200_plat.h, definition of the driver'ss platform data Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 03/21] cw1200: sbus.h, common device interface abstraction Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 04/21] cw1200: cw1200_sdio.c, implementation of SDIO wrapper for the driver Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 05/21] cw1200: hwio.*, device reg/mem map and low-level i/o primitives Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 06/21] cw1200: fwio.*, firmware downloading code for the cw1200 driver Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 07/21] cw1200: queue.*, implementation of TX queues of " Dmitry Tarnyagin
2012-03-02 8:33 ` Johannes Berg
2012-03-02 15:32 ` Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 08/21] cw1200: wsm.*, implementation of device high-level interface Dmitry Tarnyagin
2012-03-02 8:34 ` Johannes Berg
2012-03-02 8:41 ` Joe Perches
2012-03-02 1:41 ` [PATCH 09/21] cw1200: txrx.*, implementation of datapath Dmitry Tarnyagin
2012-05-07 12:53 ` Bob Copeland [this message]
2012-05-08 7:09 ` Dmitry Tarnyagin
2012-05-08 12:54 ` Bob Copeland
2012-03-02 1:41 ` [PATCH 10/21] cw1200: ht.h, small helper header with HT definitions Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 11/21] cw1200: bh.*, device serving thread Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 12/21] cw1200: sta.*, mac80211 STA callbacks Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 13/21] cw1200: ap.*, mac80211 AP callbacks Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 14/21] cw1200: scan.*, mac80211 hw_scan callback Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 15/21] cw1200: debug.*, implementation of the driver's debugfs Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 16/21] cw1200: itp.*, internal device test and calibration code Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 17/21] cw1200: pm.*, power management code Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 18/21] cw1200: main.c, core initialization code Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 19/21] cw1200: TODO list Dmitry Tarnyagin
2012-03-02 8:51 ` Johannes Berg
2012-03-02 1:41 ` [PATCH 20/21] cw1200: Credits Dmitry Tarnyagin
2012-03-02 1:41 ` [PATCH 21/21] cw1200: Kconfig + Makefile for the driver Dmitry Tarnyagin
2012-03-02 8:50 ` Johannes Berg
2012-03-02 15:45 ` Dmitry Tarnyagin
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=20120507125316.GA25400@localhost \
--to=me@bobcopeland.com \
--cc=dmitry.tarnyagin@stericsson.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).