linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] rt2x00usb: fix rx queue hang
@ 2019-07-01 10:53 Soeren Moch
  2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Soeren Moch @ 2019-07-01 10:53 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Soeren Moch, stable, Helmut Schaa, Kalle Valo, David S. Miller,
	linux-wireless, netdev, linux-kernel

Since commit ed194d136769 ("usb: core: remove local_irq_save() around
 ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
not running with interrupts disabled anymore. So this completion handler
is not guaranteed to run completely before workqueue processing starts
for the same queue entry.
Be sure to set all other flags in the entry correctly before marking
this entry ready for workqueue processing. This way we cannot miss error
conditions that need to be signalled from the completion handler to the
worker thread.
Note that rt2x00usb_work_rxdone() processes all available entries, not
only such for which queue_work() was called.

This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
of entry flags modification") did for TX processing.

This fixes a regression on a RT5370 based wifi stick in AP mode, which
suddenly stopped data transmission after some period of heavy load. Also
stopping the hanging hostapd resulted in the error message "ieee80211
phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
Other operation modes are probably affected as well, this just was
the used testcase.

Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
Cc: stable@vger.kernel.org # 4.20+
Signed-off-by: Soeren Moch <smoch@web.de>
---
Changes in v2:
 complete rework

Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/wireless/ralink/rt2x00/rt2x00usb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
index 67b81c7221c4..7e3a621b9c0d 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
@@ -372,14 +372,9 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
 	struct queue_entry *entry = (struct queue_entry *)urb->context;
 	struct rt2x00_dev *rt2x00dev = entry->queue->rt2x00dev;

-	if (!test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
+	if (!test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
 		return;

-	/*
-	 * Report the frame as DMA done
-	 */
-	rt2x00lib_dmadone(entry);
-
 	/*
 	 * Check if the received data is simply too small
 	 * to be actually valid, or if the urb is signaling
@@ -388,6 +383,11 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
 	if (urb->actual_length < entry->queue->desc_size || urb->status)
 		set_bit(ENTRY_DATA_IO_FAILED, &entry->flags);

+	/*
+	 * Report the frame as DMA done
+	 */
+	rt2x00lib_dmadone(entry);
+
 	/*
 	 * Schedule the delayed work for reading the RX status
 	 * from the device.
--
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks
  2019-07-01 10:53 [PATCH v2 1/2] rt2x00usb: fix rx queue hang Soeren Moch
@ 2019-07-01 10:53 ` Soeren Moch
  2019-07-01 11:07   ` Stanislaw Gruszka
  2019-07-24 11:42   ` Kalle Valo
  2019-07-01 11:07 ` [PATCH v2 1/2] rt2x00usb: fix rx queue hang Stanislaw Gruszka
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 8+ messages in thread
From: Soeren Moch @ 2019-07-01 10:53 UTC (permalink / raw)
  To: Stanislaw Gruszka
  Cc: Soeren Moch, Helmut Schaa, Kalle Valo, David S. Miller,
	linux-wireless, netdev, linux-kernel

In contrast to the TX path, there is no need to separately read the transfer
status from the device after receiving RX data. Consequently, there is no
real STATUS_PENDING RX processing queue entry state.
Remove the unnecessary ENTRY_DATA_STATUS_PENDING flag checks from the RX path.
Also remove the misleading comment about reading RX status from device.

Suggested-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Soeren Moch <smoch@web.de>
---
Changes in v2:
 new patch

Cc: Stanislaw Gruszka <sgruszka@redhat.com>
Cc: Helmut Schaa <helmut.schaa@googlemail.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/wireless/ralink/rt2x00/rt2x00usb.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
index 7e3a621b9c0d..bc2dfef0de22 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c
@@ -349,8 +349,7 @@ static void rt2x00usb_work_rxdone(struct work_struct *work)
 	while (!rt2x00queue_empty(rt2x00dev->rx)) {
 		entry = rt2x00queue_get_entry(rt2x00dev->rx, Q_INDEX_DONE);

-		if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
-		    !test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
+		if (test_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
 			break;

 		/*
@@ -389,8 +388,7 @@ static void rt2x00usb_interrupt_rxdone(struct urb *urb)
 	rt2x00lib_dmadone(entry);

 	/*
-	 * Schedule the delayed work for reading the RX status
-	 * from the device.
+	 * Schedule the delayed work for processing RX data
 	 */
 	queue_work(rt2x00dev->workqueue, &rt2x00dev->rxdone_work);
 }
@@ -402,8 +400,7 @@ static bool rt2x00usb_kick_rx_entry(struct queue_entry *entry, void *data)
 	struct queue_entry_priv_usb *entry_priv = entry->priv_data;
 	int status;

-	if (test_and_set_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags) ||
-	    test_bit(ENTRY_DATA_STATUS_PENDING, &entry->flags))
+	if (test_and_set_bit(ENTRY_OWNER_DEVICE_DATA, &entry->flags))
 		return false;

 	rt2x00lib_dmastart(entry);
--
2.17.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] rt2x00usb: fix rx queue hang
  2019-07-01 10:53 [PATCH v2 1/2] rt2x00usb: fix rx queue hang Soeren Moch
  2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
