[RFC/RFT PATCH 0/2 v2] rtl8187: implement conf_tx callback/correctly ack tx pkts

[RFC/RFT PATCH 0/2 v2] rtl8187: implement conf_tx callback/correctly ack tx pkts

[Herton Ronaldo Krzesinski]

Two patches for two current issues in rtl8187:
- First it should implement conf_tx callback, instead of hardcoding some tx
  queue values.
- The second one should make rate control work for 8187B sending correct
  feedback about transmitted packet status, using one of the Bulk In endpoints
  that provides this info.

Please test and check if there are issues.

[RFC/RFT PATCH v2 1/2] rtl8187: implement conf_tx callback to configure tx queues

[Herton Ronaldo Krzesinski]

Add conf_tx callback and use it to configure tx queues of 8187L/8187B.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 v2: just rediff against latest wireless-testing from latest posted version.

 drivers/net/wireless/rtl8187.h         |    2 +
 drivers/net/wireless/rtl8187_dev.c     |   75 +++++++++++++++++++++++++++++---
 drivers/net/wireless/rtl8187_rtl8225.c |    6 ---
 3 files changed, 70 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/rtl8187.h b/drivers/net/wireless/rtl8187.h
index 33725d0..f09872e 100644
--- a/drivers/net/wireless/rtl8187.h
+++ b/drivers/net/wireless/rtl8187.h
@@ -111,6 +111,8 @@ struct rtl8187_priv {
 	u8 signal;
 	u8 quality;
 	u8 noise;
+	u8 slot_time;
+	u8 aifsn[4];
 };
 
 void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
diff --git a/drivers/net/wireless/rtl8187_dev.c b/drivers/net/wireless/rtl8187_dev.c
index d49f2a7..a84bff9 100644
--- a/drivers/net/wireless/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl8187_dev.c
@@ -701,6 +701,13 @@ static int rtl8187b_init_hw(struct ieee80211_hw *dev)
 
 	rtl818x_iowrite16_idx(priv, (__le16 *)0xFFEC, 0x0800, 1);
 
+	priv->slot_time = 0x9;
+	priv->aifsn[0] = 2; /* AIFSN[AC_VO] */
+	priv->aifsn[1] = 2; /* AIFSN[AC_VI] */
+	priv->aifsn[2] = 7; /* AIFSN[AC_BK] */
+	priv->aifsn[3] = 3; /* AIFSN[AC_BE] */
+	rtl818x_iowrite8(priv, &priv->map->ACM_CONTROL, 0);
+
 	return 0;
 }
 
@@ -908,24 +915,38 @@ static int rtl8187_config_interface(struct ieee80211_hw *dev,
 	return 0;
 }
 
