Skip to contentSkip to conversation

mt76: usb: fix possible memory leak during suspend/resume

PATCH In review Lorenzo Bianconi 3 participants 12 messages Apr 12, 2019 – Apr 16, 2019 1 message not in the archive

Subsystems (2)
Arm MediaTek MediaTek MT76 wireless LAN driver
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Lorenzo Bianconi

author maintainer
#1 lore raw

PATCH 1/1 mt76: usb: fix possible memory leak during suspend/resume

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
index a3acc070063a..575207133775 100644
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);
 }
 EXPORT_SYMBOL_GPL(mt76u_stop_queues);
Signature · 1 line
2.20.1
#2 lore raw
Quoted 2 lines · Lorenzo Bianconi

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

I comprehansive have fix for that, but giving it more testing. Please drop this patch.

Stanislaw

#3 lore raw
Quoted 7 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Quoted 18 lines
Quoted 18 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -2
index a3acc070063a..575207133775 100644
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
Quoted 8 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 3 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

Regards,
Lorenzo

Quoted 5 lines

I comprehansive have fix for that, but giving it more testing. Please drop this patch.

Stanislaw

#4 lore raw
Quoted 40 lines
Quoted 7 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Quoted 18 lines
Quoted 18 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -2
index a3acc070063a..575207133775 100644
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
Quoted 8 lines

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 3 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -0
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think it is enough even for rx side since usb_kill_urb() will wait the end of mt76u_complete_rx() and tasklet_kill() will wait for the completion of mt76u_rx_tasklet(). We will need to remove tasklet_enable in resume routines.

Regards,
Lorenzo

Quoted 9 lines

Regards,
Lorenzo

Quoted 5 lines

I comprehansive have fix for that, but giving it more testing. Please drop this patch.

Stanislaw

#5 lore raw
Quoted 4 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 3 lines

I think it is enough even for rx side since usb_kill_urb() will wait the end of mt76u_complete_rx() and tasklet_kill() will wait for the completion of mt76u_rx_tasklet(). We will need to remove tasklet_enable in resume routines.

No, because rx urb's are resubmitted on mt76u_rx_tasklet(). I use usb_poison_urb() to prevent that in my patch.

Stanislaw

commit 088b1209302e17cda688c9b562e18970c92823ac

