Re: [PATCH v4 09/10] rtlwifi: use debugfs to debug.

[Kalle Valo]

Arend van Spriel  writes:

> On 7/27/2017 12:17 PM, Kalle Valo wrote:
>> Larry Finger  writes:
>>
>>> From: Ping-Ke Shih 
>>>
>>> Use debugfs to dump register and btcoex status.
>>
>> The title is useless and the commit log does not mention anything about
>> what files are added and to which location.
>>
>>> The kbuild test robot  suggested a change to use
>>> simple_open() instead of a local routine.
>>
>> This comment doesn't belong to commit log, it should be in the changelog.
>>
>>> +void rtl_debugfs_add_topdir(void)
>>> +{
>>> +   debugfs_topdir = debugfs_create_dir("rtlwifi", NULL);
>>> +}
>>> +
>>> +void rtl_debugfs_remove_topdir(void)
>>> +{
>>> +   debugfs_remove_recursive(debugfs_topdir);
>>> +}
>>
>> I'm surprised to see that rtlwifi creates it's own top level debugfs
>> directory and does not use wiphy->debugfsdir. So how is this supposed to
>> work when we have multiple rtlwifi devices on the same system?
>
> In brcmfmac we also have our own top level debugfs dir because we do
> wiphy_new()/wiphy_register() kinda late. To cover the multiple devices
> issue we create a subdir per device, ie.
> /sys/kernel/debug/brcmfmac/mmc0:0001:1/.

Yeah, that's fine. If rtlwifi does the same that's good but document
that in the commit log.

-- 
Kalle Valo

Re: [PATCH v4 09/10] rtlwifi: use debugfs to debug.

[Arend van Spriel]

On 7/27/2017 12:17 PM, Kalle Valo wrote:

Larry Finger  writes:


From: Ping-Ke Shih 

Use debugfs to dump register and btcoex status.


The title is useless and the commit log does not mention anything about
what files are added and to which location.


The kbuild test robot  suggested a change to use
simple_open() instead of a local routine.


This comment doesn't belong to commit log, it should be in the changelog.


+void rtl_debugfs_add_topdir(void)
+{
+   debugfs_topdir = debugfs_create_dir("rtlwifi", NULL);
+}
+
+void rtl_debugfs_remove_topdir(void)
+{
+   debugfs_remove_recursive(debugfs_topdir);
+}


I'm surprised to see that rtlwifi creates it's own top level debugfs
directory and does not use wiphy->debugfsdir. So how is this supposed to
work when we have multiple rtlwifi devices on the same system?


In brcmfmac we also have our own top level debugfs dir because we do 
wiphy_new()/wiphy_register() kinda late. To cover the multiple devices 
issue we create a subdir per device, ie. 
/sys/kernel/debug/brcmfmac/mmc0:0001:1/.


Regards,
Arend

Re: [PATCH v4 09/10] rtlwifi: use debugfs to debug.

[Kalle Valo]

Larry Finger  writes:

> From: Ping-Ke Shih 
>
> Use debugfs to dump register and btcoex status. The kbuild test robot
>  suggested a change to use simple_open() instead of
> a local routine.
>
> Signed-off-by: Ping-Ke Shih 
> Signed-off-by: Larry Finger 
> Cc: Yan-Hsuan Chuang 
> Cc: Birming Chiu 
> Cc: Shaofu 
> Cc: Steven Ting 

[...]

> + if (count < 3) {
> + /*printk("argument size is less than 3\n");*/
> + return -EFAULT;
> + }

> + if (num !=  3) {
> + /*printk("invalid write_reg parameter!\n");*/
> + return count;
> + }

> + default:
> + /*printk("error write length=%d", len);*/
> + break;
> + }

Dead code.

> + /* add for debug */
> + rtl_debug_add_one(hw);

> + /* remove form debug */
> + rtl_debug_remove_one(hw);

The comments are useless, the function name already tells the same.

-- 
Kalle Valo

Re: [PATCH v4 09/10] rtlwifi: use debugfs to debug.

[Kalle Valo]

Larry Finger  writes:

> From: Ping-Ke Shih 
>
> Use debugfs to dump register and btcoex status.

The title is useless and the commit log does not mention anything about
what files are added and to which location.

> The kbuild test robot  suggested a change to use
> simple_open() instead of a local routine.

This comment doesn't belong to commit log, it should be in the changelog.

> +void rtl_debugfs_add_topdir(void)
> +{
> + debugfs_topdir = debugfs_create_dir("rtlwifi", NULL);
> +}
> +
> +void rtl_debugfs_remove_topdir(void)
> +{
> + debugfs_remove_recursive(debugfs_topdir);
> +}

I'm surprised to see that rtlwifi creates it's own top level debugfs
directory and does not use wiphy->debugfsdir. So how is this supposed to
work when we have multiple rtlwifi devices on the same system?

-- 
Kalle Valo

[PATCH v4 09/10] rtlwifi: use debugfs to debug.

[Larry Finger]