+/*
+ * With 8187B, AC_*_PARAM clashes with FEMR definition in struct rtl818x_csr for
+ * example. Thus we have to use raw values for AC_*_PARAM register addresses.
+ */
+static __le32 *rtl8187b_ac_addr[4] = {
+	(__le32 *) 0xFFF0, /* AC_VO */
+	(__le32 *) 0xFFF4, /* AC_VI */
+	(__le32 *) 0xFFFC, /* AC_BK */
+	(__le32 *) 0xFFF8, /* AC_BE */
+};
+
+#define SIFS_TIME 0xa
+
 static void rtl8187_conf_erp(struct rtl8187_priv *priv, bool use_short_slot,
 			     bool use_short_preamble)
 {
 	if (priv->is_rtl8187b) {
-		u8 difs, eifs, slot_time;
+		u8 difs, eifs;
 		u16 ack_timeout;
+		int queue;
 
 		if (use_short_slot) {
-			slot_time = 0x9;
+			priv->slot_time = 0x9;
 			difs = 0x1c;
 			eifs = 0x53;
 		} else {
-			slot_time = 0x14;
+			priv->slot_time = 0x14;
 			difs = 0x32;
 			eifs = 0x5b;
 		}
-		rtl818x_iowrite8(priv, &priv->map->SIFS, 0xa);
-		rtl818x_iowrite8(priv, &priv->map->SLOT, slot_time);
+		rtl818x_iowrite8(priv, &priv->map->SIFS, SIFS_TIME);
+		rtl818x_iowrite8(priv, &priv->map->SLOT, priv->slot_time);
 		rtl818x_iowrite8(priv, &priv->map->DIFS, difs);
 
 		/*
@@ -946,18 +967,21 @@ static void rtl8187_conf_erp(struct rtl8187_priv *priv, bool use_short_slot,
 			ack_timeout += 144;
 		rtl818x_iowrite8(priv, &priv->map->CARRIER_SENSE_COUNTER,
 				 DIV_ROUND_UP(ack_timeout, 4));
+
+		for (queue = 0; queue < 4; queue++)
+			rtl818x_iowrite8(priv, (u8 *) rtl8187b_ac_addr[queue],
+					 priv->aifsn[queue] * priv->slot_time +
+					 SIFS_TIME);
 	} else {
 		rtl818x_iowrite8(priv, &priv->map->SIFS, 0x22);
 		if (use_short_slot) {
 			rtl818x_iowrite8(priv, &priv->map->SLOT, 0x9);
 			rtl818x_iowrite8(priv, &priv->map->DIFS, 0x14);
 			rtl818x_iowrite8(priv, &priv->map->EIFS, 91 - 0x14);
-			rtl818x_iowrite8(priv, &priv->map->CW_VAL, 0x73);
 		} else {
 			rtl818x_iowrite8(priv, &priv->map->SLOT, 0x14);
 			rtl818x_iowrite8(priv, &priv->map->DIFS, 0x24);
 			rtl818x_iowrite8(priv, &priv->map->EIFS, 91 - 0x24);
-			rtl818x_iowrite8(priv, &priv->map->CW_VAL, 0xa5);
 		}
 	}
 }
@@ -1006,6 +1030,42 @@ static void rtl8187_configure_filter(struct ieee80211_hw *dev,
 	rtl818x_iowrite32_async(priv, &priv->map->RX_CONF, priv->rx_conf);
 }
 
+static int rtl8187_conf_tx(struct ieee80211_hw *dev, u16 queue,
+			   const struct ieee80211_tx_queue_params *params)
+{
+	struct rtl8187_priv *priv = dev->priv;
+	u8 cw_min, cw_max;
+
+	if (queue > 3)
+		return -EINVAL;
+
+	cw_min = fls(params->cw_min);
+	cw_max = fls(params->cw_max);
+
+	if (priv->is_rtl8187b) {
+		priv->aifsn[queue] = params->aifs;
+
+		/*
+		 * This is the structure of AC_*_PARAM registers in 8187B:
+		 * - TXOP limit field, bit offset = 16
+		 * - ECWmax, bit offset = 12
+		 * - ECWmin, bit offset = 8
+		 * - AIFS, bit offset = 0
+		 */
+		rtl818x_iowrite32(priv, rtl8187b_ac_addr[queue],
+				  (params->txop << 16) | (cw_max << 12) |
+				  (cw_min << 8) | (params->aifs *
+				  priv->slot_time + SIFS_TIME));
+	} else {
+		if (queue != 0)
+			return -EINVAL;
+
+		rtl818x_iowrite8(priv, &priv->map->CW_VAL,
+				 cw_min | (cw_max << 4));
+	}
+	return 0;
+}
+
 static const struct ieee80211_ops rtl8187_ops = {
 	.tx			= rtl8187_tx,
 	.start			= rtl8187_start,
@@ -1016,6 +1076,7 @@ static const struct ieee80211_ops rtl8187_ops = {
 	.config_interface	= rtl8187_config_interface,
 	.bss_info_changed	= rtl8187_bss_info_changed,
 	.configure_filter	= rtl8187_configure_filter,
+	.conf_tx		= rtl8187_conf_tx
 };
 
 static void rtl8187_eeprom_register_read(struct eeprom_93cx6 *eeprom)
diff --git a/drivers/net/wireless/rtl8187_rtl8225.c b/drivers/net/wireless/rtl8187_rtl8225.c
index 69030be..4e75e8e 100644
--- a/drivers/net/wireless/rtl8187_rtl8225.c
+++ b/drivers/net/wireless/rtl8187_rtl8225.c
@@ -878,12 +878,6 @@ static void rtl8225z2_b_rf_init(struct ieee80211_hw *dev)
 	for (i = 0; i < ARRAY_SIZE(rtl8225z2_ofdm); i++)
 		rtl8225_write_phy_ofdm(dev, i, rtl8225z2_ofdm[i]);
 
-	rtl818x_iowrite32(priv, (__le32 *)0xFFF0, (7 << 12) | (3 << 8) | 28);
-	rtl818x_iowrite32(priv, (__le32 *)0xFFF4, (7 << 12) | (3 << 8) | 28);
-	rtl818x_iowrite32(priv, (__le32 *)0xFFF8, (7 << 12) | (3 << 8) | 28);
-	rtl818x_iowrite32(priv, (__le32 *)0xFFFC, (7 << 12) | (3 << 8) | 28);
-	rtl818x_iowrite8(priv, &priv->map->ACM_CONTROL, 0);
-
 	rtl8225_write_phy_ofdm(dev, 0x97, 0x46);
 	rtl8225_write_phy_ofdm(dev, 0xa4, 0xb6);
 	rtl8225_write_phy_ofdm(dev, 0x85, 0xfc);
-- 
1.6.0.2

[RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

Realtek 8187B has a receive command queue to feedback beacon interrupt
and transmitted packet status. Use it to feedback mac80211 about status
of transmitted packets. Unfortunately in the course of testing I found
that the sequence number reported by hardware includes entire sequence
control in a 12 bit only field, so a workaround is done to check only
lowest bits.

Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
---
 v2: fix rate stuck at 1M reported by Larry Finger, Hin-Tak and also seen by me.
     btw, I noted now with a ralink device that rt2500usb (rt2x00) has same
     issue currently.

 Hin-Tak: didn't add your tested-by yet as this patch has v2 fix comparing to
 others, please test it again so I can add your tested-by

 drivers/net/wireless/rtl8187.h     |    5 ++
 drivers/net/wireless/rtl8187_dev.c |  131 ++++++++++++++++++++++++++++++++++-
 2 files changed, 132 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/rtl8187.h b/drivers/net/wireless/rtl8187.h
index f09872e..c385407 100644
--- a/drivers/net/wireless/rtl8187.h
+++ b/drivers/net/wireless/rtl8187.h
@@ -113,6 +113,11 @@ struct rtl8187_priv {
 	u8 noise;
 	u8 slot_time;
 	u8 aifsn[4];
+	struct {
+		__le64 buf;
+		struct urb *urb;
+		struct sk_buff_head queue;
+	} b_tx_status;
 };
 
 void rtl8187_write_phy(struct ieee80211_hw *dev, u8 addr, u32 data);
diff --git a/drivers/net/wireless/rtl8187_dev.c b/drivers/net/wireless/rtl8187_dev.c
index a84bff9..7758779 100644
--- a/drivers/net/wireless/rtl8187_dev.c
+++ b/drivers/net/wireless/rtl8187_dev.c
@@ -165,8 +165,27 @@ static void rtl8187_tx_cb(struct urb *urb)
 	skb_pull(skb, priv->is_rtl8187b ? sizeof(struct rtl8187b_tx_hdr) :
 					  sizeof(struct rtl8187_tx_hdr));
 	ieee80211_tx_info_clear_status(info);
-	info->flags |= IEEE80211_TX_STAT_ACK;
-	ieee80211_tx_status_irqsafe(hw, skb);
+
+	if (!urb->status &&
+	    !(info->flags & IEEE80211_TX_CTL_NO_ACK) &&
+	    priv->is_rtl8187b) {
+		skb_queue_tail(&priv->b_tx_status.queue, skb);
+
+		/* queue is "full", discard last items */
+		while (skb_queue_len(&priv->b_tx_status.queue) > 5) {
+			struct sk_buff *old_skb;
+
+			dev_dbg(&priv->udev->dev,
+				"transmit status queue full\n");
+
+			old_skb = skb_dequeue(&priv->b_tx_status.queue);
+			ieee80211_tx_status_irqsafe(hw, old_skb);
+		}
+	} else {
+		if (!(info->flags & IEEE80211_TX_CTL_NO_ACK) && !urb->status)
+			info->flags |= IEEE80211_TX_STAT_ACK;
+		ieee80211_tx_status_irqsafe(hw, skb);
+	}
 }
 
 static int rtl8187_tx(struct ieee80211_hw *dev, struct sk_buff *skb)
@@ -208,7 +227,7 @@ static int rtl8187_tx(struct ieee80211_hw *dev, struct sk_buff *skb)
 		hdr->flags = cpu_to_le32(flags);
 		hdr->len = 0;
 		hdr->rts_duration = rts_dur;
-		hdr->retry = cpu_to_le32((info->control.rates[0].count - 1) << 8);
+		hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
 		buf = hdr;
 
 		ep = 2;
@@ -226,7 +245,7 @@ static int rtl8187_tx(struct ieee80211_hw *dev, struct sk_buff *skb)
 		memset(hdr, 0, sizeof(*hdr));
 		hdr->flags = cpu_to_le32(flags);
 		hdr->rts_duration = rts_dur;
-		hdr->retry = cpu_to_le32((info->control.rates[0].count - 1) << 8);
+		hdr->retry = cpu_to_le32(info->control.rates[0].count << 8);
 		hdr->tx_duration =
 			ieee80211_generic_frame_duration(dev, priv->vif,
 							 skb->len, txrate);
@@ -392,6 +411,105 @@ static int rtl8187_init_urbs(struct ieee80211_hw *dev)
 	return 0;
 }
 
+static void rtl8187b_status_cb(struct urb *urb)
+{
+	struct ieee80211_hw *hw = (struct ieee80211_hw *)urb->context;
+	struct rtl8187_priv *priv = hw->priv;
+	u64 val;
+	unsigned int cmd_type;
+
+	if (unlikely(urb->status)) {
+		usb_free_urb(urb);
+		return;
+	}
+
+	/*
+	 * Read from status buffer:
+	 *
+	 * bits [30:31] = cmd type:
+	 * - 0 indicates tx beacon interrupt
+	 * - 1 indicates tx close descriptor
+	 *
+	 * In the case of tx beacon interrupt:
+	 * [0:9] = Last Beacon CW
+	 * [10:29] = reserved
+	 * [30:31] = 00b
+	 * [32:63] = Last Beacon TSF
+	 * 
+	 * If it's tx close descriptor:
+	 * [0:7] = Packet Retry Count
+	 * [8:14] = RTS Retry Count
+	 * [15] = TOK
+	 * [16:27] = Sequence No
+	 * [28] = LS
+	 * [29] = FS
+	 * [30:31] = 01b
+	 * [32:47] = unused (reserved?)
+	 * [48:63] = MAC Used Time
+	 */
+	val = le64_to_cpu(priv->b_tx_status.buf);
+
+	cmd_type = (val >> 30) & 0x3;
+	if (cmd_type == 1) {
+		unsigned int pkt_rc, seq_no;
+		bool tok;
+		struct sk_buff *skb;
+		struct ieee80211_hdr *ieee80211hdr;
+		unsigned long flags;
+
+		pkt_rc = val & 0xFF;
+		tok = val & (1 << 15);
+		seq_no = (val >> 16) & 0xFFF;
+
+		spin_lock_irqsave(&priv->b_tx_status.queue.lock, flags);
+		skb_queue_reverse_walk(&priv->b_tx_status.queue, skb) {
+			ieee80211hdr = (struct ieee80211_hdr *)skb->data;
+
+			/* 
+			 * Instead of returning just the 12 bits of sequence
+			 * number, hardware is returning entire sequence
+			 * control, overflowing after some time. As a
+			 * workaround, just consider the lower bits, and expect
+			 * it's unlikely we wrongly ack some sent data
+			 */
+			if ((le16_to_cpu(ieee80211hdr->seq_ctrl) & 0xFFF) == seq_no)
+				break;
+		}
+		if (skb != (struct sk_buff *) &priv->b_tx_status.queue) {
+			struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
+
+			__skb_unlink(skb, &priv->b_tx_status.queue);
+			if (tok)
+				info->flags |= IEEE80211_TX_STAT_ACK;
+			info->status.rates[0].count = pkt_rc;
+
+			ieee80211_tx_status_irqsafe(hw, skb);
+		}
+		spin_unlock_irqrestore(&priv->b_tx_status.queue.lock, flags);
+	}
+
+	usb_submit_urb(urb, GFP_ATOMIC);
+}
+
+static int rtl8187b_init_status_urb(struct ieee80211_hw *dev)
+{
+	struct rtl8187_priv *priv = dev->priv;
+	struct urb *entry;
+
+	entry = usb_alloc_urb(0, GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+	priv->b_tx_status.urb = entry;
+
+	usb_fill_bulk_urb(entry, priv->udev, usb_rcvbulkpipe(priv->udev, 9),
+			  &priv->b_tx_status.buf, sizeof(priv->b_tx_status.buf),
+			  rtl8187b_status_cb, dev);
+
+	usb_submit_urb(entry, GFP_KERNEL);
+
+	return 0;
+}
+
 static int rtl8187_cmd_reset(struct ieee80211_hw *dev)
 {
 	struct rtl8187_priv *priv = dev->priv;
@@ -744,6 +862,7 @@ static int rtl8187_start(struct ieee80211_hw *dev)
 				  (7 << 0  /* long retry limit */) |
 				  (7 << 21 /* MAX TX DMA */));
 		rtl8187_init_urbs(dev);
+		rtl8187b_init_status_urb(dev);
 		mutex_unlock(&priv->conf_mutex);
 		return 0;
 	}
@@ -820,6 +939,9 @@ static void rtl8187_stop(struct ieee80211_hw *dev)
 		usb_kill_urb(info->urb);
 		kfree_skb(skb);
 	}
+	while ((skb = skb_dequeue(&priv->b_tx_status.queue)))
+		dev_kfree_skb_any(skb);
+	usb_kill_urb(priv->b_tx_status.urb);
 	mutex_unlock(&priv->conf_mutex);
 }
 
@@ -1307,6 +1429,7 @@ static int __devinit rtl8187_probe(struct usb_interface *intf,
 		goto err_free_dev;
 	}
 	mutex_init(&priv->conf_mutex);
+	skb_queue_head_init(&priv->b_tx_status.queue);
 
 	printk(KERN_INFO "%s: hwaddr %s, %s V%d + %s\n",
 	       wiphy_name(dev->wiphy), print_mac(mac, dev->wiphy->perm_addr),
-- 
1.6.0.2

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Tue, 4/11/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

> Realtek 8187B has a receive command queue to feedback beacon
> interrupt
> and transmitted packet status. Use it to feedback mac80211
> about status
> of transmitted packets. Unfortunately in the course of
> testing I found
> that the sequence number reported by hardware includes
> entire sequence
> control in a 12 bit only field, so a workaround is done to
> check only
> lowest bits.

> ---
>  v2: fix rate stuck at 1M reported by Larry Finger, Hin-Tak
> and also seen by me.
>      btw, I noted now with a ralink device that rt2500usb
> (rt2x00) has same
>      issue currently.


> -		hdr->retry =
> cpu_to_le32((info->control.rates[0].count - 1) <<
> 8);
> +		hdr->retry =
> cpu_to_le32(info->control.rates[0].count << 8);

> -		hdr->retry =
> cpu_to_le32((info->control.rates[0].count - 1) <<
> 8);
> +		hdr->retry =
> cpu_to_le32(info->control.rates[0].count << 8);

These are the only lines which changed between the two versions of the patch - if removing the two -1's fixes the rate stuck (and these two lines are newly introduced), shouldn't this be a separate commit if it fixes a problem introduced by an earlier commit?

I did *not* (and do not) have a rate-stuck problem, but I am running a slightly mod version of the patch against 2.6.27, which doesn't have "rates[x].count" yet, with the retry_count struct member which seems to have since been removed.


      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

On Tuesday 04 November 2008 18:27:12 Hin-Tak Leung wrote:
> --- On Tue, 4/11/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> 
wrote:
> > Realtek 8187B has a receive command queue to feedback beacon
> > interrupt
> > and transmitted packet status. Use it to feedback mac80211
> > about status
> > of transmitted packets. Unfortunately in the course of
> > testing I found
> > that the sequence number reported by hardware includes
> > entire sequence
> > control in a 12 bit only field, so a workaround is done to
> > check only
> > lowest bits.
> >
> > ---
> >  v2: fix rate stuck at 1M reported by Larry Finger, Hin-Tak
> > and also seen by me.
> >      btw, I noted now with a ralink device that rt2500usb
> > (rt2x00) has same
> >      issue currently.
> >
> >
> > -		hdr->retry =
> > cpu_to_le32((info->control.rates[0].count - 1) <<
> > 8);
> > +		hdr->retry =
> > cpu_to_le32(info->control.rates[0].count << 8);
> >
> > -		hdr->retry =
> > cpu_to_le32((info->control.rates[0].count - 1) <<
> > 8);
> > +		hdr->retry =
> > cpu_to_le32(info->control.rates[0].count << 8);
>
> These are the only lines which changed between the two versions of the
> patch - if removing the two -1's fixes the rate stuck (and these two lines
> are newly introduced), shouldn't this be a separate commit if it fixes a
> problem introduced by an earlier commit?