Author: Stanislaw Gruszka <[email protected]>
Date:   Thu Apr 4 13:37:43 2019 +0200
    mt76usb: fix tx/rx stop
    
    Disabling tasklets on stopping rx/tx is wrong. If blocked tasklet
    is scheduled and we remove device we get 100% cpu usage:
    
      PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
        9 root      20   0       0      0      0 R  93.8   0.0   1:47.19 ksoftirqd/0
    
    by using memory after free and eventually crash:
    
    [ 2068.591964] RIP: 0010:tasklet_action_common.isra.17+0x66/0x100
    [ 2068.591966] Code: 41 89 f5 eb 25 f0 48 0f ba 33 00 0f 83 b1 00 00 00 48 8b 7a 20 48 8b 42 18 e8 56 a3 b5 00 f0 80 23 fd 48 89 ea 48 85 ed 74 53 <48> 8b 2a 48 8d 5a 08 f0 48 0f ba 6a 08 01 72 0b 8b 42 10 85 c0 74
    [ 2068.591968] RSP: 0018:ffff98758c34be58 EFLAGS: 00010206
    [ 2068.591969] RAX: ffff98758e6966d0 RBX: ffff98756e69aef8 RCX: 0000000000000006
    [ 2068.591970] RDX: 01060a053d060305 RSI: 0000000000000006 RDI: ffff98758e6966d0
    [ 2068.591971] RBP: 01060a053d060305 R08: 0000000000000000 R09: 00000000000203c0
    [ 2068.591971] R10: 000003ff65b34f08 R11: 0000000000000001 R12: ffff98758e6966d0
    [ 2068.591972] R13: 0000000000000006 R14: 0000000000000040 R15: 0000000000000006
    [ 2068.591974] FS:  0000000000000000(0000) GS:ffff98758e680000(0000) knlGS:0000000000000000
    [ 2068.591975] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [ 2068.591975] CR2: 00002c5f73a6cc20 CR3: 00000002f920a001 CR4: 00000000003606e0
    [ 2068.591977] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    [ 2068.591978] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
    [ 2068.591978] Call Trace:
    [ 2068.591985]  __do_softirq+0xe3/0x30a
    [ 2068.591989]  ? sort_range+0x20/0x20
    [ 2068.591990]  run_ksoftirqd+0x26/0x40
    [ 2068.591992]  smpboot_thread_fn+0xc5/0x160
    [ 2068.591995]  kthread+0x112/0x130
    [ 2068.591997]  ? kthread_create_on_node+0x40/0x40
    [ 2068.591998]  ret_from_fork+0x35/0x40
    [ 2068.591999] Modules linked in: ccm arc4 fuse rfcomm cmac bnep sunrpc snd_hda_codec_hdmi snd_soc_skl snd_soc_core snd_soc_acpi_intel_match snd_hda_codec_realtek snd_soc_acpi snd_hda_codec_generic snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_hda_ext_core iTCO_wdt snd_hda_intel intel_rapl iTCO_vendor_support x86_pkg_temp_thermal intel_powerclamp btusb mei_wdt coretemp btrtl snd_hda_codec btbcm btintel intel_cstate snd_hwdep intel_uncore uvcvideo snd_hda_core videobuf2_vmalloc videobuf2_memops intel_rapl_perf wmi_bmof videobuf2_v4l2 intel_wmi_thunderbolt snd_seq bluetooth joydev videobuf2_common snd_seq_device snd_pcm videodev media i2c_i801 snd_timer idma64 ecdh_generic intel_lpss_pci intel_lpss mei_me mei ucsi_acpi typec_ucsi processor_thermal_device intel_soc_dts_iosf intel_pch_thermal typec thinkpad_acpi wmi snd soundcore rfkill int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel acpi_pad pcc_cpufreq uas usb_storage crc32c_intel i915 i2c_algo_bit nvme 
 serio_raw
    [ 2068.592033]  drm_kms_helper e1000e nvme_core drm video ipv6 [last unloaded: cfg80211]

Fortunate thing is that this not happen frequently, as scheduling tasklet on blocked state is very exceptional, though might happen.

Due to different RX/TX tasklet processing fix is different for those. For TX just wait until queues are empty. For RX assure rx_tasklet do fail to resubmit buffers by poisoning urb's and kill the tasklet.

Signed-off-by Stanislaw Gruszka <[email protected]>
drivers/net/wireless/mediatek/mt76/mac80211.c +2 -1
index 15825e9a458f..7bf91566e212 100644
@@ -384,7 +384,7 @@ void mt76_rx(struct mt76_dev *dev, enum mt76_rxq_id q, struct sk_buff *skb)
 }
 EXPORT_SYMBOL_GPL(mt76_rx);
 
-static bool mt76_has_tx_pending(struct mt76_dev *dev)
+bool mt76_has_tx_pending(struct mt76_dev *dev)
 {
 	struct mt76_queue *q;
 	int i;

@@ -397,6 +397,7 @@ static bool mt76_has_tx_pending(struct mt76_dev *dev)
 
 	return false;
 }
+EXPORT_SYMBOL_GPL(mt76_has_tx_pending);
 
 void mt76_set_channel(struct mt76_dev *dev)
 {
drivers/net/wireless/mediatek/mt76/mt76.h +2 -1
index b432da3f55c7..19d03294e678 100644
@@ -682,6 +682,7 @@ void mt76_release_buffered_frames(struct ieee80211_hw *hw,
 				  u16 tids, int nframes,
 				  enum ieee80211_frame_release_type reason,
 				  bool more_data);
+bool mt76_has_tx_pending(struct mt76_dev *dev);
 void mt76_set_channel(struct mt76_dev *dev);
 int mt76_get_survey(struct ieee80211_hw *hw, int idx,
 		    struct survey_info *survey);

@@ -776,9 +777,9 @@ int mt76u_vendor_request(struct mt76_dev *dev, u8 req,
 void mt76u_single_wr(struct mt76_dev *dev, const u8 req,
 		     const u16 offset, const u32 val);
 int mt76u_init(struct mt76_dev *dev, struct usb_interface *intf);
-int mt76u_submit_rx_buffers(struct mt76_dev *dev);
 int mt76u_alloc_queues(struct mt76_dev *dev);
 void mt76u_stop_queues(struct mt76_dev *dev);
+int mt76u_start_queues(struct mt76_dev *dev);
 void mt76u_stop_stat_wk(struct mt76_dev *dev);
 void mt76u_queues_deinit(struct mt76_dev *dev);
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -4
index 1ef00e971cfa..a27a3ae3dbf5 100644
@@ -327,13 +327,10 @@ static int __maybe_unused mt76x0_resume(struct usb_interface *usb_intf)
 	struct mt76_usb *usb = &dev->mt76.usb;
 	int ret;
 
-	ret = mt76u_submit_rx_buffers(&dev->mt76);
+	ret = mt76u_start_queues(&dev->mt76);
 	if (ret < 0)
 		goto err;
 
-	tasklet_enable(&usb->rx_tasklet);
-	tasklet_enable(&usb->tx_tasklet);
-
 	ret = mt76x0u_init_hardware(dev);
 	if (ret)
 		goto err;
drivers/net/wireless/mediatek/mt76/mt76x2/usb.c +2 -5
index d08bb964966b..5450fb1c3f55 100644
@@ -119,13 +119,10 @@ static int __maybe_unused mt76x2u_resume(struct usb_interface *intf)
 	struct mt76_usb *usb = &dev->mt76.usb;
 	int err;
 
-	err = mt76u_submit_rx_buffers(&dev->mt76);
-	if (err < 0)
+	err = mt76u_start_queues(&dev->mt76);
+	if (err)
 		goto err;
 
-	tasklet_enable(&usb->rx_tasklet);
-	tasklet_enable(&usb->tx_tasklet);
-
 	err = mt76x2u_init_hardware(dev);
 	if (err < 0)
 		goto err;
drivers/net/wireless/mediatek/mt76/usb.c +23 -7
index a3acc070063a..8e32a60ac1c3 100644
@@ -540,7 +540,7 @@ static void mt76u_rx_tasklet(unsigned long data)
 	rcu_read_unlock();
 }
 
-int mt76u_submit_rx_buffers(struct mt76_dev *dev)
+static int mt76u_submit_rx_buffers(struct mt76_dev *dev)
 {
 	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
 	unsigned long flags;

@@ -558,7 +558,6 @@ int mt76u_submit_rx_buffers(struct mt76_dev *dev)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(mt76u_submit_rx_buffers);
 
 static int mt76u_alloc_rx(struct mt76_dev *dev)
 {

@@ -611,7 +610,9 @@ static void mt76u_stop_rx(struct mt76_dev *dev)
 	int i;
 
 	for (i = 0; i < q->ndesc; i++)
-		usb_kill_urb(q->entry[i].urb);
+		usb_poison_urb(q->entry[i].urb);
+
+	tasklet_kill(&dev->usb.rx_tasklet);
 }
 
 static void mt76u_tx_tasklet(unsigned long data)

@@ -830,20 +831,35 @@ static void mt76u_free_tx(struct mt76_dev *dev)
 static void mt76u_stop_tx(struct mt76_dev *dev)
 {
 	struct mt76_queue *q;
-	int i, j;
+	int i, j, ret;
 
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		q = dev->q_tx[i].q;
 		for (j = 0; j < q->ndesc; j++)
 			usb_kill_urb(q->entry[j].urb);
 	}
+
+	ret = wait_event_timeout(dev->tx_wait, !mt76_has_tx_pending(dev), HZ/5);
+	if (!ret)
+		dev_err(dev->dev, "timed out waiting for pending tx\n");
 }
 
-void mt76u_stop_queues(struct mt76_dev *dev)
+int mt76u_start_queues(struct mt76_dev *dev)
 {
-	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
+	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
+	int i;
+
+	/* Nothing to do for TX */
 
+	for (i = 0; i < q->ndesc; i++)
+		usb_unpoison_urb(q->entry[i].urb);
+
+	return mt76u_submit_rx_buffers(dev);
+}
+EXPORT_SYMBOL_GPL(mt76u_start_queues);
+
+void mt76u_stop_queues(struct mt76_dev *dev)
+{
 	mt76u_stop_rx(dev);
 	mt76u_stop_tx(dev);
 }
author maintainer
#6 lore raw
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Regards,
Lorenzo

Quoted 59 lines
Quoted 4 lines

I think it is enough even for rx side since usb_kill_urb() will wait the end of mt76u_complete_rx() and tasklet_kill() will wait for the completion of mt76u_rx_tasklet(). We will need to remove tasklet_enable in resume routines.

No, because rx urb's are resubmitted on mt76u_rx_tasklet(). I use usb_poison_urb() to prevent that in my patch.

Stanislaw

commit 088b1209302e17cda688c9b562e18970c92823ac

Author: Stanislaw Gruszka <[email protected]>
Date:   Thu Apr 4 13:37:43 2019 +0200
    mt76usb: fix tx/rx stop
    
    Disabling tasklets on stopping rx/tx is wrong. If blocked tasklet
    is scheduled and we remove device we get 100% cpu usage:
    
      PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
        9 root      20   0       0      0      0 R  93.8   0.0   1:47.19 ksoftirqd/0
    
    by using memory after free and eventually crash:
    
    [ 2068.591964] RIP: 0010:tasklet_action_common.isra.17+0x66/0x100
    [ 2068.591966] Code: 41 89 f5 eb 25 f0 48 0f ba 33 00 0f 83 b1 00 00 00 48 8b 7a 20 48 8b 42 18 e8 56 a3 b5 00 f0 80 23 fd 48 89 ea 48 85 ed 74 53 <48> 8b 2a 48 8d 5a 08 f0 48 0f ba 6a 08 01 72 0b 8b 42 10 85 c0 74
    [ 2068.591968] RSP: 0018:ffff98758c34be58 EFLAGS: 00010206
    [ 2068.591969] RAX: ffff98758e6966d0 RBX: ffff98756e69aef8 RCX: 0000000000000006
    [ 2068.591970] RDX: 01060a053d060305 RSI: 0000000000000006 RDI: ffff98758e6966d0
    [ 2068.591971] RBP: 01060a053d060305 R08: 0000000000000000 R09: 00000000000203c0
    [ 2068.591971] R10: 000003ff65b34f08 R11: 0000000000000001 R12: ffff98758e6966d0
    [ 2068.591972] R13: 0000000000000006 R14: 0000000000000040 R15: 0000000000000006
    [ 2068.591974] FS:  0000000000000000(0000) GS:ffff98758e680000(0000) knlGS:0000000000000000
    [ 2068.591975] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
    [ 2068.591975] CR2: 00002c5f73a6cc20 CR3: 00000002f920a001 CR4: 00000000003606e0
    [ 2068.591977] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
    [ 2068.591978] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
    [ 2068.591978] Call Trace:
    [ 2068.591985]  __do_softirq+0xe3/0x30a
    [ 2068.591989]  ? sort_range+0x20/0x20
    [ 2068.591990]  run_ksoftirqd+0x26/0x40
    [ 2068.591992]  smpboot_thread_fn+0xc5/0x160
    [ 2068.591995]  kthread+0x112/0x130
    [ 2068.591997]  ? kthread_create_on_node+0x40/0x40
    [ 2068.591998]  ret_from_fork+0x35/0x40
    [ 2068.591999] Modules linked in: ccm arc4 fuse rfcomm cmac bnep sunrpc snd_hda_codec_hdmi snd_soc_skl snd_soc_core snd_soc_acpi_intel_match snd_hda_codec_realtek snd_soc_acpi snd_hda_codec_generic snd_soc_skl_ipc snd_soc_sst_ipc snd_soc_sst_dsp snd_hda_ext_core iTCO_wdt snd_hda_intel intel_rapl iTCO_vendor_support x86_pkg_temp_thermal intel_powerclamp btusb mei_wdt coretemp btrtl snd_hda_codec btbcm btintel intel_cstate snd_hwdep intel_uncore uvcvideo snd_hda_core videobuf2_vmalloc videobuf2_memops intel_rapl_perf wmi_bmof videobuf2_v4l2 intel_wmi_thunderbolt snd_seq bluetooth joydev videobuf2_common snd_seq_device snd_pcm videodev media i2c_i801 snd_timer idma64 ecdh_generic intel_lpss_pci intel_lpss mei_me mei ucsi_acpi typec_ucsi processor_thermal_device intel_soc_dts_iosf intel_pch_thermal typec thinkpad_acpi wmi snd soundcore rfkill int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel acpi_pad pcc_cpufreq uas usb_storage crc32c_intel i915 i2c_algo_bit nvme serio_raw
    [ 2068.592033]  drm_kms_helper e1000e nvme_core drm video ipv6 [last unloaded: cfg80211]
    
    Fortunate thing is that this not happen frequently, as scheduling
    tasklet on blocked state is very exceptional, though might happen.
    
    Due to different RX/TX tasklet processing fix is different for those.
    For TX just wait until queues are empty. For RX assure rx_tasklet
    do fail to resubmit buffers by poisoning urb's and kill the tasklet.
    
    Signed-off-by: Stanislaw Gruszka <[email protected]>
drivers/net/wireless/mediatek/mt76/mac80211.c +0 -0
index 15825e9a458f..7bf91566e212 100644

[...]

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/mac80211.c +1 -1
@@ -830,20 +831,35 @@ static void mt76u_free_tx(struct mt76_dev *dev)
 static void mt76u_stop_tx(struct mt76_dev *dev)
 {
 	struct mt76_queue *q;
-	int i, j;
+	int i, j, ret;
 
 	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
 		q = dev->q_tx[i].q;
 		for (j = 0; j < q->ndesc; j++)

drivers/net/wireless/mediatek/mt76/mac80211.c static void mt76u_free_tx(struct mt76_dev *dev) · line 847

Quoted 10 lines
drivers/net/wireless/mediatek/mt76/mac80211.c +5 -1
 			usb_kill_urb(q->entry[j].urb);
 	}
+
+	ret = wait_event_timeout(dev->tx_wait, !mt76_has_tx_pending(dev), HZ/5);
+	if (!ret)
+		dev_err(dev->dev, "timed out waiting for pending tx\n");
 }
 
-void mt76u_stop_queues(struct mt76_dev *dev)
+int mt76u_start_queues(struct mt76_dev *dev)

maybe mt76u_resume_rx_queue would be more clear

Quoted 20 lines
drivers/net/wireless/mediatek/mt76/mac80211.c +13 -2
 {
-	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
+	struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
+	int i;
+
+	/* Nothing to do for TX */
 
+	for (i = 0; i < q->ndesc; i++)
+		usb_unpoison_urb(q->entry[i].urb);
+
+	return mt76u_submit_rx_buffers(dev);
+}
+EXPORT_SYMBOL_GPL(mt76u_start_queues);
+
+void mt76u_stop_queues(struct mt76_dev *dev)
+{
 	mt76u_stop_rx(dev);
 	mt76u_stop_tx(dev);
 }
#7 lore raw
Quoted 51 lines · Lorenzo Bianconi
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

Stanislaw

#8 lore raw
Quoted 57 lines · Lorenzo Bianconi
Quoted 51 lines
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Quoted 4 lines

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

right

Quoted 3 lines

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

ack

Quoted 2 lines

Stanislaw

during device removal I guess we should also flush skbs in status queue, doing something like (after commit 0b5f71304cd9 (mt76: introduce mt76_free_device routine))

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
index 1ef00e971cfa..d4d1eb003148 100644
@@ -299,7 +299,7 @@ static void mt76x0_disconnect(struct usb_interface *usb_intf)
 	if (!initalized)
 		return;
 