From: Ping-Ke Shih 

Use debugfs to dump register and btcoex status. The kbuild test robot
 suggested a change to use simple_open() instead of
a local routine.

Signed-off-by: Ping-Ke Shih 
Signed-off-by: Larry Finger 
Cc: Yan-Hsuan Chuang 
Cc: Birming Chiu 
Cc: Shaofu 
Cc: Steven Ting 
---
v2 - no changes
v3 - no changes
v4 - replace rtl_debugfs_open() with simple_open() as found by kbuild test 
robot 
---
 drivers/net/wireless/realtek/rtlwifi/base.c|   6 +
 .../realtek/rtlwifi/btcoexist/halbtcoutsrc.c   |   3 +-
 .../realtek/rtlwifi/btcoexist/halbtcoutsrc.h   |   3 +-
 .../wireless/realtek/rtlwifi/btcoexist/rtl_btc.c   |   6 +
 .../wireless/realtek/rtlwifi/btcoexist/rtl_btc.h   |   1 +
 drivers/net/wireless/realtek/rtlwifi/debug.c   | 432 +
 drivers/net/wireless/realtek/rtlwifi/debug.h   |  12 +
 drivers/net/wireless/realtek/rtlwifi/pci.c |   6 +
 drivers/net/wireless/realtek/rtlwifi/wifi.h|   9 +
 9 files changed, 476 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c 
b/drivers/net/wireless/realtek/rtlwifi/base.c
index 4023644c3532..883e29e96edc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -2451,6 +2451,9 @@ static int __init rtl_core_module_init(void)
if (rtl_rate_control_register())
pr_err("rtl: Unable to register rtl_rc, use default RC !!\n");
 
+   /* add debugfs */
+   rtl_debugfs_add_topdir();
+
/* init some global vars */
INIT_LIST_HEAD(_global_var.glb_priv_list);
spin_lock_init(_global_var.glb_list_lock);
@@ -2462,6 +2465,9 @@ static void __exit rtl_core_module_exit(void)
 {
/*RC*/
rtl_rate_control_unregister();
+
+   /* remove debugfs */
+   rtl_debugfs_remove_topdir();
 }
 
 module_init(rtl_core_module_init);
diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
index c1eacd8352a2..e4f0d4710fb4 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.c
@@ -1504,7 +1504,8 @@ void exhalbtc_set_single_ant_path(u8 single_ant_path)
gl_bt_coexist.board_info.single_ant_path = single_ant_path;
 }
 
-void exhalbtc_display_bt_coex_info(struct btc_coexist *btcoexist)
+void exhalbtc_display_bt_coex_info(struct btc_coexist *btcoexist,
+  struct seq_file *m)
 {
if (!halbtc_is_bt_coexist_available(btcoexist))
return;
diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h
index f9b87c12db09..53aeb669cc63 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/halbtcoutsrc.h
@@ -626,7 +626,8 @@ void exhalbtc_update_min_bt_rssi(s8 bt_rssi);
 void exhalbtc_set_bt_exist(bool bt_exist);
 void exhalbtc_set_chip_type(u8 chip_type);
 void exhalbtc_set_ant_num(struct rtl_priv *rtlpriv, u8 type, u8 ant_num);
-void exhalbtc_display_bt_coex_info(struct btc_coexist *btcoexist);
+void exhalbtc_display_bt_coex_info(struct btc_coexist *btcoexist,
+  struct seq_file *m);
 void exhalbtc_signal_compensation(struct btc_coexist *btcoexist,
  u8 *rssi_wifi, u8 *rssi_bt);
 void exhalbtc_lps_leave(struct btc_coexist *btcoexist);
diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.c 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.c
index 7d296a401b6f..4d9e33078d4f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.c
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.c
@@ -52,8 +52,14 @@ static struct rtl_btc_ops rtl_btc_operation = {
.btc_is_bt_ctrl_lps = rtl_btc_is_bt_ctrl_lps,
.btc_is_bt_lps_on = rtl_btc_is_bt_lps_on,
.btc_get_ampdu_cfg = rtl_btc_get_ampdu_cfg,
+   .btc_display_bt_coex_info = rtl_btc_display_bt_coex_info,
 };
 
+void rtl_btc_display_bt_coex_info(struct rtl_priv *rtlpriv, struct seq_file *m)
+{
+   exhalbtc_display_bt_coex_info(_bt_coexist, m);
+}
+
 void rtl_btc_record_pwr_mode(struct rtl_priv *rtlpriv, u8 *buf, u8 len)
 {
u8 safe_len;
diff --git a/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.h 
b/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.h
index ac1253c46f44..40f1ce8c8a06 100644
--- a/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.h
+++ b/drivers/net/wireless/realtek/rtlwifi/btcoexist/rtl_btc.h
@@ -44,6 +44,7 @@ bool rtl_btc_is_limited_dig(struct rtl_priv *rtlpriv);
 bool rtl_btc_is_disable_edca_turbo(struct rtl_priv