No, there was other change as well:
info->status.rates[0].count = pkt_rc - 1;
to
info->status.rates[0].count = pkt_rc;

that's in fact what fixes the rate control problem with pid (from what I 
understand from the code, if rates[0].count is not zero (that will be always 
the case here) pid would register the tx with errors, and would explain the 
rate stuck at 1M).

The other changes is because now with new rate control api, it changed 
semantics but is passing a different retry_count than before now, while 
testing I found for example, connecting to an ap using wep, it is passing in 
tx packet in same conditions retry_count=6 now instead of 7 as before rate 
control api, so I considered a bug, unless this is desired (also depending on 
code path the retry count in tx packet could be set to zero now, is this 
right?); if new behaviour of retry_count in this case is right (and my change 
is wrong) please someone explain to me what is the semantic now behind the 
new rate control api (looking at the rate control api change why there is 
some differences between drivers?, as some drivers simply just replaced 
retry_count with rates[0].count usage while others changed 
to "rates[0].count - 1".)

>
> I did *not* (and do not) have a rate-stuck problem, but I am running a
> slightly mod version of the patch against 2.6.27, which doesn't have
> "rates[x].count" yet, with the retry_count struct member which seems to
> have since been removed.

Please try and check the patches with latest wireless-testing.

--
[]'s
Herton

Re: [RFC/RFT PATCH v2 1/2] rtl8187: implement conf_tx callback to configure tx queues

[Larry Finger]

Herton Ronaldo Krzesinski wrote:
> Add conf_tx callback and use it to configure tx queues of 8187L/8187B.
> 
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>

Tested-by: Larry Finger <Larry.Finger@lwfinger.net>

This patch is OK. With 1 and 2, the rate-control mechanism works the way it did
before.

Larry

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Larry Finger]

Herton Ronaldo Krzesinski wrote:
> Realtek 8187B has a receive command queue to feedback beacon interrupt
> and transmitted packet status. Use it to feedback mac80211 about status
> of transmitted packets. Unfortunately in the course of testing I found
> that the sequence number reported by hardware includes entire sequence
> control in a 12 bit only field, so a workaround is done to check only
> lowest bits.
> 
> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>

Tested-by: Larry Finger <Larry.Finger@lwfinger.net>


Quilt shows the following warning:

Warning: trailing whitespace in lines 438,468 of drivers/net/wireless/rtl8187_dev.c

scripts/checkpatch shows the following:

WARNING: line over 80 characters
#138: FILE: drivers/net/wireless/rtl8187_dev.c:475:
+                       if ((le16_to_cpu(ieee80211hdr->seq_ctrl) & 0xFFF) == seq_no)

total: 0 errors, 1 warnings, 173 lines checked

Larry

Re: [RFC/RFT PATCH v2 1/2] rtl8187: implement conf_tx callback to configure tx queues

[Felix Fietkau]

Larry Finger wrote:
> Herton Ronaldo Krzesinski wrote:
>> Add conf_tx callback and use it to configure tx queues of 8187L/8187B.
>> 
>> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> 
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
> This patch is OK. With 1 and 2, the rate-control mechanism works the way it did
> before.
Has anybody tested this thing with the minstrel rate control yet?
At some point I would like to make minstrel the default RC,
since it has shown much better performance on drivers like b43 or ath5k.

- Felix

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Tue, 4/11/08, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> > Signed-off-by: Herton Ronaldo Krzesinski
> <herton@mandriva.com.br>
> 
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>

I tried with this combo:

compat-wireless: master-2008-10-31
wireless-testing: v2.6.28-rc2-4239-gd80fe00

