Re: [PATCH v2 3/3] rt2x00: do not print error when queue is full

[Stanislaw Gruszka <sgruszka@redhat.com>]

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

On Fri, Dec 28, 2018 at 01:45:27AM +0100, Tom Psyborg wrote:
> >
> > I move prints to debug level for for "arrived at non-free entry" from
> > "Dropping frame due to full tx queue" . For some users those 5 patches
> > improve things, they fix problem of router connection hung completely.
> > Printk problem is connection stall due to CPU being busy, but
> > is possible to reconnect to AP after that. It's different issue.
> >
> yes they fix hung. same hung that does this single patch fix by moving
> to dbg so they're not printed at all. FYI "Dropping frame due to full
> tx queue" causes "arrived at non-free entry" eventually that freezes
> interface so only reboot helps, removing "Dropping frame due to full
> tx queue" from printing to logs prevents this kind of hung and is
> better solution than 5 patches.

I wonder why are you confident about this ? And also why 5 patches
fixed the hung if there were still "Dropping frame ..." prints ?

Anyway I'll ask people on the bz for confirmation, if stop printing
is sufficient to fix the wifi hung.

> oh and don't forget to add note that
> rt2x00_err/rt2x00_dbg prints can cause serious bug so you don't
> acidentally enable them again in a year or two!

I think I will remember that, for others git changelog should
be sufficient.
 
> > Also 'result in lower throughput' is over-generalized statement. More
> > precise would be 'it lower throughput on some test cases', but even
> > in those specific cases, throughput vary randomly with and without
> > patches.
> >
> 
> 100Mbps TX 70Mbps RX without and 80Mbps TX 55 Mbps RX with 5
> patches--> EA2750 HT40 CH2 4.14.82+4.19-rc5-1-1

Ok, this is different case that you presented before on USB devices.
And I'm more concerned about it, because changes with MMIO ware
much bigger (for USB only some code was moved and some flush
changed were done). Please check attached patch, if it fixes
throughput regression for you ?

Thanks
Stanislaw

[-- Attachment #2: rt2800_only_check_wcid_for_match.patch --]
[-- Type: text/plain, Size: 1619 bytes --]

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index fe365cda0841..17e87e9b3d19 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -961,8 +961,7 @@ static bool rt2800_txdone_entry_check(struct queue_entry *entry, u32 reg)
 {
 	__le32 *txwi;
 	u32 word;
-	int wcid, ack, pid;
-	int tx_wcid, tx_ack, tx_pid, is_agg;
+	int wcid, tx_wcid;
 
 	/*
 	 * This frames has returned with an IO error,
@@ -972,23 +971,17 @@ static bool rt2800_txdone_entry_check(struct queue_entry *entry, u32 reg)
 	if (test_bit(ENTRY_DATA_IO_FAILED, &entry->flags))
 		return false;
 
-	wcid	= rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
-	ack	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_ACK_REQUIRED);
-	pid	= rt2x00_get_field32(reg, TX_STA_FIFO_PID_TYPE);
-	is_agg	= rt2x00_get_field32(reg, TX_STA_FIFO_TX_AGGRE);
-
 	/*
 	 * Validate if this TX status report is intended for
-	 * this entry by comparing the WCID/ACK/PID fields.
+	 * this entry by comparing the WCID field.
 	 */
 	txwi = rt2800_drv_get_txwi(entry);
-
 	word = rt2x00_desc_read(txwi, 1);
 	tx_wcid = rt2x00_get_field32(word, TXWI_W1_WIRELESS_CLI_ID);
-	tx_ack  = rt2x00_get_field32(word, TXWI_W1_ACK);
-	tx_pid  = rt2x00_get_field32(word, TXWI_W1_PACKETID);
 
-	if (wcid != tx_wcid || ack != tx_ack || (!is_agg && pid != tx_pid)) {
+	wcid = rt2x00_get_field32(reg, TX_STA_FIFO_WCID);
+
+	if (wcid != tx_wcid) {
 		rt2x00_dbg(entry->queue->rt2x00dev,
 			   "TX status report missed for queue %d entry %d\n",
 			   entry->queue->qid, entry->entry_idx);