Return-path: Received: from nm20-vm0.bullet.mail.ird.yahoo.com ([77.238.189.221]:38058 "HELO nm20-vm0.bullet.mail.ird.yahoo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752056Ab1B1CWB convert rfc822-to-8bit (ORCPT ); Sun, 27 Feb 2011 21:22:01 -0500 Message-ID: <99164.32713.qm@web29504.mail.ird.yahoo.com> Date: Mon, 28 Feb 2011 02:21:58 +0000 (GMT) From: Hin-Tak Leung Reply-To: htl10@users.sourceforge.net Subject: Re: [PATCH] rtl8187: Change rate-control feedback To: John W Linville , Larry Finger Cc: Herton Ronaldo Krzesinski , linux-wireless@vger.kernel.org In-Reply-To: <4d6ab798.QpgTB5Dvvt1eieGL%Larry.Finger@lwfinger.net> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Sender: linux-wireless-owner@vger.kernel.org List-ID: --- On Sun, 27/2/11, Larry Finger wrote: > The driver for the RTL8187L chips > returns IEEE80211_TX_STAT_ACK for all > packets, even if the maximum number of retries was > exhausted. In addition > it fails to setup max_rates in the ieee80211_hw struct, > This behavior > may be responsible for the problems noted in Bug 14168. As > the bug is very > old, testers have not been found, and I do not have the > case where the > indicated signal is less than -70 dBm. Possibility of division by zero? The while loop dequeues - does this code always get invoked with a non-empty queue? Also it worrys me a little if tmp < retry? > + avg_retry = (tmp - retry) / > skb_queue_len(&priv->b_tx_status.queue); > while > (skb_queue_len(&priv->b_tx_status.queue) > 0) { > > Signed-off-by: Larry Finger > Cc: Stable > --- > > Index: > wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c > =================================================================== > --- > wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187/dev.c > +++ > wireless-testing/drivers/net/wireless/rtl818x/rtl8187/dev.c > @@ -869,23 +869,29 @@ static void rtl8187_work(struct > work_str > ??? /* The RTL8187 returns the retry count > through register 0xFFFA. In > ?????* addition, it appears to be > a cumulative retry count, not the > ?????* value for the current TX > packet. When multiple TX entries are > -?????* queued, the retry count > will be valid for the last one in the queue. > -?????* The "error" should not > matter for purposes of rate setting. */ > +?????* waiting in the queue, the > retry count will be the total for all. > +?????* The "error" may matter for > purposes of rate setting, but there is > +?????* no other choice with this > hardware. > +?????*/ > ??? struct rtl8187_priv *priv = > container_of(work, struct rtl8187_priv, > ??? ??? ??? > ??? ? ? work.work); > ??? struct ieee80211_tx_info *info; > ??? struct ieee80211_hw *dev = > priv->dev; > ??? static u16 retry; > ??? u16 tmp; > +??? u16 avg_retry; > > ??? mutex_lock(&priv->conf_mutex); > ??? tmp = rtl818x_ioread16(priv, (__le16 > *)0xFFFA); > +??? avg_retry = (tmp - retry) / > skb_queue_len(&priv->b_tx_status.queue); > ??? while > (skb_queue_len(&priv->b_tx_status.queue) > 0) { > ??? ??? struct sk_buff > *old_skb; > > ??? ??? old_skb = > skb_dequeue(&priv->b_tx_status.queue); > ??? ??? info = > IEEE80211_SKB_CB(old_skb); > -??? ??? > info->status.rates[0].count = tmp - retry + 1; > +??? ??? > info->status.rates[0].count = avg_retry + 1; > +??? ??? if > (info->status.rates[0].count > RETRY_COUNT) > +??? ??? ??? > info->flags &= ~IEEE80211_TX_STAT_ACK; > ??? ??? > ieee80211_tx_status_irqsafe(dev, old_skb); > ??? } > ??? retry = tmp; > @@ -931,8 +937,8 @@ static int rtl8187_start(struct > ieee8021 > ??? ??? > rtl818x_iowrite32(priv, &priv->map->TX_CONF, > ??? ??? ??? > ??? ? RTL818X_TX_CONF_HW_SEQNUM | > ??? ??? ??? > ??? ? RTL818X_TX_CONF_DISREQQSIZE | > -??? ??? ??? > ??? ? (7 << 8? /* short retry > limit */) | > -??? ??? ??? > ??? ? (7 << 0? /* long retry > limit */) | > +??? ??? ??? > ??? ? (RETRY_COUNT << 8? /* > short retry limit */) | > +??? ??? ??? > ??? ? (RETRY_COUNT << 0? /* > long retry limit */) | > ??? ??? ??? > ??? ? (7 << 21 /* MAX TX DMA > */)); > ??? ??? > rtl8187_init_urbs(dev); > ??? ??? > rtl8187b_init_status_urb(dev); > @@ -1376,6 +1382,9 @@ static int __devinit > rtl8187_probe(struc > ??? dev->flags = > IEEE80211_HW_HOST_BROADCAST_PS_BUFFERING | > ??? ??? ? > ???IEEE80211_HW_SIGNAL_DBM | > ??? ??? ? > ???IEEE80211_HW_RX_INCLUDES_FCS; > +??? /* Initialize rate-control variables > */ > +??? dev->max_rates = 1; > +??? dev->max_rate_tries = RETRY_COUNT; > > ??? eeprom.data = dev; > ??? eeprom.register_read = > rtl8187_eeprom_register_read; > Index: > wireless-testing/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h > =================================================================== > --- > wireless-testing.orig/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h > +++ > wireless-testing/drivers/net/wireless/rtl818x/rtl8187/rtl8187.h > @@ -35,6 +35,8 @@ > #define RFKILL_MASK_8187_89_97??? 0x2 > #define RFKILL_MASK_8198??? 0x4 > > +#define RETRY_COUNT??? ??? > 7 > + > struct rtl8187_rx_info { > ??? struct urb *urb; > ??? struct ieee80211_hw *dev; >