and applying the two patches on top, they applies cleanly and compile okay except for some warnings in a different module about missing symbols (<something>_sta) but the result dies very painfully with a kernel oops:
-------------
BUG: unable to handle kernel NULL pointer dereference at 0000000000000307
IP: [<ffffffffa0105988>] rtl8187_probe+0x7f/0x836 [rtl8187]
PGD 754c4067 PUD 751d5067 PMD 0 
Oops: 0002 [1] SMP 
CPU 0 
Modules linked in: rtl8187(+) snd_page_alloc video firewire_ohci firewire_core shpchp pcspkr eeprom_93cx6 output ac battery joydev r8169 crc_itu_t soundcore 
i2c_piix4 k8temp hwmon pata_atiixp radeon drm i2c_algo_bit i2c_core
Pid: 829, comm: modprobe Not tainted 2.6.27.4-73.fc10.x86_64 #1
RIP: 0010:[<ffffffffa0105988>]  [<ffffffffa0105988>] rtl8187_probe+0x7f/0x836 [rtl8187]
RSP: 0018:ffff880074de1c48  EFLAGS: 00010246
RAX: ffffffffa0108520 RBX: ffff880074de1cd0 RCX: 0000000000000001
RDX: 0000000000000000 RSI: ffff8800754763c8 RDI: 0000000000000040
RBP: ffff880074de1d08 R08: 00000000000003e8 R09: ffff8800776ed868
R10: ffff8800754761c4 R11: 000000000000000a R12: 0000000000000000
R13: ffff8800754762e0 R14: 0000000000000038 R15: ffff880074958000
FS:  00007f5c8bef66f0(0000) GS:ffffffff8158a100(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000307 CR3: 0000000074dc6000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process modprobe (pid: 829, threadinfo ffff880074de0000, task ffff880075420000)
Stack:  ffff880074de1cb8 ffffffff8110d22f ffff880074de1c78 ffff880074de1c78
 ffff88007402af50 ffff88007402af50 ffffffffa0108520 ffff88007487a000
 ffff880074958000 ffffffff81031023 ffff880074de1ca8 ffffffff81353562
Call Trace:
 [<ffffffff8110d22f>] ? sysfs_addrm_finish+0x69/0x20e
 [<ffffffff81031023>] ? need_resched+0x1e/0x28
 [<ffffffff81353562>] ? _cond_resched+0x9/0x38
 [<ffffffff81353aee>] ? mutex_lock+0x22/0x33
 [<ffffffff81240d08>] ? usb_match_one_id+0x2b/0x88
 [<ffffffff812412df>] ? usb_autopm_do_device+0xc8/0xd4
 [<ffffffff812418b5>] usb_probe_interface+0x10e/0x14b
 [<ffffffff811f64c4>] driver_probe_device+0xc0/0x16e
 [<ffffffff811f65c1>] __driver_attach+0x4f/0x79
 [<ffffffff811f6572>] ? __driver_attach+0x0/0x79
 [<ffffffff811f5d57>] bus_for_each_dev+0x4f/0x89
 [<ffffffff811f630f>] driver_attach+0x1c/0x1e
 [<ffffffff811f5677>] bus_add_driver+0xb7/0x201
 [<ffffffff811f67b0>] driver_register+0xa8/0x128
 [<ffffffff81241aff>] usb_register_driver+0x80/0xe4
 [<ffffffffa00bb000>] ? rtl8187_init+0x0/0x20 [rtl8187]
 [<ffffffffa00bb01e>] rtl8187_init+0x1e/0x20 [rtl8187]
 [<ffffffff8100a047>] do_one_initcall+0x47/0x12e
 [<ffffffff81065be1>] sys_init_module+0xa9/0x1b6
 [<ffffffff8101024a>] system_call_fastpath+0x16/0x1b


Code: 24 e1 e9 98 07 00 00 48 81 eb 88 00 00 00 48 89 5d 80 4c 8b 60 40 48 8d 5d c8 48 8b 85 70 ff ff ff 4d 8d 74 24 38 48 83 78 10 01 <41> 0f 94 84 24 07 03
 00 00 48 8b 85 78 ff ff ff 49 8b 55 28 48 
RIP  [<ffffffffa0105988>] rtl8187_probe+0x7f/0x836 [rtl8187]
 RSP <ffff880074de1c48>
CR2: 0000000000000307
---[ end trace aed852fa39b00f8c ]---
---------


Hin-Tak



      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

On Tue, 2008-11-04 at 14:31 -0800, Larry Finger wrote:
> Herton Ronaldo Krzesinski wrote:
> > Realtek 8187B has a receive command queue to feedback beacon interrupt
> > and transmitted packet status. Use it to feedback mac80211 about status
> > of transmitted packets. Unfortunately in the course of testing I found
> > that the sequence number reported by hardware includes entire sequence
> > control in a 12 bit only field, so a workaround is done to check only
> > lowest bits.
> > 
> > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> 
> Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> 
> 
> Quilt shows the following warning:
> 
> Warning: trailing whitespace in lines 438,468 of drivers/net/wireless/rtl8187_dev.c
> 
> scripts/checkpatch shows the following:
> 
> WARNING: line over 80 characters
> #138: FILE: drivers/net/wireless/rtl8187_dev.c:475:
> +                       if ((le16_to_cpu(ieee80211hdr->seq_ctrl) & 0xFFF) == seq_no)

Besides, that line looks wrong? the lowest 4 bits are teh fragment
number.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/RFT PATCH v2 1/2] rtl8187: implement conf_tx callback to configure tx queues

[Herton Ronaldo Krzesinski]

On Tuesday 04 November 2008 21:11:31 Felix Fietkau wrote:
> Larry Finger wrote:
> > Herton Ronaldo Krzesinski wrote:
> >> Add conf_tx callback and use it to configure tx queues of 8187L/8187B.
> >>
> >> Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >
> > Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> >
> > This patch is OK. With 1 and 2, the rate-control mechanism works the way
> > it did before.
>
> Has anybody tested this thing with the minstrel rate control yet?
> At some point I would like to make minstrel the default RC,
> since it has shown much better performance on drivers like b43 or ath5k.

Tested minstrel here, seems to be working fine, that is, it's changing rate 
above 1M after some seconds of traffic and sticking to a stable rate unlike 
pid where it seems to go in cicles here (from 1M to 9M and going back to 1M 
and so on, unlinke minstrel that sticks to 9M, same environment in both 
cases). Looks a better rate control in practice, didn't make any further 
verifications.

>
> - Felix

--
[]'s
Herton

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

On Wednesday 05 November 2008 09:29:12 Johannes Berg wrote:
> On Tue, 2008-11-04 at 14:31 -0800, Larry Finger wrote:
> > Herton Ronaldo Krzesinski wrote:
> > > Realtek 8187B has a receive command queue to feedback beacon interrupt
> > > and transmitted packet status. Use it to feedback mac80211 about status
> > > of transmitted packets. Unfortunately in the course of testing I found
> > > that the sequence number reported by hardware includes entire sequence
> > > control in a 12 bit only field, so a workaround is done to check only
> > > lowest bits.
> > >
> > > Signed-off-by: Herton Ronaldo Krzesinski <herton@mandriva.com.br>
> >
> > Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
> >
> >
> > Quilt shows the following warning:
> >
> > Warning: trailing whitespace in lines 438,468 of
> > drivers/net/wireless/rtl8187_dev.c
> >
> > scripts/checkpatch shows the following:
> >
> > WARNING: line over 80 characters
> > #138: FILE: drivers/net/wireless/rtl8187_dev.c:475:
> > +                       if ((le16_to_cpu(ieee80211hdr->seq_ctrl) & 0xFFF)
> > == seq_no)

Ops, I forgot to run checkpatch.pl here, will run and fix it when I submit 
next patch.

>
> Besides, that line looks wrong? the lowest 4 bits are teh fragment
> number.

It's intended, because the hardware instead of reporting just the sequence 
number in its 12 bits is reporting fragment number + sequence number, this is 
a workaround.

>
> johannes

-- 
[]'s
Herton

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Wed, 2008-11-05 at 13:38 -0200, Herton Ronaldo Krzesinski wrote:

> > > WARNING: line over 80 characters
> > > #138: FILE: drivers/net/wireless/rtl8187_dev.c:475:
> > > +                       if ((le16_to_cpu(ieee80211hdr->seq_ctrl) & 0xFFF)
> > > == seq_no)
> 
> Ops, I forgot to run checkpatch.pl here, will run and fix it when I submit 
> next patch.

I wouldn't worry about lines > 80 chars too much, breaking this down
wouldn't make more readable but less so, in my opinion. I tend to ignore
that rule where it makes the code unreadable :)

> > Besides, that line looks wrong? the lowest 4 bits are teh fragment
> > number.
> 
> It's intended, because the hardware instead of reporting just the sequence 
> number in its 12 bits is reporting fragment number + sequence number, this is 
> a workaround.

Ah. A comment might be appropriate that "seq_no" doesn't actually
contain the sequence number?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

On Wednesday 05 November 2008 00:02:07 Hin-Tak Leung wrote:
> --- On Tue, 4/11/08, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> > > Signed-off-by: Herton Ronaldo Krzesinski
> >
> > <herton@mandriva.com.br>
> >
> > Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
>
> I tried with this combo:
>
> compat-wireless: master-2008-10-31
> wireless-testing: v2.6.28-rc2-4239-gd80fe00
>
> and applying the two patches on top, they applies cleanly and compile okay
> except for some warnings in a different module about missing symbols
> (<something>_sta) but the result dies very painfully with a kernel oops:

Looks like something bad in combination of compat-wireless plus kernel 
2.6.27.4-73.fc10.x86_64, please try a plain wireless-testing tree with the 
two patches.

> -------------
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000307
> IP: [<ffffffffa0105988>] rtl8187_probe+0x7f/0x836 [rtl8187]
> PGD 754c4067 PUD 751d5067 PMD 0
> Oops: 0002 [1] SMP
> CPU 0
> Modules linked in: rtl8187(+) snd_page_alloc video firewire_ohci
> firewire_core shpchp pcspkr eeprom_93cx6 output ac battery joydev r8169
> crc_itu_t soundcore i2c_piix4 k8temp hwmon pata_atiixp radeon drm
> i2c_algo_bit i2c_core Pid: 829, comm: modprobe Not tainted
> 2.6.27.4-73.fc10.x86_64 #1
> RIP: 0010:[<ffffffffa0105988>]  [<ffffffffa0105988>]
> rtl8187_probe+0x7f/0x836 [rtl8187] RSP: 0018:ffff880074de1c48  EFLAGS:
> 00010246
> RAX: ffffffffa0108520 RBX: ffff880074de1cd0 RCX: 0000000000000001
> RDX: 0000000000000000 RSI: ffff8800754763c8 RDI: 0000000000000040
> RBP: ffff880074de1d08 R08: 00000000000003e8 R09: ffff8800776ed868
> R10: ffff8800754761c4 R11: 000000000000000a R12: 0000000000000000
> R13: ffff8800754762e0 R14: 0000000000000038 R15: ffff880074958000
> FS:  00007f5c8bef66f0(0000) GS:ffffffff8158a100(0000)
> knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 0000000000000307 CR3: 0000000074dc6000 CR4: 00000000000006e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process modprobe (pid: 829, threadinfo ffff880074de0000, task
> ffff880075420000) Stack:  ffff880074de1cb8 ffffffff8110d22f
> ffff880074de1c78 ffff880074de1c78 ffff88007402af50 ffff88007402af50
> ffffffffa0108520 ffff88007487a000 ffff880074958000 ffffffff81031023
> ffff880074de1ca8 ffffffff81353562 Call Trace:
>  [<ffffffff8110d22f>] ? sysfs_addrm_finish+0x69/0x20e
>  [<ffffffff81031023>] ? need_resched+0x1e/0x28
>  [<ffffffff81353562>] ? _cond_resched+0x9/0x38
>  [<ffffffff81353aee>] ? mutex_lock+0x22/0x33
>  [<ffffffff81240d08>] ? usb_match_one_id+0x2b/0x88
>  [<ffffffff812412df>] ? usb_autopm_do_device+0xc8/0xd4
>  [<ffffffff812418b5>] usb_probe_interface+0x10e/0x14b
>  [<ffffffff811f64c4>] driver_probe_device+0xc0/0x16e
>  [<ffffffff811f65c1>] __driver_attach+0x4f/0x79
>  [<ffffffff811f6572>] ? __driver_attach+0x0/0x79
>  [<ffffffff811f5d57>] bus_for_each_dev+0x4f/0x89
>  [<ffffffff811f630f>] driver_attach+0x1c/0x1e
>  [<ffffffff811f5677>] bus_add_driver+0xb7/0x201
>  [<ffffffff811f67b0>] driver_register+0xa8/0x128
>  [<ffffffff81241aff>] usb_register_driver+0x80/0xe4
>  [<ffffffffa00bb000>] ? rtl8187_init+0x0/0x20 [rtl8187]
>  [<ffffffffa00bb01e>] rtl8187_init+0x1e/0x20 [rtl8187]
>  [<ffffffff8100a047>] do_one_initcall+0x47/0x12e
>  [<ffffffff81065be1>] sys_init_module+0xa9/0x1b6
>  [<ffffffff8101024a>] system_call_fastpath+0x16/0x1b
>
>
> Code: 24 e1 e9 98 07 00 00 48 81 eb 88 00 00 00 48 89 5d 80 4c 8b 60 40 48
> 8d 5d c8 48 8b 85 70 ff ff ff 4d 8d 74 24 38 48 83 78 10 01 <41> 0f 94 84
> 24 07 03 00 00 48 8b 85 78 ff ff ff 49 8b 55 28 48
> RIP  [<ffffffffa0105988>] rtl8187_probe+0x7f/0x836 [rtl8187]
>  RSP <ffff880074de1c48>
> CR2: 0000000000000307
> ---[ end trace aed852fa39b00f8c ]---
> ---------
>
>
> Hin-Tak

--
[]'s
Herton

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

On Wednesday 05 November 2008 13:40:22 Johannes Berg wrote:
> On Wed, 2008-11-05 at 13:38 -0200, Herton Ronaldo Krzesinski wrote:
> > > > WARNING: line over 80 characters
> > > > #138: FILE: drivers/net/wireless/rtl8187_dev.c:475:
> > > > +                       if ((le16_to_cpu(ieee80211hdr->seq_ctrl) &
> > > > 0xFFF) == seq_no)
> >
> > Ops, I forgot to run checkpatch.pl here, will run and fix it when I
> > submit next patch.
>
> I wouldn't worry about lines > 80 chars too much, breaking this down
> wouldn't make more readable but less so, in my opinion. I tend to ignore
> that rule where it makes the code unreadable :)
>
> > > Besides, that line looks wrong? the lowest 4 bits are teh fragment
> > > number.
> >
> > It's intended, because the hardware instead of reporting just the
> > sequence number in its 12 bits is reporting fragment number + sequence
> > number, this is a workaround.
>
> Ah. A comment might be appropriate that "seq_no" doesn't actually
> contain the sequence number?

Yes, I'll improve comment there, will change by this:

/*
 * While testing, it was discovered that the seq_no doesn't
 * actually contains the sequence number. Instead
 * of returning just the 12 bits of sequence number,
 * hardware is returning entire sequence control (fragment
 * number plus sequence number) in a 12 bit only field
 * overflowing after some time. As a workaround,
 * just consider the lower bits, and expect it's unlikely
 * we wrongly ack some sent data
 */

>
> johannes

--
[]'s
Herton

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

Herton Ronaldo Krzesinski wrote:

>> and applying the two patches on top, they applies cleanly and compile okay
>> except for some warnings in a different module about missing symbols
>> (<something>_sta) but the result dies very painfully with a kernel oops:
> 
> Looks like something bad in combination of compat-wireless plus kernel 
> 2.6.27.4-73.fc10.x86_64, please try a plain wireless-testing tree with the 
> two patches.

Hmm, I'll have to check, but I was a bit puzzled that compat-wireless didn't 
build the mac80211 module and the fc10 kernel didn't have one either. (and early 
kernels have a mac80211.ko). Has this been made obsolete, or did the fc10 kernel 
shipped it compiled-in and for some good/bad reason compat-wireless doesn't
build modules that are compiled in? (obviously there is no point in building a 
module the older version of which is compiled in since it won't be used...)

The status.retry_count vs status.rate[0].count change must be in mac80211.ko
or some other more generic module?

Hin-Tak
P.S. I know building a whole wireless-testing kernel is the way to go, but
it takes *very long*...

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Herton Ronaldo Krzesinski]

On Wednesday 05 November 2008 15:11:40 Hin-Tak Leung wrote:
> Herton Ronaldo Krzesinski wrote:
> >> and applying the two patches on top, they applies cleanly and compile
> >> okay except for some warnings in a different module about missing
> >> symbols (<something>_sta) but the result dies very painfully with a
> >> kernel oops:
> >
> > Looks like something bad in combination of compat-wireless plus kernel
> > 2.6.27.4-73.fc10.x86_64, please try a plain wireless-testing tree with
> > the two patches.
>
> Hmm, I'll have to check, but I was a bit puzzled that compat-wireless
> didn't build the mac80211 module and the fc10 kernel didn't have one
> either. (and early kernels have a mac80211.ko). Has this been made
> obsolete, or did the fc10 kernel shipped it compiled-in and for some
> good/bad reason compat-wireless doesn't build modules that are compiled in?
> (obviously there is no point in building a module the older version of
> which is compiled in since it won't be used...)
>
> The status.retry_count vs status.rate[0].count change must be in
> mac80211.ko or some other more generic module?

Don't know about fc10 kernel, but the changes also include mac80211.ko

>
> Hin-Tak
> P.S. I know building a whole wireless-testing kernel is the way to go, but
> it takes *very long*...

--
[]'s
Herton

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Wed, 5/11/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:

> Don't know about fc10 kernel, but the changes also
> include mac80211.ko

Yes, I know the change includes mac802111.ko, that's why I was surprised that mac80211.ko didn't get built with compat-wireless. 

Turned out that fc10 kernels have mac80211 built-in rather than as modules, so there is no way of replacing them with compat-wireless. I am going to install a 2.6.27.x-fc9... (I am using a 2.6.27.x-fc10 for some x11-xorg drivers because there wasn't a 2.6.27.x-fc9 when I looked - and the first 2.6.27-x.fc9 has just hit koji).

I'll fill a bug in redhat's bugzilla. :-).


      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Luis R. Rodriguez]

On Wed, Nov 05, 2008 at 02:05:31PM -0800, Hin-Tak Leung wrote:
> --- On Wed, 5/11/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
> 
> > Don't know about fc10 kernel, but the changes also
> > include mac80211.ko
> 
> Yes, I know the change includes mac802111.ko, that's why I was surprised that mac80211.ko didn't get built with compat-wireless.
> 
> Turned out that fc10 kernels have mac80211 built-in rather than as modules, so there is no way of replacing them with compat-wireless. 

Umm I have an FC10 box and I see a mac80211.ko. But of course, I have an
early release of FC10, its on 2.6.27-0.244.rc2.git1.fc10.x86_64.

Did the new ones really put it built in?

  Luis

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Wed, 5/11/08, Luis R. Rodriguez <lrodriguez@atheros.com> wrote:
> Umm I have an FC10 box and I see a mac80211.ko. But of
> course, I have an
> early release of FC10, its on
> 2.6.27-0.244.rc2.git1.fc10.x86_64.
> 
> Did the new ones really put it built in?

Yes, 
$ grep 'MAC80211=' /boot/config-2.6.2*fc10*
/boot/config-2.6.27.4-69.fc10.x86_64:CONFIG_MAC80211=y
/boot/config-2.6.27.4-73.fc10.x86_64:CONFIG_MAC80211=y

I looked through koji and found it was removed between 
0.345.rc7.fc10
0.339.rc7.fc10
but the log entry between does not say why.

I filed this as https://bugzilla.redhat.com/show_bug.cgi?id=470143 ,
if John wants to grab it before somebody else assign it his way :-).

Hin-Tak








      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Luis R. Rodriguez]

On Wed, Nov 05, 2008 at 03:16:42PM -0800, Hin-Tak Leung wrote:
> --- On Wed, 5/11/08, Luis R. Rodriguez <lrodriguez@atheros.com> wrote:
> > Umm I have an FC10 box and I see a mac80211.ko. But of
> > course, I have an
> > early release of FC10, its on
> > 2.6.27-0.244.rc2.git1.fc10.x86_64.
> >
> > Did the new ones really put it built in?
> 
> Yes,
> $ grep 'MAC80211=' /boot/config-2.6.2*fc10*
> /boot/config-2.6.27.4-69.fc10.x86_64:CONFIG_MAC80211=y
> /boot/config-2.6.27.4-73.fc10.x86_64:CONFIG_MAC80211=y
> 
> I looked through koji and found it was removed between
> 0.345.rc7.fc10
> 0.339.rc7.fc10
> but the log entry between does not say why.
> 
> I filed this as https://bugzilla.redhat.com/show_bug.cgi?id=470143 ,

Thanks, I've put this on the wiki:

http://wireless.kernel.org/en/users/Download#DoIneedthisonFedora.3F

> if John wants to grab it before somebody else assign it his way :-).

  Luis

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Wed, 5/11/08, Herton Ronaldo Krzesinski <herton@mandriva.com.br> wrote:
Tested-by Hin-Tak Leung <htl10@users.sourceforge.net>

Okay, I managed to patch compat-wireless and install it (with 2.6.27.4-24.fc9.x86_64 which has mac80211 as a module, not compiled-in).

I am having the stuck-at-1Mb problem, so rate control does not work.

Before installing compat-wireless, I think the rate started at 36Mb and moves up to 54Mb and get stuck at 54Mb as soon as any traffic is getting through; after installing the patched compat-wireless, the rate changes beteen 1Mb and 9Mb for a bit but get stuck at 1Mb as soon as any substantial traffic is getting through. I can manually switch it with "iwconfig wlan2 rate 36M", for example, but with an auto, "iwconfig wlan2 rate 36M auto", it drops back to 1Mb very soon.

Baring changes in the rest of the wireless stack (which might come to the same thing), it is probably the changes surrounding status.rate[0].count vs status.retry_count which causes the 1Mb-rate-stuck... 

Did Larry get the v2 patches to do rate-control working (i.e. the rate actually changing with demand), or not? Larry's reply was "the rate-control mechanism works the way it did before" - I am  not sure about the "it did" bit - same as v1 (rate stuck at 1Mb) or same as the prototype patch for 2.6.27 (rate changing with demand)?  

Hin-Tak



      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Larry Finger]

