[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

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 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 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

[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 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 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 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 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