-	ieee80211_unregister_hw(dev->mt76.hw);
+	mt76_unregister_device(&dev->mt76);
 	mt76x0u_cleanup(dev);
 
 	usb_set_intfdata(usb_intf, NULL);
drivers/net/wireless/mediatek/mt76/mt76x2/usb.c +1 -1
index d08bb964966b..4394c7c10535 100644
@@ -94,7 +94,7 @@ static void mt76x2u_disconnect(struct usb_interface *intf)
 	struct ieee80211_hw *hw = mt76_hw(dev);
 
 	set_bit(MT76_REMOVED, &dev->mt76.state);
-	ieee80211_unregister_hw(hw);
+	mt76_unregister_device(&dev->mt76);
 	mt76x2u_cleanup(dev);
 
 	ieee80211_free_hw(hw);

Regards,
Lorenzo

#9 lore raw
Quoted 82 lines · Lorenzo Bianconi
Quoted 57 lines · Lorenzo Bianconi
Quoted 51 lines
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Quoted 4 lines

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

right

Quoted 3 lines

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

ack

Quoted 2 lines

Stanislaw

during device removal I guess we should also flush skbs in status queue, doing something like (after commit 0b5f71304cd9 (mt76: introduce mt76_free_device routine))

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +0 -0
index 1ef00e971cfa..d4d1eb003148 100644

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c static void mt76x0_disconnect(struct usb_interface *usb_intf) · line 302

Quoted 5 lines
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
@@ -299,7 +299,7 @@ static void mt76x0_disconnect(struct usb_interface *usb_intf)
 	if (!initalized)
 		return;
 
-	ieee80211_unregister_hw(dev->mt76.hw);
+	mt76_unregister_device(&dev->mt76);

mt76_unregister_device() free mmio dma. I've added mt76_tx_status_check() on mt76u_stop_tx() routine instead.

Stanislaw

#10 lore raw
Quoted 86 lines · Lorenzo Bianconi
Quoted 85 lines
Quoted 57 lines · Lorenzo Bianconi
Quoted 51 lines
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Quoted 4 lines

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

right

Quoted 3 lines

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

ack

Quoted 2 lines

Stanislaw

during device removal I guess we should also flush skbs in status queue, doing something like (after commit 0b5f71304cd9 (mt76: introduce mt76_free_device routine))

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +0 -0
index 1ef00e971cfa..d4d1eb003148 100644
@@ -299,7 +299,7 @@ static void mt76x0_disconnect(struct usb_interface *usb_intf)
 	if (!initalized)
 		return;
Quoted 5 lines

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c static void mt76x0_disconnect(struct usb_interface *usb_intf) · line 302

Quoted 2 lines
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
-	ieee80211_unregister_hw(dev->mt76.hw);
+	mt76_unregister_device(&dev->mt76);

mt76_unregister_device() free mmio dma. I've added mt76_tx_status_check() on mt76u_stop_tx() routine instead.

