* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Herbert Xu @ 2019-04-16 9:29 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Johannes Berg, Toke Høiland-Jørgensen, Felix Fietkau,
linux-wireless, netdev
In-Reply-To: <cd0715bb-b75c-63b0-0ae4-858d14afd500@broadcom.com>
On Tue, Apr 16, 2019 at 11:17:53AM +0200, Arend Van Spriel wrote:
> On 4/16/2019 10:37 AM, Johannes Berg wrote:
> > It is true because we have an entire buffering layer in mac80211 (in
> > this case at least) and never push back to the stack.
>
> Ok, so the crux is the "never push back to the stack" part? Well, the
> internal TXQ and how that is used is obviously enabling that ;-)
So assuming that these drivers all have a TX queue length of zero
and therefore do not make use of Linux queueing disciplines then
yes techincally LLTX is fine.
However, I must say that it is much better to provide real
congestion feedback to the stack when you can because otherwise
things like UDP may fall apart.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] ath10k: add support for simulate crash on SDIO chip
From: Wen Gong @ 2019-04-16 9:27 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless
The command to simulate firmware crash:
echo soft > /sys/kernel/debug/ieee80211/phy0/ath10k/simulate_fw_crash
It will send WMI_FORCE_FW_HANG_ASSERT to firmware, then it will trigger
CPU interrupt status register for SDIO chip, ath10k driver need to
configure it while enable SDIO interrupt, otherwise ath10k driver will
not get the assert error info.
After this change, it will success for simulate firmware crash.
Tested with QCA6174 SDIO with firmware
WLAN.RMH.4.4.1-00007-QCARMSWP-1.
Signed-off-by: Wen Gong <wgong@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/hw.h | 1 +
drivers/net/wireless/ath/ath10k/sdio.c | 8 ++++++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index 7131499..60521ed 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -1095,6 +1095,7 @@ struct ath10k_hw_ops {
#define MBOX_CPU_INT_STATUS_ENABLE_ADDRESS 0x00000819
#define MBOX_CPU_INT_STATUS_ENABLE_BIT_LSB 0
#define MBOX_CPU_INT_STATUS_ENABLE_BIT_MASK 0x000000ff
+#define MBOX_CPU_STATUS_ENABLE_ASSERT_MASK 0x00000001
#define MBOX_ERROR_STATUS_ENABLE_ADDRESS 0x0000081a
#define MBOX_ERROR_STATUS_ENABLE_RX_UNDERFLOW_LSB 1
#define MBOX_ERROR_STATUS_ENABLE_RX_UNDERFLOW_MASK 0x00000002
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c6..78a2f3b 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -850,6 +850,8 @@ static int ath10k_sdio_mbox_proc_cpu_intr(struct ath10k *ar)
out:
mutex_unlock(&irq_data->mtx);
+ ath10k_err(ar, "firmware crashed!\n");
+ queue_work(ar->workqueue, &ar->restart_work);
return ret;
}
@@ -1495,8 +1497,10 @@ static int ath10k_sdio_hif_enable_intrs(struct ath10k *ar)
regs->int_status_en |=
FIELD_PREP(MBOX_INT_STATUS_ENABLE_MBOX_DATA_MASK, 1);
- /* Set up the CPU Interrupt status Register */
- regs->cpu_int_status_en = 0;
+ /* Set up the CPU Interrupt Status Register, enable CPU sourced interrupt #0
+ * #0 is used for report assertion from target
+ */
+ regs->cpu_int_status_en = FIELD_PREP(MBOX_CPU_STATUS_ENABLE_ASSERT_MASK, 1);
/* Set up the Error Interrupt status Register */
regs->err_int_status_en =
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Arend Van Spriel @ 2019-04-16 9:17 UTC (permalink / raw)
To: Johannes Berg, Herbert Xu
Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless
In-Reply-To: <eab39dd804405854997e50102f3d0ff925feb8d2.camel@sipsolutions.net>
On 4/16/2019 10:37 AM, Johannes Berg wrote:
> On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
>> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>>>
>>> I was just writing up an email clarifying my question. But let me summarize
>>> this email thread. The patch from Felix adds this flag in mac80211 for
>>> drivers that indicate to support pulling packets from the internal TXQ in
>>> mac80211. I found it is deprecated, but as Felix mentioned it is used in
>>> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
>>> implementations. So its use seems to be restricted rather than deprecated.
>>> Given your response above I guess my question would be to get details about
>>> what you call "proper design" as I think you are saying with that it is not
>>> needed, right?
>>
>> Essentially the only time it would be OK to use LLTX in its current
>> form is if you have no TX queue/congestion feedback which is clearly
>> not the case with wireless drivers.
>
> It is true because we have an entire buffering layer in mac80211 (in
> this case at least) and never push back to the stack.
Ok, so the crux is the "never push back to the stack" part? Well, the
internal TXQ and how that is used is obviously enabling that ;-)
Regards,
Arend
^ permalink raw reply
* [RFC 3/3] mt76x02: remove bogus mutex usage
From: Stanislaw Gruszka @ 2019-04-16 9:13 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi
In-Reply-To: <20190416091305.4218-1-sgruszka@redhat.com>
mac .start(), .stop() callbacks are never called concurrently with other
mac callbacks. The only concurencly is with mt76 works which we cancel
on stop() and schedule on start().
This fixes possible deadlock on cancel_delayed_work_sync(&dev->mac_work)
as mac_work also take mutex.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
.../net/wireless/mediatek/mt76/mt76x0/pci.c | 6 -----
.../net/wireless/mediatek/mt76/mt76x0/usb.c | 22 +++++--------------
.../wireless/mediatek/mt76/mt76x2/pci_main.c | 5 -----
.../wireless/mediatek/mt76/mt76x2/usb_main.c | 10 ++-------
4 files changed, 7 insertions(+), 36 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
index 156d3d064ba0..ac979128386a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
@@ -25,8 +25,6 @@ static int mt76x0e_start(struct ieee80211_hw *hw)
{
struct mt76x02_dev *dev = hw->priv;
- mutex_lock(&dev->mt76.mutex);
-
mt76x02_mac_start(dev);
mt76x0_phy_calibrate(dev, true);
ieee80211_queue_delayed_work(dev->mt76.hw, &dev->mac_work,
@@ -35,8 +33,6 @@ static int mt76x0e_start(struct ieee80211_hw *hw)
MT_CALIBRATE_INTERVAL);
set_bit(MT76_STATE_RUNNING, &dev->mt76.state);
- mutex_unlock(&dev->mt76.mutex);
-
return 0;
}
@@ -62,10 +58,8 @@ static void mt76x0e_stop(struct ieee80211_hw *hw)
{
struct mt76x02_dev *dev = hw->priv;
- mutex_lock(&dev->mt76.mutex);
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
mt76x0e_stop_hw(dev);
- mutex_unlock(&dev->mt76.mutex);
}
static void
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 22c10722019d..494b9f35f80d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -81,8 +81,10 @@ static void mt76x0u_cleanup(struct mt76x02_dev *dev)
mt76u_queues_deinit(&dev->mt76);
}
-static void mt76x0u_mac_stop(struct mt76x02_dev *dev)
+static void mt76x0u_stop(struct ieee80211_hw *hw)
{
+ struct mt76x02_dev *dev = hw->priv;
+
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
cancel_delayed_work_sync(&dev->cal_work);
cancel_delayed_work_sync(&dev->mac_work);
@@ -106,11 +108,9 @@ static int mt76x0u_start(struct ieee80211_hw *hw)
struct mt76x02_dev *dev = hw->priv;
int ret;
- mutex_lock(&dev->mt76.mutex);
-
ret = mt76x0_mac_start(dev);
if (ret)
- goto out;
+ return ret;
mt76x0_phy_calibrate(dev, true);
ieee80211_queue_delayed_work(dev->mt76.hw, &dev->mac_work,
@@ -118,19 +118,7 @@ static int mt76x0u_start(struct ieee80211_hw *hw)
ieee80211_queue_delayed_work(dev->mt76.hw, &dev->cal_work,
MT_CALIBRATE_INTERVAL);
set_bit(MT76_STATE_RUNNING, &dev->mt76.state);
-
-out:
- mutex_unlock(&dev->mt76.mutex);
- return ret;
-}
-
-static void mt76x0u_stop(struct ieee80211_hw *hw)
-{
- struct mt76x02_dev *dev = hw->priv;
-
- mutex_lock(&dev->mt76.mutex);
- mt76x0u_mac_stop(dev);
- mutex_unlock(&dev->mt76.mutex);
+ return 0;
}
static const struct ieee80211_ops mt76x0u_ops = {
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
index 16dc8e2451b5..1b5caabebff5 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
@@ -22,8 +22,6 @@ mt76x2_start(struct ieee80211_hw *hw)
struct mt76x02_dev *dev = hw->priv;
int ret;
- mutex_lock(&dev->mt76.mutex);
-
ret = mt76x2_mac_start(dev);
if (ret)
goto out;
@@ -40,7 +38,6 @@ mt76x2_start(struct ieee80211_hw *hw)
set_bit(MT76_STATE_RUNNING, &dev->mt76.state);
out:
- mutex_unlock(&dev->mt76.mutex);
return ret;
}
@@ -49,10 +46,8 @@ mt76x2_stop(struct ieee80211_hw *hw)
{
struct mt76x02_dev *dev = hw->priv;
- mutex_lock(&dev->mt76.mutex);
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
mt76x2_stop_hardware(dev);
- mutex_unlock(&dev->mt76.mutex);
}
static int
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
index 0771de210c6a..32726b4906ea 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
@@ -21,30 +21,24 @@ static int mt76x2u_start(struct ieee80211_hw *hw)
struct mt76x02_dev *dev = hw->priv;
int ret;
- mutex_lock(&dev->mt76.mutex);
-
ret = mt76x2u_mac_start(dev);
if (ret)
- goto out;
+ return ret;
ieee80211_queue_delayed_work(mt76_hw(dev), &dev->mac_work,
MT_MAC_WORK_INTERVAL);
set_bit(MT76_STATE_RUNNING, &dev->mt76.state);
-out:
- mutex_unlock(&dev->mt76.mutex);
- return ret;
+ return 0;
}
static void mt76x2u_stop(struct ieee80211_hw *hw)
{
struct mt76x02_dev *dev = hw->priv;
- mutex_lock(&dev->mt76.mutex);
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
mt76u_stop_tx(&dev->mt76);
mt76x2u_stop_hw(dev);
- mutex_unlock(&dev->mt76.mutex);
}
static int
--
2.20.1
^ permalink raw reply related
* [RFC 2/3] mt76usb: fix tx/rx stop
From: Stanislaw Gruszka @ 2019-04-16 9:13 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi
In-Reply-To: <20190416091305.4218-1-sgruszka@redhat.com>
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 RX we have to assure rx_tasklet do fail to resubmit buffers
by poisoning urb's and kill the tasklet.
For TX we need to handle all stop cases properly (suspend, module
unload, device removal).
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mac80211.c | 3 +-
drivers/net/wireless/mediatek/mt76/mt76.h | 7 +-
.../net/wireless/mediatek/mt76/mt76x0/usb.c | 10 +--
.../net/wireless/mediatek/mt76/mt76x2/usb.c | 9 +--
.../wireless/mediatek/mt76/mt76x2/usb_init.c | 1 -
.../wireless/mediatek/mt76/mt76x2/usb_main.c | 1 +
drivers/net/wireless/mediatek/mt76/usb.c | 78 +++++++++++++------
7 files changed, 67 insertions(+), 42 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 4b63d061c2a0..613f770ba2f7 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -390,7 +390,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;
@@ -403,6 +403,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)
{
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index f0d34901c825..6250a11af2bf 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -683,6 +683,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);
@@ -777,10 +778,10 @@ 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);
-void mt76u_stop_stat_wk(struct mt76_dev *dev);
+void mt76u_stop_tx(struct mt76_dev *dev);
+void mt76u_stop_rx(struct mt76_dev *dev);
+int mt76u_resume_rx(struct mt76_dev *dev);
void mt76u_queues_deinit(struct mt76_dev *dev);
struct sk_buff *
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 3276382b2180..22c10722019d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -86,7 +86,7 @@ static void mt76x0u_mac_stop(struct mt76x02_dev *dev)
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
cancel_delayed_work_sync(&dev->cal_work);
cancel_delayed_work_sync(&dev->mac_work);
- mt76u_stop_stat_wk(&dev->mt76);
+ mt76u_stop_tx(&dev->mt76);
mt76x02u_exit_beacon_config(dev);
if (test_bit(MT76_REMOVED, &dev->mt76.state))
@@ -313,7 +313,7 @@ static int __maybe_unused mt76x0_suspend(struct usb_interface *usb_intf,
{
struct mt76x02_dev *dev = usb_get_intfdata(usb_intf);
- mt76u_stop_queues(&dev->mt76);
+ mt76u_stop_rx(&dev->mt76);
clear_bit(MT76_STATE_MCU_RUNNING, &dev->mt76.state);
mt76x0_chip_onoff(dev, false, false);
@@ -323,16 +323,12 @@ static int __maybe_unused mt76x0_suspend(struct usb_interface *usb_intf,
static int __maybe_unused mt76x0_resume(struct usb_interface *usb_intf)
{
struct mt76x02_dev *dev = usb_get_intfdata(usb_intf);
- struct mt76_usb *usb = &dev->mt76.usb;
int ret;
- ret = mt76u_submit_rx_buffers(&dev->mt76);
+ ret = mt76u_resume_rx(&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;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
index af90774cae32..7fd437a0b65b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
@@ -107,7 +107,7 @@ static int __maybe_unused mt76x2u_suspend(struct usb_interface *intf,
{
struct mt76x02_dev *dev = usb_get_intfdata(intf);
- mt76u_stop_queues(&dev->mt76);
+ mt76u_stop_rx(&dev->mt76);
return 0;
}
@@ -118,13 +118,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_resume_rx(&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;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
index 35bdf5ffae00..63834941d167 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_init.c
@@ -244,7 +244,6 @@ int mt76x2u_register_device(struct mt76x02_dev *dev)
void mt76x2u_stop_hw(struct mt76x02_dev *dev)
{
- mt76u_stop_stat_wk(&dev->mt76);
cancel_delayed_work_sync(&dev->cal_work);
cancel_delayed_work_sync(&dev->mac_work);
mt76x2u_mac_stop(dev);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
index eb414fb75fb1..0771de210c6a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
@@ -42,6 +42,7 @@ static void mt76x2u_stop(struct ieee80211_hw *hw)
mutex_lock(&dev->mt76.mutex);
clear_bit(MT76_STATE_RUNNING, &dev->mt76.state);
+ mt76u_stop_tx(&dev->mt76);
mt76x2u_stop_hw(dev);
mutex_unlock(&dev->mt76.mutex);
}
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index a3acc070063a..689ed7a7656c 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -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)
{
@@ -605,14 +604,29 @@ static void mt76u_free_rx(struct mt76_dev *dev)
memset(&q->rx_page, 0, sizeof(q->rx_page));
}
-static void mt76u_stop_rx(struct mt76_dev *dev)
+void mt76u_stop_rx(struct mt76_dev *dev)
{
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
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);
}
+EXPORT_SYMBOL_GPL(mt76u_stop_rx);
+
+int mt76u_resume_rx(struct mt76_dev *dev)
+{
+ struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
+ int i;
+
+ for (i = 0; i < q->ndesc; i++)
+ usb_unpoison_urb(q->entry[i].urb);
+
+ return mt76u_submit_rx_buffers(dev);
+}
+EXPORT_SYMBOL_GPL(mt76u_resume_rx);
static void mt76u_tx_tasklet(unsigned long data)
{
@@ -827,38 +841,54 @@ static void mt76u_free_tx(struct mt76_dev *dev)
}
}
-static void mt76u_stop_tx(struct mt76_dev *dev)
+void mt76u_stop_tx(struct mt76_dev *dev)
{
+ struct mt76_queue_entry entry;
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)
-{
- tasklet_disable(&dev->usb.rx_tasklet);
- tasklet_disable(&dev->usb.tx_tasklet);
+ 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);
+ }
- mt76u_stop_rx(dev);
- mt76u_stop_tx(dev);
-}
-EXPORT_SYMBOL_GPL(mt76u_stop_queues);
+ tasklet_kill(&dev->usb.tx_tasklet);
+
+ /* On phisical device removal we do not get tx complete
+ * callbacks, need to complete our skb's manually.
+ */
+ for (i = 0; i < IEEE80211_NUM_ACS; i++) {
+ q = dev->q_tx[i].q;
+
+ /* Assure we are in sync with killed tasklet. */
+ spin_lock_bh(&q->lock);
+ while (q->queued) {
+ entry = q->entry[q->head];
+ q->head = (q->head + 1) % q->ndesc;
+ q->queued--;
+
+ dev->drv->tx_complete_skb(dev, i, &entry);
+ }
+ spin_unlock_bh(&q->lock);
+ }
+ }
-void mt76u_stop_stat_wk(struct mt76_dev *dev)
-{
cancel_delayed_work_sync(&dev->usb.stat_work);
clear_bit(MT76_READING_STATS, &dev->state);
+
+ mt76_tx_status_check(dev, NULL, true);
}
-EXPORT_SYMBOL_GPL(mt76u_stop_stat_wk);
+EXPORT_SYMBOL_GPL(mt76u_stop_tx);
void mt76u_queues_deinit(struct mt76_dev *dev)
{
- mt76u_stop_queues(dev);
+ mt76u_stop_rx(dev);
+ mt76u_stop_tx(dev);
mt76u_free_rx(dev);
mt76u_free_tx(dev);
--
2.20.1
^ permalink raw reply related
* [RFC 1/3] mt76x02u: remove bogus stop on suspend
From: Stanislaw Gruszka @ 2019-04-16 9:13 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi
In-Reply-To: <20190416091305.4218-1-sgruszka@redhat.com>
On suspend mac80211 .stop callback is called before .suspend(), so
hw mac is already stopped and we do not have to do this again.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c | 1 -
drivers/net/wireless/mediatek/mt76/mt76x2/usb.c | 1 -
2 files changed, 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 1ef00e971cfa..3276382b2180 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -314,7 +314,6 @@ static int __maybe_unused mt76x0_suspend(struct usb_interface *usb_intf,
struct mt76x02_dev *dev = usb_get_intfdata(usb_intf);
mt76u_stop_queues(&dev->mt76);
- mt76x0u_mac_stop(dev);
clear_bit(MT76_STATE_MCU_RUNNING, &dev->mt76.state);
mt76x0_chip_onoff(dev, false, false);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
index d08bb964966b..af90774cae32 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
@@ -108,7 +108,6 @@ static int __maybe_unused mt76x2u_suspend(struct usb_interface *intf,
struct mt76x02_dev *dev = usb_get_intfdata(intf);
mt76u_stop_queues(&dev->mt76);
- mt76x2u_stop_hw(dev);
return 0;
}
--
2.20.1
^ permalink raw reply related
* [RFC 0/3] mt76: suspend/stop/removal fixes
From: Stanislaw Gruszka @ 2019-04-16 9:13 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi
Fixes for some coroner cases during intended to -next since problems
are not reproducible in practice and changes relay on -next patches.
Stanislaw Gruszka (3):
mt76x02u: remove bogus stop on suspend
mt76usb: fix tx/rx stop
mt76x02: remove bogus mutex usage
drivers/net/wireless/mediatek/mt76/mac80211.c | 3 +-
drivers/net/wireless/mediatek/mt76/mt76.h | 7 +-
.../net/wireless/mediatek/mt76/mt76x0/pci.c | 6 --
.../net/wireless/mediatek/mt76/mt76x0/usb.c | 33 ++------
.../wireless/mediatek/mt76/mt76x2/pci_main.c | 5 --
.../net/wireless/mediatek/mt76/mt76x2/usb.c | 10 +--
.../wireless/mediatek/mt76/mt76x2/usb_init.c | 1 -
.../wireless/mediatek/mt76/mt76x2/usb_main.c | 11 +--
drivers/net/wireless/mediatek/mt76/usb.c | 78 +++++++++++++------
9 files changed, 74 insertions(+), 80 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCH] mac80211: Fix kernel panic due to use of txq after free
From: Toke Høiland-Jørgensen @ 2019-04-16 8:49 UTC (permalink / raw)
To: Bhagavathi Perumal S, johannes, ath10k, linux-wireless
Cc: Bhagavathi Perumal S
In-Reply-To: <1555399480-30537-1-git-send-email-bperumal@codeaurora.org>
Bhagavathi Perumal S <bperumal@codeaurora.org> writes:
> The txq of vif is added to active_txqs list for ATF TXQ scheduling
> in the function ieee80211_queue_skb(), but it was not properly removed
> before freeing the txq object. It was causing use after free of the txq
> objects from the active_txqs list, result was kernel panic
> due to invalid memory access.
>
> Fix kernel invalid memory access by properly removing txq object
> from active_txqs list before free the object.
>
> Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
Nice catch, thanks!
Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
This should probably have a fixes tag:
Fixes: 1866760096bf ("mac80211: Add TXQ scheduling API")
-Toke
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Johannes Berg @ 2019-04-16 8:37 UTC (permalink / raw)
To: Herbert Xu, Arend Van Spriel
Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless
In-Reply-To: <20190416083636.5ttvezqyhzr2worg@gondor.apana.org.au>
On Tue, 2019-04-16 at 16:36 +0800, Herbert Xu wrote:
> On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
> >
> > I was just writing up an email clarifying my question. But let me summarize
> > this email thread. The patch from Felix adds this flag in mac80211 for
> > drivers that indicate to support pulling packets from the internal TXQ in
> > mac80211. I found it is deprecated, but as Felix mentioned it is used in
> > various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> > implementations. So its use seems to be restricted rather than deprecated.
> > Given your response above I guess my question would be to get details about
> > what you call "proper design" as I think you are saying with that it is not
> > needed, right?
>
> Essentially the only time it would be OK to use LLTX in its current
> form is if you have no TX queue/congestion feedback which is clearly
> not the case with wireless drivers.
It is true because we have an entire buffering layer in mac80211 (in
this case at least) and never push back to the stack.
johannes
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Herbert Xu @ 2019-04-16 8:36 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless,
johannes
In-Reply-To: <73b18131-2777-da5c-a6ee-9d9b3e13cd06@broadcom.com>
On Tue, Apr 16, 2019 at 10:04:24AM +0200, Arend Van Spriel wrote:
>
> I was just writing up an email clarifying my question. But let me summarize
> this email thread. The patch from Felix adds this flag in mac80211 for
> drivers that indicate to support pulling packets from the internal TXQ in
> mac80211. I found it is deprecated, but as Felix mentioned it is used in
> various parts of the network subsystem, ie. batman-adv, bridge, vlan, tunnel
> implementations. So its use seems to be restricted rather than deprecated.
> Given your response above I guess my question would be to get details about
> what you call "proper design" as I think you are saying with that it is not
> needed, right?
Essentially the only time it would be OK to use LLTX in its current
form is if you have no TX queue/congestion feedback which is clearly
not the case with wireless drivers.
Chers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] mt76: usb: fix possible memory leak during suspend/resume
From: Lorenzo Bianconi @ 2019-04-16 8:33 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, linux-wireless
In-Reply-To: <20190416082708.GB2833@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 5403 bytes --]
On Apr 16, Stanislaw Gruszka wrote:
> On Tue, Apr 16, 2019 at 10:12:42AM +0200, Lorenzo Bianconi wrote:
> > > On Mon, Apr 15, 2019 at 05:04:06PM +0200, Lorenzo Bianconi wrote:
> > > > > On Sat, Apr 13, 2019 at 12:10:59PM +0200, Lorenzo Bianconi wrote:
> > > > > > > On Fri, Apr 12, 2019 at 06:27:48PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > > On Fri, Apr 12, 2019 at 02:27:16PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > > > 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
> > > > > > >
> > > > > > > Yes, it's better to free on suspend, but in practice does not really matter since
> > > > > > > system is disabled till resume.
> > > > > > >
> > > > > > > > > > > Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> > > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > > ---
> > > > > > > > > > > drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
> > > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > > index a3acc070063a..575207133775 100644
> > > > > > > > > > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > > @@ -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 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
> > > > >
> > > > > 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
> > > >
> > > > >
> > > > > I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those
> > > > > issues correctly.
> > > >
> > > > ack
> > > >
> > > > >
> > > > > 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))
> > > >
> > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > > index 1ef00e971cfa..d4d1eb003148 100644
> > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > > @@ -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.
> >
> > 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
> Stanislaw
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] mt76: usb: fix possible memory leak during suspend/resume
From: Stanislaw Gruszka @ 2019-04-16 8:27 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, linux-wireless
In-Reply-To: <20190416081241.GA11046@localhost.localdomain>
On Tue, Apr 16, 2019 at 10:12:42AM +0200, Lorenzo Bianconi wrote:
> > On Mon, Apr 15, 2019 at 05:04:06PM +0200, Lorenzo Bianconi wrote:
> > > > On Sat, Apr 13, 2019 at 12:10:59PM +0200, Lorenzo Bianconi wrote:
> > > > > > On Fri, Apr 12, 2019 at 06:27:48PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > On Fri, Apr 12, 2019 at 02:27:16PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > > 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
> > > > > >
> > > > > > Yes, it's better to free on suspend, but in practice does not really matter since
> > > > > > system is disabled till resume.
> > > > > >
> > > > > > > > > > Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> > > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > > ---
> > > > > > > > > > drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
> > > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > index a3acc070063a..575207133775 100644
> > > > > > > > > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > > @@ -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 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
> > > >
> > > > 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
> > >
> > > >
> > > > I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those
> > > > issues correctly.
> > >
> > > ack
> > >
> > > >
> > > > 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))
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > index 1ef00e971cfa..d4d1eb003148 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > > @@ -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.
>
> 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
^ permalink raw reply
* Re: [PATCH] mt76: usb: fix possible memory leak during suspend/resume
From: Lorenzo Bianconi @ 2019-04-16 8:12 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: Lorenzo Bianconi, nbd, linux-wireless
In-Reply-To: <20190416080436.GA2833@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 4770 bytes --]
> On Mon, Apr 15, 2019 at 05:04:06PM +0200, Lorenzo Bianconi wrote:
> > > On Sat, Apr 13, 2019 at 12:10:59PM +0200, Lorenzo Bianconi wrote:
> > > > > On Fri, Apr 12, 2019 at 06:27:48PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > On Fri, Apr 12, 2019 at 02:27:16PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > > 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
> > > > >
> > > > > Yes, it's better to free on suspend, but in practice does not really matter since
> > > > > system is disabled till resume.
> > > > >
> > > > > > > > > Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > > ---
> > > > > > > > > drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
> > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > index a3acc070063a..575207133775 100644
> > > > > > > > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > > @@ -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 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
> > >
> > > 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
> >
> > >
> > > I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those
> > > issues correctly.
> >
> > ack
> >
> > >
> > > 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))
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > index 1ef00e971cfa..d4d1eb003148 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> > @@ -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.
nope, after commit 0b5f71304cd98fb7b3b5b3a633e470bea979fe94
(https://github.com/nbd168/wireless/commit/0b5f71304cd98fb7b3b5b3a633e470bea979fe94)
it can be used even for usb
Lorenzo
>
> Stanislaw
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] mwifiex: fix spelling mistake "capabilties" -> "capabilities"
From: Colin Ian King @ 2019-04-16 8:10 UTC (permalink / raw)
To: Mukesh Ojha, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat,
Xinming Hu, Kalle Valo, David S . Miller, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <9347fb11-1c45-3db5-da6b-593dcdc4c2f9@codeaurora.org>
On 16/04/2019 08:02, Mukesh Ojha wrote:
>
> On 4/15/2019 7:56 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> There various spelling mistakes in function names and in message
>> text. Fix these.
>>
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>
>
> I am hoping you have done the compilation test.
Indeed. It builds fine.
>
> Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
>
> Cheers,
> -Mukesh
>
>> ---
>> drivers/net/wireless/marvell/mwifiex/sta_event.c | 12 ++++++------
>> drivers/net/wireless/marvell/mwifiex/uap_event.c | 8 ++++----
>> 2 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c
>> b/drivers/net/wireless/marvell/mwifiex/sta_event.c
>> index a327fc5b36e3..8b3123cb84c8 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
>> @@ -27,9 +27,9 @@
>> #define MWIFIEX_IBSS_CONNECT_EVT_FIX_SIZE 12
>> -static int mwifiex_check_ibss_peer_capabilties(struct
>> mwifiex_private *priv,
>> - struct mwifiex_sta_node *sta_ptr,
>> - struct sk_buff *event)
>> +static int mwifiex_check_ibss_peer_capabilities(struct
>> mwifiex_private *priv,
>> + struct mwifiex_sta_node *sta_ptr,
>> + struct sk_buff *event)
>> {
>> int evt_len, ele_len;
>> u8 *curr;
>> @@ -42,7 +42,7 @@ static int
>> mwifiex_check_ibss_peer_capabilties(struct mwifiex_private *priv,
>> evt_len = event->len;
>> curr = event->data;
>> - mwifiex_dbg_dump(priv->adapter, EVT_D, "ibss peer capabilties:",
>> + mwifiex_dbg_dump(priv->adapter, EVT_D, "ibss peer capabilities:",
>> event->data, event->len);
>> skb_push(event, MWIFIEX_IBSS_CONNECT_EVT_FIX_SIZE);
>> @@ -937,8 +937,8 @@ int mwifiex_process_sta_event(struct
>> mwifiex_private *priv)
>> ibss_sta_addr);
>> sta_ptr = mwifiex_add_sta_entry(priv, ibss_sta_addr);
>> if (sta_ptr && adapter->adhoc_11n_enabled) {
>> - mwifiex_check_ibss_peer_capabilties(priv, sta_ptr,
>> - adapter->event_skb);
>> + mwifiex_check_ibss_peer_capabilities(priv, sta_ptr,
>> + adapter->event_skb);
>> if (sta_ptr->is_11n_enabled)
>> for (i = 0; i < MAX_NUM_TID; i++)
>> sta_ptr->ampdu_sta[i] =
>> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_event.c
>> b/drivers/net/wireless/marvell/mwifiex/uap_event.c
>> index ca759d9c0253..86bfa1b9ef9d 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/uap_event.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/uap_event.c
>> @@ -23,8 +23,8 @@
>> #define MWIFIEX_BSS_START_EVT_FIX_SIZE 12
>> -static int mwifiex_check_uap_capabilties(struct mwifiex_private *priv,
>> - struct sk_buff *event)
>> +static int mwifiex_check_uap_capabilities(struct mwifiex_private *priv,
>> + struct sk_buff *event)
>> {
>> int evt_len;
>> u8 *curr;
>> @@ -38,7 +38,7 @@ static int mwifiex_check_uap_capabilties(struct
>> mwifiex_private *priv,
>> evt_len = event->len;
>> curr = event->data;
>> - mwifiex_dbg_dump(priv->adapter, EVT_D, "uap capabilties:",
>> + mwifiex_dbg_dump(priv->adapter, EVT_D, "uap capabilities:",
>> event->data, event->len);
>> skb_push(event, MWIFIEX_BSS_START_EVT_FIX_SIZE);
>> @@ -201,7 +201,7 @@ int mwifiex_process_uap_event(struct
>> mwifiex_private *priv)
>> ETH_ALEN);
>> if (priv->hist_data)
>> mwifiex_hist_data_reset(priv);
>> - mwifiex_check_uap_capabilties(priv, adapter->event_skb);
>> + mwifiex_check_uap_capabilities(priv, adapter->event_skb);
>> break;
>> case EVENT_UAP_MIC_COUNTERMEASURES:
>> /* For future development */
^ permalink raw reply
* Re: [PATCH] mt76: usb: fix possible memory leak during suspend/resume
From: Stanislaw Gruszka @ 2019-04-16 8:04 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: Lorenzo Bianconi, nbd, linux-wireless
In-Reply-To: <20190415150405.GA14449@localhost.localdomain>
On Mon, Apr 15, 2019 at 05:04:06PM +0200, Lorenzo Bianconi wrote:
> > On Sat, Apr 13, 2019 at 12:10:59PM +0200, Lorenzo Bianconi wrote:
> > > > On Fri, Apr 12, 2019 at 06:27:48PM +0200, Lorenzo Bianconi wrote:
> > > > > > > On Fri, Apr 12, 2019 at 02:27:16PM +0200, Lorenzo Bianconi wrote:
> > > > > > > > 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
> > > >
> > > > Yes, it's better to free on suspend, but in practice does not really matter since
> > > > system is disabled till resume.
> > > >
> > > > > > > > Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > ---
> > > > > > > > drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
> > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > index a3acc070063a..575207133775 100644
> > > > > > > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > > > > > > @@ -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 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
> >
> > 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
>
> >
> > I'll use wait_event(dev->tx_wait) on mac80211 stop to handle those
> > issues correctly.
>
> ack
>
> >
> > 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))
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> index 1ef00e971cfa..d4d1eb003148 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
> @@ -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
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Arend Van Spriel @ 2019-04-16 8:04 UTC (permalink / raw)
To: Herbert Xu
Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless,
johannes
In-Reply-To: <20190416074444.pdubbh6fbibdnhi7@gondor.apana.org.au>
On 4/16/2019 9:44 AM, Herbert Xu wrote:
> On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
>> + Herbert
>>
>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>>
>>>> When using iTXQ, tx sequence number allocation and statistics are run at
>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>
>>> Cool, didn't know about that flag.
>>
>> It is water under the bridge as this patch got applied already, but I
>> stumbled upon it just recently and didn't know about that flag either. So I
>> looked for more information about it and found the definition [1], but the
>> comment seemed important enough to send this reply.
>>
>> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
>> /* do not use LLTX in new drivers */
>
> The most obvious problem with LLTX is that it can cause AF_PACKET
> to see packets twice. But the more subtle issue is that with a
> proper design this lock should have no contention anyway.
>
> So please explain why you want to use this in your driver and where
> the contention is coming from?
Hi Herbert,
I was just writing up an email clarifying my question. But let me
summarize this email thread. The patch from Felix adds this flag in
mac80211 for drivers that indicate to support pulling packets from the
internal TXQ in mac80211. I found it is deprecated, but as Felix
mentioned it is used in various parts of the network subsystem, ie.
batman-adv, bridge, vlan, tunnel implementations. So its use seems to be
restricted rather than deprecated. Given your response above I guess my
question would be to get details about what you call "proper design" as
I think you are saying with that it is not needed, right?
Regards,
Arend
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Herbert Xu @ 2019-04-16 7:44 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Toke Høiland-Jørgensen, Felix Fietkau, linux-wireless,
johannes
In-Reply-To: <773e4dff-29fd-22b4-e4bc-cd5a94c66dc2@broadcom.com>
On Sun, Apr 14, 2019 at 11:44:17AM +0200, Arend Van Spriel wrote:
> + Herbert
>
> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
> > Felix Fietkau <nbd@nbd.name> writes:
> >
> > > When using iTXQ, tx sequence number allocation and statistics are run at
> > > dequeue time. Because of that, it is safe to enable NETIF_F_LLTX, which
> > > allows tx handlers to run on multiple CPUs in parallel.
> >
> > Cool, didn't know about that flag.
>
> It is water under the bridge as this patch got applied already, but I
> stumbled upon it just recently and didn't know about that flag either. So I
> looked for more information about it and found the definition [1], but the
> comment seemed important enough to send this reply.
>
> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
> /* do not use LLTX in new drivers */
The most obvious problem with LLTX is that it can cause AF_PACKET
to see packets twice. But the more subtle issue is that with a
proper design this lock should have no contention anyway.
So please explain why you want to use this in your driver and where
the contention is coming from?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Arend Van Spriel @ 2019-04-16 7:34 UTC (permalink / raw)
To: linux-wireless, johannes, Herbert Xu
Cc: Felix Fietkau, Toke Høiland-Jørgensen
In-Reply-To: <16a1bd77c20.2764.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com>
On 4/14/2019 2:34 PM, Arend Van Spriel wrote:
> On April 14, 2019 1:19:49 PM Felix Fietkau <nbd@nbd.name> wrote:
>
>> On 2019-04-14 11:44, Arend Van Spriel wrote:
>>> + Herbert
>>>
>>> On 3/16/2019 7:14 PM, Toke Høiland-Jørgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>
>>>>> When using iTXQ, tx sequence number allocation and statistics are
>>>>> run at
>>>>> dequeue time. Because of that, it is safe to enable NETIF_F_LLTX,
>>>>> which
>>>>> allows tx handlers to run on multiple CPUs in parallel.
>>>>
>>>> Cool, didn't know about that flag.
>>>
>>> It is water under the bridge as this patch got applied already, but I
>>> stumbled upon it just recently and didn't know about that flag either.
>>> So I looked for more information about it and found the definition [1],
>>> but the comment seemed important enough to send this reply.
>>>
>>> NETIF_F_LLTX_BIT, /* LockLess TX - deprecated. Please */
>>> /* do not use LLTX in new drivers */
>>>
>>> Here is the commit that marked it deprecated:
>>>
>>> commit e24eb521fbf2a350ce879dfc1d8e56d4ffa2aa22
>>> Author: Christian Borntraeger <borntraeger@de.ibm.com>
>>> Date: Tue Sep 25 19:42:02 2007 -0700
>>>
>>> [NET]: note that NETIF_F_LLTX is deprecated
>>>
>>> So I am not sure we should really do this in mac80211. Maybe Herbert can
>>> comment although it has been over a decade ago.
>> There is a lot of comparable code that also uses this flag, e.g.
>> batman-adv, bridge, vlan, various tunnel implementations. I think
>> mac80211 fits well with those kinds of use cases.
>
> Ok. As said I was not sure so I can/will not argue.
>
>> If I remember correctly, the deprecation was added to avoid quirky
>> custom locking schemes in ethernet drivers.
>
> What do you mean by "quirky custom locking schemes"? You mean that TX
> path would use driver data that should actually be accessed under some
> lock?
>
> When seeing the deprecated comment I wanted to know the why and was
> hoping the commit message would divulge. It just mentions it is not
> needed. So now I am curious as to why it wouldn't be needed especially
> as you say there are (valid) use-cases in the kernel today.
Getting back to this in an attempt to clarify my question. So from what
Felix is saying the NETIF_F_LLTX flag is not deprecated, but restricted.
What I would like to know is what exactly is required from a driver to
allow the use of this flag.
Regards,
Arend
^ permalink raw reply
* [PATCH] mac80211: Fix kernel panic due to use of txq after free
From: Bhagavathi Perumal S @ 2019-04-16 7:24 UTC (permalink / raw)
To: johannes, ath10k, linux-wireless; +Cc: Bhagavathi Perumal S
The txq of vif is added to active_txqs list for ATF TXQ scheduling
in the function ieee80211_queue_skb(), but it was not properly removed
before freeing the txq object. It was causing use after free of the txq
objects from the active_txqs list, result was kernel panic
due to invalid memory access.
Fix kernel invalid memory access by properly removing txq object
from active_txqs list before free the object.
Signed-off-by: Bhagavathi Perumal S <bperumal@codeaurora.org>
---
net/mac80211/iface.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 94459b2..410685d 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1907,6 +1907,9 @@ void ieee80211_if_remove(struct ieee80211_sub_if_data *sdata)
list_del_rcu(&sdata->list);
mutex_unlock(&sdata->local->iflist_mtx);
+ if (sdata->vif.txq)
+ ieee80211_txq_purge(sdata->local, to_txq_info(sdata->vif.txq));
+
synchronize_rcu();
if (sdata->dev) {
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] staging: wilc1000: fix spelling mistake "dissconect" -> "disconnect"
From: Mukesh Ojha @ 2019-04-16 7:23 UTC (permalink / raw)
To: Colin King, Adham Abozaeid, Ajay Singh, Greg Kroah-Hartman,
linux-wireless, devel
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190415163330.28166-1-colin.king@canonical.com>
On 4/15/2019 10:03 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There is a spelling mistake in a netdev_err error message, fix it.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/staging/wilc1000/host_interface.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wilc1000/host_interface.c b/drivers/staging/wilc1000/host_interface.c
> index e1a35bb426f9..ed15bd1bcd56 100644
> --- a/drivers/staging/wilc1000/host_interface.c
> +++ b/drivers/staging/wilc1000/host_interface.c
> @@ -774,7 +774,7 @@ int wilc_disconnect(struct wilc_vif *vif)
> result = wilc_send_config_pkt(vif, WILC_SET_CFG, &wid, 1,
> wilc_get_vif_idx(vif));
> if (result) {
> - netdev_err(vif->ndev, "Failed to send dissconect\n");
> + netdev_err(vif->ndev, "Failed to send disconnect\n");
> return result;
> }
>
^ permalink raw reply
* Re: [PATCH] mwifiex: fix spelling mistake "capabilties" -> "capabilities"
From: Mukesh Ojha @ 2019-04-16 7:02 UTC (permalink / raw)
To: Colin King, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat,
Xinming Hu, Kalle Valo, David S . Miller, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190415142649.18452-1-colin.king@canonical.com>
On 4/15/2019 7:56 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> There various spelling mistakes in function names and in message
> text. Fix these.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
I am hoping you have done the compilation test.
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/net/wireless/marvell/mwifiex/sta_event.c | 12 ++++++------
> drivers/net/wireless/marvell/mwifiex/uap_event.c | 8 ++++----
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_event.c b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> index a327fc5b36e3..8b3123cb84c8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_event.c
> @@ -27,9 +27,9 @@
>
> #define MWIFIEX_IBSS_CONNECT_EVT_FIX_SIZE 12
>
> -static int mwifiex_check_ibss_peer_capabilties(struct mwifiex_private *priv,
> - struct mwifiex_sta_node *sta_ptr,
> - struct sk_buff *event)
> +static int mwifiex_check_ibss_peer_capabilities(struct mwifiex_private *priv,
> + struct mwifiex_sta_node *sta_ptr,
> + struct sk_buff *event)
> {
> int evt_len, ele_len;
> u8 *curr;
> @@ -42,7 +42,7 @@ static int mwifiex_check_ibss_peer_capabilties(struct mwifiex_private *priv,
> evt_len = event->len;
> curr = event->data;
>
> - mwifiex_dbg_dump(priv->adapter, EVT_D, "ibss peer capabilties:",
> + mwifiex_dbg_dump(priv->adapter, EVT_D, "ibss peer capabilities:",
> event->data, event->len);
>
> skb_push(event, MWIFIEX_IBSS_CONNECT_EVT_FIX_SIZE);
> @@ -937,8 +937,8 @@ int mwifiex_process_sta_event(struct mwifiex_private *priv)
> ibss_sta_addr);
> sta_ptr = mwifiex_add_sta_entry(priv, ibss_sta_addr);
> if (sta_ptr && adapter->adhoc_11n_enabled) {
> - mwifiex_check_ibss_peer_capabilties(priv, sta_ptr,
> - adapter->event_skb);
> + mwifiex_check_ibss_peer_capabilities(priv, sta_ptr,
> + adapter->event_skb);
> if (sta_ptr->is_11n_enabled)
> for (i = 0; i < MAX_NUM_TID; i++)
> sta_ptr->ampdu_sta[i] =
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_event.c b/drivers/net/wireless/marvell/mwifiex/uap_event.c
> index ca759d9c0253..86bfa1b9ef9d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_event.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_event.c
> @@ -23,8 +23,8 @@
>
> #define MWIFIEX_BSS_START_EVT_FIX_SIZE 12
>
> -static int mwifiex_check_uap_capabilties(struct mwifiex_private *priv,
> - struct sk_buff *event)
> +static int mwifiex_check_uap_capabilities(struct mwifiex_private *priv,
> + struct sk_buff *event)
> {
> int evt_len;
> u8 *curr;
> @@ -38,7 +38,7 @@ static int mwifiex_check_uap_capabilties(struct mwifiex_private *priv,
> evt_len = event->len;
> curr = event->data;
>
> - mwifiex_dbg_dump(priv->adapter, EVT_D, "uap capabilties:",
> + mwifiex_dbg_dump(priv->adapter, EVT_D, "uap capabilities:",
> event->data, event->len);
>
> skb_push(event, MWIFIEX_BSS_START_EVT_FIX_SIZE);
> @@ -201,7 +201,7 @@ int mwifiex_process_uap_event(struct mwifiex_private *priv)
> ETH_ALEN);
> if (priv->hist_data)
> mwifiex_hist_data_reset(priv);
> - mwifiex_check_uap_capabilties(priv, adapter->event_skb);
> + mwifiex_check_uap_capabilities(priv, adapter->event_skb);
> break;
> case EVENT_UAP_MIC_COUNTERMEASURES:
> /* For future development */
^ permalink raw reply
* Re: [PATCH] iwlegacy: fix spelling mistake "acumulative" -> "accumulative"
From: Mukesh Ojha @ 2019-04-16 6:47 UTC (permalink / raw)
To: Colin King, Stanislaw Gruszka, Kalle Valo, David S . Miller,
linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190415103703.11254-1-colin.king@canonical.com>
On 4/15/2019 4:07 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Fix spelling mistakes in rx stats text. I missed these from an earlier
> round of fixing the same spelling mistake.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/net/wireless/intel/iwlegacy/3945-debug.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/3945-debug.c b/drivers/net/wireless/intel/iwlegacy/3945-debug.c
> index a2960032be81..4b912e707f38 100644
> --- a/drivers/net/wireless/intel/iwlegacy/3945-debug.c
> +++ b/drivers/net/wireless/intel/iwlegacy/3945-debug.c
> @@ -185,7 +185,7 @@ il3945_ucode_rx_stats_read(struct file *file, char __user *user_buf,
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> "%-32s current"
> - "acumulative delta max\n",
> + "accumulative delta max\n",
> "Statistics_Rx - CCK:");
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> @@ -273,7 +273,7 @@ il3945_ucode_rx_stats_read(struct file *file, char __user *user_buf,
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> "%-32s current"
> - "acumulative delta max\n",
> + "accumulative delta max\n",
> "Statistics_Rx - GENERAL:");
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> @@ -346,7 +346,7 @@ il3945_ucode_tx_stats_read(struct file *file, char __user *user_buf,
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> "%-32s current"
> - "acumulative delta max\n",
> + "accumulative delta max\n",
> "Statistics_Tx:");
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> @@ -447,7 +447,7 @@ il3945_ucode_general_stats_read(struct file *file, char __user *user_buf,
> pos +=
> scnprintf(buf + pos, bufsz - pos,
> "%-32s current"
> - "acumulative delta max\n",
> + "accumulative delta max\n",
> "Statistics_General:");
> pos +=
> scnprintf(buf + pos, bufsz - pos,
^ permalink raw reply
* [PATCH] cfg80211: Notify previous user request during self managed wiphy registration
From: Sriram R @ 2019-04-16 5:46 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, Sriram R
Commit c82c06ce43d3("cfg80211: Notify all User Hints To self managed wiphys") notifies
all new user hints to self managed wiphy's after device registration.
But it doesn't notify user request (other than cell base hints) which was
set before registration.
This needs to be done during wiphy registration of a self managed device also,
so that the previous user settings are retained.
Signed-off-by: Sriram R <srirrama@codeaurora.org>
---
net/wireless/reg.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 2f1bf91..85a0b5e 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -3739,10 +3739,9 @@ void wiphy_regulatory_register(struct wiphy *wiphy)
/*
* The last request may have been received before this
* registration call. Call the driver notifier if
- * initiator is USER and user type is CELL_BASE.
+ * initiator is USER.
*/
- if (lr->initiator == NL80211_REGDOM_SET_BY_USER &&
- lr->user_reg_hint_type == NL80211_USER_REG_HINT_CELL_BASE)
+ if (lr->initiator == NL80211_REGDOM_SET_BY_USER)
reg_call_notifier(wiphy, lr);
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH wireless-drivers] rtlwifi: rtl8723ae: Fix missing break in switch statement
From: Kalle Valo @ 2019-04-16 3:36 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Ping-Ke Shih, David S. Miller, linux-wireless, netdev,
linux-kernel, Kees Cook
In-Reply-To: <20190416003626.GA16834@embeddedor>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> Add missing break statement in order to prevent the code from falling
> through to case 0x1025, and erroneously setting rtlhal->oem_id to
> RT_CID_819X_ACER when rtlefuse->eeprom_svid is equal to 0x10EC and
> none of the cases in switch (rtlefuse->eeprom_smid) match.
>
> This bug was found thanks to the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Fixes: 238ad2ddf34b ("rtlwifi: rtl8723ae: Clean up the hardware info routine")
> Cc: stable@vger.kernel.org
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
You have marked this for wireless-drivers but the commit is 2.5 years
old, so I think wireless-drivers-next is more approriate.
--
Kalle Valo
^ permalink raw reply
* [PATCH wireless-drivers] rtlwifi: rtl8723ae: Fix missing break in switch statement
From: Gustavo A. R. Silva @ 2019-04-16 0:36 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, David S. Miller
Cc: linux-wireless, netdev, linux-kernel, Gustavo A. R. Silva,
Kees Cook
Add missing break statement in order to prevent the code from falling
through to case 0x1025, and erroneously setting rtlhal->oem_id to
RT_CID_819X_ACER when rtlefuse->eeprom_svid is equal to 0x10EC and
none of the cases in switch (rtlefuse->eeprom_smid) match.
This bug was found thanks to the ongoing efforts to enable
-Wimplicit-fallthrough.
Fixes: 238ad2ddf34b ("rtlwifi: rtl8723ae: Clean up the hardware info routine")
Cc: stable@vger.kernel.org
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
index 6bab162e1bb8..655460f61bbc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c
@@ -1675,6 +1675,7 @@ static void _rtl8723e_read_adapter_info(struct ieee80211_hw *hw,
rtlhal->oem_id = RT_CID_819X_LENOVO;
break;
}
+ break;
case 0x1025:
rtlhal->oem_id = RT_CID_819X_ACER;
break;
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox