| Message ID | 20220518082318.3898514-3-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | RTW88: Add support for USB variants | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote: > The rtwdev->rf_lock spinlock protects the rf register accesses in > rtw_read_rf() and rtw_write_rf(). Most callers of these functions hold > rtwdev->mutex already with the exception of the callsites in the debugfs > code. The debugfs code doesn't justify an extra lock, so acquire the mutex > there as well before calling rf register accessors and drop the now > unnecessary spinlock. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/net/wireless/realtek/rtw88/debug.c | 11 +++++++++++ > drivers/net/wireless/realtek/rtw88/hci.h | 9 +++------ > drivers/net/wireless/realtek/rtw88/main.c | 1 - > drivers/net/wireless/realtek/rtw88/main.h | 3 --- > 4 files changed, 14 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c > b/drivers/net/wireless/realtek/rtw88/debug.c > index 1a52ff585fbc7..ba5ba852efb8c 100644 > --- a/drivers/net/wireless/realtek/rtw88/debug.c > +++ b/drivers/net/wireless/realtek/rtw88/debug.c > [...] > @@ -523,6 +527,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) > u32 addr, offset, data; > u8 path; > > + mutex_lock(&rtwdev->mutex); > + > for (path = 0; path < rtwdev->hal.rf_path_num; path++) { > seq_printf(m, "RF path:%d\n", path); > for (addr = 0; addr < 0x100; addr += 4) { > @@ -537,6 +543,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) > seq_puts(m, "\n"); > } > > + mutex_unlock(&rtwdev->mutex); > + > return 0; > } > This will take time to dump all RF registers for debugging purpose. For PCI interface, I think this would be okay. Could you try to dump registers via debufs while you are using a USB WiFi device, such as play Youtube or download files... If it doesn't work very well, I suggest to use rf_mutex to replace rf_lock inplace, but not just remove rf_lock. -- Ping-Ke
On Fri, May 20, 2022 at 03:49:06AM +0000, Pkshih wrote: > On Wed, 2022-05-18 at 10:23 +0200, Sascha Hauer wrote: > > The rtwdev->rf_lock spinlock protects the rf register accesses in > > rtw_read_rf() and rtw_write_rf(). Most callers of these functions hold > > rtwdev->mutex already with the exception of the callsites in the debugfs > > code. The debugfs code doesn't justify an extra lock, so acquire the mutex > > there as well before calling rf register accessors and drop the now > > unnecessary spinlock. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/net/wireless/realtek/rtw88/debug.c | 11 +++++++++++ > > drivers/net/wireless/realtek/rtw88/hci.h | 9 +++------ > > drivers/net/wireless/realtek/rtw88/main.c | 1 - > > drivers/net/wireless/realtek/rtw88/main.h | 3 --- > > 4 files changed, 14 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/net/wireless/realtek/rtw88/debug.c > > b/drivers/net/wireless/realtek/rtw88/debug.c > > index 1a52ff585fbc7..ba5ba852efb8c 100644 > > --- a/drivers/net/wireless/realtek/rtw88/debug.c > > +++ b/drivers/net/wireless/realtek/rtw88/debug.c > > > > [...] > > > @@ -523,6 +527,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) > > u32 addr, offset, data; > > u8 path; > > > > + mutex_lock(&rtwdev->mutex); > > + > > for (path = 0; path < rtwdev->hal.rf_path_num; path++) { > > seq_printf(m, "RF path:%d\n", path); > > for (addr = 0; addr < 0x100; addr += 4) { > > @@ -537,6 +543,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) > > seq_puts(m, "\n"); > > } > > > > + mutex_unlock(&rtwdev->mutex); > > + > > return 0; > > } > > > > This will take time to dump all RF registers for debugging > purpose. For PCI interface, I think this would be okay. > Could you try to dump registers via debufs while you are > using a USB WiFi device, such as play Youtube or download files... I just did a ping and iperf test while doing a: while true; do cat /sys/kernel/debug/ieee80211/phy0/rtw88/rf_dump ; done The register dumping has no influence on neither the throughput or the latency. Adding some debugging to the mutex_lock also tells why: rtwdev->mutex isn't acquired for normal rx/tx. It is only acquired every two seconds or so. So I would say adding the mutex_lock around the register dump is not a problem. If latency is a concern we could still move the mutex_lock() into the loop. Sascha
diff --git a/drivers/net/wireless/realtek/rtw88/debug.c b/drivers/net/wireless/realtek/rtw88/debug.c index 1a52ff585fbc7..ba5ba852efb8c 100644 --- a/drivers/net/wireless/realtek/rtw88/debug.c +++ b/drivers/net/wireless/realtek/rtw88/debug.c @@ -144,7 +144,9 @@ static int rtw_debugfs_get_rf_read(struct seq_file *m, void *v) addr = debugfs_priv->rf_addr; mask = debugfs_priv->rf_mask; + mutex_lock(&rtwdev->mutex); val = rtw_read_rf(rtwdev, path, addr, mask); + mutex_unlock(&rtwdev->mutex); seq_printf(m, "rf_read path:%d addr:0x%08x mask:0x%08x val=0x%08x\n", path, addr, mask, val); @@ -418,7 +420,9 @@ static ssize_t rtw_debugfs_set_rf_write(struct file *filp, return count; } + mutex_lock(&rtwdev->mutex); rtw_write_rf(rtwdev, path, addr, mask, val); + mutex_unlock(&rtwdev->mutex); rtw_dbg(rtwdev, RTW_DBG_DEBUGFS, "write_rf path:%d addr:0x%08x mask:0x%08x, val:0x%08x\n", path, addr, mask, val); @@ -523,6 +527,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) u32 addr, offset, data; u8 path; + mutex_lock(&rtwdev->mutex); + for (path = 0; path < rtwdev->hal.rf_path_num; path++) { seq_printf(m, "RF path:%d\n", path); for (addr = 0; addr < 0x100; addr += 4) { @@ -537,6 +543,8 @@ static int rtw_debug_get_rf_dump(struct seq_file *m, void *v) seq_puts(m, "\n"); } + mutex_unlock(&rtwdev->mutex); + return 0; } @@ -1027,6 +1035,8 @@ static void dump_gapk_status(struct rtw_dev *rtwdev, struct seq_file *m) dm_info->dm_flags & BIT(RTW_DM_CAP_TXGAPK) ? '-' : '+', rtw_dm_cap_strs[RTW_DM_CAP_TXGAPK]); + mutex_lock(&rtwdev->mutex); + for (path = 0; path < rtwdev->hal.rf_path_num; path++) { val = rtw_read_rf(rtwdev, path, RF_GAINTX, RFREG_MASK); seq_printf(m, "path %d:\n0x%x = 0x%x\n", path, RF_GAINTX, val); @@ -1036,6 +1046,7 @@ static void dump_gapk_status(struct rtw_dev *rtwdev, struct seq_file *m) txgapk->rf3f_fs[path][i], i); seq_puts(m, "\n"); } + mutex_unlock(&rtwdev->mutex); } static int rtw_debugfs_get_dm_cap(struct seq_file *m, void *v) diff --git a/drivers/net/wireless/realtek/rtw88/hci.h b/drivers/net/wireless/realtek/rtw88/hci.h index 4c6fc6fb3f83b..830d7532f2a35 100644 --- a/drivers/net/wireless/realtek/rtw88/hci.h +++ b/drivers/net/wireless/realtek/rtw88/hci.h @@ -166,12 +166,11 @@ static inline u32 rtw_read_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path, u32 addr, u32 mask) { - unsigned long flags; u32 val; - spin_lock_irqsave(&rtwdev->rf_lock, flags); + lockdep_assert_held(&rtwdev->mutex); + val = rtwdev->chip->ops->read_rf(rtwdev, rf_path, addr, mask); - spin_unlock_irqrestore(&rtwdev->rf_lock, flags); return val; } @@ -180,11 +179,9 @@ static inline void rtw_write_rf(struct rtw_dev *rtwdev, enum rtw_rf_path rf_path, u32 addr, u32 mask, u32 data) { - unsigned long flags; + lockdep_assert_held(&rtwdev->mutex); - spin_lock_irqsave(&rtwdev->rf_lock, flags); rtwdev->chip->ops->write_rf(rtwdev, rf_path, addr, mask, data); - spin_unlock_irqrestore(&rtwdev->rf_lock, flags); } static inline u32 diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c index 8b9899e41b0bb..f9864840ffd9c 100644 --- a/drivers/net/wireless/realtek/rtw88/main.c +++ b/drivers/net/wireless/realtek/rtw88/main.c @@ -1994,7 +1994,6 @@ int rtw_core_init(struct rtw_dev *rtwdev) skb_queue_head_init(&rtwdev->coex.queue); skb_queue_head_init(&rtwdev->tx_report.queue); - spin_lock_init(&rtwdev->rf_lock); spin_lock_init(&rtwdev->h2c.lock); spin_lock_init(&rtwdev->txq_lock); spin_lock_init(&rtwdev->tx_report.q_lock); diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h index 17815af9dd4ea..df6c6032bbd3b 100644 --- a/drivers/net/wireless/realtek/rtw88/main.h +++ b/drivers/net/wireless/realtek/rtw88/main.h @@ -1994,9 +1994,6 @@ struct rtw_dev { /* ensures exclusive access from mac80211 callbacks */ struct mutex mutex; - /* read/write rf register */ - spinlock_t rf_lock; - /* watch dog every 2 sec */ struct delayed_work watch_dog_work; u32 watch_dog_cnt;
The rtwdev->rf_lock spinlock protects the rf register accesses in rtw_read_rf() and rtw_write_rf(). Most callers of these functions hold rtwdev->mutex already with the exception of the callsites in the debugfs code. The debugfs code doesn't justify an extra lock, so acquire the mutex there as well before calling rf register accessors and drop the now unnecessary spinlock. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/wireless/realtek/rtw88/debug.c | 11 +++++++++++ drivers/net/wireless/realtek/rtw88/hci.h | 9 +++------ drivers/net/wireless/realtek/rtw88/main.c | 1 - drivers/net/wireless/realtek/rtw88/main.h | 3 --- 4 files changed, 14 insertions(+), 10 deletions(-)