@ 2019-07-01 11:07 ` Stanislaw Gruszka
  2019-07-15  8:48 ` Kalle Valo
  2019-07-15 17:53 ` Kalle Valo
  3 siblings, 0 replies; 8+ messages in thread
From: Stanislaw Gruszka @ 2019-07-01 11:07 UTC (permalink / raw)
  To: Soeren Moch
  Cc: stable, Helmut Schaa, Kalle Valo, David S. Miller,
	linux-wireless, netdev, linux-kernel

On Mon, Jul 01, 2019 at 12:53:13PM +0200, Soeren Moch wrote:
> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
>  ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
> not running with interrupts disabled anymore. So this completion handler
> is not guaranteed to run completely before workqueue processing starts
> for the same queue entry.
> Be sure to set all other flags in the entry correctly before marking
> this entry ready for workqueue processing. This way we cannot miss error
> conditions that need to be signalled from the completion handler to the
> worker thread.
> Note that rt2x00usb_work_rxdone() processes all available entries, not
> only such for which queue_work() was called.
> 
> This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
> of entry flags modification") did for TX processing.
> 
> This fixes a regression on a RT5370 based wifi stick in AP mode, which
> suddenly stopped data transmission after some period of heavy load. Also
> stopping the hanging hostapd resulted in the error message "ieee80211
> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
> Other operation modes are probably affected as well, this just was
> the used testcase.
> 
> Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
> Cc: stable@vger.kernel.org # 4.20+
> Signed-off-by: Soeren Moch <smoch@web.de>

Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks
  2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
@ 2019-07-01 11:07   ` Stanislaw Gruszka
  2019-07-24 11:42   ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Stanislaw Gruszka @ 2019-07-01 11:07 UTC (permalink / raw)
  To: Soeren Moch
  Cc: Helmut Schaa, Kalle Valo, David S. Miller, linux-wireless,
	netdev, linux-kernel

On Mon, Jul 01, 2019 at 12:53:14PM +0200, Soeren Moch wrote:
> In contrast to the TX path, there is no need to separately read the transfer
> status from the device after receiving RX data. Consequently, there is no
> real STATUS_PENDING RX processing queue entry state.
> Remove the unnecessary ENTRY_DATA_STATUS_PENDING flag checks from the RX path.
> Also remove the misleading comment about reading RX status from device.
> 
> Suggested-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Signed-off-by: Soeren Moch <smoch@web.de>

Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] rt2x00usb: fix rx queue hang
  2019-07-01 10:53 [PATCH v2 1/2] rt2x00usb: fix rx queue hang Soeren Moch
  2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
  2019-07-01 11:07 ` [PATCH v2 1/2] rt2x00usb: fix rx queue hang Stanislaw Gruszka
@ 2019-07-15  8:48 ` Kalle Valo
  2019-07-15 13:33   ` Soeren Moch
  2019-07-15 17:53 ` Kalle Valo
  3 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2019-07-15  8:48 UTC (permalink / raw)
  To: Soeren Moch
  Cc: Stanislaw Gruszka, stable, Helmut Schaa, David S. Miller,
	linux-wireless, netdev, linux-kernel

Soeren Moch <smoch@web.de> writes:

> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
>  ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
> not running with interrupts disabled anymore. So this completion handler
> is not guaranteed to run completely before workqueue processing starts
> for the same queue entry.
> Be sure to set all other flags in the entry correctly before marking
> this entry ready for workqueue processing. This way we cannot miss error
> conditions that need to be signalled from the completion handler to the
> worker thread.
> Note that rt2x00usb_work_rxdone() processes all available entries, not
> only such for which queue_work() was called.
>
> This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
> of entry flags modification") did for TX processing.
>
> This fixes a regression on a RT5370 based wifi stick in AP mode, which
> suddenly stopped data transmission after some period of heavy load. Also
> stopping the hanging hostapd resulted in the error message "ieee80211
> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
> Other operation modes are probably affected as well, this just was
> the used testcase.
>
> Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
> Cc: stable@vger.kernel.org # 4.20+
> Signed-off-by: Soeren Moch <smoch@web.de>

I'll queue this for v5.3.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] rt2x00usb: fix rx queue hang
  2019-07-15  8:48 ` Kalle Valo
@ 2019-07-15 13:33   ` Soeren Moch
  0 siblings, 0 replies; 8+ messages in thread
From: Soeren Moch @ 2019-07-15 13:33 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Stanislaw Gruszka, stable, Helmut Schaa, David S. Miller,
	linux-wireless, netdev, linux-kernel