Hin-Tak Leung wrote:
> 
> Before installing compat-wireless, I think the rate started at 36Mb and moves up to 54Mb and get stuck at 54Mb as soon as any traffic is getting through; after installing the patched compat-wireless, the rate changes beteen 1Mb and 9Mb for a bit but get stuck at 1Mb as soon as any substantial traffic is getting through. I can manually switch it with "iwconfig wlan2 rate 36M", for example, but with an auto, "iwconfig wlan2 rate 36M auto", it drops back to 1Mb very soon.
> 
> Baring changes in the rest of the wireless stack (which might come to the same thing), it is probably the changes surrounding status.rate[0].count vs status.retry_count which causes the 1Mb-rate-stuck... 
> 
> Did Larry get the v2 patches to do rate-control working (i.e. the rate actually changing with demand), or not? Larry's reply was "the rate-control mechanism works the way it did before" - I am  not sure about the "it did" bit - same as v1 (rate stuck at 1Mb) or same as the prototype patch for 2.6.27 (rate changing with demand)?  

On my system is started at 1 Mbs and rapidly moved to 54Mbs. It went so quickly
that by the time I started a ping, switched terminals, and issued an ivconfig,
it was already at 54 Mbs.

Larry

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Thu, 6/11/08, Larry Finger <Larry.Finger@lwfinger.net> wrote:

> On my system is started at 1 Mbs and rapidly moved to
> 54Mbs. It went so quickly
> that by the time I started a ping, switched terminals, and
> issued an ivconfig,
> it was already at 54 Mbs.

Okay, I was writing too soon. My rate does go up and down now (pinging the other end of the wireless link, it varies between 12Mb to 36Mb). I don't know why it was stuck at 1Mb for a while earlier (10-20 minutes while I was doing a fair bit of web browsing, etc), nor why it is go up and down now :-(. 

Hin-Tak 



      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Thu, 6/11/08, Larry Finger <Larry.Finger@lwfinger.net> wrote:

> On my system is started at 1 Mbs and rapidly moved to
> 54Mbs. It went so quickly
> that by the time I started a ping, switched terminals, and
> issued an ivconfig,
> it was already at 54 Mbs.

Seems that in my situation (my access point is a slow machine), the behavior of the rate control mechanism depends on the *type* of work load - I was restoring a firefox session with a lot of windows, and it is stuck at 1Mb all the time. But once all the windows are stored, doing a ping I can indeed have the rate going up (and eventually coming down) to 24-36Mb.

Hin-Tak


      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Larry Finger]

Hin-Tak Leung wrote:
> 
> Seems that in my situation (my access point is a slow machine), the behavior of the rate control mechanism depends on the *type* of work load - I was restoring a firefox session with a lot of windows, and it is stuck at 1Mb all the time. But once all the windows are stored, doing a ping I can indeed have the rate going up (and eventually coming down) to 24-36Mb.

Are those default pings? It may work differently with those short packets thaqn
it does with the longer ones found with browsing.

Larry

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

--- On Thu, 6/11/08, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Hin-Tak Leung wrote:
> > 
> > Seems that in my situation (my access point is a slow
> machine), the behavior of the rate control mechanism depends
> on the *type* of work load - I was restoring a firefox
> session with a lot of windows, and it is stuck at 1Mb all
> the time. But once all the windows are stored, doing a ping
> I can indeed have the rate going up (and eventually coming
> down) to 24-36Mb.
> 
> Are those default pings? It may work differently with those
> short packets thaqn
> it does with the longer ones found with browsing.

Yes, just default "ping 192.168.0.1" without any options. I wonder if
pid is clever enough to work out that my upstream bandwidth is only 2Mb/s (web browsing obviously goes outside) and not letting it go over 1Mb/s.

Also wondering about latency: in the absence of browser traffic, I can ping the AP/routing-machine with 0.07s round-trip time, but it increases to almost 2s when restoring a firefox session. I tried to see if pinging - which is between the client and the AP - changes the rate, compared to firefox which depends on limited upstream bandwidth. It didn't, and given how poor ping works during such time, it isn't surprising.

Hin-Tak

 


      

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[John W. Linville]

On Wed, Nov 05, 2008 at 11:16:42PM +0000, Hin-Tak Leung wrote:
> --- On Wed, 5/11/08, Luis R. Rodriguez <lrodriguez@atheros.com> wrote:
> > Umm I have an FC10 box and I see a mac80211.ko. But of
> > course, I have an
> > early release of FC10, its on
> > 2.6.27-0.244.rc2.git1.fc10.x86_64.
> > 
> > Did the new ones really put it built in?
> 
> Yes, 
> $ grep 'MAC80211=' /boot/config-2.6.2*fc10*
> /boot/config-2.6.27.4-69.fc10.x86_64:CONFIG_MAC80211=y
> /boot/config-2.6.27.4-73.fc10.x86_64:CONFIG_MAC80211=y
> 
> I looked through koji and found it was removed between 
> 0.345.rc7.fc10
> 0.339.rc7.fc10
> but the log entry between does not say why.
> 
> I filed this as https://bugzilla.redhat.com/show_bug.cgi?id=470143 ,
> if John wants to grab it before somebody else assign it his way :-).

That decision was made against my advice -- I don't know that I can
change it.

John
-- 
John W. Linville		Linux should be at the core
linville@tuxdriver.com			of your literate lifestyle.

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Luis R. Rodriguez]

On Thu, Nov 06, 2008 at 06:14:57AM -0800, John W. Linville wrote:
> On Wed, Nov 05, 2008 at 11:16:42PM +0000, Hin-Tak Leung wrote:
> > --- On Wed, 5/11/08, Luis R. Rodriguez <lrodriguez@atheros.com> wrote:
> > > Umm I have an FC10 box and I see a mac80211.ko. But of
> > > course, I have an
> > > early release of FC10, its on
> > > 2.6.27-0.244.rc2.git1.fc10.x86_64.
> > >
> > > Did the new ones really put it built in?
> >
> > Yes,
> > $ grep 'MAC80211=' /boot/config-2.6.2*fc10*
> > /boot/config-2.6.27.4-69.fc10.x86_64:CONFIG_MAC80211=y
> > /boot/config-2.6.27.4-73.fc10.x86_64:CONFIG_MAC80211=y
> >
> > I looked through koji and found it was removed between
> > 0.345.rc7.fc10
> > 0.339.rc7.fc10
> > but the log entry between does not say why.
> >
> > I filed this as https://bugzilla.redhat.com/show_bug.cgi?id=470143 ,
> > if John wants to grab it before somebody else assign it his way :-).
> 
> That decision was made against my advice -- I don't know that I can
> change it.

But you did, yay!

  Luis

Re: [RFC/RFT PATCH v2 2/2] rtl8187: feedback transmitted packets using tx close descriptor for 8187B

[Hin-Tak Leung]

The actual throughput transferring a 700MB file via sftp between my client and the AP doesn't go much over 50KB/s (according to sftp's progress meter). It used to be over 400KB/s under the same situation. The rate shown by
iwconfig is consistently 1Mb/s while manually setting it to 36/48Mb doesn't
change the throughput. (at the moment it is looking like it will take 4
hours to transfer, instead of the usual 20+ minutes).

how can I see what's wrong? (I have another few hours while it finishes, and of course, I can always try again...).