nope, after commit 0b5f71304cd98fb7b3b5b3a633e470bea979fe94
(https://github.com/nbd168/wireless/commit/0b5f71304cd98fb7b3b5b3a633e470bea979fe94)
it can be used even for usb

Lorenzo

Quoted 3 lines

Stanislaw

#11 lore raw
Quoted 92 lines · Lorenzo Bianconi
Quoted 86 lines · Lorenzo Bianconi
Quoted 85 lines
Quoted 57 lines · Lorenzo Bianconi
Quoted 51 lines
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Quoted 4 lines

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

right

Quoted 3 lines

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

ack

Quoted 2 lines

Stanislaw

during device removal I guess we should also flush skbs in status queue, doing something like (after commit 0b5f71304cd9 (mt76: introduce mt76_free_device routine))

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +0 -0
index 1ef00e971cfa..d4d1eb003148 100644
@@ -299,7 +299,7 @@ static void mt76x0_disconnect(struct usb_interface *usb_intf)
 	if (!initalized)
 		return;
Quoted 5 lines

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c static void mt76x0_disconnect(struct usb_interface *usb_intf) · line 302

Quoted 2 lines
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
-	ieee80211_unregister_hw(dev->mt76.hw);
+	mt76_unregister_device(&dev->mt76);

mt76_unregister_device() free mmio dma. I've added mt76_tx_status_check() on mt76u_stop_tx() routine instead.

Quoted 3 lines

nope, after commit 0b5f71304cd98fb7b3b5b3a633e470bea979fe94
(https://github.com/nbd168/wireless/commit/0b5f71304cd98fb7b3b5b3a633e470bea979fe94)
it can be used even for usb

Ok, but as you pointed before 'right moment to deallocate the skbs is during suspend' so I still preffer to flush statuses there.

Stanislaw

#12 lore raw
Quoted 93 lines · Stanislaw Gruszka
Quoted 92 lines · Lorenzo Bianconi
Quoted 91 lines · Lorenzo Bianconi
Quoted 82 lines
Quoted 57 lines · Lorenzo Bianconi
Quoted 51 lines
Quoted 43 lines · Lorenzo Bianconi
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines · Lorenzo Bianconi
Quoted 2 lines

Disable mt76u_tx_tasklet at the end of mt76u_stop_queues in order to properly deallocate all pending skbs during suspend/resume phase

Quoted 8 lines
Quoted 8 lines
Quoted 3 lines

On suspend/resume tx skb's are processed after tasklet_enable() in resume callback. There is issue with device removal though (during suspend or otherwise).

Hi Stanislaw,

I guess the right moment to deallocate the skbs is during suspend since resume can happen in very far future

Yes, it's better to free on suspend, but in practice does not really matter since system is disabled till resume.

Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 11 lines
Fixes b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/wireless/mediatek/mt76/usb.c +0 -0
index a3acc070063a..575207133775 100644

drivers/net/wireless/mediatek/mt76/usb.c static void mt76u_stop_tx(struct mt76_dev *dev) · line 848

Quoted 9 lines
drivers/net/wireless/mediatek/mt76/usb.c +2 -2
@@ -842,10 +842,10 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
 void mt76u_stop_queues(struct mt76_dev *dev)
 {
 	tasklet_disable(&dev->usb.rx_tasklet);
-	tasklet_disable(&dev->usb.tx_tasklet);
-
 	mt76u_stop_rx(dev);
+
 	mt76u_stop_tx(dev);
+	tasklet_disable(&dev->usb.tx_tasklet);

If tasklet is scheduled and we disable it and never enable, we end up with infinite loop in tasklet_action_common(). This patch make the problem less reproducible since tasklet_disable() is moved after usb_kill_urb() -> tasklet_schedule(), but it is still possible.

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

I can see the point here. Maybe we can just run tasklet_kill instead of tasklet_disable here (at least for tx one)

I think you have right as tasklet_kill() will wait for scheduled tasklet . Originally in my patch (see below) I used wait_event as I thought tasklet_kill() may prevent scheduled tasklet to be executed (hence cause leak) but that seems to be not true.

Quoted 4 lines

I agree with rx side (good catch!!), but on tx one I guess usb_kill_urb() is already waiting for tx pending so we just need to use tasklet_kill at the end of mt76u_stop_queues, in this way we will free pending skbs during suspend

Quoted 4 lines

I looked more into that and there are some issues with this approach. tx_tasklet do mt76_txq_schedule() which can queue tx frames. Also we do not free skb's that require status check and dev->usb.stat_work is already (correctly) stopped on mac80211.stop.

right

Quoted 3 lines

I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those issues correctly.

ack

Quoted 2 lines

Stanislaw

during device removal I guess we should also flush skbs in status queue, doing something like (after commit 0b5f71304cd9 (mt76: introduce mt76_free_device routine))

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +0 -0
index 1ef00e971cfa..d4d1eb003148 100644

drivers/net/wireless/mediatek/mt76/mt76x0/usb.c static void mt76x0_disconnect(struct usb_interface *usb_intf) · line 302

Quoted 5 lines
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c +1 -1
@@ -299,7 +299,7 @@ static void mt76x0_disconnect(struct usb_interface *usb_intf)
 	if (!initalized)
 		return;
 
-	ieee80211_unregister_hw(dev->mt76.hw);
+	mt76_unregister_device(&dev->mt76);

mt76_unregister_device() free mmio dma. I've added mt76_tx_status_check() on mt76u_stop_tx() routine instead.

Quoted 7 lines
Quoted 3 lines

nope, after commit 0b5f71304cd98fb7b3b5b3a633e470bea979fe94
(https://github.com/nbd168/wireless/commit/0b5f71304cd98fb7b3b5b3a633e470bea979fe94)
it can be used even for usb

Ok, but as you pointed before 'right moment to deallocate the skbs is during suspend' so I still preffer to flush statuses there.

I agree

Lorenzo

Quoted 1 line

Stanislaw