On 15.07.19 10:48, Kalle Valo wrote:
> Soeren Moch <smoch@web.de> writes:
>
>> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
>>  ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
>> not running with interrupts disabled anymore. So this completion handler
>> is not guaranteed to run completely before workqueue processing starts
>> for the same queue entry.
>> Be sure to set all other flags in the entry correctly before marking
>> this entry ready for workqueue processing. This way we cannot miss error
>> conditions that need to be signalled from the completion handler to the
>> worker thread.
>> Note that rt2x00usb_work_rxdone() processes all available entries, not
>> only such for which queue_work() was called.
>>
>> This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
>> of entry flags modification") did for TX processing.
>>
>> This fixes a regression on a RT5370 based wifi stick in AP mode, which
>> suddenly stopped data transmission after some period of heavy load. Also
>> stopping the hanging hostapd resulted in the error message "ieee80211
>> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
>> Other operation modes are probably affected as well, this just was
>> the used testcase.
>>
>> Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
>> Cc: stable@vger.kernel.org # 4.20+
>> Signed-off-by: Soeren Moch <smoch@web.de>
> I'll queue this for v5.3.
>
OK, thanks,
Soeren

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] rt2x00usb: fix rx queue hang
  2019-07-01 10:53 [PATCH v2 1/2] rt2x00usb: fix rx queue hang Soeren Moch
                   ` (2 preceding siblings ...)
  2019-07-15  8:48 ` Kalle Valo
@ 2019-07-15 17:53 ` Kalle Valo
  3 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2019-07-15 17:53 UTC (permalink / raw)
  To: Soeren Moch
  Cc: Stanislaw Gruszka, Soeren Moch, stable, Helmut Schaa,
	David S. Miller, linux-wireless, netdev, linux-kernel

Soeren Moch <smoch@web.de> wrote:

> Since commit ed194d136769 ("usb: core: remove local_irq_save() around
>  ->complete() handler") the handler rt2x00usb_interrupt_rxdone() is
> not running with interrupts disabled anymore. So this completion handler
> is not guaranteed to run completely before workqueue processing starts
> for the same queue entry.
> Be sure to set all other flags in the entry correctly before marking
> this entry ready for workqueue processing. This way we cannot miss error
> conditions that need to be signalled from the completion handler to the
> worker thread.
> Note that rt2x00usb_work_rxdone() processes all available entries, not
> only such for which queue_work() was called.
> 
> This patch is similar to what commit df71c9cfceea ("rt2x00: fix order
> of entry flags modification") did for TX processing.
> 
> This fixes a regression on a RT5370 based wifi stick in AP mode, which
> suddenly stopped data transmission after some period of heavy load. Also
> stopping the hanging hostapd resulted in the error message "ieee80211
> phy0: rt2x00queue_flush_queue: Warning - Queue 14 failed to flush".
> Other operation modes are probably affected as well, this just was
> the used testcase.
> 
> Fixes: ed194d136769 ("usb: core: remove local_irq_save() around ->complete() handler")
> Cc: stable@vger.kernel.org # 4.20+
> Signed-off-by: Soeren Moch <smoch@web.de>
> Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Patch applied to wireless-drivers.git, thanks.

41a531ffa4c5 rt2x00usb: fix rx queue hang

-- 
https://patchwork.kernel.org/patch/11025561/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks
  2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
  2019-07-01 11:07   ` Stanislaw Gruszka
@ 2019-07-24 11:42   ` Kalle Valo
  1 sibling, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2019-07-24 11:42 UTC (permalink / raw)
  To: Soeren Moch
  Cc: Stanislaw Gruszka, Soeren Moch, Helmut Schaa, David S. Miller,
	linux-wireless, netdev, linux-kernel

Soeren Moch <smoch@web.de> wrote:

> In contrast to the TX path, there is no need to separately read the transfer
> status from the device after receiving RX data. Consequently, there is no
> real STATUS_PENDING RX processing queue entry state.
> Remove the unnecessary ENTRY_DATA_STATUS_PENDING flag checks from the RX path.
> Also remove the misleading comment about reading RX status from device.
> 
> Suggested-by: Stanislaw Gruszka <sgruszka@redhat.com>
> Signed-off-by: Soeren Moch <smoch@web.de>
> Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>

Patch applied to wireless-drivers-next.git, thanks.

3b902fa811cf rt2x00usb: remove unnecessary rx flag checks

-- 
https://patchwork.kernel.org/patch/11025559/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-07-24 11:43 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-01 10:53 [PATCH v2 1/2] rt2x00usb: fix rx queue hang Soeren Moch
2019-07-01 10:53 ` [PATCH v2 2/2] rt2x00usb: remove unnecessary rx flag checks Soeren Moch
2019-07-01 11:07   ` Stanislaw Gruszka
2019-07-24 11:42   ` Kalle Valo
2019-07-01 11:07 ` [PATCH v2 1/2] rt2x00usb: fix rx queue hang Stanislaw Gruszka
2019-07-15  8:48 ` Kalle Valo
2019-07-15 13:33   ` Soeren Moch
2019-07-15 17:53 ` Kalle Valo

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